-
Notifications
You must be signed in to change notification settings - Fork 782
DEV: better SSL detection #986
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
Conversation
With web.ssl.template.yml added: enable configuring of ssl if /shared/ssl/ssl.crt and /shared/ssl/ssl.key exists With web.letsencrypt.ssl.template.yml added: in addition to ssl detection, enable configuring of letsencrypt if LETSENCRYPT_ACCOUNT_EMAIL exists and is valid.
Followup to #985 This also allows the letsencrypt template to still install regular ssl keys if they exist, skipping out of the letsencrypt setup early. |
@@ -121,22 +121,18 @@ run: | |||
|
|||
hooks: | |||
after_ssl: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of running after_ssl
, I'm curious if we are able to obtain the letsencrypt certs in a before_ssl
hooks so that the only responsibility of this script is to get certs and put it in the right directory for the install-ssl
script that will run after. That way, we don't need to couple the two templates together through the # after ssl
placeholders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately the build isn't able to complete the server challenges (no exposed ports, building on a separate system).
Now if we pivoted to doing dns challenges, that method could work...though then we'd require end users to be dependent on supported dns providers, which is a different kettle of fish.
re, coupling, we're also arguably already coupled since the modifications on the nginx config already assume the ones from the base ssl template already ran.
it is also a pattern we do in web template - I'm guessing it's due to the lack of job dependencies available in runit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The let's encrypt folks aren't doing notifications anymore, so there's not much reason to use LETSENCRYPT_ACCOUNT_EMAIL
https://community.letsencrypt.org/t/support-ended-for-expiration-notification-emails/238173
It seems like if they include the let's encrypt template that is safe to assume they want ssl and let's encrypt.
Way too many people use dns that you can't do dns challenges for and of those remaining, way too many would never figure out how to configure dns challenges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pfaffman thanks for mentioning that - I'll still need a way of opting in to collecting the certificate at runtime for the reasons mentioned above though.
Even if we were to move from detecting the LETSENCRYPT_ACCOUNT_EMAIL, I'd still like to add an option to enable/disable fetching/configuring the certs via an env var - If it helps, think of this as a parallel with plugins just because a site has a plugin installed does not necessarily mean it should be enabled. Just because a site has been built with this template does not necessarily mean one should be fetched.
I'm attempting to go from this:
including this template adds ssl unconditionally at runtime
to this:
including this template gives you the option to add ssl at runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: I'm not entirely sure I understand just what you're trying to do, so apologies if this isn't helpful.
I don't know who would include the template and expect it not to do what it's designed to do, but I think if you want to add that (I'm pretty sure that before discourse-setup forced you to set an address, and I think the template probably requested one regardless of whether the address was set).
Wait. I see, I think you're trying to really go after making it so people don't have to build their own images and that's what some of this is about. So you're trying to solve the problem of the I-want-docker-compose people. I've been travelling and haven't been paying as close attention to this as I might otherwise. Yes. And that explains why requiring a "ENABLE_SSL" variable made sense--I think I'm catching up.
But aren't the docker-compose people (if that is the audience for this) going to use some other reverse proxy or whatever to get certs? I may be missing something, but I think it might be OK just to not have those images ever request certs. If you really are trying to make one set of images that will work for docker compose and the current self-hosting/launcher crowd, that seems really hard.
I do think that you want to stop using LETSENCRYPT_ACCOUNT_EMAIL, though, so maybe something like USE_LETSENCRYPT.
. .. . anyway, if you want to make the template not request certs then I think you'd add a "DO_NOT_REQUEST_CERTS" variable, at least to make it least likely that you'll break things for self-hosters.
Or maybe what you do is have ENABLE_SSL and USE_LETSENCRYPT be true by default and put it on the people who want to use the images to turn that stuff off. Or, maybe, set those variables some other way with the tool that builds the new magic images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, by using env vars, it's not too difficult - pre-built images wouldn't include env vars, so by default nothing would be triggered.
For example, take the inital PR in #985, where ENABLE_SSL: 1
is set in the env.
For a launcher bootstrap, ENABLE_SSL is bound to the image, so it will be turned on by default.
For a prebuild, all non-default env -- including ENABLE_SSL -- is missing from the final image, so it will be turned OFF by default.
Something similar could happen for the letsencrypt template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Sounds like that could work.
The tricky bit (to me) is figuring out how to get those defaults to work for existing self-hosted folks while also making sensible defaults for the pre-built images. And, it sounds like you've figured out how to pull that off. Once those exist, I will definitely consider rolling out some support to get people started (and maintained) with something like traefik or nginx-proxy.
The real trick, I think, is that if and when the pre-built images are a thing, it's still going to be a whole other support vector. Hopefully it'll attract folks who at least mostly know what they are doing, or what they want to do. So many of the current self-hosted people have never touched a command line and mostly don't understand DNS.
With web.ssl.template.yml added:
enable configuring of ssl if /shared/ssl/ssl.crt and /shared/ssl/ssl.key exists
With web.letsencrypt.ssl.template.yml added:
in addition to ssl detection, enable configuring of letsencrypt if LETSENCRYPT_ACCOUNT_EMAIL exists and is valid.