fix(security): make Sigstore cert pinning fail-closed#123
Open
avrabe wants to merge 1 commit into
Open
Conversation
Audit finding C-4 flagged the SPKI pinning as a possible silent posture
downgrade. Investigation shows TLS-layer enforcement is already in place
— `PinnedCertVerifier` implements `rustls::ServerCertVerifier` and a pin
mismatch aborts the handshake (this is what failed the v0.8.3 release
when the Fulcio leaf rotated). The real residual was a fail-open path in
agent *construction*, not verification:
1. `create_agent_with_optional_pinning` fell back to an unpinned
standard agent whenever the pinned agent could not be built, unless
`WSC_REQUIRE_CERT_PINNING=1` was set.
2. Worse, `RekorClient::with_url` / `FulcioClient::with_url` caught even
that hard error and fell back to `create_standard_agent()` anyway —
so a configured pin set could still be silently dropped.
Changes:
- `create_agent_with_optional_pinning`: when a non-empty pin set is
supplied, pinning is mandatory — any failure to build the pinned agent
is propagated, never masked by an unpinned fallback. The Sigstore
production path always supplies a non-empty pin set, so pinning is now
unconditional there and `WSC_REQUIRE_CERT_PINNING` is redundant
(retained for backwards compatibility, documented as deprecated).
- `RekorClient` / `FulcioClient` constructors now return
`Result<Self, WSError>` and propagate pinned-agent construction
failure instead of downgrading to standard TLS. The unused `Default`
impls (which assumed an infallible `new()`) are removed.
- `KeylessSigner` threads the new `Result` through with `?`.
Staging endpoints are unaffected: `PinningConfig::fulcio`/`rekor`
auto-detect staging via `WSC_SIGSTORE_STAGING`, and `verify_certificate`
short-circuits to plain WebPKI when no pins are configured.
Fixes: #95
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
1 participant
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
Closes #95. Audit finding C-4 raised the SPKI cert pinning as a possible silent posture downgrade. The issue prescribed a
ureq→reqwestmigration on the premise that pins were validated post-handshake and only logged.That premise is stale. TLS-layer enforcement is already in place:
PinnedCertVerifierimplementsrustls::ServerCertVerifier, wired into theureqagent via a custom connector intransport.rs. A pin mismatch returnsErr(TlsError)and aborts the handshake — this is exactly what failed the v0.8.3 release when the Fulcio leaf rotated. No client migration is needed.The genuine residual was a fail-open path in agent construction:
create_agent_with_optional_pinningsilently fell back to an unpinned standard agent when the pinned agent couldn't be built (unlessWSC_REQUIRE_CERT_PINNING=1).RekorClient/FulcioClientconstructors then swallowed even that hard error and fell back tocreate_standard_agent()regardless — so a configured pin set could still be dropped silently.Changes
create_agent_with_optional_pinning: a non-empty pin set makes pinning mandatory — construction failure propagates, never downgrades. Sigstore production always supplies pins, so pinning is now unconditional andWSC_REQUIRE_CERT_PINNINGis redundant (kept for back-compat, documented deprecated).RekorClient/FulcioClientconstructors returnResult<Self, WSError>and propagate failure. UnusedDefaultimpls removed.KeylessSignerthreads theResultthrough with?.Staging is unaffected —
PinningConfig::fulcio/rekorauto-detect staging viaWSC_SIGSTORE_STAGING, andverify_certificateshort-circuits to plain WebPKI when no pins are configured.Test plan
cargo build --workspacecleancargo test -p wsc --lib keyless— 166 passed, 0 failedcargo clippy -p wsc --lib— no new warnings in touched files🤖 Generated with Claude Code