search

basecamp / kamal-proxy

Lightweight proxy server for Kamal
https://kamal-deploy.org/
MIT License
751 stars 31 forks source link

Added option to use custom TLS certificates instead of ACME #17

Closed kpumuk closed 1 month ago

kpumuk commented 2 months ago

In some cases it is needed to use a custom TLS certificate:

With Traefic we had an option to specify a custom certificate, and so this change brings this feature into kamal-proxy.

PS. I am still looking into adding tests, wanted to gauge opinions whether this is a feature you are interested in. covered by tests now

Related kamal changes

furkansahin commented 2 months ago

This looks amazing, I was also wondering if it would be possible to define an endpoint to pull certificate and key files from a local endpoint? At @ubicloud, we provide certificates in a local endpoint and customer is requested to pull these files and make use of them. I can also try to create a PR if you prefer.

kpumuk commented 2 months ago

At @ubicloud, we provide certificates in a local endpoint and customer is requested to pull these files and make use of them.

Do you mean like a local CA, or simply a secrets manager that returns the same certificate on every call?

Edit: Found, https://www.ubicloud.com/docs/quick-start/using-kamal-with-ubicloud

I think with the proposed CertManager interface it will be trivial to implement a caching cert fetcher. Probably out of scope for this MR, unless authors disagree :)

furkansahin commented 2 months ago

@kpumuk yes, I was specifically asking to eliminate the step https://www.ubicloud.com/docs/quick-start/using-kamal-with-ubicloud#step-9 :) It would make life much easier.

It makes sense that this is out of scope for this PR. I'll work on it and hopefully it's something useful to the community.

kevinmcconnell commented 2 months ago

I was also wondering if it would be possible to define an endpoint to pull certificate and key files from a local endpoint?

I don't think the proxy should be responsible for this. Using a Kamal hook seems like a better option to me. Or, if it turned out to be a common enough requirement, then perhaps it could even be a feature on the Kamal side. But I'd rather the proxy not be involved with gathering artefacts like this; they should be supplied to it wherever possible.

kpumuk commented 1 month ago

@kevinmcconnell seems like the documentation change was merged ahead of the functional change :-)

Do you want to change the initialization interface in this MR to return the error, or keep the mutex and refactor later?

Edit: might have been merged after this thread.

kpumuk commented 1 month ago

Benchmark WITHOUT mutex:

Running 30s test @ https://kamal.test:8091/
  5 threads and 5 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   451.35us  162.60us   7.68ms   93.69%
    Req/Sec     2.24k   101.26     2.46k    81.07%
  334834 requests in 30.00s, 264.72MB read
Requests/sec:  11159.76
Transfer/sec:      8.82MB

Benchmark WITH mutex:

Running 30s test @ https://kamal.test:8091/
  5 threads and 5 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   446.87us  144.43us   7.29ms   91.86%
    Req/Sec     2.26k    99.38     2.54k    83.11%
  338088 requests in 30.10s, 267.29MB read
Requests/sec:  11232.35
Transfer/sec:      8.88MB

So the only concern is about ergonomics — when the error is shown for inaccessible cert files - during deploy or during the first request.

kpumuk commented 1 month ago

@kevinmcconnell I have rebased the branch on top of recent changes, which required to add HTTPHandler to the certificate manager interface. Steps to hook it all up:

https://github.com/basecamp/kamal/pull/969#issuecomment-2383927911

This requires two changes on kamal side:

What do you think? Any ideas on how to simplify the setup?

Edit: Realized that proxy is shared and so might not be the brightest idea if every service mounted a volume .into the container...

Edit 2: Another option is to copy certs directly to the kamal-proxy config volume:

#!/bin/sh

set -euo pipefail

KAMAL_PROXY_TLS_CERT=$(op read "op://Private/Kamal Demo/cert.pem")
KAMAL_PROXY_TLS_PRIVATE_KEY=$(op read "op://Private/Kamal Demo/key.pem")

for ip in ${KAMAL_HOSTS//,/ }; do
  ssh -q -T -o BatchMode=yes ubuntu@"${ip}" bash --noprofile <<-EOF
    docker run --rm --volume kamal-proxy-config:/home/kamal-proxy/.config/kamal-proxy ubuntu:noble-20240801 sh -c "
      mkdir -p /home/kamal-proxy/.config/kamal-proxy/certs
      echo '${KAMAL_PROXY_TLS_CERT}' > /home/kamal-proxy/.config/kamal-proxy/certs/cert.pem
      echo '${KAMAL_PROXY_TLS_PRIVATE_KEY}' > /home/kamal-proxy/.config/kamal-proxy/certs/key.pem
    "
EOF
done

This can be used to improve ergonomics of the command, and in addition to low-level API to pass keys directly, Kamal can do something like:

proxy:
  ssl: true
  ssl_certificate: <%= KAMAL_PROXY_TLS_CERT %>
  ssl_private_key: <%= KAMAL_PROXY_TLS_PRIVATE_KEY %>

This way the values can come from a secret manager, and Kamal can store them in /home/kamal-proxy/.config/kamal-proxy/certs/<%= KAMAL_SERVICE %>/{cert,key}.pem automatically. This would eliminate the need of a custom hook and docker manipulation.

kevinmcconnell commented 1 month ago

This all looks great, @kpumuk! I have an idea in mind for how to handle the errors earlier in the deploy process as we discussed, but that also has a few knock-on effects I want to consider. So I'll get this PR merged first as-is, and then I'll explore that separately. It doesn't need to hold up this feature.

This can be used to improve ergonomics of the command, and in addition to low-level API to pass keys directly, Kamal can do something like: ...

Yes, agreed. Using the existing mounted volume makes sense, and we can have Kamal scope it to the service name to avoid conflicts. (We'll need to do something similar for the --error-pages flag anyway). Treating the cert/key content as secrets rather than requiring file paths is a nice idea too, that seems much more convenient. /cc @djmb

kevinmcconnell commented 1 month ago

Thanks for your work on this, @kpumuk! 🙏

djmb commented 1 month ago

I've reverted that documentation change for now. We'd need to make it differently anyway as configuration docs are generated from proxy.yml by the bin/docs script in Kamal.