Upgrade tonic TLS stack#541
Merged
Merged
Conversation
ti-chi-bot
Bot
added
dco-signoff: yes
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesDependency Version Bumps
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
hbisheng
force-pushed
the
cse-0.4.0-tonic-0.12-tls
branch
from
0c2b75a to
158a395
Compare
ti-chi-bot
Bot
added
size/XXL
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
hbisheng
force-pushed
the
cse-0.4.0-tonic-0.12-tls
branch
from
158a395 to
5479654
Compare
ti-chi-bot
Bot
added
size/XS
Signed-off-by: Bisheng Huang <hbisheng@gmail.com>
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
coocood
approved these changes
Jun 18, 2026
ti-chi-bot
Bot
added
needs-1-more-lgtm
LykxSassinator
approved these changes
Jun 22, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: coocood, LykxSassinator The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
2 tasks
eduralph
added a commit
to getwyrd/wyrd
that referenced
this pull request
Jul 13, 2026
…the shipped-artifact wall `cargo deny --all-features check advisories` surfaces five advisories, every one of them reachable only through tikv-client 0.4.0 behind the off-by-default `tikv` feature. There is no patch to take: tikv-client 0.4.0 pins tonic 0.10 -> rustls 0.21 -> the vulnerable rustls-webpki, and the upstream fix (tikv/client-rust#541, merged 2026-06-26) has never been released. So write the boundary down rather than chase a release that is not coming. The exceptions go in a SEPARATE config, not deny.toml cargo-deny's `[advisories] ignore` entries are keyed by advisory ID alone — 0.20 accepts only `id` and `reason`, with no crate- or feature-scoping — so an entry applies to the whole graph it is used with. Parking the tikv-client exceptions in deny.toml would mean the wall guarding the SHIPPED ARTIFACT carried them too, and a future default dependency that happened to pull an affected version of the same crate would be silently waved through — holing the exact guarantee that file exists to make. So: * deny.toml — the default graph, i.e. what we ship. Zero backend exceptions, and a test now pins that it stays that way. * deny-all-features.toml — the off-by-default `fdb`/`tikv` trees, which the default run cannot see at all. `cargo xtask ci` runs BOTH walls. Keeping the second one in xtask rather than in the workflow YAML matters: `cargo xtask ci` is the documented local equivalent of the Rust gate (ADR-0009), so a YAML-only audit would pass on a contributor's laptop and fail only in CI. It costs seconds — cargo-deny resolves Cargo.lock and compiles nothing, so auditing the optional trees needs none of their native toolchains. The comment is explicit that this is a statement about the shipped artifact and NOT a claim that the TiKV leg is safe: RUSTSEC-2026-0104 is a live DoS in the TLS cert-validation path for anyone who builds --features tikv and talks TLS-with-CRLs. Two places this departs from the issue as filed: - The delete-condition is not "#443 removes the feature". #443 stands TiKV down but RETAINS it, so these entries have no scheduled end. They are deleted by a tikv-client release carrying #541, or by an explicit future decommission decision. Recorded as such, with a 2027-01-31 review trigger so "tracked, not waived" cannot decay into a permanent silent waiver of a high-severity advisory. - The rand 0.7.3 question resolves to "no entry needed", and not for the reason the issue assumed: cargo-deny 0.20's `version = 2` config has no `unsound` key at all (setting one is a hard deserialization error), so RUSTSEC-2026-0097 is never raised. An ignore for it would be dead config. Also corrects the AUDIT POLICY header, which claimed the off-by-default FDB tree "IS audited here" by the default `cargo deny check`. It is not: that invocation passes no feature flags, so foundationdb, libloading and tikv-client are all absent from the graph it walks — which is why ISC has always warned `license-not-encountered`. That warning also exposes a real gap the header now names rather than papers over: the ADR-0003 LICENCE wall does not cover the optional trees at all (#547). Verified: `cargo deny --all-features --config deny-all-features.toml check advisories` ok (5 errors before); `cargo deny check` still ok with zero backend ignores; the same all-features graph WITHOUT the new config still shows all 5, so the split is not a no-op; `cargo xtask ci` green, running both walls; typos clean. Reviewed by `codex review` to clean (it caught both the global-ignore hole and the xtask-vs-YAML placement). Refs #420, #435, #443, #547. ADR-0003 §2, ADR-0009, ADR-0042. Closes #543 Signed-off-by: Eduard R. <eduard@ralphovi.net>
eduralph
added a commit
to getwyrd/wyrd
that referenced
this pull request
Jul 13, 2026
…the shipped-artifact wall `cargo deny --all-features check advisories` surfaces six advisories, every one of them reachable only through tikv-client 0.4.0 behind the off-by-default `tikv` feature. There is no patch to take: tikv-client 0.4.0 pins tonic 0.10 -> rustls 0.21 -> the vulnerable rustls-webpki, and the upstream fix (tikv/client-rust#541, merged 2026-06-26) has never been released. So write the boundary down rather than chase a release that is not coming. The exceptions go in a SEPARATE config, not deny.toml cargo-deny's `[advisories] ignore` entries are keyed by advisory ID alone — there is no crate- or feature-scoping — so an entry applies to the whole graph it is used with. Parking the tikv-client exceptions in deny.toml would mean the wall guarding the SHIPPED ARTIFACT carried them too, and a future default dependency that pulled an affected version of the same crate would be silently waved through — holing the exact guarantee that file exists to make. So: * deny.toml — the default graph, i.e. what we ship. Zero backend exceptions, and a test pins that it stays that way. * deny-all-features.toml — the off-by-default `fdb`/`tikv` trees, which the default run cannot see at all. `cargo xtask ci` runs BOTH walls. Keeping the second in xtask rather than in workflow YAML matters: `cargo xtask ci` is the documented local equivalent of the Rust gate (ADR-0009), so a YAML-only audit would pass on a laptop and fail only in CI. It costs seconds — cargo-deny resolves Cargo.lock and compiles nothing. Both walls now audit unsoundness across the WHOLE graph `unsound = "all"`, on both. cargo-deny's default scope is `workspace` — our own crates only — and since every crate we link is transitive, the wall was blind to precisely the class of bug it exists for. It was hiding a real one in the SHIPPED graph: RUSTSEC-2026-0190, UB in anyhow's `Error::downcast_mut()` after `Error::context`. That is fixed here by bumping anyhow to the patched 1.0.103 — the wall goes green on a fix, not on a waiver. In the optional graph the same scope raises RUSTSEC-2026-0097 (rand 0.7.3 <- fail <- tikv-client), which is ignored on the same terms as the rest of that tree. An earlier draft of this change asserted the opposite — that unsound advisories were "not checkable" because cargo-deny had no `unsound` key. That was wrong: the key exists and takes a scope, and `unsound = "deny"` had failed to parse as an invalid VALUE, not an unknown key. Caught in review. The comments are explicit that this is a statement about the shipped artifact and NOT a claim that the TiKV leg is safe: RUSTSEC-2026-0104 is a live DoS in the TLS cert-validation path for anyone who builds --features tikv and talks TLS-with-CRLs. Two places this departs from the issue as filed: - The delete-condition is not "#443 removes the feature". #443 stands TiKV down but RETAINS it, so these entries have no scheduled end. They are deleted by a tikv-client release carrying #541, or by an explicit future decommission decision. Recorded as such, with a 2027-01-31 review trigger so "tracked, not waived" cannot decay into a permanent silent waiver of a high-severity advisory. - The rand 0.7.3 question resolves to "it needs an entry after all" — see above. Also corrects the AUDIT POLICY header, which claimed the off-by-default FDB tree "IS audited here" by the default `cargo deny check`. It is not: that invocation passes no feature flags, so foundationdb, libloading and tikv-client are absent from the graph it walks — which is why ISC has always warned `license-not-encountered`. That warning also exposes a real gap the header now names rather than papers over: the ADR-0003 LICENCE wall does not cover the optional trees at all (#547). Verified: both walls green (`cargo deny check`; `cargo deny --all-features --config deny-all-features.toml check advisories`), and the same all-features graph WITHOUT the new config still shows every tikv advisory, so the split is not a no-op. `cargo xtask ci` green, running both walls. Tests pin the invariants: no backend exception in deny.toml, and `unsound = "all"` in both. typos clean. `codex review` to clean. Depends on #549 (#548): `cargo deny` is red on main until the yanked spin is bumped. Refs #420, #435, #443, #547. ADR-0003 §2, ADR-0009, ADR-0042. Closes #543 Signed-off-by: Eduard R. <eduard@ralphovi.net>
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
3 participants
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.
Upgrade
tonicfrom0.10to0.12while keeping TLS enabled.tonic 0.10'stlsfeature pulls in the olderrustls 0.21/rustls-webpki 0.101TLS stack, which is affected by RustSec advisories such asRUSTSEC-2026-0098,RUSTSEC-2026-0099, andRUSTSEC-2026-0104.tonic 0.12keeps the existingtlsfeature API but moves TLS totokio-rustls 0.26/rustls 0.23, which uses patchedrustls-webpkiversions. This keeps TLS support working while removing the vulnerable old TLS dependency path.Summary by CodeRabbit