Add RFC 3161 Timestamp Authority support for PKI signing and verification - #152
Add RFC 3161 Timestamp Authority support for PKI signing and verification#152sampras343 wants to merge 1 commit into
Conversation
sampras343
left a comment
There was a problem hiding this comment.
Review: Request Changes
The code change itself is simple and correct — `time.Now()` instead of `signingCert.NotBefore`. But this has a significant real-world consequence that the PR doesn't address, and the test doesn't cover the most important scenario.
Problem: This breaks verification of models signed with long-lived certs that have since expired
The `certificate` signing method uses long-lived certificates (unlike Sigstore's ephemeral certs). A common workflow:
- Organization signs a model with a 1-year certificate in January 2026
- Certificate expires in January 2027
- User tries to verify the model in March 2027
- Verification now fails — the cert expired 2 months ago, even though the model was signed while the cert was valid
With the old behavior (`NotBefore`), this worked. With `time.Now()`, it breaks. This is a breaking change for any user with models signed by certificates that have since expired.
The spec says "The leaf certificate MUST be within its validity period" — but this is ambiguous. It could mean:
- (a) Within validity at verification time — what this PR implements
- (b) Within validity at signing time — what the old code approximated
Option (a) is stricter and arguably more secure, but it means every model signed with `certificate` method has a built-in expiration date tied to the cert, which may surprise users.
What should be done
At minimum: Document this as a behavioral change. If option (a) is intentional, the PR description and commit message should explicitly state: "Models signed with certificates that have since expired will no longer verify. Users must re-sign with a valid certificate."
Better: Check if the bundle has a trusted timestamp (`timestampVerificationData`) or tlog entry. If a trusted timestamp exists proving the signature was created during the cert's validity period, the verification should pass even if the cert has since expired. This is standard PKI practice (RFC 3161). The spec's §4.1 table shows `timestampVerificationData` as optional for the `certificate` method — it's there for exactly this use case.
Alternative: Add a flag like `--allow-expired-certs` or `--verify-at-signing-time` for users who need to verify old models.
Test critique
The test (`TestBuildCertificatePoolsRejectsExpiredCert`) is good for proving the mechanism works, but:
-
It doesn't actually test the verifier — it creates pools and calls `leafCert.Verify()` directly with `time.Now()`. It should call `cv.extractAndVerifyCertificate()` or `cv.Verify()` to test the real code path.
-
The old-behavior verification at the end (lines 164-173) is a no-op assertion — it proves that `NotBefore` would have passed, but this is testing the old behavior that was just removed. It belongs in the commit message as motivation, not in the test suite as a permanent assertion.
-
`_ = caCert` on line 175 — suppressing an unused variable warning is a code smell. If the variable isn't needed, don't create it. `x509.ParseCertificate(caDER)` was only called to assign `caCert` which is unused.
Summary
The one-line code change is technically correct per one reading of the spec. But it's a behavioral breaking change that needs:
- Explicit documentation of the impact
- Consideration of timestamp-based verification as the proper solution
- A test that exercises the actual verifier, not a standalone `x509.Verify` call
sampras343
left a comment
There was a problem hiding this comment.
Updated Review After Cross-Checking Python Implementation: Request Changes
I cross-checked the Python reference implementation at `sigstore/model-transparency` (`src/model_signing/_signing/sign_certificate.py`). The Python client intentionally uses `NotBefore`, not `time.Now()`. The Go implementation's original behavior was correct and aligned with the reference.
Python behavior (reference implementation)
```python
signing_certificate = x509.load_der_x509_certificate(
signing_chain.certificates[0].raw_bytes
)
max_signing_time = signing_certificate.not_valid_before_utc
self._store.set_time(max_signing_time)
```
The Python client explicitly pins the OpenSSL store's verification time to `not_valid_before_utc`. This is a deliberate design choice: it allows verification of signatures created during the certificate's validity window, even after the certificate has expired. This is the standard pattern for code-signing verification — signatures don't become invalid when the signing cert expires.
Why `time.Now()` is wrong for this use case
The `certificate` signing method uses long-lived certificates (unlike Sigstore's ephemeral certs). Real-world scenario:
- Organization signs model with a 1-year certificate (Jan 2026)
- Certificate expires (Jan 2027)
- User verifies model (Mar 2027)
- With `time.Now()`: verification fails — the signature is "invalid" simply because time passed
- With `NotBefore`: verification passes — the signature is still valid because the cert was valid when it was issued
This PR would break backward compatibility with the Python client and introduce a behavioral divergence between implementations.
Recommendation
Revert this PR. The original `NotBefore` behavior is correct and matches the reference implementation. The spec's §8.2 statement ("The leaf certificate MUST be within its validity period") should be interpreted as "was within its validity period at signing time", not "must currently be valid". This interpretation is confirmed by the Python reference implementation.
If time-of-verification validity is desired in the future, it should be:
- Discussed as a spec clarification in `ossf/model-signing-spec`
- Implemented with timestamp-based verification (`timestampVerificationData`) so signatures with trusted timestamps still pass after cert expiry
- Coordinated across both Go and Python implementations
Issue #130 should be closed as "working as designed" or relabeled to track the spec clarification discussion.
When an RFC 3161 timestamp is present in the bundle, use it as the verification time. Otherwise fall back to NotBefore, matching the Python reference implementation. Fixes #130 Signed-off-by: Sachin Sampras M <sampras343@gmail.com>
6915f8f to
63d10a4
Compare
Summary
--tsa-urlflag to PKI signing commands (key, certificate, pkcs11) to include RFC 3161 trusted timestamps in signature bundles via sigstore-go'sBundleOptions.TimestampAuthoritiesNotBefore(matching the Python reference implementation)sigstoresigning method (which already has transparency log timestamps)Signing side
TSAUrlfield added toKeySignerOptions,CertificateSignerOptions, andPkcs11SignerOptionsTSAFlagsCLI flag group with--tsa-urlwired to key, certificate, and pkcs11 sign commandsVerification side
GetTimestampFromBundle()extractsgenTimefrom the first RFC 3161 timestamp usingbndl.Timestamps()(sigstore-go) +timestamp.ParseResponse(digitorus/timestamp, transitive dep)cert_verifier.gouses TSA timestamp when present, falls back toNotBeforeCross-client note
The Go implementation stores raw DER bytes in
signed_timestamp(correct per protobuf spec). Python PR sigstore/model-transparency#620 base64-encodes before storing, which may cause interoperability issues.Closes #153
Closes #130
Test plan
TestGetTimestampFromBundle_Valid— timestamp extracted and matches expected timeTestGetTimestampFromBundle_NoTimestamp— returns false when no timestampTestGetTimestampFromBundle_InvalidBytes— returns false for corrupt dataTestGetTimestampFromBundle_NilVerificationMaterial— returns false for nil materialgo test ./... -racepassesgolangci-lint run ./...— 0 issues