RFC: implement local PKINIT deployment in server/replica install #694
Conversation
install/certmonger/ipa-pkinit-submit
Outdated
| capture_output=True) | ||
| rc = result.returncode | ||
|
|
||
| if six.PY2: |
There was a problem hiding this comment.
Why do you need raw output here?
install/certmonger/ipa-pkinit-submit
Outdated
|
|
||
| return handler() | ||
|
|
||
|
|
There was a problem hiding this comment.
if __name__ == '__main__': missing here?
install/certmonger/ipa-pkinit-submit
Outdated
|
|
||
| try: | ||
| sys.exit(main()) | ||
| except Exception as e: |
ipaserver/install/krbinstance.py
Outdated
| try: | ||
| root_logger.debug("Adding '%s' CA to certmonger", PKINIT_CA_NAME) | ||
| certmonger.add_ca(PKINIT_CA_NAME, paths.IPA_PKINIT_SUBMIT) | ||
| except ValueError: |
There was a problem hiding this comment.
This looks weird, a new exception should be used, like CAAlreadyConfiguredError
ipaserver/install/krbinstance.py
Outdated
| @@ -449,6 +484,9 @@ def enable_ssl(self): | |||
| self.step("testing anonymous PKINIT", self.test_anonymous_pkinit) | |||
|
|
|||
| self.start_creation() | |||
| else: | |||
There was a problem hiding this comment.
This is duplication of if branch, can we have it without if else statement
|
? |
|
@MartinBasti I haven't thought about CA-less -> CA-full but in this case you would have local PKINIT and should configure full PKINIT manually All the other scenarios should be covered by the incoming code. Regarding your comment on the certmonger helper/special CA, we (me and @HonzaCholasta ) decided to remove it and use a self-sign CA instead. |
|
I have re-worked the PR and implemented most of the missing steps (except for API for querying PKINIT status in topology). I have also removed the PKINIT-specific CA and helper. The installer will now call either |
| self.start_creation() | ||
| else: |
There was a problem hiding this comment.
why we need if else braches here if the code is almost the same
There was a problem hiding this comment.
That's a question of UX we may discuss further. by default we inform the user that PKINIT certificate is being requested and installed. When he passes in --no-pkinit we pretend to not configure it and only use the local self-sign path. I thought that in this case we shouldn't inform the user about this, otherwise we will have plenty complaints about an installer setting up undesired feature even when instructed not to.
There was a problem hiding this comment.
A comment in the code would be nice.
There was a problem hiding this comment.
ok I will add the comment.
| @@ -1525,6 +1525,10 @@ def install(installer): | |||
| # remove the extracted replica file | |||
| remove_replica_info_dir(installer) | |||
|
|
|||
| if not options.no_pkinit: | |||
There was a problem hiding this comment.
This looks weird, you always test it but only sometimes show text
There was a problem hiding this comment.
The same point as above, we may discuss it separately.
|
Should be anon keytab removed by upgrade, are there any leftovers in LDAP to be removed during upgrade? |
|
@MartinBasti I can add some removal logic to upgrader if required. |
ipaserver/install/krbinstance.py
Outdated
| """ | ||
| record PKINIT state in the master's KDC entry in LDAP. The state is | ||
| recorded in ipaConfigString attribute as a 'pkinit_status:STATUS' where | ||
| STATUS is one of (local, external, full). |
There was a problem hiding this comment.
Why not just pkinitEnabled? There is no difference between external and full for clients, and local means it's effectively disabled.
There was a problem hiding this comment.
Ditto everywhere else, IMO the local-external-full tri-state is confusing, "local" is an implementation detail and does not tell the user anything useful, and "external" and "full" is just normal CA-less vs CA-full described in odd words.
There was a problem hiding this comment.
This was agreed on during design discussion http://www.freeipa.org/page/V4/Kerberos_PKINIT @simo5 wanted to have clear distinction between individual PKINIT configurations.
There was a problem hiding this comment.
@simo5 Although when I think about it, the external/full PKINIT distinction is indistinguishable from the client's POV so we can actually have binary disabled (local)/ enabled state without any harm even during upgrades, both external and full pkinit states mean that no further upgrade is required.
ipaserver/install/server/install.py
Outdated
| @@ -892,6 +892,10 @@ def install(installer): | |||
| # Make sure the files we crated in /var/run are recreated at startup | |||
| tasks.configure_tmpfiles() | |||
|
|
|||
| if not options.no_pkinit: | |||
| print("Testing anonymous PKINIT") | |||
| krb.test_anonymous_pkinit() | |||
There was a problem hiding this comment.
Why do we need to test anything? Are we not sure what are we doing?
There was a problem hiding this comment.
Testing PKINIT was added so that we fail early if the configuration is wrong. Otherwise you just get hard to debug issues such as login to WebUI failing suddenly. I can remove this if we are confident that the current code is robust enough.
ipaserver/install/server/upgrade.py
Outdated
|
|
||
| if api.Command.ca_is_enabled()['result']: | ||
| pkinit_setup_handler = krb.setup_full_pkinit |
There was a problem hiding this comment.
If full / external pkinit was not enabled before, we should not enable it during upgrade, otherwise --no-pkinit will work only until the first upgrade.
There was a problem hiding this comment.
that was agreed on with @abbra and @simo5 during designing the missing parts (see http://www.freeipa.org/page/V4/Kerberos_PKINIT). If you disagree we should discuss with them.
There was a problem hiding this comment.
When we designed this, the idea was that on --no-pkinit we would create an "external" self-signed certificate. This would be like local, but would prevent upgrades because it is not tracked by a CA certmonger can request certs from.
If "external" can't be used, then you have to track somewhere that the admin explicitly passed the --no-pkinit flag and prevent upgrades.
In general we do want autometic upgrades unless the admin explicitly and unequivocably set a --no-pkinit flag.
The other option is to completely ignore --no-pkinit and mark those flags as deprecated and document that, if an dmin doesn't want pkinit at all they will have to replace the pkinit certs with certs of their own that work only locally.
|
I read through the code and I believe it addresses all use cases we have been discussing. LGTM. |
|
@abbra I received an interactive review from @HonzaCholasta today and he is not very keen on idea of having ternary (absent/local/external/full) PKINIT configuration. He suggests to only have it absent/off (local implementation)/on and thus drop differentiation between PKINIT configured with IPA CA issued or 3rd party certificates. The main concern here is that the 'local' PKINIT configuration is actually an implementation detail we should not leak to clients, they should be only able to tell if it is configured for them or not. If you look into the design page, the two states (full/external) behave the same during replica installation and upgrade so the differentiation does not bring much new information to the users. So a simple on/off switch (something like pkinitStatus: off/on) could be enough and it could simplify the transition and UX. What do you think? |
|
I agree that it is internal detail whether we use local pkinit or not. However, we need to know that it is existing as oposed to not existing at all for older systems where we are going to perform upgrades. However, as you can derive this information by presence or lack of actual KDC certificate file in the file system during upgrade, this can be reduced, indeed. One more detail: we already have pkinit plugin ( Perhaps, you can remove this command and add instead |
|
We can query that PKINIT was not configured at all by a) checking the presence of KDC keypair, b) checking the sysupgrade (no presence of pkinit flag implies no configuration is present), and c) querying LDAP (no presence of ipaConfigString) so we have multiple redundant ways to determine that PKINIT is not configured at all. As for the removal of pkinit status, I intend to replace the existing command by I will then update the design page to reflect this discussion and update the implementation in this PR. |
|
Yep. Then this PR can be merged once you removed distinction external/full. |
|
I have rewritten the PKINIT state reporting code as agreed with @abbra and also re-factored the installation/upgrade logic. @HonzaCholasta also requested to remove the local PKINIT check completely and have a test suite for that. On the one hand I tend to agree, on the other I would keep the check there for now until the password authentication test is implemented. Then remove the checks once we have coverage for it. |
ipaserver/install/krbinstance.py
Outdated
| if pkinit_request_ca is None or pkinit_request_ca == "IPA": | ||
| return PKINIT_ENABLED | ||
| elif pkinit_request_ca == "SelfSign": | ||
| return PKINIT_DISABLED |
There was a problem hiding this comment.
Whether PKINIT is enabled or not is not a tri-state, it is a boolean. You are basically still exposing internal implementation detail to the user by making it a tri-state.
There was a problem hiding this comment.
@HonzaCholasta true, the selfsign/absent PKINIT information is actually required only locally during upgrade (that can be inferred from the presence of KDC certificate and its managing CA), server/replica install make decisions only based on provided options and state of topology.
It can thus be simplified further by reporting only 'pkinitEnabled' in LDAP and handling selfsign/absent only locally during upgrade. @simo5 is that ok with you?
There was a problem hiding this comment.
If it still conforms to the cases we identified with the table in the design document it is fine by me.
There was a problem hiding this comment.
@simo5 Yes it does, this information will just not be exposed to clients as it is our internal implementation detail.
There was a problem hiding this comment.
I am in favor of not over-exposing internal sausage-making.
| self._call_certmonger(certmonger_ca="SelfSign") | ||
| # for self-signed certificate, the certificate is its own CA, copy it | ||
| # as CA cert | ||
| shutil.copyfile(paths.KDC_CERT, paths.CACERT_PEM) |
There was a problem hiding this comment.
NACK, a self-signed EE cert is not a CA cert, please explicitly use paths.KDC_CERT as an achor in login_password.
There was a problem hiding this comment.
Then I will need to conditionally select between KDC_CERT and CACERT_PEM based on the PKINIT status, since the KDC_CERT is not a vaild anchor in full PKINIT scenario. This means that I will need to perform extra D-BUS call to certmonger for each login attempt but I think that won't be a huge issue from performance POV.
There was a problem hiding this comment.
@HonzaCholasta and to determine PKINIT status we need to either:
- check PKINIT status in LDAP, which we can not do from login_password because we do not have a working LDAP connection (as I found out now)
- Query certmonger's D-Bus interface about the tracking information of KDC certificate which ipaapi user is denied by the current DBus policy
So I am open to suggestions how to implement this in rpcserver layer.
There was a problem hiding this comment.
Oh wait, with your proposal I can just check for present of PKINIT's CA bundle and not do any complex magicks. I will look into that.
There was a problem hiding this comment.
After some offline discussion with @HonzaCholasta we concluded that there currently is no easy way to select KDC cert or IPA CA bundle base don PKINIT status. So we will roll with the original idea to copy KDC cert to IPA CA bundle to 'emulate' the correct anchor for now. We will think about a proper handling of this issue later.
ipaserver/install/krbinstance.py
Outdated
| def issue_ipa_ca_signed_pkinit_certs(self): | ||
| try: | ||
| self._call_certmonger() | ||
| shutil.copyfile(paths.IPA_CA_CRT, paths.CACERT_PEM) |
There was a problem hiding this comment.
paths.IPA_CA_CRT contains a set of CA certificates trusted for SSL servers, which may, but don't have to be the same as the set of CA certificates trusted for PKINIT. I'm not asking for an immediate fix, but at least a comment would be nice.
There was a problem hiding this comment.
OK I will add a comment here. Given this was the original behavior a fix will warant a separate PR.
|
@martbab, this sounds like a typical instance of a we will do it later = we will do it never situation. IMO we should remove the superfluous check right away, as that would give us more incentive to actually implement the test. |
|
@HonzaCholasta Then the best course of action is to remove the PKINIT check and raise the priority of the issue for test case. |
martbab
force-pushed
the
local-pkinit
branch
2 times, most recently
from
6164b7c
to
bf468e3
Compare
ipaserver/install/krbinstance.py
Outdated
| if os.path.exists(paths.KDC_CERT): | ||
| pkinit_request_ca = get_pkinit_request_ca() | ||
|
|
||
| if pkinit_request_ca is None or pkinit_request_ca == "IPA": |
There was a problem hiding this comment.
I would maybe go even further and say if pkinit_request_ca != 'SelfSign', I guess we should not care which CA is used as long as it isn't SelfSign.
There is some code duplication regarding setting ipaConfigString values when: * LDAP-enabling a service entry * advertising enabled KDCProxy in LDAP We can delegate the common work to a single re-usable function and thus expose it to future use-cases (like PKINIT advertising). https://pagure.io/freeipa/issue/6830
The PKINIT setup code now can configure PKINIT using IPA CA signed certificate, 3rd party certificate and local PKINIT with self-signed keypair. The local PKINIT is also selected as a fallback mechanism if the CSR is rejected by CA master or `--no-pkinit` is used. http://www.freeipa.org/page/V4/Kerberos_PKINIT https://pagure.io/freeipa/issue/6830
An API was provided to report whether PKINIT is enabled for clients or not. If yes, the pkinitEnabled value will be added to the ipaConfigString attribute of master's KDC entry. See http://www.freeipa.org/page/V4/Kerberos_PKINIT#Configuration for more details. https://pagure.io/freeipa/issue/6830
Since the anonymous principal can only use PKINIT to fetch credential cache it makes no sense to try and use its kerberos key to establish FAST channel. We should also be able to use custom PKINIT anchor for the armoring. https://pagure.io/freeipa/issue/6830
anonymous kinit using keytab never worked so we may safely remove all code that requests/uses it. https://pagure.io/freeipa/issue/6830
The upgrader has been modified to configure either local or full PKINIT depending on the CA status. Additionally, the new PKINIT configuration will be written to the master's KDC entry. https://pagure.io/freeipa/issue/6830 http://www.freeipa.org/page/V4/Kerberos_PKINIT
Local FAST armoring will now work regardless of PKINIT status so there is no need to explicitly test for working PKINIT. If there is, there should be a test case for that. https://pagure.io/freeipa/issue/6830
|
LGTM. |
|
Any volunteer to do a functional review? |
|
Works for me, ACK. |
HonzaCholasta
added
ack
|
master:
ipa-4-5:
|
This PR implements a basic local PKINIT functionality for server install with
'--no-pkinit' specified, and replica install against older masters or with
'--no-pkinit'.
These patches unblock WebUI logins/password auths on masters/replicas in the
cases proper PKINIT was not configured for whatever reasons.
Nevertheless, there are following things lacking in this PR that I will either
push on top of this one or create a new PR:
http://www.freeipa.org/page/V4/Kerberos_PKINIT