Implements support for ECDSA keys. Fixes #2163#8254
Implements support for ECDSA keys. Fixes #2163#8254atombrella wants to merge 1 commit intocertbot:masterfrom
Conversation
c4cc3bb to
dafa36c
Compare
|
Thanks @atombrella! Basically what stalled #7247 are the following things, which also applies to your PR:
@pahrohfit was not responding anymore, so if you are willing to continue the effort, that is great ! At this time sadly there is a blocker that will make the integration tests fail: some renewal parameters disappear with some association of CLI flags, like the key size, and so here also the key type and the curve for ECDSA. It is described here #7694, and a PR is under work here #7798. |
OK. I have some unpushed changes to address the elliptic curve name feature. helpful.add(
"security", "--elliptic-curve", type=str, choices=[
'sect283k1',
'sect283r1',
'sect409k1',
'sect409r1',
'sect571k1',
'sect571r1',
'secp256k1',
'secp384r1',
'secp521r1',
], metavar="N",
default=flag_default("elliptic_curve"), help=config_help("elliptic_curve"))Integration tests are also a work-in-progress. It's a good way also for me to brush up on this topic :)
OK. I'll have a closer look at those two issues/PRs. |
df61610 to
2035818
Compare
0e58b0e to
0d19ffe
Compare
atombrella
left a comment
There was a problem hiding this comment.
There are some areas where I'd like a bit of input, primarily with tests. I think it's maturing; I have some unpushed changes with all of the types of integration tests that were requested in the original PR (at least I think they are).
Searching on cabforum didn't yield a result for those three curves. An updated link/reference would be nice.
certbot-ci/certbot_integration_tests/certbot_tests/test_main.py
Outdated
Show resolved
Hide resolved
bbd324a to
b1aefc4
Compare
|
@adferrand Can you have a cursory look to see if it's taking shape, and that everything has been addressed? There are probably things in here that should be chopped (or expanded), such as the change in I couldn't really find valid email addresses so |
96997db to
a209901
Compare
|
@adferrand I think all the feedback has been addressed. Please note that I allowed maintainers to push to this branch in case you want to fix something before merging. |
|
Indeed @atombrella, the PR is ready to be merged in my opinion. Thanks a lot for your work and you patience regarding the various requirements on that feature. I am just waiting for #7694 to be fixed, it should happen in the next days. |
|
Block on #8343 |
|
I see the PR for #8343 is awaiting review. Is there any chance someone with write access can review and approve both #8343 and this PR for #8254 ? This PR addresses the top priority item that is not currently on the core certbot dev priorities list. It was mentioned in https://community.letsencrypt.org/t/certbot-support-for-ecdsa-certificates/132857/20 as an item that the community could help with, and needed some refinement. I see above the refinement has been completed, and it would greatly help my certbot integration work if this could be merged and released in the very near future. |
a209901 to
95dac3f
Compare
I think it doesn't matter so much in which order these two PRs are merged, as long as they both land in the same release. Perhaps this could be a good compromise? Based on the feedback, this PR seems to have been approved; however, maybe @bmw or @alexzorin would have some final words. I'm sorry to be a bit pushy :) |
|
This PR and #8343 are certainly on our radar and I expect both will be merged in the next couple weeks. Thanks again for working on this! |
|
I'm trying to add a test similar to the one added in #8343 for the ecdsa key. I guess that'd be one last necessity before the PR will be merged? |
|
For that PR, it is. We need to keep the flags associated to the key type and the curve chosen, and have a unit test covering it. Now there are two other elements, one related to flags interactions, the other related to documentation for key migration, that need to be implemented according to the high level design. We decided to integrate them after this PR, and put this PR on a dedicated branch to have all the features merged together in master by the next release. This release will happen in one month. I will open the relevant issues tonight. Based on these issues, if you are willing to implement them, it will be much appreciated! |
Good. The test is currently in my local branch, and when I get it running, I'll push it. I added a new
Please reference them in this PR, so I don't miss them. Thank you!
Yes. My goal was to get this PR merged, so yes, I'd at least make an effort at starting, and asking for help if I get stuck. |
|
@atombrella I linked the two issues related to the improvements to add on top of this PR. |
| "security", "--elliptic-curve", type=str, choices=[ | ||
| 'secp256r1', | ||
| 'secp384r1', | ||
| 'secp521r1', |
There was a problem hiding this comment.
Note that openssl ec_params -list-curves might not be complete due openssl/openssl#7920. Support for Ed25519 has already been requested in #2625 (comment).
There was a problem hiding this comment.
Thank you! I added it. I think the list is based on what's supported in browsers. I think secp521r1 will be deprecated; but I need to find references for it.
There was a problem hiding this comment.
I'm not sure if you'd want to have a curve already "active" while it's not allowed by the BR? I can imagine someone adding it to the Boulder code, but not enable it in production until it's actually allowed. That way, it would be possible to enable it relatively quickly.
But certbot doesn't work with configurations the same way. Everything you code here is immediately active and usable by users. It could be confusing for users if they use it in certbot, but get an error from Boulder (or other public ACME endpoints).
Also, if you do decide to add Ed25519 to the code, don't forget Ed448. It's in RFC 8446 too.
There was a problem hiding this comment.
@osirisinferi It's in my local branch. I'll push when I get the renewal test working 👍
There was a problem hiding this comment.
It wouldn't hurt to add them, but until Boulder supports them, have it throw an error or warning that they are not yet implemented in the ACME back end. When Boulder is updated to support it, remove the error/warning. If it's in certbot and throwing an error/warning that is easy to understand, someone will open an issue or PR to have it enabled once Boulder supports it.
There was a problem hiding this comment.
I am in favor of using the most conservative set of curves, and add new ones with a proper error handling in a future PR.
There was a problem hiding this comment.
OK. I've removed them. If possible, I'll have a look at the golang boulder code once this PR has been merged.
905d5c5 to
a861be9
Compare
a861be9 to
f4b57bd
Compare
|
@adferrand I've addressed the design document, and had initially overseen the "Key Type" for the certificates command. I've added a line for it, but I don't see any code coverage for the command. Adding it is doable, but maybe the PR is growing a bit in size. The documentation tasks are a bit "tricky" for me to address. I've made an attempt at one of them, and the other one is a bit confusing to me. The one about commands on already existing ecdsa certificates; can you guide a bit, and possibly suggest a section where this would be a good fit to write something about it? I took out parts about ed448 and ed25519. Maybe it'd be a good opportunity to toy a bit with Golang when this PR has been merged, to add support for those two key types. Thank you! |
|
Is there anything blocking this PR from being merged and those topics from being discussed elsewhere? |
Thanks to @pahrohfit and @Tomoyuki-GH for previous efforts to implement suport for this. Co-Authored-By: Robert Dailey <rob@wargam.es>
f4b57bd to
aa19503
Compare
adferrand
left a comment
There was a problem hiding this comment.
Some remaining comments and this is good!
Once done, could you reopen the PR against the ecdsa branch in Certbot repository that I just created (it is up to date with master)? This way we can implement the remaining tasks, then merge everything in one go to the main branch.
|
Closing, and will re-open shortly in a separate PR. |
I don't really know what stalled the other PR; I updated it a bit, moved
CHANGELOGentries to the top. And then also tweaked a couple of tests. It's unclear to me if 256 and 384 should be the only two allowed key sizes for EC; I added 521, but can remove it again. Anyway, I took it up again, and really hope this time that the 3rd active PR to get support for EC keys will be merged. I'll do my best to address feedback promptly. I had a look at the design document that was referenced in #7247 and I believe everything was already addressed? Please correct me if I'm wrong. Since @pahrohfit did the work, and I only updated it a bit, he's added inCo-Authored-By. It could be reversed, if desired.Some big websites have adopted elliptic curve cryptography, and I hope it'll become more widespread once
certbotwill support it. https://smallstep.com/certificate-manager/ supports it. It's similar to certbot in some ways, although it works also behind a firewall, in an internal network.echo | openssl s_client -connect yahoo.com:443 2>/dev/null | openssl x509 -text -noout | grep "Public-Key"Pull Request Checklist
mastersection ofcertbot/CHANGELOG.mdto include a description of the change being made.AUTHORS.mdif you like.