fix(operator): stop advertised cert data removing a site from routing - #81
fix(operator): stop advertised cert data removing a site from routing#81hexfusion wants to merge 1 commit into
Conversation
f0119dd to
335272e
Compare
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Downgrades the SWIM-advertised certificate mismatch from a trust failure (phase demotion) to an informational record, so that a stale or unparseable gossiped copy cannot remove a verified peer from routing.
Overall Assessment
The core behavior change is correct and well-motivated: the live TLS handshake is the trust authority, so a mismatch on the gossiped copy should not veto it. The extracted advertised_leaf_der function is clean, the doc updates are consistent, and the new unit tests for the extracted function are thorough.
Two gaps below.
Findings
| Severity | File | Finding |
|---|---|---|
| Large | grid_site.rs |
Missing site_phase_next tests for Connecting and Unreachable starting phases |
| Medium | grid_site.rs |
Duplicate doc comment on site_with_advertised |
| Medium | grid_site.rs |
event_type_for_reason emits Warning for the new success-path reason |
Non-inline Finding
[Medium] event_type_for_reason (line 544 of grid_site.rs) maps AdvertisedCertMismatch to EventType::Warning via the catch-all arm. After this PR, that reason is produced by the success() path and the site stays Active. An operator seeing repeated Warning events on a stable Active site could misinterpret them as actionable trust failures. If Warning is intentional for this reason, add it to the explicit match with a comment explaining why an informational-only reason still warrants Warning. If it should be Normal, add it to the Normal arm alongside TlsVerified.
There was a problem hiding this comment.
Thanks a bunch @hexfusion!
The core change looks correct: trust remains anchored in the live TLS handshake, and stale advertised certificate data no longer removes an authenticated site from routing.
One small, non-blocking cleanup request that is separate from the bot findings: the existing comments in operator/src/resources/tls_probe.rs around ProbeConfig.advertised_leaf_der and the probe_gateway sequence still describe the SWIM-advertised certificate as authorization material and say it is verified against an authorized rotation pin. With this change, that comparison is diagnostic only. Suggested wording:
/// Optional SWIM-advertised leaf certificate DER for diagnostic comparison
/// with the configured rotation pins.
And for the probe sequence:
/// 5. If present, compare the SWIM-advertised leaf with the configured pins
/// and record a mismatch without failing the verified connection.
This does not block approval.
ff62dc0 to
68fb5eb
Compare
The gateway probe verified the live leaf against the configured pins, then compared the SWIM-advertised copy of that certificate to the same pins and demoted the site on mismatch. The live handshake is the trust authority, so a stale or unparseable gossiped copy could remove an authenticated peer from routing, and grid_network.rs requires exactly Active for eligibility. Record the mismatch instead of acting on it. The site stays Active and keeps its reason, so the condition is visible without changing eligibility. Since the reason now comes from the success path, map it to a Normal event: a Warning on a site that is verified and routing reads as actionable when it is not. Extract advertised_leaf_der so the parse is testable and so an unparseable gossiped PEM is ignored rather than short-circuiting the probe before the handshake that authenticates the peer. Cover the wrapper for every probe-eligible starting phase, matching what the probe tests already do, and correct the tls_probe wording that still described the advertised certificate as authorization material. Reported by @nerdalert. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
68fb5eb to
8615c43
Compare
|
@hexfusion thanks! |
3 participants
What type of PR is this?
bug
What this PR does / why we need it:
The gateway probe sees two certificates: the live leaf the peer presents during the handshake, and a copy the peer gossips over SWIM, which the operator stores in
status.publicCertPem. Both were checked against the configured pins.A mismatch on the gossiped copy was treated as a trust failure and moved the site out of
Active. OnlyActivesites are routing-eligible, so that peer's providers dropped out of overlay generation.The gossiped copy is not trust material, and the live leaf checked immediately before it is what authenticates the peer. So a mismatch there is now recorded in
status.reasonand no longer changes the phase.An unparseable gossiped PEM reached the same outcome by a second route, through
TrustMaterialInvalid. That reason is for locally configured material, and returning anyErron that path skips the handshake entirely, so it is fixed here rather than left as another way to demote a verified peer.The docs disagreed about this.
auth.mddescribes verification as checking the live leaf;operations.mdandcrds.mddocument the gossiped check and its demotion. This adopts theauth.mdreading and updates the other two, plus a stale note in the rotation verifier.Which issue(s) this PR fixes:
Part of #80
Special notes for your reviewer:
An unparseable gossiped PEM now produces only a log line, where a mismatch is recorded in
status.reason. Happy to close that asymmetry here if you would rather it land together.