Skip to content

Add --reuse-key feature #5901

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

Merged
merged 24 commits into from
Jun 1, 2018
Merged

Add --reuse-key feature #5901

merged 24 commits into from
Jun 1, 2018

Conversation

schoen
Copy link
Contributor

@schoen schoen commented Apr 26, 2018

Intended to supersede #4610 (with a different syntax per discussion with @ohemorange ).

Fixes #3788; fixes #3709.

@jschlyter
Copy link

Not tested, but feature-wise I agree this is a better solution than #4610.

@schoen schoen changed the title WIP for --reuse-key feature Add --reuse-key feature May 3, 2018
@schoen
Copy link
Contributor Author

schoen commented May 3, 2018

This appears to be working well now and I tested it quite a bit.

I had a red herring because when I renewed with certbot certonly --force-renewal (without explicit --reuse-key) I lost the --reuse-key setting from the renewal configuration. By contrast, certbot renew worked as expected. This is a behavior that @bmw has discussed a number of times and that I've even sometimes helped people out on the forum with.

Anyway, issuing with --reuse-key and then renewing with certbot renew currently works as expected in terms of using the same private key for the renewal.

The only remaining problem that I know about is that I don't have test coverage for all four combinations of --dry-run --reuse-key, --dry-run --no-reuse-key, --reuse-key, and --no-reuse-key (I think effectively three of them are covered by the current unit tests here). I tried all of them manually and they did the right thing, but maybe they should all be covered explicitly in unit tests.

Fixes #3788 and #3709.

@schoen
Copy link
Contributor Author

schoen commented May 3, 2018

I tried all of them manually and they did the right thing, but maybe they should all be covered explicitly in unit tests.

Per discussion with @ohemorange, I'm going to add one more unit test related to this.

@schoen
Copy link
Contributor Author

schoen commented May 4, 2018

This is ready for review!

@bmw
Copy link
Member

bmw commented May 4, 2018

@ohemorange, can you add this to your review queue?

@ohemorange
Copy link
Contributor

already on it!

@innovia
Copy link

innovia commented May 7, 2018

awesome work thank you - i'm using this branch

Copy link
Contributor

@ohemorange ohemorange left a comment

Choose a reason for hiding this comment

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

Added some suggestions to increase pythonicness and such

@@ -266,7 +266,7 @@ def obtain_certificate_from_csr(self, csr, orderr=None):
cert, chain = crypto_util.cert_and_chain_from_fullchain(orderr.fullchain_pem)
return cert.encode(), chain.encode()

def obtain_certificate(self, domains):
def obtain_certificate(self, domains, oldkeypath=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

old_key_path or old_keypath for pythonicness?

# Create CSR from names
if self.config.dry_run:
key = util.Key(file=None,
pem=crypto_util.make_key(self.config.rsa_key_size))
if oldkeypath is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

key = key or ...

csr = util.CSR(file=None, form="pem",
data=acme_crypto_util.make_csr(
key.pem, domains, self.config.must_staple))
else:
key = crypto_util.init_save_key(
self.config.rsa_key_size, self.config.key_dir)
if oldkeypath is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

key = key or ...

keypath = oldkeypath
keypem = f.read()
key = util.Key(file=keypath, pem=keypem)
logger.info("New key obtained by reusing existing private key "
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps clearer to drop the "new key obtained by"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think so because it does create a new file, but maybe users will be able to figure it out!

key = util.Key(file=keypath, pem=keypem)
logger.info("New key obtained by reusing existing private key "
"from %s.", oldkeypath)

Copy link
Contributor

Choose a reason for hiding this comment

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

if you stick an else: key = None here, the rest gets a little cleaner

certbot/cli.py Outdated
"certificate.")
helpful.add(
"automation", "--no-reuse-key", dest="reuse_key",
action="store_false", default=flag_default("reuse_key"),
Copy link
Contributor

Choose a reason for hiding this comment

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

did we ever figure out why this works? as in where the magic code lives. just out of curiosity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I'm not really sure!

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this exactly answers your question, but we have a number of flags like --foo and --no-foo. For example, take a look at --redirect.

--redirect sets redirect on the config object to True while --no-redirect sets it to False. Both --redirect and --no-redirect share the same default which isn't True or False, but None. This allows us to differentiate between the user saying yes or no to a redirect and not answering the question.

Do we want to do anything special in the case where the user doesn't provide either --reuse-key or --no-reuse-key? If not, I think we can probably get rid of --no-reuse-key and keep the default reuse_key value to False. If we do want this for some reason, we should probably change the default value from False to None.

Copy link
Contributor

Choose a reason for hiding this comment

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

faaascinating.

the only reason I could think of to have --no-reuse-key would be to override the saved setting in the renewal config file, but users can just delete that line.

otherwise, I'm for just having --reuse-key and having the default be False.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After talking to @bmw, I more clearly understand the point that for options that don't relate to prompts, we don't need a separate --no version because simply leaving the option out when reissuing with certonly is already sufficient to set it to False.

@@ -295,7 +295,13 @@ def renew_cert(config, domains, le_client, lineage):
_avoid_invalidating_lineage(config, lineage, original_server)
if not domains:
domains = lineage.names()
new_cert, new_chain, new_key, _ = le_client.obtain_certificate(domains)
if config.reuse_key:
Copy link
Contributor

Choose a reason for hiding this comment

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

old_key = lineage.privkey if config.reuse_key else None
new_cert, new_chain, new_key, _ = le_client.obtain_certificate(domains, old_key)

# pylint: disable=too-many-locals,too-many-arguments
quiet_mode=False, expiry_date=datetime.datetime.now(),
reuse_key=False):
# pylint: disable=too-many-locals,too-many-arguments,too-many-branches
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to add the tests that doesn't stick the checks into _test_renewal_common? We already overload this with too much stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but I think it might look even more complex by defining a whole new set of ways to test renewals!

ls -l "${root}/conf/archive/reusekey.le.wtf/privkey"*
# The final awk command here exits successfully if its input consists of
# exactly two lines with identical first fields, and unsuccessfully otherwise.
sha256sum "${root}/conf/archive/reusekey.le.wtf/privkey"* | awk '{a[$1] = 1}; END {exit(NR !=2 || length(a)!=1)}'
Copy link
Contributor

Choose a reason for hiding this comment

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

let's stick this awk command into a named parametrized function

@ohemorange ohemorange assigned schoen and unassigned ohemorange May 23, 2018
@ohemorange ohemorange merged commit e2d6faa into master Jun 1, 2018
@ohemorange ohemorange deleted the reuse-key branch June 1, 2018 22:21
@bmw bmw added this to the 0.25.0 milestone Jun 6, 2018
@ArchangeGabriel
Copy link

Maybe I’m missing something, but I don’t understand when/how old_keypath is set.

@ohemorange
Copy link
Contributor

It's passed in as an argument to obtain_certificate when called from renewal.py. Are you working on a PR that you're looking for help with?

@ArchangeGabriel
Copy link

No, I’m trying to understand how to get certbot to reuse the same key for each request on my server.

@ArchangeGabriel
Copy link

(For existing lineage as well as new ones)

@pgporada
Copy link

Seth,

This just came in extremely handy to test a system we've been working on. Thank you.

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.

please let me reuse a key during cert renewal Add full support for using an existing private key
7 participants