hotfix: Caddy HandShake #105
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReplaced Caddy internal mTLS listener with explicit server cert/key and simplified client CA trust_pool syntax; removed IP-based gating from PublicMiddleware (deleted HasInvalidIP and its invocation); added a Makefile target Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant M as PublicMiddleware
participant R as RateLimiter
participant H as AppHandler
Note over M: IP-based gate removed (HasInvalidIP deleted)
C->>M: HTTP request
M->>M: Validate timestamp
M->>M: Check duplicate request ID
M->>R: Request rate token / account usage
alt Allowed
R-->>M: OK
M->>H: Forward request
H-->>M: Response
M-->>C: Response
else Rejected (rate/timestamp/duplicate)
R-->>M: Reject / M forms error
M-->>C: Error response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @gocanto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This hotfix addresses an issue with Caddy's TLS handshake by adjusting its mTLS configuration to use explicit certificate and key files. Concurrently, an IP validation check has been removed from a public middleware, streamlining the request handling process and potentially delegating IP-based access control to another system or layer.
Highlights
- Caddy mTLS Configuration Update: Modified the
Caddyfile.prodto explicitly define TLS certificates and keys (/etc/caddy/mtls/ca.pem,/etc/caddy/mtls/ca.key) for the mTLS entrypoint, replacing thetls internaldirective. - Removal of IP Validation Middleware: Eliminated the
HasInvalidIPfunction and its invocation frompkg/middleware/public_middleware.go, removing IP address validation from this specific public middleware.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to fix a Caddy handshake issue and modifies a middleware. My review found two significant issues:
- A critical security vulnerability in
caddy/Caddyfile.prodwhere a CA private key is used as a server key. - In
pkg/middleware/public_middleware.go, IP validation logic was removed, but the corresponding tests were not updated, which will likely break the build.
Please address these issues to improve security and maintain code quality.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
caddy/Caddyfile.prod (3)
101-104: Mirror upstream hardening (timeouts, Host) for the mTLS pathThe main site’s proxy sets
dial_timeoutandresponse_header_timeoutand preserves theHostheader. Do the same here to avoid hanging connections and subtle header differences.handle_path /api/generate-signature* { - reverse_proxy api:8080 + reverse_proxy api:8080 { + header_up Host {host} + transport http { + dial_timeout 10s + response_header_timeout 30s + } + } }
105-107: Consider 404 instead of 403 for non-matching pathsReturning 404 on the internal listener reduces signal to unauthenticated scanners about protected endpoints. Keep 403 if you prefer explicit denial.
- handle { - respond 403 - } + handle { + respond 404 + }
99-99: Minor: compression likely unnecessary on the internal mTLS portIf 8443 only serves
/api/generate-signature*, dropencodeto save a few CPU cycles. Low impact.- encode gzip zstd + # encode gzip zstd # likely unnecessary on this narrow internal endpoint
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
caddy/Caddyfile.prod(1 hunks)pkg/middleware/public_middleware.go(0 hunks)
💤 Files with no reviewable changes (1)
- pkg/middleware/public_middleware.go
🔇 Additional comments (2)
caddy/Caddyfile.prod (2)
92-97: Version pin check: trust_pool shorthand requires Caddy v2.8+
trust_pool file /etc/.../ca.pem(inline form) is valid on v2.8+; older versions expect the block formtrust_pool file { pem_file /path }. Ensure your image/tag is ≥ v2.8 or switch to the block syntax. (pkg.go.dev)If you’re not sure of the running Caddy version, exec into the container and run
caddy version.
92-97: Critical: server is presenting CA cert/key — replace with a leaf server cert/keyUsing /etc/caddy/mtls/ca.pem and ca.key as the server cert/key is dangerous: the server must present a leaf certificate whose SAN matches the hostname; a CA cert (BasicConstraints CA:TRUE) will fail validation and storing the CA private key on the webserver is a severe secret-management risk. Keep the CA only for client verification via trust_pool.
Verification failed: openssl not found in the environment, so I couldn't confirm whether ca.pem is a CA or whether server.pem has the correct SANs. Install openssl and re-run the check below (or paste its output).
Apply this diff (adjust paths to your actual server leaf cert/key):
- tls /etc/caddy/mtls/ca.pem /etc/caddy/mtls/ca.key { + tls /etc/caddy/mtls/server.pem /etc/caddy/mtls/server.key { client_auth { mode require_and_verify trust_pool file /etc/caddy/mtls/ca.pem } }Run after installing openssl:
#!/bin/bash set -euo pipefail echo "CA basic constraints for /etc/caddy/mtls/ca.pem:" openssl x509 -in /etc/caddy/mtls/ca.pem -noout -text | sed -n '/X509v3 Basic Constraints/,/X509v3/p' || true echo echo "Server cert SANs for /etc/caddy/mtls/server.pem:" openssl x509 -in /etc/caddy/mtls/server.pem -noout -text | sed -n '/Subject Alternative Name/,/X509v3/p' || true
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
caddy/Caddyfile.prod (1)
102-104: Correct: enforce mTLS on a dedicated listener, not by path.TLS client auth can’t vary by path since the handshake precedes HTTP routing; a separate
:8443site is the right approach. Optionally mirror upstream timeouts here as on the public/api/*block for parity. (caddy.community)handle_path /api/generate-signature* { - reverse_proxy api:8080 + reverse_proxy api:8080 { + transport http { + dial_timeout 10s + response_header_timeout 30s + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
caddy/Caddyfile.prod(1 hunks)metal/makefile/caddy.mk(1 hunks)
🔇 Additional comments (8)
caddy/Caddyfile.prod (3)
92-99: mTLS config is syntactically correct for Caddy v2.10; keep using the new trust_pool shorthand.
tls /etc/caddy/mtls/server.pem /etc/caddy/mtls/server.keywithclient_auth { mode require_and_verify; trust_pool file /etc/caddy/mtls/ca.pem }is valid and preferred over deprecated fields. (caddy.community)
100-100: Compression on :8443 is fine.No issues; harmless for the protected listener.
106-108: Default-deny fallback looks good.403 catch-all on
:8443prevents accidental exposure of non-protected routes.metal/makefile/caddy.mk (5)
1-1: PHONY updated correctly.Including
caddy-freshavoids stale targets.
7-9: Nice QoL target.
caddy-freshchaining delete+gen is handy for local resets.
47-48: Potential permission issue inside the caddy container.
chmod 600 server.keymay block the (non-root)caddyuser from reading the key when bind-mounted. Consider640and setting group to match the container’s effective GID, or run the container as root explicitly. Please confirm your runtime user.
57-62: Good: cleanup includes server key/cert.Removes all relevant artifacts; avoids partial state.
65-68: Validate with the same Caddy version you run.You target
caddy:2.10.0here; ensure production uses 2.10+ so thetrust_poolsyntax is recognized. (caddy.community)
| tls /etc/caddy/mtls/server.pem /etc/caddy/mtls/server.key { | ||
| client_auth { | ||
| mode require_and_verify | ||
| trust_pool file { | ||
| pem_file /etc/caddy/mtls/ca.pem | ||
| } | ||
| # The client must present a cert trusted by the CA | ||
| trust_pool file /etc/caddy/mtls/ca.pem | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Blocking issue: server.pem must have SANs matching the SNI clients use.
Your Makefile generates a server cert with only CN=oullin_proxy_prod and no subjectAltName. Modern clients (Chrome 58+, Go 1.15+) ignore CN for hostname verification; without SANs, the TLS handshake will fail even before HTTP routing/mTLS verification. See my Makefile comment with a patch to add SAN/EKU. (alexanderzeitler.com)
🤖 Prompt for AI Agents
In caddy/Caddyfile.prod around lines 92 to 99, the server certificate file
server.pem was created with only a CN and no subjectAltName (SAN), which modern
TLS clients ignore for hostname verification; update the Makefile target that
generates server.pem to include proper SANs (DNS names and any IPs) that match
the SNI hostnames clients use and ensure the certificate includes the serverAuth
EKU; implement this by adding a custom OpenSSL config or command-line flags to
inject subjectAltName and extendedKeyUsage when creating/signing the server cert
(or reissue the cert via the CA), then regenerate server.pem and server.key and
verify the SANs match the service hostnames used by clients.
| openssl genrsa -out "$(CADDY_MTLS_DIR)/ca.key" 4096; \ | ||
| printf "$(BLUE)📜 Creating self-signed CA certificate...$(NC)\n"; \ | ||
| openssl req -x509 -new -key "$(CADDY_MTLS_DIR)/ca.key" -sha256 -days 3650 \ | ||
| -subj "/CN=oullin-mtls-ca" -out "$(CADDY_MTLS_DIR)/ca.pem" >/dev/null 2>&1; \ | ||
| -subj "/CN=oullin-mtls-ca" -out "$(CADDY_MTLS_DIR)/ca.pem"; \ | ||
| printf '01\n' > "$(CADDY_MTLS_DIR)/ca.srl"; \ | ||
| chmod 600 "$(CADDY_MTLS_DIR)/ca.key"; \ | ||
| chmod 644 "$(CADDY_MTLS_DIR)/ca.pem" "$(CADDY_MTLS_DIR)/ca.srl"; \ | ||
| printf "$(GREEN)✅ CA written to %s$(NC)\n" "$(CADDY_MTLS_DIR)"; \ | ||
| printf "$(WHITE)🔍 CA fingerprint:$(NC)\n"; \ | ||
| openssl x509 -noout -fingerprint -sha256 -in "$(CADDY_MTLS_DIR)/ca.pem" | sed 's/^/ /'; \ | ||
| fi | ||
| fi; \ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
CA certificate lacks essential v3 extensions.
As written, the CA may be missing basicConstraints/keyUsage, which some stacks expect on CAs. Add v3 CA extensions at issuance. (github.com)
- openssl genrsa -out "$(CADDY_MTLS_DIR)/ca.key" 4096; \
- printf "$(BLUE)📜 Creating self-signed CA certificate...$(NC)\n"; \
- openssl req -x509 -new -key "$(CADDY_MTLS_DIR)/ca.key" -sha256 -days 3650 \
- -subj "/CN=oullin-mtls-ca" -out "$(CADDY_MTLS_DIR)/ca.pem"; \
+ openssl genrsa -out "$(CADDY_MTLS_DIR)/ca.key" 4096; \
+ printf "$(BLUE)📜 Creating self-signed CA certificate...$(NC)\n"; \
+ openssl req -x509 -new -key "$(CADDY_MTLS_DIR)/ca.key" -sha256 -days 3650 \
+ -subj "/CN=oullin-mtls-ca" \
+ -addext "basicConstraints=critical,CA:TRUE" \
+ -addext "keyUsage=critical,keyCertSign,cRLSign" \
+ -addext "subjectKeyIdentifier=hash" \
+ -out "$(CADDY_MTLS_DIR)/ca.pem"; \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| openssl genrsa -out "$(CADDY_MTLS_DIR)/ca.key" 4096; \ | |
| printf "$(BLUE)📜 Creating self-signed CA certificate...$(NC)\n"; \ | |
| openssl req -x509 -new -key "$(CADDY_MTLS_DIR)/ca.key" -sha256 -days 3650 \ | |
| -subj "/CN=oullin-mtls-ca" -out "$(CADDY_MTLS_DIR)/ca.pem" >/dev/null 2>&1; \ | |
| -subj "/CN=oullin-mtls-ca" -out "$(CADDY_MTLS_DIR)/ca.pem"; \ | |
| printf '01\n' > "$(CADDY_MTLS_DIR)/ca.srl"; \ | |
| chmod 600 "$(CADDY_MTLS_DIR)/ca.key"; \ | |
| chmod 644 "$(CADDY_MTLS_DIR)/ca.pem" "$(CADDY_MTLS_DIR)/ca.srl"; \ | |
| printf "$(GREEN)✅ CA written to %s$(NC)\n" "$(CADDY_MTLS_DIR)"; \ | |
| printf "$(WHITE)🔍 CA fingerprint:$(NC)\n"; \ | |
| openssl x509 -noout -fingerprint -sha256 -in "$(CADDY_MTLS_DIR)/ca.pem" | sed 's/^/ /'; \ | |
| fi | |
| fi; \ | |
| openssl genrsa -out "$(CADDY_MTLS_DIR)/ca.key" 4096; \ | |
| printf "$(BLUE)📜 Creating self-signed CA certificate...$(NC)\n"; \ | |
| openssl req -x509 -new -key "$(CADDY_MTLS_DIR)/ca.key" -sha256 -days 3650 \ | |
| -subj "/CN=oullin-mtls-ca" \ | |
| -addext "basicConstraints=critical,CA:TRUE" \ | |
| -addext "keyUsage=critical,keyCertSign,cRLSign" \ | |
| -addext "subjectKeyIdentifier=hash" \ | |
| -out "$(CADDY_MTLS_DIR)/ca.pem"; \ | |
| printf '01\n' > "$(CADDY_MTLS_DIR)/ca.srl"; \ | |
| chmod 600 "$(CADDY_MTLS_DIR)/ca.key"; \ | |
| chmod 644 "$(CADDY_MTLS_DIR)/ca.pem" "$(CADDY_MTLS_DIR)/ca.srl"; \ | |
| printf "$(GREEN)✅ CA written to %s$(NC)\n" "$(CADDY_MTLS_DIR)"; \ | |
| printf "$(WHITE)🔍 CA fingerprint:$(NC)\n"; \ | |
| openssl x509 -noout -fingerprint -sha256 -in "$(CADDY_MTLS_DIR)/ca.pem" | sed 's/^/ /'; \ | |
| fi; \ |
🤖 Prompt for AI Agents
In metal/makefile/caddy.mk around lines 24 to 34, the CA is created without v3
CA extensions; update the OpenSSL invocation to include standard CA v3
extensions (basicConstraints=CA:TRUE, keyUsage=critical,keyCertSign,cRLSign, and
recommended subjectKeyIdentifier/authorityKeyIdentifier) by either supplying an
-extfile with a small temporary config section (with an [ v3_ca ] stanza) or
using -addext flags when running openssl req/x509 so the issued ca.pem contains
the proper basicConstraints/keyUsage and related extensions.
| printf "$(YELLOW)⚠️ Server certificate already exists.$(NC)\n"; \ | ||
| else \ | ||
| umask 077; \ | ||
| printf "$(BLUE)🔑 Generating Server private key...$(NC)\n"; \ | ||
| openssl genrsa -out "$(CADDY_MTLS_DIR)/server.key" 4096; \ | ||
| printf "$(WHITE)📜 Creating Server signing request...$(NC)\n"; \ | ||
| openssl req -new -key "$(CADDY_MTLS_DIR)/server.key" -subj "/CN=oullin_proxy_prod" -out "$(CADDY_MTLS_DIR)/server.csr"; \ | ||
| printf "$(NC)🖊️ Signing Server certificate with CA...$(NC)\n"; \ | ||
| openssl x509 -req -in "$(CADDY_MTLS_DIR)/server.csr" \ | ||
| -CA "$(CADDY_MTLS_DIR)/ca.pem" -CAkey "$(CADDY_MTLS_DIR)/ca.key" -CAserial "$(CADDY_MTLS_DIR)/ca.srl" \ | ||
| -out "$(CADDY_MTLS_DIR)/server.pem" -days 1095 -sha256; \ | ||
| chmod 600 "$(CADDY_MTLS_DIR)/server.key"; \ | ||
| chmod 644 "$(CADDY_MTLS_DIR)/server.pem"; \ | ||
| rm "$(CADDY_MTLS_DIR)/server.csr"; \ | ||
| printf "$(GREEN)✅ Server certificate written to %s$(NC)\n" "$(CADDY_MTLS_DIR)"; \ | ||
| fi; \ | ||
| printf "$(NC)🔎 Verifying server cert against CA...$(NC)\n"; \ | ||
| openssl verify -CAfile "$(CADDY_MTLS_DIR)/ca.pem" "$(CADDY_MTLS_DIR)/server.pem" | ||
|
|
There was a problem hiding this comment.
Blocking issue: server cert is missing SAN and EKU—clients will reject it.
Without SANs, hostname verification fails; add SAN plus key usages for a TLS server. Go and modern browsers require SANs. (github.com)
- openssl x509 -req -in "$(CADDY_MTLS_DIR)/server.csr" \
- -CA "$(CADDY_MTLS_DIR)/ca.pem" -CAkey "$(CADDY_MTLS_DIR)/ca.key" -CAserial "$(CADDY_MTLS_DIR)/ca.srl" \
- -out "$(CADDY_MTLS_DIR)/server.pem" -days 1095 -sha256; \
+ openssl x509 -req -in "$(CADDY_MTLS_DIR)/server.csr" \
+ -CA "$(CADDY_MTLS_DIR)/ca.pem" -CAkey "$(CADDY_MTLS_DIR)/ca.key" -CAserial "$(CADDY_MTLS_DIR)/ca.srl" \
+ -days 1095 -sha256 \
+ -addext "subjectAltName=${CADDY_SERVER_SANS}" \
+ -addext "keyUsage=critical,digitalSignature,keyEncipherment" \
+ -addext "extendedKeyUsage=serverAuth" \
+ -out "$(CADDY_MTLS_DIR)/server.pem"; \Add this variable once near the top (outside this hunk) to control SANs:
# e.g., DNS names clients/SNI will use; add IPs if connecting by IP
CADDY_SERVER_SANS ?= DNS:oullin.ioIf your OpenSSL lacks -addext, use -extfile with a v3 ext stanza; I can provide that variant if needed.
🤖 Prompt for AI Agents
metal/makefile/caddy.mk around lines 35-53: the generated server certificate
lacks SAN and EKU which causes modern clients (including Go and browsers) to
reject it; add a new variable near the top such as CADDY_SERVER_SANS ?=
DNS:oullin.io (allowing IP: entries as needed), then when creating/signing the
server cert generate or reference an OpenSSL extfile that sets subjectAltName =
${CADDY_SERVER_SANS} and extendedKeyUsage = serverAuth and pass that extfile to
openssl x509 using -extfile (or use -addext if you know the OpenSSL supports
it); ensure the Make rule creates the extfile with secure permissions, uses it
in the signing step, and removes it after signing so the resulting server.pem
includes both SAN and TLS server EKU.
Summary by CodeRabbit
New Features
Chores