Conversation
Wire production TLS terminus via golang.org/x/crypto/acme/autocert when --domain is set. Binds :443 for WSS and :80 for the ACME http-01 challenge; non-challenge :80 traffic gets 404 (explicit failure, not redirect) and :443 requests with a Host header that does not match --domain get 421 Misdirected Request per the protocol spec. NewAutocertManager creates the cache dir at 0700 if missing and refuses to start if an existing dir is group/world-readable (ErrCacheDirInsecure). TLSConfig pins MinVersion to TLS 1.2. Closes #9.
Contributor
Author
Code Review: #9Decision: PASS Findings
SummaryClean implementation that matches the spec verbatim. The Verified:
Ready to merge. |
Add feature doc covering the two-listener wiring (`:443` WSS via autocert, `:80` ACME http-01), the dual host gates (`HostWhitelist` for ACME issuance + `EnforceHost` for request-time 421s), the stricter-than-AC cache-dir permission policy, and the TLS 1.2 floor. ADR-0002 records the explicit-failure 404 on `:80` and why `HTTPHandler(nil)` is wrong for our posture (autocert redirects GET/HEAD to HTTPS by default; we want loud failure for misconfigured clients). Update PROJECT-MEMORY (autocert listed as built; first non-stdlib dep noted; loud-failure pattern added) and lessons.md with four new gotchas: HTTPHandler(nil) behaviour, SNI vs Host divergence, port stripping on r.Host, and gosec G402 / MinVersion on autocert's TLSConfig.
ilmoniemi
added a commit
that referenced
this pull request
May 8, 2026
Resolves additive doc conflicts from concurrent dispatch of #3 (registry), #9 (autocert), and #11 (threat-model): - docs/PROJECT-MEMORY.md: keep registry, autocert, and threat-model rows; drop superseded "TLS / autocert | Not started" placeholder; merge all Patterns established bullets. - docs/knowledge/INDEX.md: keep both new feature bullets; renumber the registry ADR link to 0003. - docs/lessons.md: keep all six new lesson sections (registry + autocert). - docs/knowledge/decisions/0002-connection-registry-passive-store.md -> 0003-connection-registry-passive-store.md (autocert ADR landed first via #9 / PR #13 and owns 0002). - docs/knowledge/features/connection-registry.md: update internal ADR link to 0003. No code changes. make vet / make test -race / make build all clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cmd/pyrycode-relay/main.goautocert path::443(WSS viamanager.TLSConfig) and:80(ACME http-01 viamgr.HTTPHandler(http.NotFoundHandler())— explicit 404, not 302) with the same timeouts as the insecure path.internal/relay/tls.goexportsNewAutocertManager,EnforceHost(returns421 Misdirected Requeston Host mismatch),TLSConfig(pinsMinVersion = TLS 1.2), and the sentinelErrCacheDirInsecure.0700if missing and rejected (ErrCacheDirInsecure) if an existing dir is group/world-readable — stricter than the AC's "no-op on existing" wording, justified in the spec's security review (TLS keys live there).golang.org/x/crypto/acme/autocerttogo.mod(first non-stdlib dep). README's Run section updated with the production command, ACME caveat, and cache-mode note.Issue
Closes #9.
Testing
internal/relay/tls_test.gocovers cache-dir creation (0700), no-op on existing secure dir, rejection of insecure dir, non-directory path, missing args,HostPolicyaccept/reject (case-mismatch + suffix attack),EnforceHosttable (port-tolerant, case-insensitive, 421 on mismatch with no body andnextnot invoked), andTLSConfigMinVersion pinning.go test -race ./...✅go vet ./...✅go build ./cmd/pyrycode-relay✅:443/:80listener startup are deliberately not tested (per spec — verified manually on first deploy).Architecture compliance
MinVersionpinning followdocs/specs/architecture/9-autocert-tls.mdexactly.http.NotFoundHandler()tomgr.HTTPHandler(notnil) so non-challenge:80traffic gets404instead of autocert's default302redirect — matches the AC's stated intent of explicit-failure semantics. Inline comment inmain.gonotes the rationale.HostWhitelist(*domain)); no graceful shutdown, no multi-domain, no--acme-email, no metrics — all explicitly out of scope.🤖 Generated with Claude Code