Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement to the CLI: CaImportCertDirCommand.java #711

Open
wants to merge 6,608 commits into
base: main
Choose a base branch
from

Conversation

Stueypoo
Copy link
Contributor

@Stueypoo Stueypoo commented Nov 15, 2024

  • 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 certificate that is stored in the DB. A problem with this is when the certificates being imported had been issued from the CA's previous certificate. An example CA that is impacted by this is a CSCA used for ePassports. These CA are renewed often, and the end-entity certificate have a long validity.

To remedy this, I added a "--cacert" option that allows the Operator to provide an alternate CA certificate that will then be used to verify the certificate being imported.

  • Added an option to get the revocation details from the filename.

When importing revoked certificates, the Operator should supply the revocation REASON and INVALIDITY_TIME. If you have a lot of revoked certificates to import, then a better option is required.

One idea is to have the REASON and TIME value encoded within the filename of each certificate, and the code then extracts these revocation details. The filename format being: CertName!REASON!TIME

REASON is the reason code value or the name. eg., For suspended certs, use either "6" or "CERTIFICATEHOLD" or "CERTIFICATE_HOLD"
TIME format is YYYY.MM.DD-hh.mm

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

primetomas and others added 30 commits April 2, 2024 12:06
ECA-12018: Add pkcs7 response format to pkcs10enroll

Closes ECA-12018

See merge request ejbca/ejbca!1462
fix: update p11ng to support Ed448

Closes ECA-12337

See merge request ejbca/ejbca!1520
…est' into 'main'

ECA-12287: fix adjust timeout in test

Closes ECA-12287

See merge request ejbca/ejbca!1519
no-ticket: update swagger to match api definitions

See merge request ejbca/ejbca!1525
…-empty-name' into 'main'

ECA-11972 - Block clone/creation of certificate profile when the name is empty or contains

Closes ECA-11972

See merge request ejbca/ejbca!1517
…' into 'main'

ECA-12338 - Add user-friendly error message for rename approval profile action

Closes ECA-12338

See merge request ejbca/ejbca!1515
'fb-ECA-12237-Remove_ca.keystorepass_and_ca.cmskeystorepass' of
https://neo.repoman.primekey.com/ejbca/ejbca into
fb-ECA-12237-Remove_ca.keystorepass_and_ca.cmskeystorepass
…oken in RaMasterApiProxyBean and RaMasterApiPeerImpl
Comment out .image.tag so the correct version number from Chart.yaml
is used and update src/internal.properties to make Jenkins X compute
the correct build number.
ECA-12104: p11ng-cli command to list usable key pairs

Closes ECA-12104

See merge request ejbca/ejbca!1378
CI pipeline step name typo fix

See merge request ejbca/ejbca!1528
@primetomas
Copy link
Collaborator

Awesome, thank you. There is a system test for this command in modules/systemtests/src-test/org/ejbca/ui/cli/ca/CaImportCertCommandSystemTest.java

Can you add some testing of the new parameters there?

You can easily run the test with

ant test:runone -Dtest.runone=CaImportCertCommandSystemTest

@Stueypoo
Copy link
Contributor Author

Sure, I'll take a look at the system test cases.

Changed to using Exclamation character as the filename seperator. Also, the colon in the time will cause issus with Windows, so replaced with a period.
Added test cases for revocation data in the filename, and a test with the the 'cacert' parameter
Re-added the imports which didn't copy across.
@Stueypoo
Copy link
Contributor Author

Test cases added. Also updated the code to address the following:

  • Change the filename seperator to be the exclamation mark (!)
  • Revocation reason label can have underscore characters or not have them (IE., CERTIFICATE_HOLD or CERTIFICATEHOLD will work)
  • The revocation time format of HH:mm is a problem with Windows filenames. Changed the format to HH.mm

System tests for this change are run with:
ant test:runone -Dtest.runone=CaImportCertDirCommandTest

Copy link
Collaborator

@primetomas primetomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some review comments. Nothing major.

Fixing the license banner that i somehow messed up.
Fixes as suggested by Keyfactor
Fixes as recommended by Keyfactor.
@Stueypoo
Copy link
Contributor Author

Stueypoo commented Dec 9, 2024

Thanks @primetomas for the review.

Removed unused imports
@hesunmark hesunmark force-pushed the main branch 2 times, most recently from 647a632 to e3961c2 Compare December 19, 2024 11:02
@primetomas
Copy link
Collaborator

primetomas commented Dec 19, 2024

I'm sorry, but we just pushed EJBCA 9.0 to GitHub, which means the PR is messed up and have to be redone/rebased.
Apart from that, I like it very much and I should be able to merge it next year.
Can you rebase the PR for the updated ejbca-ce git?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.