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

implement the On-Demand TLS feature #63

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,20 @@ applications. To enable this, add the `--tls` flag when deploying an instance:
kamal-proxy deploy service1 --target web-1:3000 --host app1.example.com --tls


### On-demand TLS

In addition of the automatic TLS functionality, Kamal Proxy can also dynamically obtain a TLS certificate
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"
Copy link
Collaborator

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.

Copy link
Author

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?

Copy link
Collaborator

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.


The On-demand URL endpoint will have to answer a 200 HTTP status code.
Kamal Proxy will call the on-demand URL with a query string of `?host=` containing the host received by Kamal Proxy.

It also must respond as fast as possible, a couple of milliseconds top.


### Custom TLS certificate

When you obtained your TLS certificate manually, manage your own certificate authority,
Expand Down
1 change: 1 addition & 0 deletions internal/cmd/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ func newDeployCommand() *deployCommand {
deployCommand.cmd.Flags().StringSliceVar(&deployCommand.args.Hosts, "host", []string{}, "Host(s) to serve this target on (empty for wildcard)")

deployCommand.cmd.Flags().BoolVar(&deployCommand.args.ServiceOptions.TLSEnabled, "tls", false, "Configure TLS for this target (requires a non-empty host)")
deployCommand.cmd.Flags().StringVar(&deployCommand.args.ServiceOptions.TLSOnDemandUrl, "tls-on-demand-url", "", "Will make an HTTP request to the given URL, asking whether a host is allowed to have a certificate issued.")
deployCommand.cmd.Flags().BoolVar(&deployCommand.tlsStaging, "tls-staging", false, "Use Let's Encrypt staging environment for certificate provisioning")
deployCommand.cmd.Flags().StringVar(&deployCommand.args.ServiceOptions.TLSCertificatePath, "tls-certificate-path", "", "Configure custom TLS certificate path (PEM format)")
deployCommand.cmd.Flags().StringVar(&deployCommand.args.ServiceOptions.TLSPrivateKeyPath, "tls-private-key-path", "", "Configure custom TLS private key path (PEM format)")
Expand Down
41 changes: 40 additions & 1 deletion internal/server/service.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
package server

import (
"context"
"crypto/sha256"
"encoding/hex"
"encoding/json"
"errors"
"fmt"
"log/slog"
"net"
"net/http"
"net/url"
"os"
"path"
"strings"
Expand Down Expand Up @@ -66,6 +69,7 @@ type HealthCheckConfig struct {

type ServiceOptions struct {
TLSEnabled bool `json:"tls_enabled"`
TLSOnDemandUrl string `json:"tls_on_demand_url"`
TLSCertificatePath string `json:"tls_certificate_path"`
TLSPrivateKeyPath string `json:"tls_private_key_path"`
ACMEDirectory string `json:"acme_directory"`
Expand Down Expand Up @@ -318,14 +322,49 @@ func (s *Service) createCertManager(hosts []string, options ServiceOptions) (Cer
}
}

hostPolicy, err := s.createAutoCertHostPolicy(hosts, options)
if err != nil {
return nil, err
}

return &autocert.Manager{
Prompt: autocert.AcceptTOS,
Cache: autocert.DirCache(options.ScopedCachePath()),
HostPolicy: autocert.HostWhitelist(hosts...),
HostPolicy: hostPolicy,
Client: &acme.Client{DirectoryURL: options.ACMEDirectory},
}, nil
}

func (s *Service) createAutoCertHostPolicy(hosts []string, options ServiceOptions) (autocert.HostPolicy, error) {
Copy link
Collaborator

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.

Copy link
Author

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.

if options.TLSOnDemandUrl == "" {
return autocert.HostWhitelist(hosts...), nil
}

_, err := url.ParseRequestURI(options.TLSOnDemandUrl)
Copy link
Collaborator

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?

Copy link
Author

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good!

if err != nil {
slog.Error("Unable to parse the tls_on_demand_url URL")
return nil, err
}

slog.Info("Will use the tls_on_demand_url URL", "url", options.TLSOnDemandUrl)

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)))
Copy link
Collaborator

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.

Copy link
Author

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)

Copy link
Collaborator

@kevinmcconnell kevinmcconnell Dec 17, 2024

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.

if err != nil {
slog.Error("Unable to reach the TLS on demand URL", host, err)
return err
}

if resp.StatusCode != http.StatusOK {
return fmt.Errorf("%s is not allowed to get a certificate", host)
}

return nil
}, nil
}

func (s *Service) createMiddleware(options ServiceOptions, certManager CertManager) (http.Handler, error) {
var err error
var handler http.Handler = http.HandlerFunc(s.serviceRequestWithTarget)
Expand Down
Loading