Implement attestation/credential revokation validation in verifier. - #532
Conversation
There was a problem hiding this comment.
Pull request overview
Adds revocation checking support to the verifier by introducing a credential-format-agnostic revocation API and a Token Status List–based implementation, then wiring it into the verification flow behind configuration.
Changes:
- Introduce
pkg/revocation(registry + checker interface) with a Token Status List checker implementation. - Add Token Status List parsing/checking utilities in
pkg/tokenstatuslist. - Add verifier configuration (
RevocationConfig) and enforce revocation checks during direct-post verification.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/tokenstatuslist/checker.go | Adds a status-list checker + status reference extraction for Token Status Lists. |
| pkg/tokenstatuslist/checker_test.go | Unit tests for status reference extraction and status code mapping. |
| pkg/revocation/revocation.go | Defines revocation scheme/status types and the Checker interface. |
| pkg/revocation/registry.go | Adds a registry that selects an appropriate checker and validates revocation. |
| pkg/revocation/extract.go | Extracts a status list reference from credential claims into a generic Reference. |
| pkg/revocation/status_list.go | Implements revocation checking using Token Status Lists (HTTP fetch + parse + status lookup). |
| pkg/revocation/revocation_test.go | Tests for extraction, registry behavior, and status list checking. |
| pkg/model/config.go | Adds verifier revocation configuration (RevocationConfig). |
| internal/verifier/apiv1/client.go | Initializes and registers the revocation checker registry when enabled. |
| internal/verifier/apiv1/handlers_verification.go | Performs revocation validation during verification, with scope exemptions and fail-open/fail-closed behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Suppressed comments (5)
pkg/tokenstatuslist/checker.go:240
- WithCacheExpiry/cacheExpiry currently has no effect: getStatusList() always calls cache.Set() with the cache backend’s default TTL. This makes the option misleading and can lead to unexpectedly long/short caching depending on backend defaults.
func (sc *StatusChecker) getStatusList(ctx context.Context, uri string) ([]uint8, error) {
if statuses, ok := sc.cache.Get(ctx, uri); ok {
return statuses, nil
}
statuses, err := sc.fetchStatusList(ctx, uri)
if err != nil {
return nil, err
}
sc.cache.Set(ctx, uri, statuses)
return statuses, nil
}
pkg/revocation/revocation_test.go:90
- NewStatusListChecker now requires a KeyResolver and returns an error when it’s missing. This test ignores the error and registers a nil *StatusListChecker in the Registry, which will panic when Validate() calls methods on it.
t.Run("no status claim returns nil", func(t *testing.T) {
statusCache := cache.NewMemoryCache[[]uint8](5 * time.Minute)
checker, _ := NewStatusListChecker(WithCache(statusCache))
r := NewRegistry(checker)
result, err := r.Validate(t.Context(), map[string]any{"iss": "x"})
pkg/revocation/status_list.go:267
- The status_list claim’s required "bits" value is ignored. This code assumes the decompressed "lst" is a 1-byte-per-entry array, which is only correct when bits==8; for bits in {1,2,4} the revocation status mapping will be wrong.
statusListClaim, ok := claims["status_list"].(map[string]any)
if !ok {
return nil, errors.New("status_list claim not found or invalid")
}
pkg/tokenstatuslist/checker.go:74
- ExtractStatusReference’s docstring says it “returns error if status claim is malformed”, but the implementation currently returns (nil, nil) for malformed/unsupported shapes (and never returns a non-nil error). This is misleading for callers.
// ExtractStatusReference extracts a StatusReference from SD-JWT credential claims.
// Returns nil if no status claim is present (credential is not revocable).
// Returns error if status claim is malformed.
func ExtractStatusReference(claims map[string]any) (*StatusReference, error) {
pkg/revocation/status_list.go:241
- ResolveKey is called with context.Background(), so caller cancellation/deadlines from the request ctx are ignored during JWKS discovery/fetch. Under network issues this can cause revocation checks to outlive the request that triggered them.
kid, _ := token.Header["kid"].(string)
if issuer == "" {
return nil, errors.New("status list token missing iss claim")
}
return c.keyResolver.ResolveKey(context.Background(), issuer, kid)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (7)
pkg/tokenstatuslist/checker.go:104
- When idx is decoded as float64 (common after json.Unmarshal), the code blindly casts to int64, which silently truncates non-integer values (e.g. 1.9 -> 1). This can cause the verifier to check the wrong status list index.
switch idx := statusList["idx"].(type) {
case float64:
index = int64(idx)
case int64:
pkg/revocation/extract.go:37
- When idx is decoded as float64, the code casts to int64 without validating it is an integer, which can silently truncate (e.g. 1.9 -> 1) and check the wrong list entry.
switch idx := statusList["idx"].(type) {
case float64:
index = int64(idx)
case int64:
pkg/tokenstatuslist/checker.go:238
- WithCacheExpiry configures sc.cacheExpiry, but the value is never used when writing to the cache. This makes cache expiry configuration ineffective for caches that rely on per-entry TTL (and is inconsistent with cache.Cache supporting SetWithTTL).
sc.cache.Set(ctx, uri, statuses)
pkg/revocation/revocation_test.go:90
- Test constructs StatusListChecker without the required KeyResolver and ignores the returned error. With the current NewStatusListChecker contract this will return (nil, err), making the registry contain a nil checker (and hiding the constructor failure).
statusCache := cache.NewMemoryCache[[]uint8](5 * time.Minute)
checker, _ := NewStatusListChecker(WithCache(statusCache))
r := NewRegistry(checker)
result, err := r.Validate(t.Context(), map[string]any{"iss": "x"})
pkg/tokenstatuslist/checker.go:73
- The doc comment says this function returns an error for malformed status claims, but the implementation currently returns (nil, nil) for all malformed cases. This mismatch can mislead callers into expecting an error they will never see.
// ExtractStatusReference extracts a StatusReference from SD-JWT credential claims.
// Returns nil if no status claim is present (credential is not revocable).
// Returns error if status claim is malformed.
pkg/revocation/status_list.go:226
- ResolveKey is called with context.Background(), which ignores cancellation/deadlines from the incoming request context passed to CheckStatus. This can cause revocation checks to continue running after the request is canceled and makes timeouts harder to control.
// Resolve signing key and verify signature
key, err := c.keyResolver.ResolveKey(context.Background(), issuer, kid)
if err != nil {
return nil, fmt.Errorf("failed to resolve CWT signing key: %w", err)
}
pkg/revocation/status_list.go:297
- JWT key resolution uses context.Background(), so JWKS fetches / key resolution won't respect the request context cancellation/deadline from CheckStatus.
keyFunc := func(token *jwt.Token) (any, error) {
claims, _ := token.Claims.(jwt.MapClaims)
issuer, _ := claims["iss"].(string)
kid, _ := token.Header["kid"].(string)
if issuer == "" {
return nil, errors.New("status list token missing iss claim")
}
return c.keyResolver.ResolveKey(context.Background(), issuer, kid)
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Suppressed comments (9)
pkg/revocation/revocation_test.go:28
- base64RawURL() is only used to build an unsigned 'alg: none' JWT in this test file. After switching the test to a real signed token, this helper should be removed to avoid unused code and to keep tests aligned with production verification behavior.
func base64RawURL(data []byte) string {
return base64.RawURLEncoding.EncodeToString(data)
}
pkg/revocation/revocation_test.go:105
- This test constructs an unsigned JWT with alg=none. If parseJWTStatusList() restricts accepted algorithms (as it should), this test will start failing and no longer reflects production behavior. Prefer generating a real ES256-signed JWT and serving that from the test server.
// Compress and encode to create a raw JWT payload (no signature verification in this test)
encoded, err := tokenstatuslist.CompressAndEncode(statuses)
require.NoError(t, err)
// Build a minimal unsigned JWT (header.payload.signature) for testing
pkg/revocation/revocation_test.go:122
- NewStatusListChecker() in this test uses an allow-all key resolver that specifically enables alg=none. Once the test uses a real signature, pass a resolver that returns the matching public key so the verification path is exercised.
WithCache(statusCache),
WithHTTPClient(server.Client()),
// In production, the key resolver verifies the issuer's signature.
// For testing, we use an allow-all resolver.
WithKeyResolver(allowAllKeyResolver{}),
pkg/revocation/status_list.go:158
- fetchStatusList() uses io.ReadAll(resp.Body) with no upper bound. A malicious or misconfigured status list endpoint can cause excessive memory usage. Prefer a size-limited read (and error if exceeded).
body, err := io.ReadAll(resp.Body)
if err != nil {
return nil, fmt.Errorf("failed to read response body: %w", err)
}
pkg/revocation/status_list.go:196
- parseCWTStatusList() ignores failed type assertions for the COSE_Sign1 components (protected/unprotected/payload/signature). This can lead to signature verification running with nil/empty values and producing misleading errors. Validate the component types explicitly and return a clear error when the structure is malformed.
protectedBytes, _ := components[0].([]byte)
unprotected, _ := components[1].(map[any]any)
payloadBytes, _ := components[2].([]byte)
signature, _ := components[3].([]byte)
pkg/revocation/status_list.go:220
- parseCWTStatusList() extracts the CWT issuer (iss) but doesn't validate that it is present/non-empty before calling the KeyResolver. Returning an explicit error here makes failures clearer and avoids attempting key resolution with an empty issuer.
var claims map[int]any
if err := cbor.Unmarshal(payloadBytes, &claims); err != nil {
return nil, fmt.Errorf("failed to decode CWT claims: %w", err)
}
issuer, _ := claims[1].(string) // CWT claim 1 = iss
pkg/revocation/revocation_test.go:17
- The status list checker is (and should remain) strict about accepted JWT signature algorithms. This test currently constructs an unsigned JWT (alg: none) and uses base64 helpers to bypass signature verification, which would fail once jwt.WithValidMethods(...) is enforced. Update the test to generate a real ES256-signed JWT and remove the now-unneeded base64 import.
This issue also appears in the following locations of the same file:
- line 26
- line 101
- line 118
import (
"context"
"encoding/base64"
"net/http"
"net/http/httptest"
internal/verifier/apiv1/client.go:150
- cacheTTL is derived directly from cfg.Verifier.Revocation.CacheTTL. If the config is missing defaults or is set to 0/negative, this results in a zero/negative TTL (effectively disabling caching or behaving unexpectedly). Clamp to a sane default when CacheTTL <= 0.
if cfg.Verifier.Revocation != nil && cfg.Verifier.Revocation.Enabled {
cacheTTL := time.Duration(cfg.Verifier.Revocation.CacheTTL) * time.Second
statusCache := pkgcache.NewMemoryCache[[]uint8](cacheTTL)
internal/verifier/apiv1/handlers_verification.go:302
- Revocation enforcement is new behavior in VerificationDirectPost (rejecting revoked/suspended credentials and optionally failing open/closed on transient errors), but there are no handler-level tests covering these branches in internal/verifier/apiv1/handlers_verification_test.go. Add tests for: (1) revoked credential -> request rejected, (2) revocation endpoint unreachable with fail_open=true -> allowed, (3) revocation endpoint unreachable with fail_open=false -> rejected, and (4) skip_scopes bypasses revocation for specific scopes.
// Revocation status verification (ARF 3.0 §6.6.3.7)
if c.revocationRegistry != nil && c.cfg.Verifier.Revocation != nil && c.cfg.Verifier.Revocation.Enabled {
skipScopes := c.cfg.Verifier.Revocation.SkipScopes
for _, scope := range authCtx.Scopes {
if slices.Contains(skipScopes, scope) {
c.log.Debug("Skipping revocation check for exempt scope", "scope", scope)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
pkg/revocation/status_list.go:147
- The status list
uriis extracted from presented credential claims and fetched server-side. Only validating the scheme (http/https) still allows SSRF to localhost/private/link-local targets. Consider applying the same SSRF protections used elsewhere (e.g.safe_urivalidation logic inpkg/helpers/validate.go) to block localhost and private IP ranges before issuing the request.
// Reject non-HTTP(S) schemes to reduce SSRF surface
switch req.URL.Scheme {
case "http", "https":
// allowed
default:
pkg/revocation/revocation_test.go:108
- The test builds an unsigned JWT with
alg: none, butStatusListChecker.parseJWTStatusListenforcesjwt.WithValidMethodsthat excludesnone, so this test will fail once revocation checking actually parses/validates the token. Update the test to generate a signed status-list JWT using one of the allowed algorithms (e.g. ES256/EdDSA/RS256) and use a KeyResolver that returns the corresponding public key.
privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
require.NoError(t, err)
// Create a properly signed JWT status list token
pkg/revocation/status_list.go:190
- CWT/COSE status list parsing and signature verification (
parseCWTStatusList) is new behavior but isn't covered by tests in this package. Adding unit tests for the CWT path (including signature verification + extractingstatus_list/lst) would reduce the risk of regressions and spec-incompatibilities.
func (c *StatusListChecker) parseCWTStatusList(ctx context.Context, data []byte) ([]uint8, error) {
// Decode COSE_Sign1 (CBOR Tag 18)
var coseTag cbor.Tag
if err := cbor.Unmarshal(data, &coseTag); err != nil {
return nil, fmt.Errorf("failed to decode COSE_Sign1: %w", err)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
internal/verifier/apiv1/handlers_verification.go:319
StatusUnknownis currently treated the same as revoked/suspended (any non-validstatus rejects the presentation), and the log message says "revoked or suspended" even when the status is actuallyunknown. This also makesfail_openineffective for unknown/unsupported status codes. Consider handlingunknownexplicitly: always reject only forinvalid/suspended, and treatunknownas an error that followsfail_open(or update messaging accordingly).
} else if result != nil && result.Status != revocation.StatusValid {
// Authoritative revocation/suspension — always reject regardless of fail_open
c.log.Error(nil, "credential has been revoked or suspended", "scope", scope, "status", result.Status.String(), "uri", result.URI, "index", result.Index)
return nil, fmt.Errorf("credential for scope %s has been %s", scope, result.Status.String())
pkg/revocation/revocation_test.go:180
- This test ignores the error return from
NewStatusListChecker. If construction fails in the future,checkerwill be nil and the test will panic when callingSupports. Userequire.NoError(as in the other tests) to make failures explicit.
func TestStatusListChecker_Supports(t *testing.T) {
statusCache := cache.NewMemoryCache[[]uint8](5 * time.Minute)
checker, _ := NewStatusListChecker(WithCache(statusCache), WithKeyResolver(testKeyResolver{}))
assert.True(t, checker.Supports(SchemeStatusList))
pkg/revocation/status_list.go:200
- The revocation checker has a full CWT/COSE_Sign1 verification path (
parseCWTStatusList), but the current tests only cover the JWT branch. Adding a test that serves a signed CWT status list token would help prevent regressions in CBOR/COSE decoding and signature verification behavior.
func (c *StatusListChecker) parseCWTStatusList(ctx context.Context, data []byte) ([]uint8, error) {
// Decode COSE_Sign1 (CBOR Tag 18)
var coseTag cbor.Tag
if err := cbor.Unmarshal(data, &coseTag); err != nil {
return nil, fmt.Errorf("failed to decode COSE_Sign1: %w", err)
}
if coseTag.Number != 18 {
return nil, fmt.Errorf("invalid COSE tag: expected 18 (COSE_Sign1), got %d", coseTag.Number)
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
pkg/revocation/revocation_test.go:112
- StatusListChecker implements both JWT and CWT status list token parsing/verification, but the tests currently only exercise the JWT path. Adding a CWT-based test (including signature verification and lst decompression) would help prevent regressions in the more complex CWT/COSE handling.
// Create a properly signed JWT status list token
token := jwt.NewWithClaims(jwt.SigningMethodES256, jwt.MapClaims{
"iss": "https://registry.test",
"status_list": map[string]any{"lst": encoded},
})
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (5)
pkg/revocation/status_list.go:234
- parseCWTStatusList uses the (untrusted) CWT payload to resolve the signing key, but it doesn't require an "iss" claim to be present. If a KeyResolver implementation tolerates an empty issuer, a token missing iss could still be accepted. Mirror the JWT path by rejecting tokens without iss.
var claims map[int]any
if err := cbor.Unmarshal(payloadBytes, &claims); err != nil {
return nil, fmt.Errorf("failed to decode CWT claims: %w", err)
}
issuer, _ := claims[1].(string) // CWT claim 1 = iss
pkg/revocation/extract.go:39
- In the float64 "idx" path, converting to int64 without a range check can yield implementation-dependent results for very large values (out of int64 range), which could turn a malformed index into an unexpected negative/valid value. Add an explicit int64 range check before converting.
case float64:
if idx != float64(int64(idx)) {
return nil // Non-integer index
}
index = int64(idx)
pkg/revocation/status_list.go:179
- Content-Type matching here is exact, so a real-world header like "application/statuslist+jwt; charset=utf-8" will fall into the default branch. Strip optional Content-Type parameters before switching so media types with parameters are handled correctly.
This issue also appears on line 230 of the same file.
func (c *StatusListChecker) parseStatusListToken(ctx context.Context, data []byte, contentType string) ([]uint8, error) {
switch contentType {
case tokenstatuslist.MediaTypeCWT:
return c.parseCWTStatusList(ctx, data)
case tokenstatuslist.MediaTypeJWT:
internal/verifier/apiv1/client.go:150
- If cache_ttl is set to 0 (or negative), pkg/cache.NewMemoryCache will configure ttlcache with TTL=0, which means items never expire. That can lead to unbounded growth and stale revocation data. Consider enforcing a minimum/default TTL when the configured value is <= 0.
if cfg.Verifier.Revocation != nil && cfg.Verifier.Revocation.Enabled {
cacheTTL := time.Duration(cfg.Verifier.Revocation.CacheTTL) * time.Second
statusCache := pkgcache.NewMemoryCache[[]uint8](cacheTTL)
statusListChecker, err := revocation.NewStatusListChecker(
pkg/revocation/status_list.go:196
- CWT parsing and COSE signature verification are implemented here, but the revocation package tests only cover the JWT status list path. Add unit tests for the CWT path (valid token, bad signature, missing iss, and Content-Type auto-detect) to prevent regressions in this security-sensitive code.
func (c *StatusListChecker) parseCWTStatusList(ctx context.Context, data []byte) ([]uint8, error) {
// Decode COSE_Sign1 (CBOR Tag 18)
var coseTag cbor.Tag
if err := cbor.Unmarshal(data, &coseTag); err != nil {
return nil, fmt.Errorf("failed to decode COSE_Sign1: %w", err)


No description provided.