Repository navigation
feat(gateway): support HTTPS with manual and ACME certificates - #408
zlzGithub-0801 wants to merge 1 commit into
Conversation
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| DisableHTTPChallenge: true, | ||
| DisableTLSALPNChallenge: solver != nil, | ||
| ListenHost: host, | ||
| AltTLSALPNPort: portNumber, |
There was a problem hiding this comment.
When solver == nil (the default tls-alpn-01 path), CertMagic uses AltTLSALPNPort for its TLS-ALPN challenge listener. This is set to the gateway's own HTTPS port, which has already been bound by net.Listen before ManageAsync starts, so CertMagic cannot bind the challenge listener and issuance/renewal fails. Configure a distinct challenge port with the appropriate public 443 mapping, or arrange for the existing listener to handle the challenge instead of starting a second listener.
| Challenge string `json:"challenge"` | ||
| DNSProvider string `json:"dns_provider"` | ||
| // Sensitive: never include this field in logs. | ||
| CloudflareAPIToken string `json:"cloudflare_api_token"` |
There was a problem hiding this comment.
[high] This makes the credential part of the normal JSON representation of GatewayTLSConfig/Config. Any generic config dump, diagnostic response, or log using json.Marshal will emit the API token, contradicting the “never include this field in logs” requirement. Keep the field out of marshaling (while retaining a custom/unmarshal path for loading the JSON value), or implement an explicit redacted marshaler for the containing config types.
| blocked := []string{ | ||
| "0.0.0.0/8", "100.64.0.0/10", "192.0.0.0/24", "192.0.2.0/24", | ||
| "192.88.99.0/24", "198.18.0.0/15", "198.51.100.0/24", "203.0.113.0/24", "240.0.0.0/4", | ||
| "2001::/23", "2001:db8::/32", "2002::/16", "3fff::/20", |
There was a problem hiding this comment.
[medium] This allowlist still accepts the IPv6 well-known IPv4/IPv6 translation prefix 64:ff9b::/96: it is global-unicast and within 2000::/3, but is not in the blocked prefixes. That permits an address that cannot be used as a publicly issued certificate identity, and the repository's TestACMEIPAddressValidation explicitly expects 64:ff9b::808:808 to be rejected. Add this and the other relevant special-purpose prefixes to the rejection policy (or use an authoritative special-purpose check).
Suggestion:
| "2001::/23", "2001:db8::/32", "2002::/16", "3fff::/20", | |
| "64:ff9b::/96", "2001::/23", "2001:db8::/32", "2002::/16", "3fff::/20", |
a75934a to
eba2520
Compare
| _, port, err := net.SplitHostPort(g.HTTPSListenAddr) | ||
| n, portErr := strconv.Atoi(port) | ||
| if err != nil || portErr != nil || n < 1 || n > 65535 { |
There was a problem hiding this comment.
This only validates that the port parses; it does not validate the host or detect an address collision with g.MetricsListenAddr. For example, HTTPSListenAddr set to "bad host:443" passes here and fails later at net.Listen, while setting it equal to the metrics address causes the metrics server's bind to fail after startup. Validate the listener address/collision (or return a configuration error) before starting the gateway.
| blocked := []string{ | ||
| "0.0.0.0/8", "100.64.0.0/10", "192.0.0.0/24", "192.0.2.0/24", | ||
| "192.88.99.0/24", "198.18.0.0/15", "198.51.100.0/24", "203.0.113.0/24", "240.0.0.0/4", | ||
| "2001::/23", "2001:db8::/32", "2002::/16", "3fff::/20", | ||
| } |
There was a problem hiding this comment.
The IPv6 special-purpose 64:ff9b::/96 range is not included, so publicCertificateIP(netip.MustParseAddr("64:ff9b::808:808")) returns true even though this range is reserved for IPv4/IPv6 translation and cannot be used as a public ACME certificate identifier. The repository's TestACMEIPAddressValidation already expects this address to be rejected, so validation currently fails; add the missing reserved ranges (at least this one) or use a maintained special-purpose classification.
This branch has not been deployed
1 participant
What
Add gateway HTTPS support with three modes:
off(default): HTTP only.manual: HTTPS using supplied certificate and private key files.acme: HTTPS with automatic certificate issuance and renewal through CertMagic.Why
Allow clients to connect securely to the gateway.
Related issue
N/A
Scope and non-goals
Gateway TLS termination only. Rust nodes and CLI certificate trust configuration are unchanged.
Design and behavior changes
The gateway runs either HTTP or HTTPS. Manual certificates load at startup; ACME certificates are managed by CertMagic.
Compatibility and operations
gateway.tls; defaults tooff.offto restore HTTP.Validation
make fmtmake clippymake test-unitmake -C services test(required whenservices/changes)maketargetRisks and reviewer notes
Manual certificates require replacement and gateway restart before expiry. ACME deployments require correct challenge configuration and persistent certificate storage.
Checklist