-
Notifications
You must be signed in to change notification settings - Fork 34
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
implement the On-Demand TLS feature #63
base: main
Are you sure you want to change the base?
Conversation
alright, I've made it work with my own fork of The modifications were super light, actually it was just a matter of accepting a new config option for kamal-proxy (+ point to my own Docker image of kamal-proxy). So, my proxy:
ssl: true
hosts: [""]
tls_on_demand_url: "http://staging-app-web-latest:3000/locomotive/api/allowed_host"
app_port: 3000
forward_headers: true Notes:
|
I also needed this feature to run the LocomotiveCMS hosting platform behind Kamal-Proxy 👉 did#1 |
thanks @kwilczynski for the code review! |
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.
Hey @did,
Thanks for getting this started! This does seem like a useful addition.
I left a couple of comments on the details. Let me know what you think about addressing those. (I'm also happy to make the changes myself if you don't have time).
from any host allowed by an external API endpoint of your choice. | ||
This avoids hard-coding hosts in the configuration, especially when you don't know the hosts at the startup. | ||
|
||
kamal-proxy deploy service1 --target web-1:3000 --host "" --tls --tls-on-demand-url="http://localhost:4567/check" |
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.
We shouldn't need to set an empty --host
here. We could modify this restriction so that --host
is not required with the On Demand feature.
This probably applies when using custom certificates as well, come to think of it.
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.
Agreed! But internally, we should use the empty string (or any *) as the host for the internal router, right?
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.
Yes, exactly! If it’s empty it will match everything, so if they are using on-demand TLS we can let them omit the flag to do that.
Wildcard routing could still be useful to allow multiple apps to run side by side, even if one or more of them is using on-demand TLS. So people should be allowed to specify the host string when they want. It’s just optional in that case.
return autocert.HostWhitelist(hosts...), nil | ||
} | ||
|
||
_, err := url.ParseRequestURI(options.TLSOnDemandUrl) |
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.
I'd expect that the on demand URL will usually point to an endpoint in the application that's deployed, rather than to some other external app. In which case, it would be simpler for this to be a path rather than an absolute URL. We'd then automatically call it on the currently deployed target (a bit like how the health check paths work).
That way you don't have to worry about having a stable hostname to reach for all versions of the app, etc., because the proxy takes care of that for you.
I'm not sure if there's a common enough need to support an external on demand URL as well, but for simplicity's sake it would be nice to have this be path-only if possible.
What do you think?
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.
👌 I haven't thought about it!!!
In my production case, it would work perfectly because my Rails app was always in charge of returning that list of hostnames (even when I was hosting it with k8s).
Indeed, it'd have saved me a lot of time, trying to figure out the hostname of my endpoint.
Perhaps (it's a guess), we should keep the URL as well for developers who prefers to move the responsibility of this endpoint to another app (and probably deployed by Kamal too) for performance or architecture reasons.
Let's keep it simple in a first time so let's use the path only :-)
(I will make the modifications)
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.
Sounds good!
return func(ctx context.Context, host string) error { | ||
slog.Info("Get a certificate", "host", host) | ||
|
||
resp, err := http.Get(fmt.Sprintf("%s?host=%s", options.TLSOnDemandUrl, url.QueryEscape(host))) |
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.
Would be worth having a timeout on this request. I don't think it needs to be user-configurable; just something reasonable like a couple seconds.
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.
Agreed. I'm wondering that even 2 or 2 seconds is probably too long and the TLS certificate generation will fail.
(I'll make the modifications)
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.
I think a couple of seconds should work OK, it just all leads to a slower experience for the client for the first request when a new cert is first generated. The initial cert generation typically takes a few seconds anyway. But, it could be worth testing that out to be sure.
Client: &acme.Client{DirectoryURL: options.ACMEDirectory}, | ||
}, nil | ||
} | ||
|
||
func (s *Service) createAutoCertHostPolicy(hosts []string, options ServiceOptions) (autocert.HostPolicy, error) { |
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.
Would be nice to have some test coverage of this, now that's getting a little more involved than just a HostWhiteList
. Same with the options parsing.
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.
Sure. I'll try to write some.
Big fan of Kamal & Kamal proxy here. Great job 👏
However, for my 2 Rails projects (website hosting for LocomotiveCMS & Maglev), I was missing the On-Demand TLS feature in Caddy.
It turns out it was easy to implement it in Kamal-Proxy since the
autocert.HostPolicy
type is just a function returning an error if the host is not allowed to get a certificate.So, just by setting a new config option (
--tls-on-demand-url
), we can test dynamically if a host can get a TLS certificate, just by calling the--tls-on-demand-url
endpoint.It just has to return a 200 HTTP code (http://my-api-end-point/any-path?host=).
In order to test it on my server, I implemented a little Sinatra service to test but I didn't include in my PR because it was in Ruby and the single example in your repository was written in Go. Besides, this is a super niche feature, so it might not require an example.
My next step is to build a Kamal proxy docker image and use it in the Kamal deployment of one of my projects.
Let me know if you want me to re-work the PR to match your PR rules.
Thanks!