Skip to content

Commit c853e86

Browse files
ThomasK33ThomasK33
committed
fix: enforce API key lifetime and expiry constraints
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.
1 parent f684831 commit c853e86

11 files changed

Lines changed: 50 additions & 8 deletions

‎coderd/apikey_test.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import (
1616
"github.com/coder/coder/v2/coderd/database"
1717
"github.com/coder/coder/v2/coderd/database/dbgen"
1818
"github.com/coder/coder/v2/coderd/database/dbtestutil"
19-
"github.com/coder/coder/v2/coderd/database/dbtime"
2019
"github.com/coder/coder/v2/coderd/httpapi"
2120
"github.com/coder/coder/v2/codersdk"
2221
"github.com/coder/coder/v2/testutil"
@@ -342,7 +341,7 @@ func TestSessionExpiry(t *testing.T) {
342341
err = db.UpdateAPIKeyByID(ctx, database.UpdateAPIKeyByIDParams{
343342
ID: apiKey.ID,
344343
LastUsed: apiKey.LastUsed,
345-
ExpiresAt: dbtime.Now().Add(-time.Hour),
344+
ExpiresAt: apiKey.CreatedAt,
346345
IPAddress: apiKey.IPAddress,
347346
})
348347
require.NoError(t, err)

‎coderd/database/check_constraint.go‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbpurge/dbpurge_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -660,7 +660,7 @@ func TestExpireOldAPIKeys(t *testing.T) {
660660
TemplateID: tpl.ID,
661661
})
662662
createAPIKey = func(userID uuid.UUID, name string) database.APIKey {
663-
k, _ := dbgen.APIKey(t, db, database.APIKey{UserID: userID, TokenName: name, ExpiresAt: now.Add(time.Hour)}, func(iap *database.InsertAPIKeyParams) {
663+
k, _ := dbgen.APIKey(t, db, database.APIKey{UserID: userID, TokenName: name, CreatedAt: now, ExpiresAt: now.Add(time.Hour)}, func(iap *database.InsertAPIKeyParams) {
664664
iap.TokenName = name
665665
})
666666
return k

‎coderd/database/dump.sql‎

Lines changed: 3 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000389_api_key_created_at_constraint.down.sql‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
-- Drop all CHECK constraints added in the up migration
2+
ALTER TABLE api_keys
3+
DROP CONSTRAINT api_keys_lifetime_seconds_positive;
4+
5+
ALTER TABLE api_keys
6+
DROP CONSTRAINT api_keys_expires_at_after_created;

‎coderd/database/migrations/000389_api_key_created_at_constraint.up.sql‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
-- Defensively update any API keys with zero or negative lifetime_seconds to default 86400 (24 hours)
2+
-- This aligns with application logic that converts 0 to 86400
3+
UPDATE api_keys
4+
SET lifetime_seconds = 86400
5+
WHERE lifetime_seconds <= 0;
6+
7+
-- Add CHECK constraint to ensure lifetime_seconds is positive
8+
-- This enforces positive lifetimes at database level
9+
ALTER TABLE api_keys
10+
ADD CONSTRAINT api_keys_lifetime_seconds_positive
11+
CHECK (lifetime_seconds > 0);
12+
13+
-- Defensivey update any API keys expires_at with its created_at value
14+
-- This ensures all existing keys have a non-null expires_at before
15+
-- adding the constraint.
16+
UPDATE api_keys
17+
SET expires_at = created_at
18+
WHERE expires_at < created_at;
19+
20+
-- Add CHECK constraint to ensure expires_at is at or after created_at
21+
-- This prevents illogical data where a key expires before it's created
22+
ALTER TABLE api_keys
23+
ADD CONSTRAINT api_keys_expires_at_after_created
24+
CHECK (expires_at >= created_at);

‎coderd/httpmw/apikey_test.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,7 @@ func TestAPIKey(t *testing.T) {
248248
user = dbgen.User(t, db, database.User{})
249249
_, token = dbgen.APIKey(t, db, database.APIKey{
250250
UserID: user.ID,
251+
CreatedAt: dbtime.Now().Add(-2 * time.Hour),
251252
ExpiresAt: time.Now().Add(time.Hour * -1),
252253
})
253254

@@ -516,6 +517,7 @@ func TestAPIKey(t *testing.T) {
516517
sentAPIKey, token = dbgen.APIKey(t, db, database.APIKey{
517518
UserID: user.ID,
518519
LastUsed: dbtime.Now().AddDate(0, 0, -1),
520+
CreatedAt: dbtime.Now().AddDate(0, 0, -2),
519521
ExpiresAt: dbtime.Now().AddDate(0, 0, -1),
520522
LoginType: database.LoginTypeOIDC,
521523
})
@@ -566,6 +568,7 @@ func TestAPIKey(t *testing.T) {
566568
sentAPIKey, token = dbgen.APIKey(t, db, database.APIKey{
567569
UserID: user.ID,
568570
LastUsed: dbtime.Now().AddDate(0, 0, -1),
571+
CreatedAt: dbtime.Now().AddDate(0, 0, -2),
569572
ExpiresAt: dbtime.Now().AddDate(0, 0, -1),
570573
LoginType: database.LoginTypeOIDC,
571574
})

‎coderd/httpmw/rfc6750_extended_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,7 @@ func TestOAuth2WWWAuthenticateCompliance(t *testing.T) {
374374
// Create an expired API key
375375
_, expiredToken := dbgen.APIKey(t, db, database.APIKey{
376376
UserID: user.ID,
377+
CreatedAt: dbtime.Now().Add(-2 * time.Hour),
377378
ExpiresAt: dbtime.Now().Add(-time.Hour), // Expired 1 hour ago
378379
})
379380

‎coderd/httpmw/rfc6750_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ func TestRFC6750BearerTokenAuthentication(t *testing.T) {
3232
// Create an OAuth2 provider app token (which should work with bearer token authentication)
3333
key, token := dbgen.APIKey(t, db, database.APIKey{
3434
UserID: user.ID,
35+
CreatedAt: dbtime.Now(),
3536
ExpiresAt: dbtime.Now().Add(testutil.WaitLong),
3637
})
3738

@@ -115,6 +116,7 @@ func TestRFC6750BearerTokenAuthentication(t *testing.T) {
115116
// Create an expired token
116117
_, expiredToken := dbgen.APIKey(t, db, database.APIKey{
117118
UserID: user.ID,
119+
CreatedAt: dbtime.Now().Add(-2 * testutil.WaitShort),
118120
ExpiresAt: dbtime.Now().Add(-testutil.WaitShort), // Expired
119121
})
120122

‎coderd/provisionerdserver/provisionerdserver_test.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ func TestHeartbeat(t *testing.T) {
134134
heartbeatInterval: testutil.IntervalFast,
135135
})
136136

137-
for i := 0; i < numBeats; i++ {
137+
for range numBeats {
138138
testutil.TryReceive(ctx, t, heartbeatChan)
139139
}
140140
// goleak.VerifyTestMain ensures that the heartbeat goroutine does not leak
@@ -4081,6 +4081,8 @@ func TestServer_ExpirePrebuildsSessionToken(t *testing.T) {
40814081
existingKey, _ = dbgen.APIKey(t, db, database.APIKey{
40824082
UserID: database.PrebuildsSystemUserID,
40834083
TokenName: provisionerdserver.WorkspaceSessionTokenName(database.PrebuildsSystemUserID, workspace.ID),
4084+
CreatedAt: dbtime.Now(),
4085+
ExpiresAt: dbtime.Now().Add(time.Hour),
40844086
})
40854087
)
40864088

@@ -4124,7 +4126,7 @@ func setup(t *testing.T, ignoreLogErrors bool, ov *overrides) (proto.DRPCProvisi
41244126
defOrg, err := db.GetDefaultOrganization(context.Background())
41254127
require.NoError(t, err, "default org not found")
41264128

4127-
deploymentValues := &codersdk.DeploymentValues{}
4129+
deploymentValues := coderdtest.DeploymentValues(t)
41284130
var externalAuthConfigs []*externalauth.Config
41294131
tss := testTemplateScheduleStore()
41304132
uqhss := testUserQuietHoursScheduleStore()
@@ -4292,7 +4294,7 @@ func (s *fakeStream) Send(j *proto.AcquiredJob) error {
42924294
func (s *fakeStream) Recv() (*proto.CancelAcquire, error) {
42934295
s.c.L.Lock()
42944296
defer s.c.L.Unlock()
4295-
for !(s.canceled || s.closed) {
4297+
for !s.canceled && !s.closed {
42964298
s.c.Wait()
42974299
}
42984300
if s.canceled {
@@ -4335,7 +4337,7 @@ func (s *fakeStream) Close() error {
43354337
func (s *fakeStream) waitForJob() (*proto.AcquiredJob, error) {
43364338
s.c.L.Lock()
43374339
defer s.c.L.Unlock()
4338-
for !(s.sendCalled || s.closed) {
4340+
for !s.sendCalled && !s.closed {
43394341
s.c.Wait()
43404342
}
43414343
if s.sendCalled {

0 commit comments

Comments
 (0)