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

Don't store cert backups #124

Merged
merged 1 commit into from
Apr 30, 2019
Merged

Don't store cert backups #124

merged 1 commit into from
Apr 30, 2019

Conversation

gohai
Copy link
Contributor

@gohai gohai commented Apr 18, 2018

Those are inconvenient if you don't want to run Redis as LRU cache. We are more interested in not silently discarding any certificates, and scaling Redis with increasing number of domains, so we don't.

If this change is not acceptable, another scheme could be to store the previous (":old") certificate in addition to the current one (":latest"). This way we'd only be storing at most two certificates per domain in Redis, and not a growing number due to renewals.

Those are inconvenient if you don't want to run Redis as LRU cache. We are more interested in not silently discarding any certificates, and scaling Redis with increasing number of domains, so we don't.

If this change is not acceptable, another scheme could be to store the previous (":old") certificate in addition to the current one (":latest"). This way we'd only be storing at most two certificates per domain in Redis, and not a growing number due to renewals.
@GUI
Copy link
Collaborator

GUI commented Apr 28, 2018

Apologies for the delay on this one too, but I’ll try to take a look at this in the next couple weeks. Thanks again!

@GUI
Copy link
Collaborator

GUI commented May 7, 2018

Ah, I had forgotten that we were indefinitely storing all the previous certificates, so thanks for catching this! I can see how this could become an issue with lots of certificates, particularly for in-memory stores like Redis.

That being said, I would like to retain the ability to keep previous certificates for debugging purposes by making this more configurable, instead of getting rid of the backups completely. I might be overly paranoid, since I'm not sure I've actually ever had to use the old certificates, but I'd probably prefer to keep a few old copies around for my own installations (again, I just personally tend to be overly-paranoid about deleting things :).

So my general idea for how we could maybe make this more configurable might be something like:

  • Keep the timestamped certs as-is.
  • Introduce a new keep_previous_certs option that you could use to tune how many previous certs to keep. We could maybe default it to 3 (or maybe that's just me still being overly-paranoid), but you could set it to 0 for your use-case of not wanting any backups.
  • After storing a new latest cert, we'd run a cleanup process to delete all the old certs except the last keep_previous_certs ones. This cleanup process would also help prune old certs from existing installations.

Does something like that approach make sense to you? If so, I know this more configurable approach might not be something you need for your use-case, so while you're more than welcome to implement it, I'm also happy to implement these changes at some point.

Thanks again!

@gohai
Copy link
Contributor Author

gohai commented May 8, 2018

@GUI Thanks for having a look!

From the point of our use-case: I feel that the benefit of having extra backups in Redis is limited unless there is a built-in mechanism to fall back to those backups in the event of an issue. We never had any issues with storing certificates - and I believe even accidentally deleting and re-requesting one is much less of an issue in terms of Let's Encrypt rate-limits, compared to the rate-limits one runs during other failure modes (e.g. Too Many Pending Authorizations).

So I believe in case of a problem we would most likely simply switch to a day-old backup, and study the differences with help of the log file. But any scheme that limits the amount of backups being held per-domain is certainly acceptable for us!

On our systems, I purged all backups from Redis (after harvesting their timestamps to set expiry) with:

EVAL "local keys = redis.call('keys', ARGV[1]) \n for i=1,#keys,5000 do \n redis.call('del', unpack(keys, i, math.min(i+4999, #keys))) \n end \n return keys" 0 *:15*

@bacheson
Copy link

bacheson commented Jun 7, 2018

I have 800mb of certs clogging a redis instance....I'd love to see the keep_previous_certs setting make it to the next release

@bacheson
Copy link

is this going to be merged in? this is becoming a huge problem for us

@GUI GUI merged commit 1b2dd8f into auto-ssl:master Apr 30, 2019
@GUI GUI added this to the v0.13.0 milestone Apr 30, 2019
@GUI
Copy link
Collaborator

GUI commented Sep 30, 2019

This is finally part of a release in v0.13.0 that's now published. Thanks!

@gohai gohai deleted the no-backups branch December 3, 2019 18:28
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.

3 participants