feat: select HTTPS certificates by TLS server name (SNI) - #746
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (20)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSecured listeners can now use named certificate/key pairs selected by the TLS ClientHello SNI hostname. A listener can also specify an optional default pair. Parser, static validation, runtime TLS setup, tests, examples, and documentation cover the new configuration. ChangesTLS SNI certificate selection
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~50 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant WFLInterpreter
participant TLSConfigLoader
participant TLSClient
participant CertificateResolver
WFLInterpreter->>TLSConfigLoader: Load default and named certificate configuration
TLSClient->>CertificateResolver: Send ClientHello with SNI hostname
CertificateResolver->>TLSClient: Return matching certificate or explicit default
Merge Risk: ⚪ Minimal · up to Secured listeners can now serve different certificates for different domains on one HTTPS port. Listeners with only named certificates reject unknown or missing names, and an optional first unnamed certificate serves as the fallback. Existing single-certificate and configuration-based setups keep their behavior. Invalid certificate configurations fail at startup, before the listener binds. No concrete defects remain, so the change is ready to merge once the normal CI checks pass. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 38.10% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 6 files. (14 skipped: 12 unsupported, 2 too large.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69a49ea8c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Please review final head b1de5a4. The operand-order finding is fixed with retained Red → Green evidence. The fallback concern is addressed with a verified TLS regression demonstrating that the locked Rustls resolver does not fall back for a known incompatible key; details are in that review thread. Local validation: all 36 focused TLS tests, formatting, Clippy, and static hygiene passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1de5a4fff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Please review head 9c7cd99. The new unused-variable finding is fixed with retained Red/Green CLI and analyzer regressions (7074570 → 9c7cd99). Earlier operand-order and TLS fallback review dispositions remain covered. All 38 analyzer tests and all 37 TLS tests pass locally; final-head CI is being followed. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex security review Please examine final head 9c7cd99 for the R3 security-focused review required by testing.md: SNI hostname/config validation, certificate/key consistency and coverage, explicit-default-only fallback, malformed input limits, and TLS lifecycle. The fresh general review completed without findings; security regression evidence is in Engineering/evidence/2026-09-23-tls-sni.md and tests/web_server_sni_test.rs. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Final validation for 9c7cd99:
The branch is clean and the PR has no merge conflicts. Left open for maintainer merge; no merge or deployment performed. |
1 participant
Behavior
A secured WFL listener currently serves one certificate regardless of the requested domain. This change lets one HTTPS port select the appropriate certificate using TLS SNI:
Named-only listeners reject unknown or missing SNI. An unnamed certificate/key pair placed first provides an explicit fallback. Existing single-certificate and bare
securedforms retain their behavior.All named pairs are validated before binding: exact DNS names, case-insensitive duplicate detection, key/certificate consistency, certificate hostname coverage, and a 128-entry limit. Selection uses Rustls's SNI resolver and keeps the existing transport/lifecycle implementation. HTTP Host/path routing remains application-controlled.
Includes parser/analyzer/typechecker/linter integration, documentation, WFL regression programs, retained parser fuzz seeds, and a dev diary.
Test evidence
6ad75a47precedes implementationb1f8d747; original failure chronology and rebase provenance are retained in the evidence record.mainto exclude unrelated ES256 work from the original checkout. Post-rebase and review-fix validation passed all 37 focused TLS tests and 38 analyzer tests, Clippy, formatting, and static hygiene. Operand-order regression: Red02972e3a→ fixb1de5a4f. Unused-variable regression: Red70745701→ fix9c7cd998. Final-head CI, Docker validation, and config lint all passed on9c7cd998. Fresh general and security-focused reviews completed without new findings; all inline findings are resolved.Summary by CodeRabbit