Implement support for HSM for CA private key storage #7238
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rcritten
added
needs review
|
@rcritten PKI hasn't released 11.5.0 yet in fedora but there are builds in the copr repo @pki/master. Testing in progress at freeipa-pr-ci2#3415 |
|
I suppose I can drop the pki requires temporarily. pki-master copr is enabled for the testing. This will get it past the Azure failure. |
rcritten
force-pushed
the
hsm
branch
2 times, most recently
from
0eeecdc
to
fcc5bd4
Compare
mrizwan93
force-pushed
the
hsm
branch
2 times, most recently
from
50e6f43
to
9e922f8
Compare
mrizwan93
force-pushed
the
hsm
branch
2 times, most recently
from
92e1279
to
1d512ff
Compare
rcritten
force-pushed
the
hsm
branch
2 times, most recently
from
761e916
to
dc26451
Compare
flo-renaud
reviewed
Apr 26, 2024
There was a problem hiding this comment.
Hi @rcritten
please find my inline comments.
I am not completely done yet with the review (I would like to play a bit with the replica install and renewal scenarios, and check the tests) but I don't want to loose my comments in the mean time...
|
Additional comment: the token_password is written in clear in /var/log/ipaserver-install.log and ipaserver-kra-install.log |
flo-renaud
reviewed
Apr 26, 2024
| topology = 'star' | ||
|
|
||
| @classmethod | ||
| def install(cls, mh): |
There was a problem hiding this comment.
@mrizwan93
The install and uninstall methods are almost identical for all the test classes. It is possible to define a class BaseHSMTest(IntegrationTest) and define in this class install/uninstall, and inherit in the other test classes.
|
I added a chunk of code for replica installs which should make all the CA and/or KRA certificates visible in the softoken and fixes the different nickname for the IPA CA cert. |
|
I added a temporary patch which will fix the CI testing. But I don't think it should have failed since it should be using a softhsm2 token. I'll check the logs on the other side. |
rcritten
force-pushed
the
hsm
branch
3 times, most recently
from
a3e99ad
to
22f2c4e
Compare
This certificate should not be renewed this way. ipa-cacert-manage renew should be used. Signed-off-by: Rob Crittenden <rcritten@redhat.com>
A number of files that need to be managed by certmonger have unconfined_u:object_r:pki_common_t:s0. Signed-off-by: Rob Crittenden <rcritten@redhat.com>
This is simple, a port needs to be available to certmonger to communicate during renewals of CA subsystem certificats. Signed-off-by: Rob Crittenden <rcritten@redhat.com>
|
The base class changes passed CI. I sqaushed a bunch of WIP commits and hopefully didn't introduce any breaking changes in the process. I believe that if this passes then all comments have been addressed. |
flo-renaud
reviewed
May 13, 2024
There was a problem hiding this comment.
Hi @rcritten
Only 1 new comment in the test TestADTrustInstallWithDNS_KRA_ADTrust, all other comments were addressed.
flo-renaud
reviewed
May 14, 2024
| cls.token_name, cls.token_password = get_hsm_token(cls.master) | ||
| tasks.install_master( | ||
| cls.master, setup_dns=cls.master_with_dns, | ||
| setup_kra=cls.master_with_kra, |
There was a problem hiding this comment.
setup_adtrust=cls.master_with_ad is missing here
Use SoftHSM2 to install an IPA CA to store the keys in an HSM. Whenenver new keys are generated either in the initial install or if a KRA is installed then the token needs to be synced between all servers prior to installing a new CA or KRA. Fixes: https://pagure.io/freeipa/issue/9273 Signed-off-by: Mohammad Rizwan <myusuf@redhat.com>
Arguments were added to the configuration file to allow specifying the token option values. These needed to be included into the defaults as well. This should be merged into the tests prior to pushing. Signed-off-by: Rob Crittenden <rcritten@redhat.com>
On a non-HSM, non-renewal-server replica we look in LDAP for an updated certificate. If the certificates don't match then we have a new one and write it out. If they match the assumption is that it hasn't been renewed yet so go into CA_WORKING. The problem is that for networked HSMs the cert will already be visible in the database so certmonger will always be in CA_WORKING. In this case we can assume that if the certs are the same then that's just fine. Related: https://pagure.io/freeipa/issue/9273 Signed-off-by: Rob Crittenden <rcritten@redhat.com>
If the password wasn't provided by --token-password then an empty value would be passed into the CA installer which promptly failed. Related: https://pagure.io/freeipa/issue/9273 Signed-off-by: Rob Crittenden <rcritten@redhat.com>
Not all HSMs support PKCS#1 v1.5. The nShield nFast is one we know of so force the KRA to use OAEP in this case.. This can be seen in HSMs where the device doesn't support the PKCS#1 v1.5 mechanism. It will error out with either "invalid algorithm" or CKR_FUNCTION_FAILED. There is currently no good way to test for this capability in advance of configuration. Testing for mechanisms alone is insufficient. The only real way to test would be to attempt a wrap/unwrap but it is very complex. If the list of affected HSMs increases we can use a table instead based on "best guess" of some sort of property but looking for a unique string inside the library path is a pretty straigthforward way. Note that this doesn't preclude someone from wanting to require OAEP directly by modifying the KRA CS.cfg and it won't impact FIPs mode which requires OAEP. Related: https://pagure.io/freeipa/issue/9191 Signed-off-by: Rob Crittenden <rcritten@redhat.com>
If a certificate on a token does not have NSS trust set then it won't be visible in the softoken. This can be disconcerting for those used to seeing all the certificates. Loop through the possibilities and set no trust (or Peer) for all the certificates on the token. Also ensure that the CA certificate has the correct nickname. Related: https://pagure.io/freeipa/issue/9273 Signed-off-by: Rob Crittenden <rcritten@redhat.com>
* Switch to CA user when saving NSS certificates * Add new certs to internal token, try harder to remove on renewal * Don't restrict tokens to CKM_RSA_X_509 Related: https://pagure.io/freeipa/issue/9273 Signed-off-by: Rob Crittenden <rcritten@redhat.com>
Related: https://pagure.io/freeipa/issue/9273 Signed-off-by: Rob Crittenden <rcritten@redhat.com>
hsm_validator was validating that the token was available but not that the provided password worked. Add that capability. Also call it early in the CA and KRA installation cycle so that it errors out early. This is particularly important for the KRA because there is no uninstaller. Bump the minimum PKI release to 11.5.0 as that contains important fixes for the HSM. Remove an unused arguments to hsm_version and hsm_validator. Related: https://pagure.io/freeipa/issue/9273 Signed-off-by: Rob Crittenden <rcritten@redhat.com>
Don't blow up if the expected module is not installed but warn about it. Hopefully users will actually read the output and/or the installation log. This is done by looking for strings in the path. Not great but it's at least something. Related: https://pagure.io/freeipa/issue/9273 Signed-off-by: Rob Crittenden <rcritten@redhat.com>
Additional SELinux rules are necessary for the HSM to be managed by IPA and certmonger. Given the infinite possible naming combinations of library paths and modules this is a best effort. A message is logged if a missing module is detected. Related: https://pagure.io/freeipa/issue/9273 Signed-off-by: Rob Crittenden <rcritten@redhat.com>
|
Passing test link for future reference: |
|
@mrizwan93 thanks for the update, LGTM. You can remove the temp commit |
flo-renaud
added
ack
|
master:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement the design for HSM support.
This PR set adds options for HSM token, library path and token password to the CA/KRA-related installers.
The intention is to test this in CI using a SoftHSM2 token. This is not recommended for production since it lacks the Hardware part of HSM.
It has additionally been tested using an nCipher and Thales Luna HSM with CA installation.
The HSM code requires PKI 11.5.0+.
Non-HSM installations should not be affected.