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

4638 implement es256 #4705

Closed
wants to merge 39 commits into from
Closed

Conversation

bak-minsu
Copy link

@bak-minsu bak-minsu commented Mar 21, 2024

Looking to address #4638 through a new common class called ACMEAlgorithm.

RFC 7518 fortunately includes a table of JWS to JCA mapping of these algorithms in its appendix. Rather than relying on a second source, this table directly within the IETF's JWS RFC allowed me to have confidence that I was implementing the translation correctly.

Additionally, as I was tinkering around with CMake a bit, the IDE I was using warned me that compatibility for CMake versions older than 3.5 will no longer be supported in newer releases of CMake. It appears in the release notes for CMake 3.27.

Added a test to generate a CSR for ES256 and attempt to get a signed certificate using certbot. This should cover the bounds of the original request.

Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR! I think generally it looks good but there's a problem with the string comparison which probably caused most of the CI failures. Please see my comments below.

@bak-minsu
Copy link
Author

I will work on getting the tests updated to check for these new algorithms as well

Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

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

Thanks for the update! The changes look good. Please let me know whether this can be merged or whether you're still working on it.

@edewata
Copy link
Contributor

edewata commented Mar 22, 2024

Just FYI, there's a known failure in the tools tests so that can be ignored for now.

@bak-minsu
Copy link
Author

bak-minsu commented Mar 22, 2024

Thanks for the update! The changes look good. Please let me know whether this can be merged or whether you're still working on it.

Still working on it! I'd want to at the very least include ES256 in the test. Might be another week or two. Thanks for checking in

@bak-minsu
Copy link
Author

bak-minsu commented Mar 24, 2024

Looks like we may not be able to implement the HMAC algorithms listed by RFC 7518. Haven't dug through the whole call stack, but the JSS provider seems to support a smaller number of algorithms.

@bak-minsu bak-minsu marked this pull request as ready for review March 24, 2024 23:03
@bak-minsu
Copy link
Author

I believe I'm ready, though I have yet to verify that the ES256 test will pass.

Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

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

Sorry, it looks like there's a problem with the openssl command:
https://github.com/dogtagpki/pki/actions/runs/8406928183/job/23066092344#step:26:8

@bak-minsu
Copy link
Author

Sorry, it looks like there's a problem with the openssl command: https://github.com/dogtagpki/pki/actions/runs/8406928183/job/23066092344#step:26:8

Ah that's on me. I've tested it locally this time and made sure it would generate a CSR and key

@bak-minsu
Copy link
Author

Looks like another failure. I'll continue to investigate

@bak-minsu bak-minsu marked this pull request as draft March 26, 2024 18:00
@bak-minsu
Copy link
Author

Looks like I was missing the pipe character before the CSR generation command

@bak-minsu bak-minsu marked this pull request as ready for review March 28, 2024 02:37
@bak-minsu bak-minsu marked this pull request as draft March 28, 2024 03:04
@bak-minsu bak-minsu marked this pull request as ready for review March 28, 2024 03:15
@edewata
Copy link
Contributor

edewata commented Mar 28, 2024

No worries, thanks for the updates! We're not in a hurry so take your time.

It looks like there's still a problem with the ES256 test:
https://github.com/dogtagpki/pki/actions/runs/8461911873/job/23201890102?pr=4705#step:29:16
You probably can ignore the other CI failures.

It would be nice if you can run the CI in your own repo so you'll get the test results more quickly. I'm not sure exactly how to do it (since it was a long time ago) but you might want to check out this doc:
https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository

@bak-minsu bak-minsu marked this pull request as draft March 28, 2024 23:31
@bak-minsu
Copy link
Author

Figured out how to run it on my own repo, whoo!

I'll keep this in draft until I can get the tests to be successful.

@bak-minsu
Copy link
Author

bak-minsu commented Mar 30, 2024

@edewata Looks like the JSS KeyFactory needs to be updated to support EC public key generation. I can also try re-implementing that in this project, but probably is wise to update it there. I'll look into opening up an issue there.

@bak-minsu
Copy link
Author

Taking another look, it looks like I may be able to accomplish this through X509 Encoded Spec

@edewata
Copy link
Contributor

edewata commented Apr 1, 2024

@jmagne @ladycfu @fmarco76 fyi

@edewata
Copy link
Contributor

edewata commented Apr 1, 2024

@bak-minsu If you want, feel free to create a separate PR for the cleanups (reformatting the code, removing unused imports, etc.) so they can be merged first, so this PR will just focus on the ES256 implementation.

Copy link

sonarcloud bot commented Apr 1, 2024

@bak-minsu bak-minsu closed this Oct 24, 2024
@bak-minsu bak-minsu deleted the 4638-implement-ES256 branch October 24, 2024 04:17
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.

2 participants