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 <s...@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 <s...@redhat.com>
    Reviewed-By: Jan Cholasta <jchol...@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.

Simo.

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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to