Skip to content

Commit 0646b30

Browse files
sreyasreya
committed
Refactor cryptographic key handling for OIDC and API keys
This commit updates the cryptographic key handling by separating workspace app token and API key features. It corrects feature identifiers for clearer distinction between OIDC conversion and API key usage, enhancing the maintainability and clarity around cryptographic key usage within the system. Additionally, reworks related tests and migration scripts to align with these changes.
1 parent b98bff0 commit 0646b30

17 files changed

+113
-71
lines changed

‎coderd/coderd.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -616,7 +616,10 @@ func New(options *Options) *API {
616616
api.Logger.Fatal(api.ctx, "start key rotator", slog.Error(err))
617617
}
618618

619-
api.oauthConvertKeycache = cryptokeys.NewDBCache(api.Logger.Named("oauth_convert_keycache"), api.Database, database.CryptoKeyFeatureOidcConvert)
619+
api.oauthConvertKeycache, err = cryptokeys.NewSigningCache(api.Logger.Named("oauth_convert_keycache"), api.Database, database.CryptoKeyFeatureOIDCConvert)
620+
if err != nil {
621+
api.Logger.Fatal(api.ctx, "failed to initialize oauth convert key cache", slog.Error(err))
622+
}
620623

621624
api.statsReporter = workspacestats.NewReporter(workspacestats.ReporterOptions{
622625
Database: options.Database,
@@ -1402,7 +1405,7 @@ type API struct {
14021405
// resumeTokenKeycache is used to fetch and cache keys used for signing JWTs
14031406
// oauthConvertKeycache is used to fetch and cache keys used for signing JWTs
14041407
// during OAuth conversions. See userauth.go.convertUserToOauth.
1405-
oauthConvertKeycache cryptokeys.Keycache
1408+
oauthConvertKeycache cryptokeys.SigningKeycache
14061409
}
14071410

14081411
// Close waits for all WebSocket connections to drain before returning.

‎coderd/cryptokeys/rotate.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -229,9 +229,9 @@ func (k *rotator) rotateKey(ctx context.Context, tx database.Store, key database
229229

230230
func generateNewSecret(feature database.CryptoKeyFeature) (string, error) {
231231
switch feature {
232-
case database.CryptoKeyFeatureWorkspaceApps:
232+
case database.CryptoKeyFeatureWorkspaceAppsAPIKey:
233233
return generateKey(32)
234-
case database.CryptoKeyFeatureOidcConvert:
234+
case database.CryptoKeyFeatureOIDCConvert:
235235
return generateKey(64)
236236
case database.CryptoKeyFeatureTailnetResume:
237237
return generateKey(64)
@@ -250,9 +250,9 @@ func generateKey(length int) (string, error) {
250250

251251
func tokenDuration(feature database.CryptoKeyFeature) time.Duration {
252252
switch feature {
253-
case database.CryptoKeyFeatureWorkspaceApps:
253+
case database.CryptoKeyFeatureWorkspaceAppsAPIKey:
254254
return WorkspaceAppsTokenDuration
255-
case database.CryptoKeyFeatureOidcConvert:
255+
case database.CryptoKeyFeatureOIDCConvert:
256256
return OIDCConvertTokenDuration
257257
case database.CryptoKeyFeatureTailnetResume:
258258
return TailnetResumeTokenDuration

‎coderd/cryptokeys/rotate_internal_test.go

Lines changed: 32 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -38,15 +38,15 @@ func Test_rotateKeys(t *testing.T) {
3838
clock: clock,
3939
logger: logger,
4040
features: []database.CryptoKeyFeature{
41-
database.CryptoKeyFeatureWorkspaceApps,
41+
database.CryptoKeyFeatureWorkspaceAppsAPIKey,
4242
},
4343
}
4444

4545
now := dbnow(clock)
4646

4747
// Seed the database with an existing key.
4848
oldKey := dbgen.CryptoKey(t, db, database.CryptoKey{
49-
Feature: database.CryptoKeyFeatureWorkspaceApps,
49+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
5050
StartsAt: now,
5151
Sequence: 15,
5252
})
@@ -69,11 +69,11 @@ func Test_rotateKeys(t *testing.T) {
6969

7070
// The new key should be created and have a starts_at of the old key's expires_at.
7171
newKey, err := db.GetCryptoKeyByFeatureAndSequence(ctx, database.GetCryptoKeyByFeatureAndSequenceParams{
72-
Feature: database.CryptoKeyFeatureWorkspaceApps,
72+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
7373
Sequence: oldKey.Sequence + 1,
7474
})
7575
require.NoError(t, err)
76-
requireKey(t, newKey, database.CryptoKeyFeatureWorkspaceApps, oldKey.ExpiresAt(keyDuration), nullTime, oldKey.Sequence+1)
76+
requireKey(t, newKey, database.CryptoKeyFeatureWorkspaceAppsAPIKey, oldKey.ExpiresAt(keyDuration), nullTime, oldKey.Sequence+1)
7777

7878
// Advance the clock just before the keys delete time.
7979
clock.Advance(oldKey.DeletesAt.Time.UTC().Sub(now) - time.Second)
@@ -123,15 +123,15 @@ func Test_rotateKeys(t *testing.T) {
123123
clock: clock,
124124
logger: logger,
125125
features: []database.CryptoKeyFeature{
126-
database.CryptoKeyFeatureWorkspaceApps,
126+
database.CryptoKeyFeatureWorkspaceAppsAPIKey,
127127
},
128128
}
129129

130130
now := dbnow(clock)
131131

132132
// Seed the database with an existing key
133133
existingKey := dbgen.CryptoKey(t, db, database.CryptoKey{
134-
Feature: database.CryptoKeyFeatureWorkspaceApps,
134+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
135135
StartsAt: now,
136136
Sequence: 123,
137137
})
@@ -179,15 +179,15 @@ func Test_rotateKeys(t *testing.T) {
179179
clock: clock,
180180
logger: logger,
181181
features: []database.CryptoKeyFeature{
182-
database.CryptoKeyFeatureWorkspaceApps,
182+
database.CryptoKeyFeatureWorkspaceAppsAPIKey,
183183
},
184184
}
185185

186186
now := dbnow(clock)
187187

188188
// Seed the database with an existing key
189189
deletingKey := dbgen.CryptoKey(t, db, database.CryptoKey{
190-
Feature: database.CryptoKeyFeatureWorkspaceApps,
190+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
191191
StartsAt: now.Add(-keyDuration),
192192
Sequence: 789,
193193
DeletesAt: sql.NullTime{
@@ -232,15 +232,15 @@ func Test_rotateKeys(t *testing.T) {
232232
clock: clock,
233233
logger: logger,
234234
features: []database.CryptoKeyFeature{
235-
database.CryptoKeyFeatureWorkspaceApps,
235+
database.CryptoKeyFeatureWorkspaceAppsAPIKey,
236236
},
237237
}
238238

239239
now := dbnow(clock)
240240

241241
// Seed the database with an existing key
242242
deletingKey := dbgen.CryptoKey(t, db, database.CryptoKey{
243-
Feature: database.CryptoKeyFeatureWorkspaceApps,
243+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
244244
StartsAt: now,
245245
Sequence: 456,
246246
DeletesAt: sql.NullTime{
@@ -281,7 +281,7 @@ func Test_rotateKeys(t *testing.T) {
281281
clock: clock,
282282
logger: logger,
283283
features: []database.CryptoKeyFeature{
284-
database.CryptoKeyFeatureWorkspaceApps,
284+
database.CryptoKeyFeatureWorkspaceAppsAPIKey,
285285
},
286286
}
287287

@@ -291,7 +291,7 @@ func Test_rotateKeys(t *testing.T) {
291291
keys, err := db.GetCryptoKeys(ctx)
292292
require.NoError(t, err)
293293
require.Len(t, keys, 1)
294-
requireKey(t, keys[0], database.CryptoKeyFeatureWorkspaceApps, clock.Now().UTC(), nullTime, 1)
294+
requireKey(t, keys[0], database.CryptoKeyFeatureWorkspaceAppsAPIKey, clock.Now().UTC(), nullTime, 1)
295295
})
296296

297297
// Assert we insert a new key when the only key was manually deleted.
@@ -312,14 +312,14 @@ func Test_rotateKeys(t *testing.T) {
312312
clock: clock,
313313
logger: logger,
314314
features: []database.CryptoKeyFeature{
315-
database.CryptoKeyFeatureWorkspaceApps,
315+
database.CryptoKeyFeatureWorkspaceAppsAPIKey,
316316
},
317317
}
318318

319319
now := dbnow(clock)
320320

321321
deletedkey := dbgen.CryptoKey(t, db, database.CryptoKey{
322-
Feature: database.CryptoKeyFeatureWorkspaceApps,
322+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
323323
StartsAt: now,
324324
Sequence: 19,
325325
DeletesAt: sql.NullTime{
@@ -338,7 +338,7 @@ func Test_rotateKeys(t *testing.T) {
338338
keys, err := db.GetCryptoKeys(ctx)
339339
require.NoError(t, err)
340340
require.Len(t, keys, 1)
341-
requireKey(t, keys[0], database.CryptoKeyFeatureWorkspaceApps, now, nullTime, deletedkey.Sequence+1)
341+
requireKey(t, keys[0], database.CryptoKeyFeatureWorkspaceAppsAPIKey, now, nullTime, deletedkey.Sequence+1)
342342
})
343343

344344
// This tests ensures that rotation works with multiple
@@ -389,14 +389,14 @@ func Test_rotateKeys(t *testing.T) {
389389

390390
// Insert a key that should be rotated.
391391
rotatedKey := dbgen.CryptoKey(t, db, database.CryptoKey{
392-
Feature: database.CryptoKeyFeatureWorkspaceApps,
392+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
393393
StartsAt: now.Add(-keyDuration + time.Hour),
394394
Sequence: 42,
395395
})
396396

397397
// Insert a key that should not trigger an action.
398398
validKey := dbgen.CryptoKey(t, db, database.CryptoKey{
399-
Feature: database.CryptoKeyFeatureOidcConvert,
399+
Feature: database.CryptoKeyFeatureOIDCConvert,
400400
StartsAt: now,
401401
Sequence: 17,
402402
})
@@ -412,29 +412,29 @@ func Test_rotateKeys(t *testing.T) {
412412
require.NoError(t, err)
413413

414414
// No actions on OIDC convert.
415-
require.Len(t, kbf[database.CryptoKeyFeatureOidcConvert], 1)
415+
require.Len(t, kbf[database.CryptoKeyFeatureOIDCConvert], 1)
416416
// Workspace apps should have been rotated.
417-
require.Len(t, kbf[database.CryptoKeyFeatureWorkspaceApps], 2)
417+
require.Len(t, kbf[database.CryptoKeyFeatureWorkspaceAppsAPIKey], 2)
418418
// No existing key for tailnet resume should've
419419
// caused a key to be inserted.
420420
require.Len(t, kbf[database.CryptoKeyFeatureTailnetResume], 1)
421421

422-
oidcKey := kbf[database.CryptoKeyFeatureOidcConvert][0]
422+
oidcKey := kbf[database.CryptoKeyFeatureOIDCConvert][0]
423423
tailnetKey := kbf[database.CryptoKeyFeatureTailnetResume][0]
424-
requireKey(t, oidcKey, database.CryptoKeyFeatureOidcConvert, now, nullTime, validKey.Sequence)
424+
requireKey(t, oidcKey, database.CryptoKeyFeatureOIDCConvert, now, nullTime, validKey.Sequence)
425425
requireKey(t, tailnetKey, database.CryptoKeyFeatureTailnetResume, now, nullTime, deletedKey.Sequence+1)
426426

427-
newKey := kbf[database.CryptoKeyFeatureWorkspaceApps][0]
428-
oldKey := kbf[database.CryptoKeyFeatureWorkspaceApps][1]
427+
newKey := kbf[database.CryptoKeyFeatureWorkspaceAppsAPIKey][0]
428+
oldKey := kbf[database.CryptoKeyFeatureWorkspaceAppsAPIKey][1]
429429
if newKey.Sequence == rotatedKey.Sequence {
430430
oldKey, newKey = newKey, oldKey
431431
}
432432
deletesAt := sql.NullTime{
433433
Time: rotatedKey.ExpiresAt(keyDuration).Add(WorkspaceAppsTokenDuration + time.Hour),
434434
Valid: true,
435435
}
436-
requireKey(t, oldKey, database.CryptoKeyFeatureWorkspaceApps, rotatedKey.StartsAt.UTC(), deletesAt, rotatedKey.Sequence)
437-
requireKey(t, newKey, database.CryptoKeyFeatureWorkspaceApps, rotatedKey.ExpiresAt(keyDuration), nullTime, rotatedKey.Sequence+1)
436+
requireKey(t, oldKey, database.CryptoKeyFeatureWorkspaceAppsAPIKey, rotatedKey.StartsAt.UTC(), deletesAt, rotatedKey.Sequence)
437+
requireKey(t, newKey, database.CryptoKeyFeatureWorkspaceAppsAPIKey, rotatedKey.ExpiresAt(keyDuration), nullTime, rotatedKey.Sequence+1)
438438
})
439439

440440
t.Run("UnknownFeature", func(t *testing.T) {
@@ -478,11 +478,11 @@ func Test_rotateKeys(t *testing.T) {
478478
keyDuration: keyDuration,
479479
clock: clock,
480480
logger: logger,
481-
features: []database.CryptoKeyFeature{database.CryptoKeyFeatureWorkspaceApps},
481+
features: []database.CryptoKeyFeature{database.CryptoKeyFeatureWorkspaceAppsAPIKey},
482482
}
483483

484484
expiringKey := dbgen.CryptoKey(t, db, database.CryptoKey{
485-
Feature: database.CryptoKeyFeatureWorkspaceApps,
485+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
486486
StartsAt: now.Add(-keyDuration),
487487
Sequence: 345,
488488
})
@@ -522,19 +522,19 @@ func Test_rotateKeys(t *testing.T) {
522522
keyDuration: keyDuration,
523523
clock: clock,
524524
logger: logger,
525-
features: []database.CryptoKeyFeature{database.CryptoKeyFeatureWorkspaceApps},
525+
features: []database.CryptoKeyFeature{database.CryptoKeyFeatureWorkspaceAppsAPIKey},
526526
}
527527

528528
now := dbnow(clock)
529529

530530
expiredKey := dbgen.CryptoKey(t, db, database.CryptoKey{
531-
Feature: database.CryptoKeyFeatureWorkspaceApps,
531+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
532532
StartsAt: now.Add(-keyDuration - 2*time.Hour),
533533
Sequence: 19,
534534
})
535535

536536
deletedKey := dbgen.CryptoKey(t, db, database.CryptoKey{
537-
Feature: database.CryptoKeyFeatureWorkspaceApps,
537+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
538538
StartsAt: now,
539539
Sequence: 20,
540540
Secret: sql.NullString{
@@ -587,9 +587,9 @@ func requireKey(t *testing.T, key database.CryptoKey, feature database.CryptoKey
587587
require.NoError(t, err)
588588

589589
switch key.Feature {
590-
case database.CryptoKeyFeatureOidcConvert:
590+
case database.CryptoKeyFeatureOIDCConvert:
591591
require.Len(t, secret, 64)
592-
case database.CryptoKeyFeatureWorkspaceApps:
592+
case database.CryptoKeyFeatureWorkspaceAppsAPIKey:
593593
require.Len(t, secret, 32)
594594
case database.CryptoKeyFeatureTailnetResume:
595595
require.Len(t, secret, 64)

‎coderd/cryptokeys/rotate_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func TestRotator(t *testing.T) {
5858
now := clock.Now().UTC()
5959

6060
rotatingKey := dbgen.CryptoKey(t, db, database.CryptoKey{
61-
Feature: database.CryptoKeyFeatureWorkspaceApps,
61+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
6262
StartsAt: now.Add(-cryptokeys.DefaultKeyDuration + time.Hour + time.Minute),
6363
Sequence: 12345,
6464
})
@@ -85,7 +85,7 @@ func TestRotator(t *testing.T) {
8585
require.NoError(t, err)
8686
require.Len(t, keys, initialKeyLen+1)
8787

88-
newKey, err := db.GetLatestCryptoKeyByFeature(ctx, database.CryptoKeyFeatureWorkspaceApps)
88+
newKey, err := db.GetLatestCryptoKeyByFeature(ctx, database.CryptoKeyFeatureWorkspaceAppsAPIKey)
8989
require.NoError(t, err)
9090
require.Equal(t, rotatingKey.Sequence+1, newKey.Sequence)
9191
require.Equal(t, rotatingKey.ExpiresAt(cryptokeys.DefaultKeyDuration), newKey.StartsAt.UTC())

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2246,13 +2246,13 @@ func (s *MethodTestSuite) TestCryptoKeys() {
22462246
}))
22472247
s.Run("InsertCryptoKey", s.Subtest(func(db database.Store, check *expects) {
22482248
check.Args(database.InsertCryptoKeyParams{
2249-
Feature: database.CryptoKeyFeatureWorkspaceApps,
2249+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
22502250
}).
22512251
Asserts(rbac.ResourceCryptoKey, policy.ActionCreate)
22522252
}))
22532253
s.Run("DeleteCryptoKey", s.Subtest(func(db database.Store, check *expects) {
22542254
key := dbgen.CryptoKey(s.T(), db, database.CryptoKey{
2255-
Feature: database.CryptoKeyFeatureWorkspaceApps,
2255+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
22562256
Sequence: 4,
22572257
})
22582258
check.Args(database.DeleteCryptoKeyParams{
@@ -2262,7 +2262,7 @@ func (s *MethodTestSuite) TestCryptoKeys() {
22622262
}))
22632263
s.Run("GetCryptoKeyByFeatureAndSequence", s.Subtest(func(db database.Store, check *expects) {
22642264
key := dbgen.CryptoKey(s.T(), db, database.CryptoKey{
2265-
Feature: database.CryptoKeyFeatureWorkspaceApps,
2265+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
22662266
Sequence: 4,
22672267
})
22682268
check.Args(database.GetCryptoKeyByFeatureAndSequenceParams{
@@ -2272,14 +2272,14 @@ func (s *MethodTestSuite) TestCryptoKeys() {
22722272
}))
22732273
s.Run("GetLatestCryptoKeyByFeature", s.Subtest(func(db database.Store, check *expects) {
22742274
dbgen.CryptoKey(s.T(), db, database.CryptoKey{
2275-
Feature: database.CryptoKeyFeatureWorkspaceApps,
2275+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
22762276
Sequence: 4,
22772277
})
2278-
check.Args(database.CryptoKeyFeatureWorkspaceApps).Asserts(rbac.ResourceCryptoKey, policy.ActionRead)
2278+
check.Args(database.CryptoKeyFeatureWorkspaceAppsAPIKey).Asserts(rbac.ResourceCryptoKey, policy.ActionRead)
22792279
}))
22802280
s.Run("UpdateCryptoKeyDeletesAt", s.Subtest(func(db database.Store, check *expects) {
22812281
key := dbgen.CryptoKey(s.T(), db, database.CryptoKey{
2282-
Feature: database.CryptoKeyFeatureWorkspaceApps,
2282+
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
22832283
Sequence: 4,
22842284
})
22852285
check.Args(database.UpdateCryptoKeyDeletesAtParams{
@@ -2289,7 +2289,7 @@ func (s *MethodTestSuite) TestCryptoKeys() {
22892289
}).Asserts(rbac.ResourceCryptoKey, policy.ActionUpdate)
22902290
}))
22912291
s.Run("GetCryptoKeysByFeature", s.Subtest(func(db database.Store, check *expects) {
2292-
check.Args(database.CryptoKeyFeatureWorkspaceApps).
2292+
check.Args(database.CryptoKeyFeatureWorkspaceAppsAPIKey).
22932293
Asserts(rbac.ResourceCryptoKey, policy.ActionRead)
22942294
}))
22952295
}

‎coderd/database/dbgen/dbgen.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -943,7 +943,7 @@ func CustomRole(t testing.TB, db database.Store, seed database.CustomRole) datab
943943
func CryptoKey(t testing.TB, db database.Store, seed database.CryptoKey) database.CryptoKey {
944944
t.Helper()
945945

946-
seed.Feature = takeFirst(seed.Feature, database.CryptoKeyFeatureWorkspaceApps)
946+
seed.Feature = takeFirst(seed.Feature, database.CryptoKeyFeatureWorkspaceAppsAPIKey)
947947

948948
// An empty string for the secret is interpreted as
949949
// a caller wanting a new secret to be generated.
@@ -1048,9 +1048,9 @@ func takeFirst[Value comparable](values ...Value) Value {
10481048

10491049
func newCryptoKeySecret(feature database.CryptoKeyFeature) (string, error) {
10501050
switch feature {
1051-
case database.CryptoKeyFeatureWorkspaceApps:
1051+
case database.CryptoKeyFeatureWorkspaceAppsAPIKey:
10521052
return generateCryptoKey(32)
1053-
case database.CryptoKeyFeatureOidcConvert:
1053+
case database.CryptoKeyFeatureOIDCConvert:
10541054
return generateCryptoKey(64)
10551055
case database.CryptoKeyFeatureTailnetResume:
10561056
return generateCryptoKey(64)

‎coderd/database/dump.sql

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

‎coderd/database/migrations/000262_site_to_crypto_key.down.sql

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
-- Step 1: Remove the new entries from crypto_keys table
2+
DELETE FROM crypto_keys
3+
WHERE feature IN ('workspace_apps_token', 'workspace_apps_api_key', 'tailnet_resume')
4+
AND sequence = 1;
5+
6+
CREATE TYPE crypto_key_feature_old AS ENUM (
7+
'workspace_apps',
8+
'oidc_convert',
9+
'tailnet_resume'
10+
);
11+
12+
ALTER TABLE crypto_keys
13+
ALTER COLUMN feature TYPE crypto_key_feature_old
14+
USING (feature::text::crypto_key_feature_old);
15+
16+
DROP TYPE crypto_key_feature;
17+
18+
ALTER TYPE crypto_key_feature_old RENAME TO crypto_key_feature;
19+

‎coderd/database/migrations/000262_site_to_crypto_key.u.sql renamed to ‎coderd/database/migrations/000262_site_to_crypto_key.up.sql

File renamed without changes.

0 commit comments

Comments
 (0)