ref certbot/certbot#2163 EC cert generation support#7247
ref certbot/certbot#2163 EC cert generation support#7247pahrohfit wants to merge 39 commits intocertbot:masterfrom
Conversation
…c_key_size` options
|
Seems complete. Look forward to feedback. |
adferrand
left a comment
There was a problem hiding this comment.
Hello @pahrohfit, and thanks a lot for your PR. Indeed the support of ECDSA cert is really expected, and was planned to be done by the Certbot core team next year. But if someone is willing to support the integration of this feature, we would gladly review the subsequent PRs to make the functionality available in Certbot.
On that matter, it is clear that ECDSA certificates is a major feature. So it needs to be carefully handled, in order to avoid regressions, erroneous/failed configurations or even post-release fallbacks. These events are more likely to happen and to have a major impact considering the number of Certbot users.
That is why this kind of feature is associated with a documented specification to organize its implementation. We try at most to gather and anticipate impacts, retro-compatibility considerations and usability in this document. For ECDSA certificates it can be found here: https://docs.google.com/document/d/1JaTZn-ZZeCJcg1j0D1RhIOl5B5joifRtqExGQTfzrAE/edit?usp=sharing
I invite you to read it and make your opinion about the work to do, and maybe propose ideas to improve it.
Then, would you be willing to contribute in a part or more of the integration path described in the document?
|
Is there anything blocking merging this as the foundation for future work? I don't think volunteer PRs can't be reasonably expected to implement every single feature specified in the design doc, I doubt even the team implements it all in one PR, this one here just provides the functionality to at least handle ECDSA keys properly which has been long due. |
|
@TaaviE IMO the parameter should be named |
Okay, the design doc also says it should be @pahrohfit Could you make that small change please? Would you accept pull requests to your branch? |
|
absolutely... i’ll get it done next day or so |
…idation to allow multiple `--key-type` values, but report a parse error for more than one Multiple `--key-type` values will be supported at a later time, when all of certbot can handle multiple key types during certificate generation.
Completed and code checks passed |
|
@adferrand thoughts on the above? Seems to have addressed the minimum requirements in the documentation set. |
There was a problem hiding this comment.
Thanks a lot for you PR. In the scope of its purpose, I largely agree with the implementation. I have mainly few requests to add more tests and one question about how the flags are set by the CLI.
Now, I think we still need to decide about the strategy used by your PR here to implement EC certificates. Let's view the pro and cons.
Pros:
Your PR adds immediately, and with very few modifications, the EC certificate feature. And if someone does not use or is aware of --key-type, Certbot behavior stays exactly the same than before. Finally, naming convention follows the initial design, while the feature on its own does not add everything in a row, but only one deliverable unit of logic.
This is really a quick win.
Cons:
Depending on how --key-type is invoked for each certificates, standard paths live/[LINEAGE]/{cert,privkey,chain,fullchain}.pem will contain either an RSA or ECDSA key/cert.
- First drawback is that it is not obvious from the path if the files contain RSA or ECDSA stuff
- Second drawback is when multiple values will be authorized in
--key-type. The idea was to suffix all files with-ecdsafor ECDSA stuff. Here with your PR merged, some migration path will be needed to handle the fact that one given path file may change from ECDSA to RSA based material when--key-typechanges fromecdsatorsa,ecdsa.
My opinion: let's merge it!
I think Pros win over Cons in the absolute, and the fact that this feature is waited by a lot of people adds one more motivation to merge this PR.
Concerning the first drawback: this can be fixed quickly in a future PR to change what certbot certificates displays. For now, having an ECDSA cert in the standard path is the result of an explicit action from the caller, and I think in this case he will be ready to make the appropriate modifications in any workflow to take that into account.
Concerning the second drawback: any modification to the paths used by certbot will imply a lot of modifications everywhere in the code, making the availability of EC certificates uncertain in a near future. I prefer to get it now, and deal with migration later, if this kind of migration is really needed.
Because in term of migration, I think it would make sense to use explicit and dedicated paths for both types (privkey-rsa.pem and privkey-ecdsa.pem) when --key-type is used with two values. On the contrary using the standard non explicit path is suitable for any single value on --key-type. I think it makes things clear, in particular for the dual-stack configuration since it will require explicit modifications outside certbot anyway to use that correctly. And finally I am not convinced that rsa,ecdsa should become the default value for --key-type in certbot (if you are interested to know why I think that, we can discuss it on another thread). In that case, there is no migration path to handle, it is just another way to use certbot.
Now I will invoke @bmw here to have a second opinion ^^
certbot/cli.py
Outdated
| "security", "--ecdsa-key-size", type=int, metavar="N", | ||
| default=flag_default("ecdsa_key_size"), help=config_help("ecdsa_key_size")) | ||
| helpful.add( | ||
| "security", "--key-type", type=str, choices=['rsa', 'ecdsa'], nargs="+", |
There was a problem hiding this comment.
On top of the unit tests added here, I would like an integration test in certbot-ci/certbot_integration_tests/certbot_tests/test_main.py to check that certbot behaves as expected when the new flags are added. Typical scenarios I have in mind:
- pass no flag or
--key-type=rsaand expect a RSA key/cert generated with the default RSA size - pass
--key-type=ecdsaand expect an ECDSA key/cert generated with the default ECDSA size - pass wrong
--key-typeand expect certbot to crash with the expected error message - handle key size control flags (ECDA and RSA) may be added in a modified version of the existing test
test_renew_with_changed_private_key_complexity
… --key-type option sent.
…pe/size ... keeping it DRY(er)
|
Not sure why that one lint test is failing ... any suggestions? |
Thanks for the heads up @adferrand |
adferrand
left a comment
There was a problem hiding this comment.
Hello again @pahrohfit
I am still waiting for an integration test on the ECDSA feature, as in https://github.com/certbot/certbot/pull/7247/files#r321684775. Will you have the time to add it? Beside that, I added a remark to keep updating the changelog.
CHANGELOG.md
Outdated
|
|
||
| ## 0.37.0 - 2019-09-03 | ||
|
|
||
| ### Added |
There was a problem hiding this comment.
This entry should be removed.
CHANGELOG.md
Outdated
|
|
||
| ## 0.39.0 - 2019-09-05 | ||
|
|
||
| ### Added |
There was a problem hiding this comment.
These entries should be moved up to the master section to be included in the next release changelog.
@pahrohfit How hard would it be to change the existing PR to suffix things with -ecdsa to make the future dual-certificate support easier to merge? |
Sorry, been traveling and changing jobs... yeap, working on the last tests now (also had to get a linux vm setup since I work on fbsd) |
I'd hold off on this piece for now. Since this comment was written, we've been having some conversations internally about the design here and may not need/want this piece. |
|
I withdraw my comment/request, too. It seems that ECDSA-only would be fine at this point. This presentation from 2015 seems to suggest that ECDSA support in browsers is better than I might have expected: https://csrc.nist.gov/csrc/media/events/workshop-on-elliptic-curve-cryptography-standards/documents/presentations/session2-andrews-rick.pdf More importantly, I just ran ssllabs.com against one of my websites (which is configured per current Mozilla SSL Generator guidance) and looked at each of the clients to determine what would be lost if I switched from RSA to ECDSA. It looks like I'd only lose "Chrome 49 / XP SP3", which I'm okay with. |
|
@adferrand are the changes requested still actual or could this PR be finally merged? |
|
The current status of this is we initially wanted the behavior in the design doc at https://docs.google.com/document/d/1JaTZn-ZZeCJcg1j0D1RhIOl5B5joifRtqExGQTfzrAE/edit?usp=sharing which is not implemented in this PR. After talking about this internally though, we think some features there such as needing to support dual ECDSA and RSA certificates as part of supporting ECDSA certificates at all is no longer necessary. We came up with some new design questions about Certbot's behavior and UI around this feature that need to be answered if we're deviating from that design though. We think this is important to get right initially because making changes to this stuff after the feature is released is painful. I'm planning on proposing something to the core team, getting everyone's approval, and then posting the changes we want to see within the next month. This may be a little slower than what you want, but this PR/feature is on our radar and if you haven't heard from me by February, by all means mention me for another update. |
|
I wrote a new design for Certbot's behavior around ECDSA subject keys which no one on the Certbot team has objected to at #2163 (comment). |
|
Hello @pahrohfit, are you willing to continue the work to align with the new design posted by @bmw, or do you want me to take this PR on my own ? |
|
Yeap; just read it the other day, I’ll get to work on it this week. Seems must more straightforward than the last, so I’m good |
|
Hello @pahrohfit, I updated your PR with master, to not let it rot. Are you still willing to work on it? |
Obtained from: certbot#7247 PR: certbot#2163
|
Since #8431 has been merged, which is based on top of your work @pahrohfit, I think we can close this PR. |
Two new options:
--key-type-- Takes either 'rsa' or 'ec' as options (defaults to RSA for backwards compatibility).--ec-key-size-- Takes [160, 224, 384, 521] as size, defaults to 384.Did everything to maintain full backwards compatibility with existing code/plugins, so it could be prettier if that was abandoned.
Pull Request Checklist
mastersection ofCHANGELOG.mdto include a description ofthe change being made.
annotations
for any functions that were added or modified.
AUTHORS.mdif you like.