feat(tls): rfc 0030 green (config) — *_tls blocks, preflight, plaintext warning - #442
Conversation
…xt warning
The RFC0030.5 + RFC0030.7 slice. TlsSettings::from_parts is the single
validation path for a *_tls block (field-named errors; all-unset =
plaintext opt-out) and load() builds the rustls ServerConfig at startup
(ring provider pinned; path-named PEM errors; client CA => require-and-
verify). The file front-end gains grpc_tls/http_tls/querier http_tls
sections with ${env:} substitution; resolve_config preflights every
configured block so bad material fails startup, not the first
handshake; startup_guards warns once per plaintext listener when
credentials are configured (registry event
ourios.server.tls.plaintext_credentials via weaver).
.7 moves to the server harness (leaf name unchanged, never asserted as
a stub) — it observes the spawned binary's stderr, which only the
server crate can do; RFC §6 amended to match. rcgen mints all test
material at test time.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces TLS/mTLS support for RFC 0030 listeners: ourios-ingester gains a ChangesTLS/mTLS Listener Support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Main as ourios-server main
participant ConfigFile
participant TlsSettings
participant Rustls
participant StartupGuards
Main->>ConfigFile: server_config_from_file()
ConfigFile->>TlsSettings: tls_settings(TlsSection)
Main->>Main: resolve_config()
Main->>TlsSettings: preflight_tls(config)
TlsSettings->>Rustls: load() per configured listener
Rustls-->>TlsSettings: ServerConfig or Err
Main->>StartupGuards: startup_guards(config)
StartupGuards->>StartupGuards: warn_if_plaintext_credentials()
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR implements the first “green” slice of RFC 0030 by introducing a validated TLS settings seam (TlsSettings), adding config-file *_tls blocks for receiver/querier, preflighting TLS material at startup, and emitting a structured warning when credentials are served over plaintext.
Changes:
- Added
ourios_ingester::receiver::tls::{TlsSettings, TlsMinVersion}with single-path validation (from_parts) and PEM-loading (load) intorustls::ServerConfig. - Extended the server config file schema with
receiver.{grpc_tls,http_tls}andquerier.http_tls, added TLS preflight on startup, and added a plaintext-credentials warning event. - Updated RFC 0030 docs and moved/implemented RFC0030.7 as a server integration test that spawns the binary and inspects startup logs.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| semconv/registry/events.yaml | Adds semconv event definition for plaintext-credentials warning. |
| docs/rfcs/0030-tls-mtls-listeners.md | Updates testing-strategy mapping for scenario ownership (moves .7 to server). |
| crates/ourios-server/tests/it/rfc0030_tls.rs | Implements RFC0030.7 by spawning ourios-server and asserting warning behavior. |
| crates/ourios-server/src/main.rs | Carries *_tls config into resolved params, preflights TLS, and warns on plaintext credentials. |
| crates/ourios-server/src/config/file.rs | Adds TlsSection blocks to the file-config schema and env-substitution plumbing. |
| crates/ourios-server/Cargo.toml | Adds rcgen dev-dependency for TLS-related tests. |
| crates/ourios-semconv/src/lib.rs | Adds generated constant for the new plaintext-credentials event name. |
| crates/ourios-ingester/tests/it/rfc0030_tls.rs | Implements RFC0030.5 seam-level validation/load tests; removes .7 stub from ingester harness. |
| crates/ourios-ingester/src/receiver/tls.rs | Introduces TLS seam: validation + PEM loading into rustls config (new module). |
| crates/ourios-ingester/src/receiver.rs | Exposes new receiver::tls module. |
| crates/ourios-ingester/Cargo.toml | Adds direct rustls and rustls-pki-types dependencies for the TLS seam. |
| Cargo.lock | Locks new dependencies pulled in by TLS seam and rcgen. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ourios-ingester/tests/it/rfc0030_tls.rs (1)
208-225: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a test arm for valid mTLS (client CA) loading.
The test exercises the empty-CA error case and the no-CA success case, but never tests
load()with a validclient_ca_file. TheWebPkiClientVerifier::builder(roots).build()→with_client_cert_verifier()path is untested with a non-empty trust store. Since mTLS is security-critical, consider adding an arm that mints a CA cert withrcgenand verifies the config builds successfully.♻️ Suggested test arm
// And a valid pair loads — for both min_version selections. for (raw, expected) in [ (None, TlsMinVersion::V1_2), (Some("1.3"), TlsMinVersion::V1_3), ] { let settings = TlsSettings::from_parts( "receiver.grpc_tls", Some(&cert_path.display().to_string()), Some(&key_path.display().to_string()), None, raw, None, ) .expect("valid settings") .expect("configured"); assert_eq!(settings.min_version, expected); settings.load().expect("a valid PEM pair builds"); } + + // A valid pair with a client CA builds an mTLS config. + let ca = rcgen::generate_simple_self_signed(vec!["test-ca".into()]) + .expect("mint a CA"); + let ca_path = tmp.path().join("ca.crt"); + std::fs::write(&ca_path, ca.cert.pem()).expect("write CA"); + let mtls = TlsSettings::from_parts( + "receiver.grpc_tls", + Some(&cert_path.display().to_string()), + Some(&key_path.display().to_string()), + Some(&ca_path.display().to_string()), + None, + None, + ) + .expect("valid mTLS settings") + .expect("configured"); + mtls.load().expect("a valid pair with a client CA builds an mTLS config"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-ingester/tests/it/rfc0030_tls.rs` around lines 208 - 225, Add a test arm in rfc0030_tls.rs to cover successful mTLS loading with a real client CA, since only the empty-CA error and no-CA success paths are exercised now. Use the existing TlsSettings::from_parts and settings.load() flow, but provide a non-empty client_ca_file by minting a CA cert with rcgen and writing it to a temp file. Verify the WebPkiClientVerifier::builder(roots).build() and with_client_cert_verifier() path succeeds for this valid trust store.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/ourios-ingester/tests/it/rfc0030_tls.rs`:
- Around line 208-225: Add a test arm in rfc0030_tls.rs to cover successful mTLS
loading with a real client CA, since only the empty-CA error and no-CA success
paths are exercised now. Use the existing TlsSettings::from_parts and
settings.load() flow, but provide a non-empty client_ca_file by minting a CA
cert with rcgen and writing it to a temp file. Verify the
WebPkiClientVerifier::builder(roots).build() and with_client_cert_verifier()
path succeeds for this valid trust store.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bf47e564-d85c-46bf-b9a9-d38e73f8beb7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/ourios-ingester/Cargo.tomlcrates/ourios-ingester/src/receiver.rscrates/ourios-ingester/src/receiver/tls.rscrates/ourios-ingester/tests/it/rfc0030_tls.rscrates/ourios-semconv/src/lib.rscrates/ourios-server/Cargo.tomlcrates/ourios-server/src/config/file.rscrates/ourios-server/src/main.rscrates/ourios-server/tests/it/rfc0030_tls.rsdocs/rfcs/0030-tls-mtls-listeners.mdsemconv/registry/events.yaml
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
First RFC 0030 green slice (post sign-off): RFC0030.5 + RFC0030.7 un-ignored and passing.
What
ourios_ingester::receiver::tls— the seam (§3.2):TlsSettings::from_partsis the single §3.1 validation path (errors name the exact{prefix}.*field; all-unset ⇒None, TLS stays opt-in),TlsSettings::load()reads the PEMs and builds the listener'srustls::ServerConfig(ring provider pinned explicitly;client_ca_file⇒WebPkiClientVerifierrequire-and-verify; min-version 1.2/1.3 selection). Errors name the offending path.receiver.grpc_tls/receiver.http_tls/querier.http_tlsblocks (raw string leaves,${env:…}substitution,deny_unknown_fields), attached toReceiverParams/QuerierParams— carried, not yet consumed; the acceptor slice wires them into the listeners next.preflight_tlsinresolve_config: every configured block is loaded at startup, so unreadable/malformed material is a startup error naming block + path, not a first-handshake surprise.warn_if_plaintext_credentialsinstartup_guards(§3.4): one warning per plaintext listener when credentials are configured, carrying the new registry eventourios.server.tls.plaintext_credentials(weaver-generated constant; semconv no-diff holds).Tests
querier.http_addrbefore readiness; the same listener with itshttp_tlsblock ⇒ zero.Invariants / hazards
§3.7-adjacent (credential confidentiality); no storage, schema, or query semantics change. New deps:
rustls/rustls-pki-types(already transitive, now direct at the seam),rcgendev-only — no committed key material.Verification
cargo fmt --check✓,cargo clippy --all-targets --all-features -- -D warningsexit 0, both it harnesses green (RFC0030.5 + .7 pass, remaining seven stubs skipped),mdbook build✓, weaver semconv regenerated.🤖 Generated with Claude Code
Summary by CodeRabbit