[Freeipa-devel] [PATCHSET] Replica promotion patches

[Jan Cholasta]

On 23.9.2015 02:47, Simo Sorce wrote:
> On Tue, 2015-09-22 at 10:57 -0400, Simo Sorce wrote:
>> On Tue, 2015-09-22 at 10:45 +0200, Jan Cholasta wrote:
>>> Hi,
>>>
>>> On 9.9.2015 20:25, Simo Sorce wrote:
>>>> On Wed, 2015-08-26 at 17:27 -0400, Simo Sorce wrote:
>>>>> This patchset implements https://fedorahosted.org/freeipa/ticket/2888
>>>>> and introduces a number of required  changes and dependencies to achieve
>>>>> this goal.
>>>>> This work requires the custodia project to securely transfer keys
>>>>> between ipa servers.
>>>>>
>>>>> This work is not 100% complete, it still misses the ability to install
>>>>> kra instances and the ability to install a CA (via ipa-ca-install) with
>>>>> externally signed certs.
>>>>>
>>>>> However it is massive enough that warrants review and pushing, the resat
>>>>> of the changes can be applied later as this work should not disrupt the
>>>>> classic install methods.
>>>>>
>>>>> In order to build my previous patches (530-533) are needed as well as a
>>>>> number of updated components.
>>>>>
>>>>> I used the following coprs for testing:
>>>>> simo/jwcrypto
>>>>> simo/custodia
>>>>> abbra/sssd-kkdcproxy (for sssd 1.13.1)
>>>>> lkrispen/389-ds-current (for 389 > 1.3.4.4)
>>>>> vakwetu/dogtag_10.2.7_test_builds (for dogtag 10.2.7)
>>>>> mkosek/freeipa-4.2-fedora-22 (misc)
>>>>> fedora/updates-testing (python-gssapi 1.1.2)
>>>>>
>>>>> Ludwig's copr is necessary to have a functional DNA plugin in replicas,
>>>>> eventually his patches should be committed in 389-ds-base 1.3.4.4 when
>>>>> it will be released.
>>>>>
>>>>> We are aware of a dogtag bug https://fedorahosted.org/pki/ticket/1580
>>>>> that may cause installation issues in some case (re-install of a
>>>>> replica).
>>>>>
>>>>> The domain must be raised to level 1 in order to use replica promotion.
>>>>>
>>>>> In order to promote a replica the server must be first joined as a
>>>>> regular client to the domain.
>>>>>
>>>>> This is the flow I usually use for testing:
>>>>>
>>>>> # ipa-client-install
>>>>> # kinit admin
>>>>> # ipa-replica-install --promote --setup-ca
>>>>> <perform operations like add user, get keytabs, get certificates,
>>>>> etc...>
>>>>>
>>>>> These patches are also available in this git tree rebnase on current
>>>>> master:
>>>>> https://fedorapeople.org/cgit/simo/public_git/freeipa.git/log/?h=custodia-review
>>>>
>>>> FYI: I rebased this branch on top of master and applied minor changes to
>>>> one of the DNS patches. I also added the missing support to install KRA.
>>>>
>>>> DS 1.3.4.4 is also in Fedora updates testing, so ludwig's copr is not
>>>> needed anymore.
>>>>
>>>> Dogtag's ticket is not fixed yet so running both --setup-ca and
>>>> --setup-kra at the same time will still yield an error and install will
>>>> fail.
>>>>
>>>> Please let me know if there are any major issues with this patchset, I'd
>>>> like to push it to master and attack the remaining issues as add ons
>>>> (install with external certs not supported yet for example)
>>>
>>> So far I have only read through the code without running it (mostly).
>>>
>>>
>>> "Remove unused arguments": ACK
>>>
>>>
>>> "Simplify the install_replica_ca function": ACK
>>
>> Thanks for pushing these.
>>
>>>
>>> "IPA Custodia Daemon":
>>>
>>> 1) Instead of putting the code in "ipakeys" package, could you put it in
>>> "ipapython.keys"? This way it would be consistent with DNSSEC, which has
>>> binaries in daemons/dnssec/ and modules in ipapython/dnssec/.
>>
>> I think I can do this, it was originally all in daemon becuse that's
>> where I had the custodia submodules, but we do not carry a copy anymore.
>>
>>> 2) Is it safe to create cn=custodia in update file only? Updates are
>>> executed late in ipa-server-install. Is is guaranteed that nothing will
>>> try to access cn=custodia before the updates are run?
>>>
>>> (Nevermind, it is added to bootstrap-template.ldif 2 commits below.)
>>>
>>> 3) Shouldn't cn=custodia be created only when domain level >= 1?
>>
>> It is used only at >= 1 level, but we have to create it when we update
>> the code, otherwise you cannot switch to level 1.
>> Switching a level ion LDAP cannot cause an update script to be run so
>> you would have incomplete servers publicizing level 1 but not offering a
>> critical service for level 1.

Makes sense, thanks.

>>
>>> 4) pylint fails with:
>>>
>>> daemons/ipa-custodia/ipakeys/kem.py:156: [E1002(super-on-old-class),
>>> IPAKEMKeys.__init__] Use of super on an old style class)
>>> daemons/ipa-custodia/ipakeys/kem.py:178: [E1101(no-member),
>>> IPAKEMKeys.generate_server_keys] Instance of 'IPAKEMKeys' has no
>>> 'config' member)
>>> daemons/ipa-custodia/ipakeys/kem.py:187: [E1101(no-member),
>>> IPAKEMKeys.server_keys] Instance of 'IPAKEMKeys' has no 'config' member)
>>
>> On what pylint version ?
>> I had to disable pylint for a while but it currently runs and doesn't
>> complain to me ...
>
> pylint looks fine for everything I touched now.

As I said in the other thread, this is without custodia installed, so 
just ignore it.

>
>>> 5) There are some PEP8 transgressions:
>>>
>>> ./daemons/ipa-custodia/ipakeys/kem.py:202:80: E501 line too long (82 >
>>> 79 characters)
>>> ./daemons/ipa-custodia/ipakeys/kem.py:203:80: E501 line too long (82 >
>>> 79 characters)
>>> ./daemons/ipa-custodia/ipakeys/store.py:33:1: E302 expected 2 blank
>>> lines, found 1
>>> ./daemons/ipa-custodia/setup.py:8:9: E251 unexpected spaces around
>>> keyword / parameter equals
>>> ./daemons/ipa-custodia/setup.py:8:11: E251 unexpected spaces around
>>> keyword / parameter equals
>>> ./daemons/ipa-custodia/setup.py:9:12: E251 unexpected spaces around
>>> keyword / parameter equals
>>> ./daemons/ipa-custodia/setup.py:9:14: E251 unexpected spaces around
>>> keyword / parameter equals
>>> ./daemons/ipa-custodia/setup.py:10:12: E251 unexpected spaces around
>>> keyword / parameter equals
>>> ./daemons/ipa-custodia/setup.py:10:14: E251 unexpected spaces around
>>> keyword / parameter equals
>>> ./daemons/ipa-custodia/setup.py:11:15: E251 unexpected spaces around
>>> keyword / parameter equals
>>> ./daemons/ipa-custodia/setup.py:11:17: E251 unexpected spaces around
>>> keyword / parameter equals
>>> ./daemons/ipa-custodia/setup.py:12:21: E251 unexpected spaces around
>>> keyword / parameter equals
>>> ./daemons/ipa-custodia/setup.py:12:23: E251 unexpected spaces around
>>> keyword / parameter equals
>>> ./daemons/ipa-custodia/setup.py:14:13: E251 unexpected spaces around
>>> keyword / parameter equals
>>> ./daemons/ipa-custodia/setup.py:14:15: E251 unexpected spaces around
>>> keyword / parameter equals
>>> ./daemons/ipa-custodia/setup.py:16:1: W391 blank line at end of file
>>
>> I'll take care of these.
>
>>> "Add Custodia Client code":
>>>
>>> 1) Is there any significance in having this in a separate commit? I
>>> would think it can be safely squashed in the previous commit.
>>
>> Easier review (and rebasing), I want to keep chunks smaller.
>>
>>> 2) There is one PEP8 transgression:
>>>
>>> ./daemons/ipa-custodia/ipakeys/client.py:45:5: E303 too many blank lines (2)
>>>
>>>
>>> "Install ipa-custodia with the rest of ipa":
>>>
>>> 1) python-jwcrypto dependency is missing in the spec file.
>>
>> It shouldn't be necessary as custodia already depends on it.

IMO it is a good practice to require all direct dependencies, because 
you can't control indirect dependencies. For example, if one day 
custodia switched from jwcrypto to something different, ipa would lose 
the jwcrypto dependency without us knowing.

>>
>>> 2) I believe the daemons/ipa-custodia/* and
>>> install/share/bootstrap-template.ldif should be in the "IPA Custodia
>>> Daemon" commit. At the very least it would make reviews easier.
>>
>> ok
>
> I ended up squashing together these first 3 commits, after looking more
> carefully into them they are less messy w/o submodules, and make sense
> to have all of them in a single unit.

OK.

>
>>> 3) There is one PEP8 transgressions:
>>>
>>> ./ipaserver/install/custodiainstance.py:10:1: E302 expected 2 blank
>>> lines, found 1
>>>
>>>
>>> "Require a DS version that has working DNA plugin": ACK
>>>
>>>
>>> "Implement replica promotion functionality":
>>>
>>> 1) I think a RuntimeError or sys.exit with an error message would be
>>> better than NotImplementedError for the unimplemented options.
>>>
>>> (Nevermind, these are removed in further commits.)
>>
>> They were for my use indeed :)
>>
>>> 2) There is no domain level check when installing the replica from a
>>> replica file.
>
>> We discussed about preventing generation of a replica file at level 1,
>> but that is not part of this patchset.
>
> Will be added later.

What I mean is that installing a replica using an already existing 
replica file should be prevented at level 1 as well:

root at ipa1# ipa-server-install --domain-level=0
root at ipa1# ipa-replica-prepare ipa2.example.com
root at ipa1# ipa domainlevel-set 1

root at ipa2# ipa-replica-install replica-info-ipa2.example.com.gpg
ERROR: Can't install replica from a replica file at domain level > 0

>
>
>>> 3) Instead of returning INSTALL_ERROR from promote_check() (which has no
>>> effect), raise an exception or call sys.exit().
>>
>> ok
>>
>>> 4) The --promote option is redundant. You can safely decide whether to
>>> promote or not based on whether replica file was provided or not and the
>>> domain level of the remote server.
>>
>> Except there is a chick/egg issue in getting there, I am ok removing
>> --promote in a followup patch, but not changing it in this patchset, it
>> would require too much churn.

OK, no problem.

>
> To be more precise we need to check that we are level 1 to be able to do
> promotion but we'll discover what level we are only after we can talk to
> the master.
>
> Should we assume level 1 if someone try to run ipa-replica-install on a
> client that is already joined ?
> I guess this can be done, and then we can fail later if it turns out the
> domain is not at level 1 just yet, what do you think ?
> Should I consider it enough that /et/cipa/defualt.conf is present to
> assume --promote ?

I think it should be good enough to assume promotion if replica file is 
not provided, and check domain level later when we can connect to the 
remote server:

     if replica file provided:
         # assume domain level == 0
         # we can connect to the remote server using info from the 
replica file
         connect to the remote server
         if domain level != 0:
             fail
         install replica using the replica file
     else:
         # assume domain level > 0
         if client not installed:
             install client
         # we can connect to the remote server when client is installed
         connect to remote server
         if domain level == 0:
             if client installed by us:
                 uninstall client
             fail
         install replica using promotion

>
>>> 5) There are some PEP8 transgressions:
>>>
>>> ./ipaserver/install/cainstance.py:264:35: E231 missing whitespace after ','
>>> ./ipaserver/install/cainstance.py:264:49: E231 missing whitespace after ','
>>> ./ipaserver/install/cainstance.py:264:63: E231 missing whitespace after ','
>>> ./ipaserver/install/cainstance.py:267:49: E203 whitespace before ','
>>> ./ipaserver/install/dsinstance.py:258:80: E501 line too long (88 > 79
>>> characters)
>>> ./ipaserver/install/dsinstance.py:317:80: E501 line too long (90 > 79
>>> characters)
>>> ./ipaserver/install/dsinstance.py:457:5: E303 too many blank lines (2)
>>> ./ipaserver/install/installutils.py:1115:1: E302 expected 2 blank lines,
>>> found 1
>>> ./ipaserver/install/krbinstance.py:185:80: E501 line too long (85 > 79
>>> characters)
>>> ./ipaserver/install/krbinstance.py:186:80: E501 line too long (85 > 79
>>> characters)
>>> ./ipaserver/install/krbinstance.py:191:80: E501 line too long (92 > 79
>>> characters)
>>> ./ipaserver/install/server/replicainstall.py:412:1: E302 expected 2
>>> blank lines, found 1
>>> ./ipaserver/install/server/replicainstall.py:862:25: E128 continuation
>>> line under-indented for visual indent
>>> ./ipaserver/install/server/replicainstall.py:864:25: E128 continuation
>>> line under-indented for visual indent
>>> ./ipaserver/install/server/replicainstall.py:865:25: E128 continuation
>>> line under-indented for visual indent
>>> ./ipaserver/install/server/replicainstall.py:1014:13: E128 continuation
>>> line under-indented for visual indent
>>> ./ipaserver/install/server/replicainstall.py:1044:42: E231 missing
>>> whitespace after ','
>>> ./ipaserver/install/server/replicainstall.py:1125:5: E303 too many blank
>>> lines (2)
>>>
>>>
>>> "Change DNS installer code to use passed in api": LGTM
>>>
>>>
>>> "Allow ipa-replica-conncheck to use default creds": LGTM
>>>
>>>
>>> "Add function to extract CA certs for install": LGTM
>>>
>>>
>>> "Allow to setup the CA when promoting a replica":
>>>
>>> 1) There are some PEP8 transgressions:
>>>
>>> ./ipaserver/install/ca.py:35:13: E125 continuation line with same indent
>>> as next logical line
>>> ./ipaserver/install/cainstance.py:1488:5: E303 too many blank lines (2)
>>> ./ipaserver/install/replication.py:421:24: E231 missing whitespace after ','
>>> ./ipaserver/install/replication.py:421:35: E231 missing whitespace after ','
>>> ./ipaserver/install/replication.py:421:41: E231 missing whitespace after ','
>>> ./ipaserver/install/replication.py:422:24: E231 missing whitespace after ','
>>> ./ipaserver/install/replication.py:422:40: E231 missing whitespace after ','
>>> ./ipaserver/install/replication.py:422:46: E231 missing whitespace after ','
>>> ./ipaserver/install/replication.py:524:37: E128 continuation line
>>> under-indented for visual indent
>>> ./ipaserver/install/replication.py:524:38: E201 whitespace after '['
>>> ./ipaserver/install/replication.py:524:80: E501 line too long (187 > 79
>>> characters)
>>> ./ipaserver/install/replication.py:526:80: E501 line too long (106 > 79
>>> characters)
>>> ./ipaserver/install/replication.py:560:80: E501 line too long (86 > 79
>>> characters)
>>> ./ipaserver/install/replication.py:847:80: E501 line too long (81 > 79
>>> characters)
>>> ./ipaserver/install/replication.py:850:80: E501 line too long (89 > 79
>>> characters)
>>> ./ipaserver/install/replication.py:1761:80: E501 line too long (81 > 79
>>> characters)
>>> ./ipaserver/install/replication.py:1766:17: E128 continuation line
>>> under-indented for visual indent
>>> ./ipaserver/install/replication.py:1807:80: E501 line too long (89 > 79
>>> characters)
>>>
>>>
>>> "Make checks for existing credentials reusable":
>>>
>>> 1) There are some PEP8 transgressions:
>>>
>>> ./ipaserver/install/installutils.py:1140:1: E302 expected 2 blank lines,
>>> found 1
>>> ./ipaserver/install/installutils.py:1192:25: E128 continuation line
>>> under-indented for visual indent
>>> ./ipaserver/install/installutils.py:1194:25: E128 continuation line
>>> under-indented for visual indent
>>> ./ipaserver/install/installutils.py:1195:25: E128 continuation line
>>> under-indented for visual indent
>>>
>>>
>>> "Allow ipa-ca-install to use the new promotion code":
>>>
>>> 1) The --replica option is redundant. You can safely decide whether this
>>> is the first CA master or not based on information in cn=masters.
>>>
>>> 2) There are some PEP8 transgressions:
>>>
>>> ./ipaserver/install/dsinstance.py:173:26: E231 missing whitespace after ','
>>> ./ipaserver/install/dsinstance.py:173:40: E231 missing whitespace after ','
>>>
>>>
>>> "Remove unused kra option": ACK
>>>
>>>
>>> "Allow to install the KRA on a promoted server":
>>>
>>> 1) There are some PEP8 transgressions:
>>>
>>> ./ipaserver/install/ipa_kra_install.py:198:33: E127 continuation line
>>> over-indented for visual indent
>>> ./ipaserver/install/krainstance.py:55:1: E302 expected 2 blank lines,
>>> found 1
>>> ./ipaserver/install/krainstance.py:382:13: E128 continuation line
>>> under-indented for visual indent
>>> ./ipaserver/install/server/replicainstall.py:1073:18: E225 missing
>>> whitespace around operator
>>> ./ipaserver/install/server/replicainstall.py:1081:5: E303 too many blank
>>> lines (2)
>>> ./ipaserver/install/service.py:103:1: E302 expected 2 blank lines, found 1
>>> ./ipaserver/install/service.py:115:52: E203 whitespace before ','
>>>
>
> I also pep8ed every single patch.
> "git show -U0 | pep8 --diff" is awesome :)

Thanks. That's exactly what I used :-)

>
>>>
>>> Pushed first 2 commits to master: d8b1f42f171d666068583548315b4684e5f3c3a4
>>
>> Ok, I'll rework the patches as possible, do you want a new patchset sent
>> to the list ? Or is it ok to rebnase the custodia-review branch in my
>> freeipa.git tree ?
>
> For now I simply updated my custodia-review branch, let me know if you
> want a patch-bomb to the list too.

Git is fine.

>
> Simo.
>

-- 
Jan Cholasta