fix: enforce API key lifetime and expiry constraints

ThomasK33 · ThomasK33 · commit a8fd73e5ae3e · 2025-10-28T09:07:58.000+01:00
Add checks to require positive API key lifetime and non-retro expiry.
Backfill bad rows before constraint to crash fast on regressions.
Update test API keys to assert sensible CreatedAt timestamps.
diff --git a/coderd/apikey_test.go b/coderd/apikey_test.go
@@ -16,7 +16,6 @@ import (
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/dbgen"
 	"github.com/coder/coder/v2/coderd/database/dbtestutil"
-	"github.com/coder/coder/v2/coderd/database/dbtime"
 	"github.com/coder/coder/v2/coderd/httpapi"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/testutil"
@@ -342,7 +341,7 @@ func TestSessionExpiry(t *testing.T) {
 	err = db.UpdateAPIKeyByID(ctx, database.UpdateAPIKeyByIDParams{
 		ID:        apiKey.ID,
 		LastUsed:  apiKey.LastUsed,
-		ExpiresAt: dbtime.Now().Add(-time.Hour),
+		ExpiresAt: apiKey.CreatedAt,
 		IPAddress: apiKey.IPAddress,
 	})
 	require.NoError(t, err)
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -326,6 +326,10 @@ func New(options *Options) *API {
 		panic("developer error: options.PrometheusRegistry is nil and not running a unit test")
 	}
 
+	if err := options.DeploymentValues.Validate(); err != nil {
+		panic(fmt.Sprintf("developer error: deployment values are invalid: %s", err))
+	}
+
 	if options.DeploymentValues.DisableOwnerWorkspaceExec {
 		rbac.ReloadBuiltinRoles(&rbac.RoleOptions{
 			NoOwnerWorkspaceExec: true,
diff --git a/coderd/database/check_constraint.go b/coderd/database/check_constraint.go
diff --git a/coderd/database/dbpurge/dbpurge_test.go b/coderd/database/dbpurge/dbpurge_test.go
@@ -660,7 +660,7 @@ func TestExpireOldAPIKeys(t *testing.T) {
 			TemplateID: tpl.ID,
 		})
 		createAPIKey = func(userID uuid.UUID, name string) database.APIKey {
-			k, _ := dbgen.APIKey(t, db, database.APIKey{UserID: userID, TokenName: name, ExpiresAt: now.Add(time.Hour)}, func(iap *database.InsertAPIKeyParams) {
+			k, _ := dbgen.APIKey(t, db, database.APIKey{UserID: userID, TokenName: name, CreatedAt: now, ExpiresAt: now.Add(time.Hour)}, func(iap *database.InsertAPIKeyParams) {
 				iap.TokenName = name
 			})
 			return k
diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
diff --git a/coderd/database/migrations/000391_api_key_created_at_constraint.down.sql b/coderd/database/migrations/000391_api_key_created_at_constraint.down.sql
@@ -0,0 +1,6 @@
+-- Drop all CHECK constraints added in the up migration
+ALTER TABLE api_keys
+DROP CONSTRAINT api_keys_lifetime_seconds_positive;
+
+ALTER TABLE api_keys
+DROP CONSTRAINT api_keys_expires_at_after_created;
diff --git a/coderd/database/migrations/000391_api_key_created_at_constraint.up.sql b/coderd/database/migrations/000391_api_key_created_at_constraint.up.sql
@@ -0,0 +1,24 @@
+-- Defensively update any API keys with zero or negative lifetime_seconds to default 86400 (24 hours)
+-- This aligns with application logic that converts 0 to 86400
+UPDATE api_keys
+SET lifetime_seconds = 86400
+WHERE lifetime_seconds <= 0;
+
+-- Add CHECK constraint to ensure lifetime_seconds is positive
+-- This enforces positive lifetimes at database level
+ALTER TABLE api_keys
+ADD CONSTRAINT api_keys_lifetime_seconds_positive
+CHECK (lifetime_seconds > 0);
+
+-- Defensivey update any API keys expires_at with its created_at value
+-- This ensures all existing keys have a non-null expires_at before
+-- adding the constraint.
+UPDATE api_keys
+SET expires_at = created_at
+WHERE expires_at < created_at;
+
+-- Add CHECK constraint to ensure expires_at is at or after created_at
+-- This prevents illogical data where a key expires before it's created
+ALTER TABLE api_keys
+ADD CONSTRAINT api_keys_expires_at_after_created
+CHECK (expires_at >= created_at);
diff --git a/coderd/httpmw/apikey_test.go b/coderd/httpmw/apikey_test.go
@@ -248,6 +248,7 @@ func TestAPIKey(t *testing.T) {
 			user     = dbgen.User(t, db, database.User{})
 			_, token = dbgen.APIKey(t, db, database.APIKey{
 				UserID:    user.ID,
+				CreatedAt: dbtime.Now().Add(-2 * time.Hour),
 				ExpiresAt: time.Now().Add(time.Hour * -1),
 			})
 
@@ -516,6 +517,7 @@ func TestAPIKey(t *testing.T) {
 			sentAPIKey, token = dbgen.APIKey(t, db, database.APIKey{
 				UserID:    user.ID,
 				LastUsed:  dbtime.Now().AddDate(0, 0, -1),
+				CreatedAt: dbtime.Now().AddDate(0, 0, -2),
 				ExpiresAt: dbtime.Now().AddDate(0, 0, -1),
 				LoginType: database.LoginTypeOIDC,
 			})
@@ -566,6 +568,7 @@ func TestAPIKey(t *testing.T) {
 			sentAPIKey, token = dbgen.APIKey(t, db, database.APIKey{
 				UserID:    user.ID,
 				LastUsed:  dbtime.Now().AddDate(0, 0, -1),
+				CreatedAt: dbtime.Now().AddDate(0, 0, -2),
 				ExpiresAt: dbtime.Now().AddDate(0, 0, -1),
 				LoginType: database.LoginTypeOIDC,
 			})
diff --git a/coderd/httpmw/rfc6750_extended_test.go b/coderd/httpmw/rfc6750_extended_test.go
@@ -374,6 +374,7 @@ func TestOAuth2WWWAuthenticateCompliance(t *testing.T) {
 		// Create an expired API key
 		_, expiredToken := dbgen.APIKey(t, db, database.APIKey{
 			UserID:    user.ID,
+			CreatedAt: dbtime.Now().Add(-2 * time.Hour),
 			ExpiresAt: dbtime.Now().Add(-time.Hour), // Expired 1 hour ago
 		})
 
diff --git a/coderd/httpmw/rfc6750_test.go b/coderd/httpmw/rfc6750_test.go
@@ -32,6 +32,7 @@ func TestRFC6750BearerTokenAuthentication(t *testing.T) {
 	// Create an OAuth2 provider app token (which should work with bearer token authentication)
 	key, token := dbgen.APIKey(t, db, database.APIKey{
 		UserID:    user.ID,
+		CreatedAt: dbtime.Now(),
 		ExpiresAt: dbtime.Now().Add(testutil.WaitLong),
 	})
 
@@ -115,6 +116,7 @@ func TestRFC6750BearerTokenAuthentication(t *testing.T) {
 		// Create an expired token
 		_, expiredToken := dbgen.APIKey(t, db, database.APIKey{
 			UserID:    user.ID,
+			CreatedAt: dbtime.Now().Add(-2 * testutil.WaitShort),
 			ExpiresAt: dbtime.Now().Add(-testutil.WaitShort), // Expired
 		})
 
diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go
@@ -204,6 +204,9 @@ func NewServer(
 	if deploymentValues == nil {
 		return nil, xerrors.New("deploymentValues is nil")
 	}
+	if err := deploymentValues.Validate(); err != nil {
+		return nil, xerrors.Errorf("invalid deployment values: %w", err)
+	}
 	if acquirer == nil {
 		return nil, xerrors.New("acquirer is nil")
 	}
diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go
@@ -134,7 +134,7 @@ func TestHeartbeat(t *testing.T) {
 		heartbeatInterval: testutil.IntervalFast,
 	})
 
-	for i := 0; i < numBeats; i++ {
+	for range numBeats {
 		testutil.TryReceive(ctx, t, heartbeatChan)
 	}
 	// goleak.VerifyTestMain ensures that the heartbeat goroutine does not leak
@@ -192,11 +192,8 @@ func TestAcquireJob(t *testing.T) {
 				// Set the max session token lifetime so we can assert we
 				// create an API key with an expiration within the bounds of the
 				// deployment config.
-				dv := &codersdk.DeploymentValues{
-					Sessions: codersdk.SessionLifetime{
-						MaximumTokenDuration: serpent.Duration(time.Hour),
-					},
-				}
+				dv := coderdtest.DeploymentValues(t)
+				dv.Sessions.MaximumTokenDuration = serpent.Duration(time.Hour)
 				gitAuthProvider := &sdkproto.ExternalAuthProviderResource{
 					Id: "github",
 				}
@@ -4081,6 +4078,8 @@ func TestServer_ExpirePrebuildsSessionToken(t *testing.T) {
 		existingKey, _ = dbgen.APIKey(t, db, database.APIKey{
 			UserID:    database.PrebuildsSystemUserID,
 			TokenName: provisionerdserver.WorkspaceSessionTokenName(database.PrebuildsSystemUserID, workspace.ID),
+			CreatedAt: dbtime.Now(),
+			ExpiresAt: dbtime.Now().Add(time.Hour),
 		})
 	)
 
@@ -4124,7 +4123,7 @@ func setup(t *testing.T, ignoreLogErrors bool, ov *overrides) (proto.DRPCProvisi
 	defOrg, err := db.GetDefaultOrganization(context.Background())
 	require.NoError(t, err, "default org not found")
 
-	deploymentValues := &codersdk.DeploymentValues{}
+	deploymentValues := coderdtest.DeploymentValues(t)
 	var externalAuthConfigs []*externalauth.Config
 	tss := testTemplateScheduleStore()
 	uqhss := testUserQuietHoursScheduleStore()
@@ -4292,7 +4291,7 @@ func (s *fakeStream) Send(j *proto.AcquiredJob) error {
 func (s *fakeStream) Recv() (*proto.CancelAcquire, error) {
 	s.c.L.Lock()
 	defer s.c.L.Unlock()
-	for !(s.canceled || s.closed) {
+	for !s.canceled && !s.closed {
 		s.c.Wait()
 	}
 	if s.canceled {
@@ -4335,7 +4334,7 @@ func (s *fakeStream) Close() error {
 func (s *fakeStream) waitForJob() (*proto.AcquiredJob, error) {
 	s.c.L.Lock()
 	defer s.c.L.Unlock()
-	for !(s.sendCalled || s.closed) {
+	for !s.sendCalled && !s.closed {
 		s.c.Wait()
 	}
 	if s.sendCalled {
diff --git a/codersdk/deployment.go b/codersdk/deployment.go
@@ -3422,7 +3422,6 @@ func (c *DeploymentValues) Validate() error {
 	if access == 0 {
 		return xerrors.New("developer error: sessions configuration appears uninitialized - ensure all values are loaded before validation")
 	}
-
 	if refresh <= access {
 		return xerrors.Errorf(
 			"default OAuth refresh lifetime (%s) must be strictly greater than session duration (%s); set --default-oauth-refresh-lifetime to a value greater than --session-duration",
diff --git a/site/site_test.go b/site/site_test.go
@@ -91,6 +91,7 @@ func TestInjectionFailureProducesCleanHTML(t *testing.T) {
 	_, token := dbgen.APIKey(t, db, database.APIKey{
 		UserID:    user.ID,
 		LastUsed:  dbtime.Now().Add(-time.Hour),
+		CreatedAt: dbtime.Now().Add(-time.Hour),
 		ExpiresAt: dbtime.Now().Add(-time.Second),
 		LoginType: database.LoginTypeGithub,
 	})
