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

Delete extra cert files after adding the cert to storage #155

Merged
merged 1 commit into from
Apr 30, 2019

Conversation

gohai
Copy link
Contributor

@gohai gohai commented Oct 30, 2018

dehydrated creates 5 files per domain, renewal. While they are not big in size, a seizable number of (sub-) domains can silently exhaust the number of available inodes on a system this way.

@gohai gohai force-pushed the delete-dehydrated-cert-files branch from be2c0db to fd915ca Compare October 30, 2018 20:06
dehydrated creates 5 files per domain, per renewal. While they are not big in size, a seizable number of (sub-) domains can this silently exhaust the number of available inodes.
@gohai gohai force-pushed the delete-dehydrated-cert-files branch from fd915ca to 1a1a1fb Compare October 31, 2018 18:53
@GUI
Copy link
Collaborator

GUI commented Nov 26, 2018

@gohai: Sorry for the delay in getting to this, but many thanks for submitting this!

So we do currently have some code in place that could potentially use these dehydrated certificate files as something of a backup if something goes wrong with the storage:

https://github.com/GUI/lua-resty-auto-ssl/blob/0fe6ab67f091d75821a865d8d71122dc04e8ff0d/lib/resty/auto-ssl/ssl_providers/lets_encrypt.lua#L55-L83

However, in thinking about all this again, I think that approach probably stemmed from when I was doing early local testing/debugging or just paranoid about the stability of things. But now I'm not sure we really need to keep that code around, since that case really shouldn't be hit, and even if it was, it's not the end of the world (the approach also wouldn't be very effective on a multi-server setup).

So I think all this looks good, I'm just getting the CI test suite to run again (in CircleCI 2.0, since our old 1.0 test suite is no longer running). After I get that working, I'll look to merge this in and cleanup some of that related code that this would make unnecessary.

@gohai
Copy link
Contributor Author

gohai commented Nov 26, 2018

Thanks for looking into this @GUI!

I thought along the same lines: in a multi-server setup you would not have the dehydrated files on all instances, which made the idea of deleting them locally (once they are in storage) more appealing.

As for the code that attempts to re-upload dehydrated files to storage: I think this could potentially still have some merit in the case that storage:set_cert fails (e.g. because Redis was unavailable). In this case my patch is not deleting any local files, so presumably the code you quoted could attempt to re-upload it then? (perhaps the comment at top could be clarified though)

@GUI GUI merged commit 52967b5 into auto-ssl:master Apr 30, 2019
@GUI GUI added this to the v0.13.0 milestone Apr 30, 2019
GUI added a commit that referenced this pull request May 1, 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 delete-dehydrated-cert-files 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.

2 participants