[Freeipa-devel] [PATCHSET] Replica promotion patches
Jan Cholasta
jcholast at redhat.com
Thu Oct 1 05:43:05 UTC 2015
On 30.9.2015 15:15, Simo Sorce wrote:
> On 30/09/15 08:32, Jan Cholasta wrote:
>> On 24.9.2015 15:10, Simo Sorce wrote:
>>> On 24/09/15 04:43, Martin Basti wrote:
>>>>
>>>>
>>>> On 09/24/2015 02:25 AM, Martin Basti wrote:
>>>>>
>>>>>
>>>>> On 09/22/2015 10:45 AM, 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
>>>>>>
>>>>>>
>>>>>> "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/.
>>>>>>
>>>>>> 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?
>>>>>>
>>>>>> 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)
>>>>>>
>>>>>> 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
>>>>>>
>>>>>>
>>>>>> "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.
>>>>>>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> 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.)
>>>>>>
>>>>>> 2) There is no domain level check when installing the replica from a
>>>>>> replica file.
>>>>>>
>>>>>> 3) Instead of returning INSTALL_ERROR from promote_check() (which has
>>>>>> no effect), raise an exception or call sys.exit().
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> 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 ','
>>>>>>
>>>>>>
>>>>>> Pushed first 2 commits to master:
>>>>>> d8b1f42f171d666068583548315b4684e5f3c3a4
>>>>>>
>>>>>>
>>>>>> Honza
>>>>>>
>>>>>
>>>>> Hello,
>>>>>
>>>>> we have broken ipa-kra-install in master.
>>>>> https://fedorahosted.org/freeipa/ticket/5321
>>>>>
>>>>> I did git bisect and it found:
>>>>>
>>>>> 953b1079cfc8d2da486ebb65b9cfd4d85f680c67 is the first bad commit
>>>>> commit 953b1079cfc8d2da486ebb65b9cfd4d85f680c67
>>>>> Author: Simo Sorce <simo at redhat.com>
>>>>> Date: Tue Aug 25 13:38:05 2015 -0400
>>>>>
>>>>> Remove unused arguments
>>>>>
>>>>> In the dogtag/ca/kra instances self.domain is never used.
>>>>> Remove it.
>>>>>
>>>>> Signed-off-by: Simo Sorce <simo at redhat.com>
>>>>> Reviewed-By: Jan Cholasta <jcholast at redhat.com>
>>>>>
>>>>> :040000 040000 d4ad87647b181b182a3a56bdac6c95c008ec2623
>>>>> 8728f38996eeaa387617880c29fdd8b4c1f21ad4 M ipaserver
>>>>>
>>>>>
>>>>> I do not see anything bad in the commit, so I maybe I did bisect
>>>>> wrong, I will investigate later. But it is caused by a recent change,
>>>>> because it worked from master branch on moday.
>>>>>
>>>>> Martin^2
>>>>>
>>>> I confirm. I reverted this commit in my branch, and ipa-kra-install
>>>> works again. So unused variable is really used somewhere, but I cannot
>>>> find where.
>>>
>>> I think the problem is that the patch was pushed prematurely.
>>> The option should become unused once the other patches in this patchset
>>> are applied, that is why that patch was not on top of the list but
>>> rather down close to the bottom.
>>
>> The patch *was* on the top of the list, it was literally the first patch
>> in the patchset.
>>
>> Anyway, found the cause, see the attached patch for a fix.
>
> ACK
Pushed to master: c388dbd4de36442773079a2c6e52547b4b60b993
--
Jan Cholasta
More information about the Freeipa-devel
mailing list