[Freeipa-devel] [PATCHSET] Replica promotion patches

[Simo Sorce]

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.
> 
> > 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.

> > 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.
> 
> > 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.

> > 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.


> > 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.

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 ?

> > 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 :)

> > 
> > 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.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York