Remove CSR from CS.cfg and store them in certs folder #4588

fmarco76 · 2023-10-12T17:37:38Z

CSR are created and stored in <instance_config>/certs folder as <certs_id>.csr files.

edewata

Please see my comments below.

base/server/upgrade/11.5.0/04-RemoveCertCSRfromConfig.py

edewata · 2023-10-12T18:21:20Z

docs/changes/v11.5.0/Server-Changes.adoc

+The parameters `<subsystem_name>.<cert_id>.cert` and `<subsystem_name>.<cert_id>.certreq` are removed from `CS.cfg` files.
+Certificates are retrieved from the nssdb configured and they are not stored in other places.
+CSR are stored in the folder `<instance_config>/certs` as `<cert_nickname>.csr` and they are retrieved from this location.
+For the cloning operation CSRs have to be specified in `pkispawn` configuration because they cannot be retrieved from the master configuration.


This means if someone has an automation for the cloning process (e.g. QE, IPA) this change will break it. I think initially we should continue supporting the existing mechanism (i.e. retrieving the CSRs from master) but we can generate a warning saying that we're deprecating this process and they will need to provide the CSRs to pkispawn. Then in the future we can drop the old code.

We probably should also consider providing a tool to export the CSRs more easily, e.g. creating a tarball containing all CSRs so the admin doesn't need to deal with multiple files.

You might also want to update the IPA clone test to verify that the <instance>/certs/<nickname>.csr files exist on the clones.

@edewata I miss this last comment. I'll add before merge or when working on the CSR healtcheck test.

edewata · 2023-10-12T18:23:56Z

base/server/python/pki/server/deployment/__init__.py

-        tags = subsystem.config['preop.cert.list'].split(',')
-        for tag in tags:
-            if tag == 'sslserver':
-                continue
-
-            # check CSR in CS.cfg
-            param = '%s.%s.certreq' % (subsystem.name, tag)
-            csr = subsystem.config.get(param)
-
-            if csr:
-                # CSR already exists
-                continue
-
-            # CSR doesn't exist, import from master
-            names.append(param)
-
        if subsystem.name == 'ca':


I think this code needs to be restored, but instead of checking the <subsystem>.<tag>.certreq we should check the <instance>/certs/<nickname>.csr file. If the file exists we can skip it, but otherwise we need to import the CSR from the master.

On the master side, we'll need to check if the requested param equals <subsystem>.<tag>.certreq we'll need to get the value from the CSR file:
https://github.com/dogtagpki/pki/blob/master/base/server/src/main/java/com/netscape/cms/servlet/csadmin/GetConfigEntries.java#L156

I cannot verify what is present in the folder. It is not sure if it will be created but maybe I can check if they are provided. I'll do some tests.

.github/workflows/ca-clone-hsm-test.yml

edewata

Thanks for the updates!

Please see my comments. I think we should fix the folder & file ownership, but the rest is just minor suggestions. If you prefer to fix the folder/file ownership in a separate PR that's fine too since we still have time before the release, so feel free to update/merge.

base/server/python/pki/server/deployment/__init__.py

base/server/src/main/java/com/netscape/cms/servlet/csadmin/GetConfigEntries.java

edewata

Thanks for the update! I added a few more suggestions, feel free to update/merge.

base/server/python/pki/server/subsystem.py

base/server/python/pki/server/deployment/__init__.py

base/server/upgrade/11.5.0/04-RemoveCertCSRfromConfig.py

CSR are created and stored in `<instance_config>/certs` folder as `<certs_id>.csr` files. Fix dogtagpki#2111

Add a check of CSR between CA master and clone.

edewata · 2023-10-16T17:48:06Z

Thanks for the update! Sorry, I just realized something else. It looks like the CSR files are sometimes called <nickname>.csr and sometimes <cert ID>.csr. In the CI test it is fine since we use the same values, but in real deployments they could be different. I think it should be <cert ID>.csr so they will be consistent regardless of the actual nicknames.

fmarco76 · 2023-10-16T18:00:22Z

Thanks for the update! Sorry, I just realized something else. It looks like the CSR files are sometimes called <nickname>.csr and sometimes <cert ID>.csr. In the CI test it is fine since we use the same values, but in real deployments they could be different. I think it should be <cert ID>.csr so they will be consistent regardless of the actual nicknames.

I think CSRs are always called with <nickname>.csr. Initially, there were some cases different but it has been fixed. In the CI I did for IPA I get the nickname from the configuration so I do not guess the file name.

I used the nickname because the same cert_id is used among different subsystems. As an example, the audit_signing is defined for KRA and CA. The nickname should be unique, unless they use different token.
If different subsystem must the same certificates if <cert_id> is the same than we can use it.
I think this is last point to take care and then we can merge.

edewata · 2023-10-16T19:45:42Z

Sorry, I was thinking about cert IDs returned by pki-server cert-find command (yeah, it's a bit confusing since there are several things called cert ID too). These cert IDs are unique for the whole instance (e.g. ca_audit_signing) except for sslserver and subsystem:
https://github.com/dogtagpki/pki/blob/master/base/server/python/pki/server/cli/cert.py#L175-L176

But let's merge this PR as is, then we can discuss further about this issue later. Thanks!

fmarco76 · 2023-10-17T13:57:29Z

@edewata I have modified the file name so now they are stored as <cert_id>.csr where cert_id has the value reported by the command pki-server cert-find command.

If all the test are OK I think we can merge. We have to define how the healcheck should work now.

sonarqubecloud · 2023-10-17T14:19:40Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication

edewata · 2023-10-17T15:08:53Z

Awesome! Thanks @fmarco76!

fmarco76 · 2023-10-17T15:29:03Z

@edewata Thanks!

fmarco76 requested a review from edewata October 12, 2023 17:37

edewata reviewed Oct 12, 2023

View reviewed changes

fmarco76 force-pushed the ConfigurationFreeCSR branch from 28f4894 to c001e0f Compare October 13, 2023 08:27

fmarco76 marked this pull request as draft October 13, 2023 10:48

fmarco76 force-pushed the ConfigurationFreeCSR branch 7 times, most recently from bb2698e to 15f9fda Compare October 13, 2023 19:05

fmarco76 marked this pull request as ready for review October 13, 2023 19:06

edewata approved these changes Oct 13, 2023

View reviewed changes

fmarco76 force-pushed the ConfigurationFreeCSR branch 3 times, most recently from 908b8df to 436f01b Compare October 16, 2023 10:50

edewata approved these changes Oct 16, 2023

View reviewed changes

fmarco76 added 2 commits October 16, 2023 16:45

Remove CSR from CS.cfg and store them in certs folder

655ce29

CSR are created and stored in `<instance_config>/certs` folder as `<certs_id>.csr` files. Fix dogtagpki#2111

Add CSR job in IPA cline test

ce97033

Add a check of CSR between CA master and clone.

fmarco76 force-pushed the ConfigurationFreeCSR branch from d4f81ec to ce97033 Compare October 16, 2023 16:17

fmarco76 force-pushed the ConfigurationFreeCSR branch 4 times, most recently from a4e394c to 3e6614a Compare October 17, 2023 13:26

Concert CSR file name to cert_id

cc58713

fmarco76 force-pushed the ConfigurationFreeCSR branch from 3e6614a to cc58713 Compare October 17, 2023 13:54

fmarco76 merged commit 61c3c27 into dogtagpki:master Oct 17, 2023
143 of 144 checks passed

fmarco76 deleted the ConfigurationFreeCSR branch October 17, 2023 16:25

fmarco76 mentioned this pull request Oct 18, 2023

Enable cert healthcheck #4590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove CSR from CS.cfg and store them in certs folder #4588

Remove CSR from CS.cfg and store them in certs folder #4588

fmarco76 commented Oct 12, 2023

edewata left a comment

edewata Oct 12, 2023

edewata Oct 12, 2023

fmarco76 Oct 13, 2023

edewata Oct 12, 2023

edewata Oct 12, 2023

fmarco76 Oct 13, 2023

edewata left a comment

edewata left a comment

edewata commented Oct 16, 2023

fmarco76 commented Oct 16, 2023

edewata commented Oct 16, 2023

fmarco76 commented Oct 17, 2023

sonarqubecloud bot commented Oct 17, 2023

edewata commented Oct 17, 2023

fmarco76 commented Oct 17, 2023

Remove CSR from CS.cfg and store them in certs folder #4588

Remove CSR from CS.cfg and store them in certs folder #4588

Conversation

fmarco76 commented Oct 12, 2023

edewata left a comment

Choose a reason for hiding this comment

edewata Oct 12, 2023

Choose a reason for hiding this comment

edewata Oct 12, 2023

Choose a reason for hiding this comment

fmarco76 Oct 13, 2023

Choose a reason for hiding this comment

edewata Oct 12, 2023

Choose a reason for hiding this comment

edewata Oct 12, 2023

Choose a reason for hiding this comment

fmarco76 Oct 13, 2023

Choose a reason for hiding this comment

edewata left a comment

Choose a reason for hiding this comment

edewata left a comment

Choose a reason for hiding this comment

edewata commented Oct 16, 2023

fmarco76 commented Oct 16, 2023

edewata commented Oct 16, 2023

fmarco76 commented Oct 17, 2023

sonarqubecloud bot commented Oct 17, 2023

edewata commented Oct 17, 2023

fmarco76 commented Oct 17, 2023