From 04849fb9a54b9d736b2538a4fcf8b90d3d761d0b Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Sun, 21 Jun 2026 11:09:15 +0200 Subject: [PATCH] fix(controlplane): surface token expiry instead of federated error for expired tokens An expired Chainloop token presented to an attestation endpoint with federated authentication enabled fell through to the federated provider, which masked the expiry with an unrelated authorization error (e.g. "no issuers configured"). Expired tokens now short-circuit and return the proper expiry error so the CLI reports "your authentication token has expired". Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: b42ff77f-1a2b-41f5-a09c-cb9a0399bdec --- .../attjwtmiddleware/attmiddleware.go | 14 +++++ .../attjwtmiddleware/attmiddleware_test.go | 56 +++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/app/controlplane/internal/usercontext/attjwtmiddleware/attmiddleware.go b/app/controlplane/internal/usercontext/attjwtmiddleware/attmiddleware.go index ef109ae44..b7dffddca 100644 --- a/app/controlplane/internal/usercontext/attjwtmiddleware/attmiddleware.go +++ b/app/controlplane/internal/usercontext/attjwtmiddleware/attmiddleware.go @@ -275,6 +275,20 @@ func WithJWTMulti(l log.Logger, opts ...JWTOption) middleware.Middleware { // Check if it's the last provider and still failed if err != nil { + // A well-formed, correctly-signed Chainloop token that has simply + // expired must surface as an expiry error. Trying the remaining + // providers or falling back to the federated path would only mask + // the real reason (e.g. "no issuers configured"), leaving the user + // with a misleading message instead of "token has expired". + // + // runProviderValidator returns the ErrTokenExpired sentinel directly, + // so we compare by identity. We intentionally avoid errors.Is here: + // kratos errors compare only Code and Reason, so every 401/UNAUTHORIZED + // error (e.g. an invalid signature) would wrongly match ErrTokenExpired. + if err == ErrTokenExpired { //nolint:errorlint // sentinel identity, see comment above + return nil, ErrTokenExpired + } + if idx < tokenProviderLen-1 { continue } diff --git a/app/controlplane/internal/usercontext/attjwtmiddleware/attmiddleware_test.go b/app/controlplane/internal/usercontext/attjwtmiddleware/attmiddleware_test.go index 10a66eb3d..01078e0c8 100644 --- a/app/controlplane/internal/usercontext/attjwtmiddleware/attmiddleware_test.go +++ b/app/controlplane/internal/usercontext/attjwtmiddleware/attmiddleware_test.go @@ -19,12 +19,19 @@ import ( "context" "io" "net/http" + "net/http/httptest" "testing" + "time" + conf "github.com/chainloop-dev/chainloop/app/controlplane/internal/conf/controlplane/config/v1" "github.com/chainloop-dev/chainloop/app/controlplane/internal/usercontext/attjwtmiddleware" + "github.com/chainloop-dev/chainloop/app/controlplane/pkg/jwt/user" + "github.com/chainloop-dev/chainloop/pkg/cache" "github.com/go-kratos/kratos/v2/log" "github.com/go-kratos/kratos/v2/transport" + "github.com/golang-jwt/jwt/v5" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) var emptyHandler = func(_ context.Context, _ interface{}) (interface{}, error) { return nil, nil } @@ -137,3 +144,52 @@ func TestAttestationAPITokenProvider(t *testing.T) { }) } } + +// newExpiredUserToken builds a Chainloop user token whose expiration is in the +// past, signed with the given key. +func newExpiredUserToken(t *testing.T, key string) string { + t.Helper() + token := jwt.NewWithClaims(user.SigningMethod, jwt.RegisteredClaims{ + Audience: jwt.ClaimStrings{user.Audience}, + ExpiresAt: jwt.NewNumericDate(time.Now().Add(-1 * time.Hour)), + IssuedAt: jwt.NewNumericDate(time.Now().Add(-2 * time.Hour)), + }) + signed, err := token.SignedString([]byte(key)) + require.NoError(t, err) + return signed +} + +// TestExpiredTokenWithFederatedProvider ensures that an expired Chainloop token +// surfaces the proper "token has expired" error even when the federated +// provider is enabled. Otherwise the expiry is masked by the federated path +// (e.g. "no issuers configured"), leaving the user with a misleading message. +func TestExpiredTokenWithFederatedProvider(t *testing.T) { + // Federated endpoint that mimics the masking behaviour: it rejects the + // expired token with an unrelated authorization error. + federatedSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"code":1,"message":"no issuers configured"}`)) + })) + defer federatedSrv.Close() + + claimsCache, err := cache.New[*jwt.MapClaims](cache.WithTTL(time.Minute)) + require.NoError(t, err) + + logger := log.NewStdLogger(io.Discard) + header := newTokenHeader("Authorization", "Bearer "+newExpiredUserToken(t, signingKey)) + ctx := transport.NewServerContext(context.Background(), &mockTransport{reqHeader: header}) + + m := attjwtmiddleware.WithJWTMulti(logger, + attjwtmiddleware.NewRobotAccountProvider(signingKey), + attjwtmiddleware.NewAPITokenProvider(signingKey), + attjwtmiddleware.NewUserTokenProvider(signingKey), + attjwtmiddleware.WithFederatedProvider(&conf.FederatedAuthentication{Enabled: true, Url: federatedSrv.URL}), + attjwtmiddleware.WithClaimsCache(claimsCache), + ) + + _, err = m(emptyHandler)(ctx, nil) + require.Error(t, err) + // The expiry must be reported, not the masked federated error. + assert.ErrorContains(t, err, "JWT token has expired") + assert.NotContains(t, err.Error(), "no issuers configured") +}