fix(auth): address review feedback on access token security

- Extract magic number 300 to MaxAccessTokenLifetime constant
- Add isEphemeralAccessToken helper with iat check for recent issuance
- Enforce expiry constraints in GenerateAccessToken (must be > 0 and <= 5min)
- Improve test assertion using InDelta for timing tolerance
- Add test for invalid expiry rejection
- Add test verifying long-lived tokens require store validation
- Add test verifying never-expiring tokens require store validation

commit: 43a7f63548bacb408804f5ce324028f8c5f9499d
author: brice · 9mo ago · 2026-01-15 11:46 UTC
parent: 62688af
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +150 -10

M internal/usecase/auth/service.go +46 -8


@@ -22,6 +22,11 @@ const (
22	22		TokenIssuer = "gordon-registry"
23	23		// DefaultBcryptCost is the default cost for bcrypt hashing.
24	24		DefaultBcryptCost = 12
	25	+	// MaxAccessTokenLifetime is the maximum lifetime for ephemeral access tokens.
	26	+	// Tokens with this lifetime or less skip store validation.
	27	+	MaxAccessTokenLifetime = 5 * time.Minute
	28	+	// maxAccessTokenLifetimeSecs is MaxAccessTokenLifetime in seconds for JWT comparisons.
	29	+	maxAccessTokenLifetimeSecs = int64(MaxAccessTokenLifetime / time.Second)
25	30		)
26	31
27	32		// Config holds the authentication configuration.

@@ -101,10 +106,11 @@ func (s Service) ValidateToken(ctx context.Context, tokenString string) (domai
101	106
102	107		tokenClaims := buildTokenClaims(claims)
103	108
104		-	// Access tokens (short-lived, ≤5min) are not stored - skip store validation
105		-	// CLI-generated tokens must exist in store to prevent use of externally-created tokens
106		-	isAccessToken := tokenClaims.ExpiresAt > 0 &&
107		-	(tokenClaims.ExpiresAt-tokenClaims.IssuedAt) <= 300
	109	+	// Access tokens (short-lived, ≤5min, recently issued) skip store validation.
	110	+	// CLI-generated tokens must exist in store to prevent use of externally-created tokens.
	111	+	// Security: require token to be recently issued (within MaxAccessTokenLifetime) to prevent
	112	+	// attackers with stolen secrets from creating arbitrary short-lived tokens.
	113	+	isAccessToken := s.isEphemeralAccessToken(tokenClaims)
108	114
109	115		if !isAccessToken {
110	116		if err := s.ensureTokenExists(ctx, tokenClaims, log); err != nil {

@@ -233,6 +239,32 @@ func ensureTokenNotExpired(tokenClaims *domain.TokenClaims, log zerowrap.Logger)
233	239		return nil
234	240		}
235	241
	242	+	// isEphemeralAccessToken checks if a token is a short-lived access token that
	243	+	// doesn't require store validation. Access tokens must:
	244	+	// 1. Have a positive expiry (not never-expiring)
	245	+	// 2. Have lifetime ≤ MaxAccessTokenLifetime
	246	+	// 3. Be recently issued (within MaxAccessTokenLifetime) to prevent replay attacks
	247	+	func (s Service) isEphemeralAccessToken(claims domain.TokenClaims) bool {
	248	+	if claims.ExpiresAt <= 0 {
	249	+	return false // Never-expiring tokens require store validation
	250	+	}
	251	+
	252	+	lifetime := claims.ExpiresAt - claims.IssuedAt
	253	+	if lifetime > maxAccessTokenLifetimeSecs {
	254	+	return false // Long-lived tokens require store validation
	255	+	}
	256	+
	257	+	// Security: ensure token was recently issued to prevent attackers with
	258	+	// stolen secrets from creating arbitrary short-lived tokens
	259	+	now := time.Now().Unix()
	260	+	tokenAge := now - claims.IssuedAt
	261	+	if tokenAge > maxAccessTokenLifetimeSecs {
	262	+	return false // Old tokens require store validation
	263	+	}
	264	+
	265	+	return true
	266	+	}
	267	+
236	268		// GenerateToken creates a new JWT token for the given subject.
237	269		func (s *Service) GenerateToken(ctx context.Context, subject string, scopes []string, expiry time.Duration) (string, error) {
238	270		ctx = zerowrap.CtxWithFields(ctx, map[string]any{

@@ -294,6 +326,7 @@ func (s *Service) GenerateToken(ctx context.Context, subject string, scopes []st
294	326
295	327		// GenerateAccessToken creates a short-lived JWT for registry access without storing it.
296	328		// Used by /v2/token endpoint - these tokens don't need persistence.
	329	+	// Expiry must be > 0 and <= MaxAccessTokenLifetime to prevent misuse.
297	330		func (s *Service) GenerateAccessToken(ctx context.Context, subject string, scopes []string, expiry time.Duration) (string, error) {
298	331		ctx = zerowrap.CtxWithFields(ctx, map[string]any{
299	332		zerowrap.FieldLayer: "usecase",

@@ -302,6 +335,14 @@ func (s *Service) GenerateAccessToken(ctx context.Context, subject string, scope
302	335		})
303	336		log := zerowrap.FromCtx(ctx)
304	337
	338	+	// Enforce expiry constraints to prevent misuse
	339	+	if expiry <= 0 {
	340	+	return "", fmt.Errorf("access token expiry must be positive")
	341	+	}
	342	+	if expiry > MaxAccessTokenLifetime {
	343	+	return "", fmt.Errorf("access token expiry exceeds maximum of %v", MaxAccessTokenLifetime)
	344	+	}
	345	+
305	346		tokenID := uuid.New().String()
306	347		now := time.Now()
307	348

@@ -311,10 +352,7 @@ func (s *Service) GenerateAccessToken(ctx context.Context, subject string, scope
311	352		"iss": TokenIssuer,
312	353		"iat": now.Unix(),
313	354		"scopes": scopes,
314		-	}
315		-
316		-	if expiry > 0 {
317		-	claims["exp"] = now.Add(expiry).Unix()
	355	+	"exp": now.Add(expiry).Unix(),
318	356		}
319	357
320	358		jwtToken := jwt.NewWithClaims(jwt.SigningMethodHS256, claims)

 	TokenIssuer = "gordon-registry"
 	// DefaultBcryptCost is the default cost for bcrypt hashing.
 	DefaultBcryptCost = 12
+	// MaxAccessTokenLifetime is the maximum lifetime for ephemeral access tokens.
+	// Tokens with this lifetime or less skip store validation.
+	MaxAccessTokenLifetime = 5 * time.Minute
+	// maxAccessTokenLifetimeSecs is MaxAccessTokenLifetime in seconds for JWT comparisons.
+	maxAccessTokenLifetimeSecs = int64(MaxAccessTokenLifetime / time.Second)
 )
 // Config holds the authentication configuration.
 	tokenClaims := buildTokenClaims(claims)
-	// Access tokens (short-lived, ≤5min) are not stored - skip store validation
+	// Access tokens (short-lived, ≤5min, recently issued) skip store validation.
-	// CLI-generated tokens must exist in store to prevent use of externally-created tokens
+	// CLI-generated tokens must exist in store to prevent use of externally-created tokens.
-	isAccessToken := tokenClaims.ExpiresAt > 0 &&
+	// Security: require token to be recently issued (within MaxAccessTokenLifetime) to prevent
-		(tokenClaims.ExpiresAt-tokenClaims.IssuedAt) <= 300
+	// attackers with stolen secrets from creating arbitrary short-lived tokens.
+	isAccessToken := s.isEphemeralAccessToken(tokenClaims)
 	if !isAccessToken {
 		if err := s.ensureTokenExists(ctx, tokenClaims, log); err != nil {
 	return nil
 }
+// isEphemeralAccessToken checks if a token is a short-lived access token that
+// doesn't require store validation. Access tokens must:
+// 1. Have a positive expiry (not never-expiring)
+// 2. Have lifetime ≤ MaxAccessTokenLifetime
+// 3. Be recently issued (within MaxAccessTokenLifetime) to prevent replay attacks
+func (s *Service) isEphemeralAccessToken(claims *domain.TokenClaims) bool {
+	if claims.ExpiresAt <= 0 {
+		return false // Never-expiring tokens require store validation
+	}
+	lifetime := claims.ExpiresAt - claims.IssuedAt
+	if lifetime > maxAccessTokenLifetimeSecs {
+		return false // Long-lived tokens require store validation
+	}
+	// Security: ensure token was recently issued to prevent attackers with
+	// stolen secrets from creating arbitrary short-lived tokens
+	now := time.Now().Unix()
+	tokenAge := now - claims.IssuedAt
+	if tokenAge > maxAccessTokenLifetimeSecs {
+		return false // Old tokens require store validation
+	}
+	return true
+}
 // GenerateToken creates a new JWT token for the given subject.
 func (s *Service) GenerateToken(ctx context.Context, subject string, scopes []string, expiry time.Duration) (string, error) {
 	ctx = zerowrap.CtxWithFields(ctx, map[string]any{
 // GenerateAccessToken creates a short-lived JWT for registry access without storing it.
 // Used by /v2/token endpoint - these tokens don't need persistence.
+// Expiry must be > 0 and <= MaxAccessTokenLifetime to prevent misuse.
 func (s *Service) GenerateAccessToken(ctx context.Context, subject string, scopes []string, expiry time.Duration) (string, error) {
 	ctx = zerowrap.CtxWithFields(ctx, map[string]any{
 		zerowrap.FieldLayer:   "usecase",
 	})
 	log := zerowrap.FromCtx(ctx)
+	// Enforce expiry constraints to prevent misuse
+	if expiry <= 0 {
+		return "", fmt.Errorf("access token expiry must be positive")
+	}
+	if expiry > MaxAccessTokenLifetime {
+		return "", fmt.Errorf("access token expiry exceeds maximum of %v", MaxAccessTokenLifetime)
+	}
 	tokenID := uuid.New().String()
 	now := time.Now()
 		"iss":    TokenIssuer,
 		"iat":    now.Unix(),
 		"scopes": scopes,
-	}
+		"exp":    now.Add(expiry).Unix(),
-	if expiry > 0 {
-		claims["exp"] = now.Add(expiry).Unix()
 	}
 	jwtToken := jwt.NewWithClaims(jwt.SigningMethodHS256, claims)

M internal/usecase/auth/service_test.go +104 -2


@@ -201,8 +201,110 @@ func TestService_GenerateAccessToken_NoStorage(t *testing.T) {
201	201		assert.NoError(t, err)
202	202		assert.Equal(t, "testuser", claims.Subject)
203	203		assert.Contains(t, claims.Scopes, "repository:myrepo:push,pull")
204		-	// Access token should have short expiry (5 min = 300 seconds)
205		-	assert.True(t, claims.ExpiresAt-claims.IssuedAt <= 300)
	204	+	// Access token should have exactly 5 min expiry (allow 2 sec tolerance for test execution)
	205	+	assert.InDelta(t, 300, claims.ExpiresAt-claims.IssuedAt, 2)
	206	+	}
	207	+
	208	+	func TestService_GenerateAccessToken_RejectsInvalidExpiry(t *testing.T) {
	209	+	tokenStore := mocks.NewMockTokenStore(t)
	210	+
	211	+	svc := NewService(Config{
	212	+	Enabled: true,
	213	+	AuthType: domain.AuthTypeToken,
	214	+	TokenSecret: []byte("test-secret-key-for-jwt-signing"),
	215	+	}, tokenStore, zerowrap.Default())
	216	+
	217	+	ctx := testContext()
	218	+
	219	+	// Zero expiry should be rejected
	220	+	_, err := svc.GenerateAccessToken(ctx, "testuser", []string{"repository:myrepo:pull"}, 0)
	221	+	assert.Error(t, err)
	222	+	assert.Contains(t, err.Error(), "expiry must be positive")
	223	+
	224	+	// Negative expiry should be rejected
	225	+	_, err = svc.GenerateAccessToken(ctx, "testuser", []string{"repository:myrepo:pull"}, -time.Minute)
	226	+	assert.Error(t, err)
	227	+	assert.Contains(t, err.Error(), "expiry must be positive")
	228	+
	229	+	// Expiry exceeding max should be rejected
	230	+	_, err = svc.GenerateAccessToken(ctx, "testuser", []string{"repository:myrepo:pull"}, 10*time.Minute)
	231	+	assert.Error(t, err)
	232	+	assert.Contains(t, err.Error(), "exceeds maximum")
	233	+	}
	234	+
	235	+	func TestService_ValidateToken_LongLivedRequiresStore(t *testing.T) {
	236	+	// Tokens with expiry > 5 minutes must exist in store
	237	+	tokenStore := mocks.NewMockTokenStore(t)
	238	+
	239	+	svc := NewService(Config{
	240	+	Enabled: true,
	241	+	AuthType: domain.AuthTypeToken,
	242	+	TokenSecret: []byte("test-secret-key-for-jwt-signing"),
	243	+	}, tokenStore, zerowrap.Default())
	244	+
	245	+	ctx := testContext()
	246	+
	247	+	// Generate a long-lived token (stored)
	248	+	var capturedToken *domain.Token
	249	+	tokenStore.EXPECT().
	250	+	SaveToken(mock.Anything, mock.Anything, mock.Anything).
	251	+	Run(func(_ context.Context, t *domain.Token, _ string) {
	252	+	capturedToken = t
	253	+	}).
	254	+	Return(nil)
	255	+
	256	+	token, err := svc.GenerateToken(ctx, "testuser", []string{"push", "pull"}, time.Hour)
	257	+	assert.NoError(t, err)
	258	+
	259	+	// Validation should call GetToken and IsRevoked
	260	+	tokenStore.EXPECT().
	261	+	GetToken(mock.Anything, "testuser").
	262	+	Return(token, capturedToken, nil)
	263	+	tokenStore.EXPECT().
	264	+	IsRevoked(mock.Anything, capturedToken.ID).
	265	+	Return(false, nil)
	266	+
	267	+	claims, err := svc.ValidateToken(ctx, token)
	268	+	assert.NoError(t, err)
	269	+	assert.Equal(t, "testuser", claims.Subject)
	270	+	}
	271	+
	272	+	func TestService_ValidateToken_NeverExpiringRequiresStore(t *testing.T) {
	273	+	// Tokens with ExpiresAt = 0 (never expires) must exist in store
	274	+	tokenStore := mocks.NewMockTokenStore(t)
	275	+
	276	+	svc := NewService(Config{
	277	+	Enabled: true,
	278	+	AuthType: domain.AuthTypeToken,
	279	+	TokenSecret: []byte("test-secret-key-for-jwt-signing"),
	280	+	}, tokenStore, zerowrap.Default())
	281	+
	282	+	ctx := testContext()
	283	+
	284	+	// Generate a never-expiring token (stored)
	285	+	var capturedToken *domain.Token
	286	+	tokenStore.EXPECT().
	287	+	SaveToken(mock.Anything, mock.Anything, mock.Anything).
	288	+	Run(func(_ context.Context, t *domain.Token, _ string) {
	289	+	capturedToken = t
	290	+	}).
	291	+	Return(nil)
	292	+
	293	+	token, err := svc.GenerateToken(ctx, "ci-bot", []string{"push", "pull"}, 0)
	294	+	assert.NoError(t, err)
	295	+
	296	+	// Validation should call GetToken and IsRevoked (not bypass store)
	297	+	tokenStore.EXPECT().
	298	+	GetToken(mock.Anything, "ci-bot").
	299	+	Return(token, capturedToken, nil)
	300	+	tokenStore.EXPECT().
	301	+	IsRevoked(mock.Anything, capturedToken.ID).
	302	+	Return(false, nil)
	303	+
	304	+	claims, err := svc.ValidateToken(ctx, token)
	305	+	assert.NoError(t, err)
	306	+	assert.Equal(t, "ci-bot", claims.Subject)
	307	+	assert.Equal(t, int64(0), claims.ExpiresAt) // Never expires
206	308		}
207	309
208	310		func TestService_ValidateToken_Success(t *testing.T) {


@@ -201,8 +201,110 @@ func TestService_GenerateAccessToken_NoStorage(t *testing.T) {
201		assert.NoError(t, err)	201		assert.NoError(t, err)
202		assert.Equal(t, "testuser", claims.Subject)	202		assert.Equal(t, "testuser", claims.Subject)
203		assert.Contains(t, claims.Scopes, "repository:myrepo:push,pull")	203		assert.Contains(t, claims.Scopes, "repository:myrepo:push,pull")
204	-	// Access token should have short expiry (5 min = 300 seconds)	204	+	// Access token should have exactly 5 min expiry (allow 2 sec tolerance for test execution)
205	-	assert.True(t, claims.ExpiresAt-claims.IssuedAt <= 300)	205	+	assert.InDelta(t, 300, claims.ExpiresAt-claims.IssuedAt, 2)
			206	+	}
			207	+
			208	+	func TestService_GenerateAccessToken_RejectsInvalidExpiry(t *testing.T) {
			209	+	tokenStore := mocks.NewMockTokenStore(t)
			210	+
			211	+	svc := NewService(Config{
			212	+	Enabled: true,
			213	+	AuthType: domain.AuthTypeToken,
			214	+	TokenSecret: []byte("test-secret-key-for-jwt-signing"),
			215	+	}, tokenStore, zerowrap.Default())
			216	+
			217	+	ctx := testContext()
			218	+
			219	+	// Zero expiry should be rejected
			220	+	_, err := svc.GenerateAccessToken(ctx, "testuser", []string{"repository:myrepo:pull"}, 0)
			221	+	assert.Error(t, err)
			222	+	assert.Contains(t, err.Error(), "expiry must be positive")
			223	+
			224	+	// Negative expiry should be rejected
			225	+	_, err = svc.GenerateAccessToken(ctx, "testuser", []string{"repository:myrepo:pull"}, -time.Minute)
			226	+	assert.Error(t, err)
			227	+	assert.Contains(t, err.Error(), "expiry must be positive")
			228	+
			229	+	// Expiry exceeding max should be rejected
			230	+	_, err = svc.GenerateAccessToken(ctx, "testuser", []string{"repository:myrepo:pull"}, 10*time.Minute)
			231	+	assert.Error(t, err)
			232	+	assert.Contains(t, err.Error(), "exceeds maximum")
			233	+	}
			234	+
			235	+	func TestService_ValidateToken_LongLivedRequiresStore(t *testing.T) {
			236	+	// Tokens with expiry > 5 minutes must exist in store
			237	+	tokenStore := mocks.NewMockTokenStore(t)
			238	+
			239	+	svc := NewService(Config{
			240	+	Enabled: true,
			241	+	AuthType: domain.AuthTypeToken,
			242	+	TokenSecret: []byte("test-secret-key-for-jwt-signing"),
			243	+	}, tokenStore, zerowrap.Default())
			244	+
			245	+	ctx := testContext()
			246	+
			247	+	// Generate a long-lived token (stored)
			248	+	var capturedToken *domain.Token
			249	+	tokenStore.EXPECT().
			250	+	SaveToken(mock.Anything, mock.Anything, mock.Anything).
			251	+	Run(func(_ context.Context, t *domain.Token, _ string) {
			252	+	capturedToken = t
			253	+	}).
			254	+	Return(nil)
			255	+
			256	+	token, err := svc.GenerateToken(ctx, "testuser", []string{"push", "pull"}, time.Hour)
			257	+	assert.NoError(t, err)
			258	+
			259	+	// Validation should call GetToken and IsRevoked
			260	+	tokenStore.EXPECT().
			261	+	GetToken(mock.Anything, "testuser").
			262	+	Return(token, capturedToken, nil)
			263	+	tokenStore.EXPECT().
			264	+	IsRevoked(mock.Anything, capturedToken.ID).
			265	+	Return(false, nil)
			266	+
			267	+	claims, err := svc.ValidateToken(ctx, token)
			268	+	assert.NoError(t, err)
			269	+	assert.Equal(t, "testuser", claims.Subject)
			270	+	}
			271	+
			272	+	func TestService_ValidateToken_NeverExpiringRequiresStore(t *testing.T) {
			273	+	// Tokens with ExpiresAt = 0 (never expires) must exist in store
			274	+	tokenStore := mocks.NewMockTokenStore(t)
			275	+
			276	+	svc := NewService(Config{
			277	+	Enabled: true,
			278	+	AuthType: domain.AuthTypeToken,
			279	+	TokenSecret: []byte("test-secret-key-for-jwt-signing"),
			280	+	}, tokenStore, zerowrap.Default())
			281	+
			282	+	ctx := testContext()
			283	+
			284	+	// Generate a never-expiring token (stored)
			285	+	var capturedToken *domain.Token
			286	+	tokenStore.EXPECT().
			287	+	SaveToken(mock.Anything, mock.Anything, mock.Anything).
			288	+	Run(func(_ context.Context, t *domain.Token, _ string) {
			289	+	capturedToken = t
			290	+	}).
			291	+	Return(nil)
			292	+
			293	+	token, err := svc.GenerateToken(ctx, "ci-bot", []string{"push", "pull"}, 0)
			294	+	assert.NoError(t, err)
			295	+
			296	+	// Validation should call GetToken and IsRevoked (not bypass store)
			297	+	tokenStore.EXPECT().
			298	+	GetToken(mock.Anything, "ci-bot").
			299	+	Return(token, capturedToken, nil)
			300	+	tokenStore.EXPECT().
			301	+	IsRevoked(mock.Anything, capturedToken.ID).
			302	+	Return(false, nil)
			303	+
			304	+	claims, err := svc.ValidateToken(ctx, token)
			305	+	assert.NoError(t, err)
			306	+	assert.Equal(t, "ci-bot", claims.Subject)
			307	+	assert.Equal(t, int64(0), claims.ExpiresAt) // Never expires
206		}	308		}
207			309
208		func TestService_ValidateToken_Success(t *testing.T) {	310		func TestService_ValidateToken_Success(t *testing.T) {