New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Configure Anonymous PKINIT on server install #62
Conversation
|
Note, I haven't looked into the upgrade of an existing server, so just posting it here for an initial review, and also for someone to pick it up if I can't finish the work on the upgrade path. @abbra @frasertweedale please take a look |
0fdf136
to
973fe14
Compare
|
Thanks. Looks good. I'll work on upgrade next week and will do actual testing. |
|
Regarding requesting certificate for krbtgt, we plan to fix cert-request in a more systematic manner to allow requesting certificate for any principal in IPA realm (see https://fedorahosted.org/freeipa/ticket/6295) so hopefully the cert-request fixes would not be needed eventually. As a side question is the separate profile needed due to some custom extensions required for PKINIT certificate? |
|
|
I thought so, it would be nice to have this mentioned somewhere, e.g. in profile description so that the future selves will know why this is needed. |
|
Yes, we need to create a design page for PKINIT support. I'll make sure it is done. |
|
Thank you |
|
For those running 4.4.2, is there a workaround to enable ANONYMOUS support? |
|
@splashx you would have to manually configure each KDC and give them certs, it is doable. |
|
@simo5 done, however not successfully. It's not really my first time on the pkinit rodeo, so I'm wondering if FreeIPA's got something on top. I've got one freeipa instance for testing purposes, so not fussing with several servers. For debug purposes, I have done: /etc/kdc.conf The anonymous user (created manually first with I made sure the certificate's common name matches the fqdn, but still getting on the client side: Any thoughts would be helpful. Thanks in advance |
|
@splashx we are starting to pollute this PR here now. Please provide KDC logs on the user's mailing list and let's proceed there. |
|
@simo5 I did a rebase a while a go and maintain it rebased against the master. I'll submit a new PR with the rebase. |
|
@simo5 https://github.com/abbra/freeipa/tree/kdc-pkinit can be used for rebase of this PR |
|
Should I push your branch over my PR ? |
|
Up to you. We can either resync yours or switch over to mine. I need to merge updater changes too before submitting it upstream, though. |
|
On Fri, 2016-12-02 at 02:18 -0800, Alexander Bokovoy wrote:
Up to you. We can either resync yours or switch over to mine. I need to merge updater changes too before submitting it upstream, though.
I'll rebase for now, when it is time for the additional work, we can
decide if we are better opening a new PR or continue with this one.
SImo.
…--
Simo Sorce * Red Hat, Inc * New York
|
f627124
to
d0139ed
Compare
|
Rebased on latest master |
|
Rebasing this code is becoming a little difficult, @frasertweedale can you take a look and confirm the changes in cert.py are ok ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few small comments on the patch,
| @@ -521,6 +521,11 @@ def install_check(installer): | |||
| dirsrv_pkcs12_info = (dirsrv_pkcs12_file.name, dirsrv_pin) | |||
|
|
|||
| if options.pkinit_cert_files: | |||
| if not options.no_pkinit: | |||
| sys.exit("Cannot create KDC PKINIT certificate and use provided " | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please raise ScriptError here instead of hard exit. This is currently advised practice in installers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| # Restart krb after configurations have been changed | ||
| # Restart ds and krb after configurations have been changed | ||
| service.print_msg("Restarting the directory server") | ||
| ds.restart() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the DS restart necessary here? IIRC it is restarted at the end of update phase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, I tink this came from a change @abbra made to forward port the patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at it I think it should probably be a krb.restart(), but I did not have any issue installing, so perhaps it can simply go now, I'll wait for @abbra to comment if he recalls anything around this part.
| @@ -216,6 +217,16 @@ def caacl_check(principal_type, principal, ca, profile_id): | |||
| ) | |||
|
|
|||
|
|
|||
| def ca_kdc_check(ldap, basedn, hostname): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may simplify this check by several means: You can use server show/find hostname to check if it is really an IPA master, or you can call config_show and then check if the hostname is listed among IPA masters (hostname in result['ipa_master_server']. The latter is probably safer for your use-case as it queries the presence of KDC/HTTP/etc entries directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| @@ -793,6 +838,9 @@ def execute(self, csr, all=False, raw=False, **kw): | |||
| api.Command['host_mod'](principal.hostname, **kwargs) | |||
| elif principal_type == USER: | |||
| api.Command['user_mod'](principal.username, **kwargs) | |||
| elif principal_type == KRBTGT: | |||
| root_logger.error("Profiles used to store cert should't be " | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use self.log.error here, according to logging best practices root_logger should never be used directly to emit messages so let's not spread the bad practices further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tried root_logger was the only way to use it IIRC.
I do not think this calss has a self.log
|
I have a few small comments on this PR, nothing serious. |
27e72f6
to
7bab75c
Compare
|
@martbab your concerns should be addressed in this revision |
|
@simo5 I tried to run the branch as an upgrade against Fedora 25 version (4.4.2-1.fc25) and it failed at first because I was running in SELinux enforcing: Re-running TypeError: must be str, unicode, tuple, Name, RDN or DN, got <type 'NoneType'> instead |
| @@ -215,6 +216,11 @@ def caacl_check(principal_type, principal, ca, profile_id): | |||
| ) | |||
| ) | |||
|
|
|||
| def ca_kdc_check(ldap, basedn, hostname): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You re-wrote the implementation but forgot to remove basedn param from signature, that's why you get pylint errors about no value for argument.
|
@simo5 I highlighted the code givin pylint issues, basically you forgot to update ca_kdc_check signature. |
|
@martbab sometimes you are blind to your own code ... |
|
@abbra I have an idea of what it might be |
641691c
to
13caff8
Compare
All requeted changes should have been addressed
5b28776
to
eba8fa4
Compare
|
Thanks @simo5. Except SELinux changes this PR is ready to be accepted.
Also, to document the decisions we made when moving forward with this PR, anonymous PKINIT principal is created in all configurations. Its use for non-PKINIT case will be detailed in the privilege separation patchset:
This allows us to fully utilize Anonymous Kerberos principal potential. ACK. |
| princ_realm = krb.get_anonymous_principal_name() | ||
| dn = DN(('krbprincipalname', princ_realm), krb.get_realm_suffix()) | ||
| try: | ||
| _ = api.Backend.ldap2.get_entry(dn) # pylint: disable=unused-variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that instead of disabling the check, you could have renamed the variable to _entry - the unused variable regex is _.+ in IPA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that one does not even need to assign the result to a variable if it just checks for the entry's existence.
Allow anonymous pkinit to be used so that unenrolled hosts can perform FAST authentication (necessary for 2FA for example) using an anonymous krbtgt obtained via Pkinit. https://fedorahosted.org/freeipa/ticket/5678 Signed-off-by: Simo Sorce <simo@redhat.com>
|
Fixed upstream |
Allow anonymous pkinit to be used so that unenrolled hosts can perform FAST
authentication (necessary for 2FA for example) using an anonymous krbtgt
obtained via Pkinit.
Signed-off-by: Simo Sorce simo@redhat.com