Improvement to client function certimportdir(v9) #775

Stueypoo · 2025-01-17T03:29:50Z

Describe your changes

Added an option to use an alternate CA certificate.

When importing certificates into a newly migrated CA, the code performs a certificate verification based upon the CA's current certificate. A problem with this occurs when the certificates being imported had been issued from one of the CA's earlier certificates (ie., the CA was renewed but active end-enty certificates still need to be imported).

To remedy this, a new "--cacert" option allows the Operator to provide the CA's previous certificate, and this CA certificate will then be used to verify the certificates being imported.

How has this been tested?

A systemtest has been ammended to test this change:
ant test:runone -Dtest.runone=CaImportCertDirCommandSystemTest

Checklist before requesting a review

[X ] I have performed a self-review of my code
[X ] I have kept the patch limited to only change the parts related to the patch
This change requires a documentation update

NOTE: This pull request is to replace #711

Added ability to provide an alternate CA certificate (for renewed CAs) and allow revocation details to be derived from the filename.

Added additional tests for changes to the associated client function.

primetomas · 2025-04-08T06:24:14Z

I'm sorry, but this fell between the chairs due to travels. If you rebase this one I promise to get it fixed for the next release.

…timportdir(v9)

Stueypoo · 2025-04-11T11:40:02Z

Synced to latest keyfactor:main.

primetomas · 2025-04-27T10:18:59Z

Internal ticket tracker: ECA-13369

primetomas · 2025-04-27T10:41:33Z

modules/ejbca-ejb-cli/src/org/ejbca/ui/cli/ca/CaImportCertDirCommand.java

+                            if ( sRevCode.equals("CESSATIONOFOPERATION")) sRevCode = "CESSATION_OF_OPERATION";
+                            if ( sRevCode.equals("CERTIFICATEHOLD")) sRevCode = "CERTIFICATE_HOLD";
+                            if ( sRevCode.equals("PRIVILEGESWITHDRAWN")) sRevCode = "PRIVILEGES_WITHDRAWN";
+                            if ( sRevCode.equals("CERTIFICATEHOLD")) sRevCode = "AA_COMPROMISE";


Bug here in string names

Well spotted. Will fix that.

primetomas · 2025-04-27T10:43:49Z

modules/ejbca-ejb-cli/src/org/ejbca/ui/cli/ca/CaImportCertDirCommand.java

+                "Specify an alternate CA certificate file (in PEM). Use this option when importing certificates that were issued by the previous CA certificate. Please note that the supplied certificate is not verified."));
+        registerParameter(new Parameter(REVOKEDETAILS, "Revocation Details", MandatoryMode.OPTIONAL, StandaloneMode.FORBID, ParameterMode.FLAG,
+                "Revocation details are to be derived from the filename of the certificate. The filename must end with '!<REASON>!<INVALIDITY_TIME>'. The REASON can be the value or label as described in RFC5280 section 5.3.1. "
+                + "INVALIDITY_TIME is formatted as '"+DATE_FORMAT_WINSAFE+"' and assumed to be the local timezone. Note: Filename extensions (ie., '.crt. or '.pem') are not supported. Please also note that any file without "


I think it would feel more consistent if revocation reason was an integer, the same as used when revoking certificates with ejbca.sh ra revokecert. Unessecary to have different parameters values in different places.

Well it can be...I think the ability to use label complicates things and I would rather do without it. See RevokeCertificateCommand

Perhaps. The CaImportCertCommand takes a text entry for revocation reason and not an integer code. The revocation reason text was confusing to me as the EJBCA's RevocationReason takes a different form than that in the RFC. I thought I'd allow an integer code, the RFC text label, as well as the EJBCA's text label.

primetomas · 2025-04-27T10:46:32Z

modules/ejbca-ejb-cli/src/org/ejbca/ui/cli/ca/CaImportCertDirCommand.java

+                            }
+
+                        } catch (NumberFormatException e) {
+                            // Not an integer, must be the full text


We could do without the full text imho. Keeping parameters simpler.

See earlier comment.

primetomas · 2025-04-27T10:50:51Z

modules/ejbca-ejb-cli/src/org/ejbca/ui/cli/ca/CaImportCertDirCommand.java

+                    // Find the revocation details from the filename. The details are separated with an exclamation (!) character.
+                    final String[] sa = file.getName().split("!");
+                    if (sa.length <3) {
+                        log.error("ERROR: The revocation details are not found in filename '"+file.getName()+"'. Ignoring this file.");


Simply ignoring a file makes the import process fragile. The import job reports error about succesful or non-succesful imports. Simply ignoring it here makes this reporting not reporting correctly. Either the report job should instead report that details was missing for this file and thus import failed, or we should allow missing revocation reason and then allowing "standard" import for this.

Because the operation is for a bulk set of files, the thinking here is to report an error, but allow the other files in the batch to be tried. Of course, if the whole batch is incorrectly labelled, then all files will show an error.

I think the skipping of import and error reporting should be part of the CertificateImporter then. If so it will be part of the import summary printed in the end.

I don't really follow this comment. If the option to use '--revoke-details-in-filename' is enabled, then the code should report any issues with each filename in the directory being imported, especially if the Revocation Reason or Invalidity Time is not provided in the filename. The Operator should not use the option '--revoke-details-in-filename' if the certificate filenames do not include the extra revocation syntax in the filename.

But there is still some files will be imported that have the correct filename while other will be skipped with just a warning in the log that can easily disappear since it's only logged to the console. And in the end the command will state an "Import summary" which does not report anything on these skipped records. That is a bad "Import summary" imho. '--revoke-details-in-filename' should be robust against filenaming errors similar to it handling issuer errors, or errors with the file content being wrong. Wrong filename is a similar error to wrong file content and should be dealt with similarly.

This wh9le parsing for the reason code can be done in CertificateImporter instead, why not do it there?

Added any errors to the Results, so as to fix the Import Summary. Example below of 3 incorrect filenames:

ERROR: The revocation details are not found in filename 'A_15710.cer'. Ignoring this file.
ERROR: '2023.OCT.09-12.01' was not a valid revocation time. Use this time format 'yyyy.MM.dd-HH.mm'. Ignoring this file '2024_11.!6!2023.OCT.09-12.01'.
ERROR: 'KEYSCOMPROMISE' is not a valid revocation reason. Ignoring this file '2024_12!KEYSCOMPROMISE!2022.05.04-20.59'.

Import summary:
0 certificates were imported successfully.
Time: 0.0 seconds (Infinity tps)
3 certificates were not imported due to other errors.

The Revocation "Reason Code" is not checked by CertificateImporter, so checking it is legal needs to be done in this code..

Fixed a typo.

Stueypoo

Added replies and one code fix.

…timportdir(v9)

primetomas · 2025-06-02T07:33:19Z

I just though of a question on the revocation options. After importing certificates the CRL should also be imported which would also set the options of they are on the CRL. You anyhow must import the latest CRL for the CRL number to become correct.

Can you describe the benefits of this in addition to importing the CRL?
(I can think of some)

Stueypoo · 2025-06-03T04:38:35Z

Hey Tomas. That is a good idea! An ability to import a pre-existing CRL, and thus set the revocation data using the CRL (and set the CRL number too). One issue to be mindful of is that CRLs can contain references to expired certs, but you may not have wanted to import all of these old inactive certs into EJBCA. I guess the command would just need to list out any serial numbers present in the CRL which are not known by EJBCA. Another alternative is to have a "-crl <crl file>" option for the certimportdir command. As certs are being imported, the CRL data is then checked and any certificates listed in the CRL are automatically revoked when imported. This would be a better approach than using the filename structure as described in this pull request. I should look into this further.

…

On Mon, Jun 2, 2025 at 5:33 PM Tomas Gustavsson ***@***.***> wrote: *primetomas* left a comment (Keyfactor/ejbca-ce#775) <#775 (comment)> I just though of a question on the revocation options. After importing certificates the CRL should also be imported which would also set the options of they are on the CRL. You anyhow must import the latest CRL for the CRL number to become correct. Can you describe the benefits of this in addition to importing the CRL? (I can think of some) — Reply to this email directly, view it on GitHub <#775 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A2VKRJGHSWES6WT5D5AIYLL3BP45JAVCNFSM6AAAAABVLAOSIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSMRZGIZDEMBSG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

primetomas · 2025-06-03T07:44:19Z

There is this command today: bin/ejbca.sh ca importcrl --help

Stueypoo · 2025-06-11T04:38:48Z

Oh, I had not noticed the 'importcrl' command. I did some testing and this command works perfectly to revoke certs after importing them into the DB. I guess there isn't any real need for this pull request now. Feel free to cancel it.

primetomas · 2025-06-16T06:58:12Z

Great to hear it worked well. Perhaps we can point to the importcrl command in the help text for importcert?

Stueypoo · 2025-08-01T11:43:39Z

I just realised that the --cacert option is still useful. I'll have another look at this and trim the change down.

…timportdir(v9)

Stueypoo · 2025-08-09T08:03:05Z

Updates done to trim down code back to include the '--cacert' option only.

primetomas · 2025-08-21T14:51:43Z

modules/systemtests/src-test/org/ejbca/ui/cli/ca/CaImportCertDirCommandSystemTest.java

+        } finally {
+            fileOutputStream.close();
+        }
+        // Delete the CA


Since the command doc says it is for old, i.e. renewed CA certificates. Wouldn't it make sense to renew the CA instead of deleting and re-creating it? See for example RenewCASystemTest for a simple example.

Yes, good point. However the reason one would be importing certs into a new CA is that they are migrated an existing CA over to EJBCA. If this migrated CA had previous CA certs, then EJBCA does not know about them. This is reason to add the --cacert option to this command. Testing needs to follow that same process. If one was to renew the CA in EJBCA, then that CA would know the previous CA cert and the test may be flawed.

Stueypoo added 2 commits January 17, 2025 14:00

Update CaImportCertDirCommand.java

7bb6389

Added ability to provide an alternate CA certificate (for renewed CAs) and allow revocation details to be derived from the filename.

Update CaImportCertDirCommandSystemTest.java

15b49f1

Added additional tests for changes to the associated client function.

hesunmark force-pushed the main branch from 4b94d53 to 668b7bf Compare March 28, 2025 14:13

Merge branch 'Keyfactor:main' into Improvement-to-client-function-cer…

9627d96

…timportdir(v9)

primetomas reviewed Apr 27, 2025

View reviewed changes

Update CaImportCertDirCommand.java

905df45

Fixed a typo.

Stueypoo commented Apr 28, 2025

View reviewed changes

Stueypoo and others added 2 commits May 5, 2025 09:36

Merge branch 'Keyfactor:main' into Improvement-to-client-function-cer…

f73881d

…timportdir(v9)

Added to 'results' if there is an error detected in the filename.

67ca9c5

Stueypoo and others added 3 commits August 9, 2025 16:38

Merge branch 'Keyfactor:main' into Improvement-to-client-function-cer…

991f982

…timportdir(v9)

Reverting the code to just include the -cacert option.

0a19ab0

Removing an unused import.

8cbbd9f

primetomas reviewed Aug 21, 2025

View reviewed changes

Improvement to client function certimportdir(v9) #775

Are you sure you want to change the base?

Improvement to client function certimportdir(v9) #775

Uh oh!

Conversation

Stueypoo commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

How has this been tested?

Checklist before requesting a review

Uh oh!

primetomas commented Apr 8, 2025

Uh oh!

Stueypoo commented Apr 11, 2025

Uh oh!

primetomas commented Apr 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stueypoo left a comment

Choose a reason for hiding this comment

Uh oh!

primetomas commented Jun 2, 2025

Uh oh!

Stueypoo commented Jun 3, 2025 via email

Uh oh!

primetomas commented Jun 3, 2025

Uh oh!

Stueypoo commented Jun 11, 2025

Uh oh!

primetomas commented Jun 16, 2025

Uh oh!

Stueypoo commented Aug 1, 2025

Uh oh!

Stueypoo commented Aug 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Stueypoo commented Jan 17, 2025 •

edited

Loading