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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ECDSA subject key support #2660

Closed
wants to merge 31 commits into
base: master
from

Conversation

Projects
None yet
@osirisinferi
Contributor

osirisinferi commented Mar 13, 2016

I've made a stub for ECDSA key support. It probably isn't the nicest work of coding, but it does work. 馃槢 Fixes #2163

All comments are welcome, so please do 馃槃

osirisinferi added some commits Mar 13, 2016

@pde pde added this to the 0.6.0 milestone Mar 15, 2016

@pde

This comment has been minimized.

Member

pde commented Mar 15, 2016

Tagging this for 0.6.0, although we can merge it sooner if it's ready sooner.

@@ -1583,6 +1584,10 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):
"security", "--rsa-key-size", type=int, metavar="N",
default=flag_default("rsa_key_size"), help=config_help("rsa_key_size"))
helpful.add(
"security", "--ecdsa_curve", default=flag_default("ecdsa_curve"))

This comment has been minimized.

@jsha

jsha Mar 16, 2016

Contributor

This should be --ecdsa-curve to match the other flags.

This comment has been minimized.

@jsha

jsha Mar 16, 2016

Contributor

Also, it needs help text, and should list the available options.

This comment has been minimized.

@jsha

jsha Mar 16, 2016

Contributor

And should validate its input against the available options. :-)

This comment has been minimized.

@osirisinferi

osirisinferi Mar 16, 2016

Contributor

Help text needs to be added indeed. Good point about the switch style.

For now, the options are hardcoded, but it should be configurable methinks.

@@ -1583,6 +1584,10 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):
"security", "--rsa-key-size", type=int, metavar="N",
default=flag_default("rsa_key_size"), help=config_help("rsa_key_size"))
helpful.add(
"security", "--ecdsa_curve", default=flag_default("ecdsa_curve"))
helpful.add(
"security", "--privkey_signature_algorithm", default=flag_default("privkey_signature_algorithm"))

This comment has been minimized.

@jsha

jsha Mar 16, 2016

Contributor

I would instead call this flag --key-types and make it a list of key types. Same comment above about listing options and validating them applies.

The reason to make it a list is that I expect that for people using the autoconfiguration features, they will most likely want to issue both an RSA cert and and ECDSA cert, and configure both of them in their web server, to be offered based on client handshake.

There's some additional groundwork to be laid for having multiple "live" certs per name, which I don't think has to happen in this change. The main thing is that if we're introducing a new flag, I'd like to have its name and semantics be what we want to use for the long term. For now, the validator can reject any list of key types with length greater than one.

logger = logging.getLogger(__name__)
# High level functions
def init_save_key(key_size, key_dir, keyname="key-letsencrypt.pem"):
def init_save_key(key_algo, rsa_key_size, ecdsa_curve, key_dir, keyname="key-letsencrypt.pem"):

This comment has been minimized.

@jsha

jsha Mar 16, 2016

Contributor

This function now has a weird signature, in that you're required to specify rsa_key_size even if you're creating an ECDSA key. It probably makes more sense to change this function to save_key(key_pem, key_dir...) and have the code in client.py call make_key_rsa or make_key_ecdsa depending on the config, and then call save_key on the result.

@@ -56,7 +68,10 @@ def init_save_key(key_size, key_dir, keyname="key-letsencrypt.pem"):
with key_f:
key_f.write(key_pem)
logger.info("Generating key (%d bits): %s", key_size, key_path)
if key_algo == "RSA":
logger.info("Generating RSA key (%d bits): %s", rsa_key_size, key_path)

This comment has been minimized.

@jsha

jsha Mar 16, 2016

Contributor

I think this log function was originally in the wrong place, since it comes after the key is already generated. These should happen before the key is generated (and outside of this function, which is now about saving keys).

osirisinferi added some commits Mar 17, 2016

@osirisinferi

This comment has been minimized.

Contributor

osirisinferi commented Mar 17, 2016

The tests need work.. But first I've got to learn more about tests in the first place 馃槢

@pde

This comment has been minimized.

Member

pde commented Mar 17, 2016

@osirisinferi writing mock tests is often hard. Feel free to come and ask us for help on IRC if you're stuck on things!

osirisinferi added some commits Mar 19, 2016

@osirisinferi

This comment has been minimized.

Contributor

osirisinferi commented Mar 19, 2016

Uch, tox locally wasn't complaining, but Travis is.. 馃槢

@osirisinferi

This comment has been minimized.

Contributor

osirisinferi commented Mar 19, 2016

I think the test_success and test_key_failure areis broken, because init_save_key() is now split between client.py and crypto_util.py, correct?

test_key_failure is looking for a ValueError exception, which isn't in crypto_util.py anymore, but in client.py..

Edit:
Fixed test_success locally.. Now I need to move test_key_failure, but don't really know yet how 馃槢

@osirisinferi

This comment has been minimized.

Contributor

osirisinferi commented Mar 20, 2016

Well, at least all tests are succeeding now, but that's because I didn't include a test for test_key_failure yet.. Any help is much appreciated! (I'm idling in #letsencrypt 馃槃 )

Just the lint and coverage left..

@bmw

This comment has been minimized.

Contributor

bmw commented Mar 21, 2016

By splitting make_key_rsa out of save_key, test_key_failure isn't really necessary anymore.

To fix cover, you should write tests for functions like make_key_ecdsa. In general, we like for pull requests to have full test coverage for all modified lines.

As for lint, fixing the errors below this line will solve the problem.

@osirisinferi

This comment has been minimized.

Contributor

osirisinferi commented Mar 21, 2016

AttributeError: 'MakeKeyECDSATest' object has no attribute 'assertIsInstance'

FFS 馃槣 That attribute is new in 2.7......

At least lint is OK now.. 馃槃

And about the coverage: I probably should add all kinds of tests testing for all the things that could go wrong in obtain_certificate(), right? Two for checking correctness of rsa and ecdsa key types, one for an invalid key type and one for more than one key type (at least, until we support multiple key types at once ofcourse)?

@bmw

This comment has been minimized.

Contributor

bmw commented Mar 21, 2016

Great!

Yeah all of those tests sound good to me! If functions like make_key_rsa and make_key_ecdsa already have good test coverage, you can mock out those functions in obtain_certificate() for relevant tests if it makes your life easier.

@@ -1583,6 +1584,16 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):
"security", "--rsa-key-size", type=int, metavar="N",
default=flag_default("rsa_key_size"), help=config_help("rsa_key_size"))
helpful.add(
"security", "--ecdsa-curve", default=flag_default("ecdsa_curve"),
help="Set the certificate ECDSA curve. Current possible curves: "
"prime256v1 (default) or secp384r1")

This comment has been minimized.

@jsha

jsha Mar 23, 2016

Contributor

Let's use the more easily remembered FIPS 186-4 names for these curves: P-256 and P-384. Also, please extract them to a constant and use it in both a validator for the flag and in obtain_certificate.

Also, should this be config_help("ecdsa_curve")?

@jsha

This comment has been minimized.

Contributor

jsha commented Mar 29, 2016

After thinking about this some more, I think that all files related to ECDSA certificates (and any new certificate types, like ed25519 in the future), should have a suffix so they can coexist in the same directory with RSA certificates. So they would be like cert-ecdsa-1.pem, chain-ecdsa-1.pem, fullchain-ecdsa-1.pem, key-ecdsa-1.pem. And they would be symlinked into the live directory under cert-ecdsa.pem, chain-ecdsa.pem, etc.

Ideally RSA keys would be named in the same pattern, but we're more or less locked into our existing naming scheme for them now, so we shouldn't change those.

@bmw

This comment has been minimized.

Contributor

bmw commented Jan 31, 2017

Kicking this to 0.12.0. There's a design discussion that needs to happen about how lineages work while supporting multiple key types.

@bmw bmw modified the milestones: 0.12.0, 0.11.0 Jan 31, 2017

@jonnybarnes

This comment has been minimized.

jonnybarnes commented Feb 22, 2017

Is there an issue with public discussion about the design decision regarding lineages?

@jsha

This comment has been minimized.

Contributor

jsha commented Feb 25, 2017

@jonnybarnes Not previously, but I'll summarize it here:

Right now, we store four files in /etc/letsencrypt/archive/example.com/: certN.pem, chainN.pem, fullchainN.pem, privkeyN.pem, where N increases over time. We also symlink the most recent of each of these files into /etc/letsencrypt/live/example.com, with the N removed, so server software can be configured with a reliable filename. This symlinking step is considered "atomic"[1], so that all four files in live/ match each other at any given time.

The question is: If you have identical certificates for multiple key types, should we store them in separate lineages (i.e. archive/example.com for RSA certs and archive/example.com-ecdsa for ECDSA certs), or should we consider them part of the same lineage, adding new filenames to the collection (i.e. archive/example.com/ecdsa-certN.pem, archive/example.com/ecdsa-privkeyN.pem).

Mostly this comes down to a question of what we want to do if renewal fails for one key type, but succeeds for the other. Do we consider both renewals to have failed, and retry both later? Or do take the successful one and configure it, and retry only the failed one later? The former implies the "same lineage" approach. The latter implies the "separate lineages" approach. I think we are slowly arriving at consensus on the "separate lineages" approach. Is that your feeling too, @bmw, @ohemorange, @ynasser?

[1] Note that the symlinking is not actually atomic in the Unix operation sense, and a poorly-timed crash could result in an inconsistent state, but that is a totally different issue.

@J0WI

This comment has been minimized.

J0WI commented Feb 25, 2017

This big question about the naming convention affect only the scenario of #2632. This bug is a blocker of #2632, but it does not depend on it. If you just want to use ECDSA, you can continue using the same naming convention and change it later, together with dual cert support. Another usecase is, if you using Certbot just to get an ECDSA cert, which you put manually to your preferred location.
Other ACME clients already support ECDSA certs. So I think it would be helpful, when Certbot at least have the switch for issuing ECDSA certs.

@jsha

This comment has been minimized.

Contributor

jsha commented Feb 25, 2017

@J0WI: I agree that I'd like to get this landed soon. But I do think the storage decision is a blocker. We'd like to store ECDSA certificates in a consistent place, so we don't change assumptions on people in the future, after they've come to depend on certificates getting stored in a certain location. Note that of course you can still use the --csr flag to manually issue ECDSA certs with Certbot, even though it's a pain (and doesn't support auto-install).

@bmw

This comment has been minimized.

Contributor

bmw commented Mar 1, 2017

FWIW, when @schoen and I specced this out between the two of us we settled on separate lineages but @pde was strongly in favor of putting things in a single lineage to more easily offer things like configuring your webserver to use both RSA and ECDSA certificates.

@jonnybarnes

This comment has been minimized.

jonnybarnes commented Mar 1, 2017

@bmw well, looking at nginx, you just need to specify ssl_certificate and ssl_certificate_key twice, one pointing to the rsa version, and one pointing to the ecdsa version. It doesn鈥檛 really matter wther there in seprate folders so long as its consistent.

@bmw

This comment has been minimized.

Contributor

bmw commented Mar 1, 2017

Yep. We can do it either way, it's just easier due to Certbot's architecture to have them in the same lineage. For example, currently at most one lineage is created during a run and is then passed around through Certbot code. If we put them in separate lineages, we'd have to change this or have people run Certbot twice with some kind of flag/prompt to distinguish between people who want to replace the certificates for a domain and those who want to add additional ones. Certainly not an unsolvable problem, but I believe @pde's position was putting them in the same lineage could save us a lot of work.

Responding to earlier in this thread though, I think we may be able to kick this decision to a later date. Regardless of how support this long term, I suspect we'll want people to be able to request a lineage containing only an RSA or ECDSA certificate. If this is the case, we could support ECDSA certs in their own lineage for now and potentially offer dual lineages later. I'd definitely have to get more of the Certbot team on board with this decision though.

@bmw bmw modified the milestones: 0.12.0, 0.13.0 Mar 2, 2017

@bmw

This comment has been minimized.

Contributor

bmw commented Mar 3, 2017

To keep this thread up to date, here's my IRC messages about the current status of this:

2017-03-03 07:46:53	+bmw	OsirisInferi: my inclination wasn't to make a distinguished name for the lineage, but I brought it up the rest of the Certbot team and got disagreement from some people
2017-03-03 07:49:03	+bmw	their concern was if we want to support a single lineage with both key types, we'll need to change file names with the current thought to make the ECDSA files "cert-ecdsa.pem", "privkey-ecdsa.pem", etc.
2017-03-03 07:49:24	+bmw	and if we're going to do that, let's be consistent and always do that
2017-03-03 07:49:35	+bmw	so unfortunately, we're back to blocking on the larger decision on how to support both :(
2017-03-03 07:49:41	+bmw	I'll try to push the decision along though

@bmw bmw removed this from the 0.13.0 milestone Mar 28, 2017

osirisinferi added some commits Apr 25, 2017

@jsha jsha referenced this pull request Aug 28, 2017

Open

Add --key-type flag #2625

osirisinferi added some commits Jan 14, 2018

@jgillula

This comment has been minimized.

Contributor

jgillula commented Apr 18, 2018

@bmw should we keep this PR open?

@bmw

This comment has been minimized.

Contributor

bmw commented Apr 18, 2018

I think @ohemorange should make the call here as she wanted to own this task when she finds the time. Personally, I don't care too much whether or not this PR stays open, but if we close it, we should keep this code around in a branch somewhere to build off of.

@osirisinferi

This comment has been minimized.

Contributor

osirisinferi commented May 21, 2018

It has been a whole year since the last "we still need to make a decision" bit.

In the mean time, this soccer ball has become a nightmare (for me) to manage with all the frequent master updates and subsequent conflicts. I've got very little spare time nowadays and I mostly made these changes so I could generate ECDSA certificates easily on my server.

Therefore, I've made a new ECDSA branch on my own fork (just implemented the same ECDSA code in the most recent master code), forgo all the tests and just made it working for me.

Personally, I don't care either if you keep this PR open or not. I have ECDSA support for myself.. I just want to mention I don't think it's very polite if people take the effort in helping to improve certbot and subsequently just closing the PR without much gratitude shown. Wouldn't be the first time.

Further more, on the Community, people are frequently asking about ECDSA support. I understand Let's Encrypt/EFF certbot team is small and has many pressing issues to resolve, but one might think this choice might have been resolved within a year.

@bmw

This comment has been minimized.

Contributor

bmw commented May 22, 2018

@osirisinferi, I'm sorry we haven't been able to land this yet and maintenance has been time consuming for you. I'm also sorry that we and the Let's Encrypt team haven't seemed appreciative to you for the time you've spent contributing on the project.

I can assure you that we are grateful for your work and we have every intention of using this PR. If we close this, I want us to add your code to a branch on the Certbot repo so it can be used as a base to build off to finish our ECDSA support. I probably should have made it clearer in my recent comment that the reason I want this if we close this PR is because the work that's been done here is valuable addition to the project that should be used in the future.

I won't try and estimate when this feature will land or justify why it hasn't landed yet, but we are certainly still planning on adding this to Certbot. While we've prioritized different pieces of the project, I'm glad you've found a solution that works for you in the meantime and I'm hoping we can finally get around to adding this feature sooner rather than later.

@gbdlin

This comment has been minimized.

gbdlin commented Jul 26, 2018

Is there any decision about if this should live in the same lineage or different?

@gbdlin

This comment has been minimized.

gbdlin commented Jul 26, 2018

Actually, isn't this just not an issue at all? We can use --cert-name to save ecdsa certificate in separate lineage. That way you can have both RSA and ECDSA and it implies minimal amount of changes. With that in mind, we don't really need to put them in one lineage together.

@bmw

This comment has been minimized.

Contributor

bmw commented Sep 12, 2018

I've pushed the work in this branch to https://github.com/certbot/certbot/tree/osiris-ecdsa to be used when we or someone else has time to work on this issue.

@bmw bmw closed this Sep 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment