Improve the implementation of PKINIT certificate retrieval #584

martbab · 2017-03-14T15:32:12Z

The original PKINIT cert request code contained numerous defects, namely:

nearly absent handling of rejected requests and CA errors which resulted
e.g. in an unusable WebUI after replica installation
and
certificate request logic that was not consistent with the rest of the
installers (DS, HTTP for example): what caused hard errors in their case
went unnoticed in PKINIT setup

This PR consolidates this code so that errors arising from CA rejecting the
PKINIT cert request cause the installers to abort immediately. The PKINIT step
was also split into a separate method executed before LDAP updates. The name
was chosen to be enable_ssl in order to make the planned refactoring of
certificate requesting code (https://pagure.io/freeipa/issue/6429) easier: the
method name is not accurate but at least it is consistent with e.g. LDAP
installer co the common code can be grepper with greater ease.

https://pagure.io/freeipa/issue/6739

abbra

I left some comments inline.

abbra · 2017-03-14T15:40:15Z

install/share/kdc.conf.template

@@ -12,6 +12,6 @@
  dict_file = $DICT_WORDS
  default_principal_flags = +preauth
 ;  admin_keytab = $KRB5KDC_KADM5_KEYTAB
-  pkinit_identity = FILE:$KDC_CERT,$KDC_KEY
-  pkinit_anchors = FILE:$CACERT_PEM
+$NOPK  pkinit_identity = FILE:$KDC_CERT,$KDC_KEY


This is not really needed. krb5kdc will complain that pkinit_identity is not readable and PKINIT cannot be configured but the rest will continue working. We have pkinit_identity and pkinit_anchors in install/share/kdc.conf.template since 2010, they contained wrong values and missing certs, and KDC did work fine.

abbra · 2017-03-14T15:40:49Z

install/share/kdc.conf.template

-  pkinit_identity = FILE:$KDC_CERT,$KDC_KEY
-  pkinit_anchors = FILE:$CACERT_PEM
+$NOPK  pkinit_identity = FILE:$KDC_CERT,$KDC_KEY
+$NOPK  pkinit_anchors = FILE:$CACERT_PEM


Same here. Do not comment out this configuration option.

abbra · 2017-03-14T15:41:15Z

ipaserver/install/krbinstance.py

@@ -221,6 +220,7 @@ def __setup_sub_dict(self):
                             KRB5KDC_KADM5_ACL=paths.KRB5KDC_KADM5_ACL,
                             DICT_WORDS=paths.DICT_WORDS,
                             KRB5KDC_KADM5_KEYTAB=paths.KRB5KDC_KADM5_KEYTAB,
+                             NOPK=';',


no need for the comment out.

abbra · 2017-03-14T15:41:44Z

ipaserver/install/krbinstance.py

@@ -393,6 +394,15 @@ def setup_pkinit(self):
        # have any selinux issues with the file context
        shutil.copyfile(paths.IPA_CA_CRT, paths.CACERT_PEM)

+        # Now modify configuration to add pkinit anchors and restart KDC
+        self.sub_dict['NOPK'] = ''


no need to update kdc.conf here. Just do the restart.

abbra · 2017-03-14T15:42:44Z

ipaserver/install/krbinstance.py

+        except Exception:
+            root_logger.critical("krb5kdc service failed to restart")
+            raise
+


I also think we should actually check that anonymous PKINIT works with new configuration -- since krb5kdc will start just fine with non-working PKINIT, you really need to try to 'kinit -n' to see if anonymous pkinit works.

That sound like a really good idea that can prevent regressions like the one reported in the ticket to happen. I will add the anon pkinit check and fail with hard error if it does not work.

MartinBasti · 2017-03-14T17:35:01Z

Since I applied this PR, I cannot pass through failing client side installation:

Forwarding 'ping' to json server 'https://vm-024.abc.idm.lab.eng.brq.redhat.com/ipa/json'
Cannot connect to the server due to Kerberos error: No valid Negotiate header in server response. Trying with delegate=True
trying https://vm-024.abc.idm.lab.eng.brq.redhat.com/ipa/json
Forwarding 'ping' to json server 'https://vm-024.abc.idm.lab.eng.brq.redhat.com/ipa/json'
Second connect with delegate=True also failed: No valid Negotiate header in server response
Installation failed. As this is IPA server, changes will not be rolled back.
Cannot connect to the IPA server RPC interface: No valid Negotiate header in server response
The ipa-client-install command failed. See /var/log/ipaclient-install.log for more information
ipa.ipapython.install.cli.install_tool(CompatServerMasterInstall): ERROR    Configuration of client side components failed!
ipa.ipapython.install.cli.install_tool(CompatServerMasterInstall): ERROR    The ipa-server-install command failed. See /var/log/ipaserver-install.log for more information

martbab · 2017-03-15T06:32:56Z

@simo5 are you OK with @abbra's inline suggestions (it is your commit after all :))?
@MartinBasti hmmm I will try to reproduce the issue with fresh VMs.

The certmonger request handling code during pkinit setup actually never correctly handled situations when certificate request was rejected by the CA or CA was unreachable. This led to subtle errors caused by broken anonymous pkinit (e.g. failing WebUI logins) which are hard to debug. The code should behave as other service installers, e. g. use `request_and_wait_for_cert` method which raises hard error when request times out or is not granted by CA. On master contact Dogtag CA endpoint directly as is done in DS installation. https://pagure.io/freeipa/issue/6739

On the first master the framework may not be fully functional to server certificate requests. It is safer to configure helper that contacts Dogtag REST API directly. https://pagure.io/freeipa/issue/6739

This is to ensure that we can request PKINIT certs once all the following requirements are in place: * CA is configured or PKCS#12 file is provided * LDAP, KDC and Apache are configured and the master role is thus completed and enabled https://pagure.io/freeipa/issue/6739

Instead of only logging errors when timeout is reached or query for the entry fails for other reasons, `wait_for_entry` should raise exceptions so that we can handle them in caller or let them propagate and fail early. https://pagure.io/freeipa/issue/6739

https://pagure.io/freeipa/issue/6739

This prevents replication-based race conditions to break PKINIT certificate requests on replica installation. https://pagure.io/freeipa/issue/6739

After PKINIT certificate is requested and everything is set up, we should attempt to perform anonymous PKINIT and fail hard if it does not work for some reason. https://pagure.io/freeipa/issue/6739

martbab · 2017-03-15T14:46:21Z

I have reworked the PR quite a bit and added/changed a few checks due to replication race conditions affecting PKINIT requests from replica to master.

MartinBasti · 2017-03-15T15:38:00Z

Works for me and code looks OK

MartinBasti · 2017-03-15T15:40:15Z

master:

95768de Make PKINIT certificate request logic consistent with other installers
b5b23e0 Request PKINIT cert directly from Dogtag API on first master
bd18b5f Move PKINIT configuration to a later stage of server/replica install
0699484 Make wait_for_entry raise exceptions
8f4abf7 check that the master requesting PKINIT cert has KDC enabled
b45629f check for replica's KDC entry on master before requesting PKINIT cert
a1686a9 Try out anonymous PKINIT after it is configured

abbra requested changes Mar 14, 2017

View reviewed changes

Martin Babinsky added 7 commits March 15, 2017 09:45

Request PKINIT cert directly from Dogtag API on first master

20c2797

On the first master the framework may not be fully functional to server certificate requests. It is safer to configure helper that contacts Dogtag REST API directly. https://pagure.io/freeipa/issue/6739

check that the master requesting PKINIT cert has KDC enabled

2748d2c

https://pagure.io/freeipa/issue/6739

check for replica's KDC entry on master before requesting PKINIT cert

824ccd3

This prevents replication-based race conditions to break PKINIT certificate requests on replica installation. https://pagure.io/freeipa/issue/6739

Try out anonymous PKINIT after it is configured

6a86ff2

After PKINIT certificate is requested and everything is set up, we should attempt to perform anonymous PKINIT and fail hard if it does not work for some reason. https://pagure.io/freeipa/issue/6739

martbab force-pushed the kdc-pkinit-fixes branch from b129c93 to 6a86ff2 Compare March 15, 2017 14:45

martbab mentioned this pull request Mar 15, 2017

Configure KDC to use certs after they are deployed #567

Closed

MartinBasti added the ack Pull Request approved, can be merged label Mar 15, 2017

MartinBasti added the pushed Pull Request has already been pushed label Mar 15, 2017

MartinBasti closed this Mar 15, 2017

martbab deleted the kdc-pkinit-fixes branch March 15, 2017 16:36

Improve the implementation of PKINIT certificate retrieval #584

Improve the implementation of PKINIT certificate retrieval #584

martbab commented Mar 14, 2017 •

edited by MartinBasti

abbra left a comment

abbra Mar 14, 2017

abbra Mar 14, 2017

abbra Mar 14, 2017

abbra Mar 14, 2017

abbra Mar 14, 2017

martbab Mar 15, 2017

MartinBasti commented Mar 14, 2017

martbab commented Mar 15, 2017

martbab commented Mar 15, 2017

MartinBasti commented Mar 15, 2017

MartinBasti commented Mar 15, 2017

Improve the implementation of PKINIT certificate retrieval #584

Improve the implementation of PKINIT certificate retrieval #584

Conversation

martbab commented Mar 14, 2017 • edited by MartinBasti

abbra left a comment

Choose a reason for hiding this comment

abbra Mar 14, 2017

Choose a reason for hiding this comment

abbra Mar 14, 2017

Choose a reason for hiding this comment

abbra Mar 14, 2017

Choose a reason for hiding this comment

abbra Mar 14, 2017

Choose a reason for hiding this comment

abbra Mar 14, 2017

Choose a reason for hiding this comment

martbab Mar 15, 2017

Choose a reason for hiding this comment

MartinBasti commented Mar 14, 2017

martbab commented Mar 15, 2017

martbab commented Mar 15, 2017

MartinBasti commented Mar 15, 2017

MartinBasti commented Mar 15, 2017

martbab commented Mar 14, 2017 •

edited by MartinBasti