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.
coder · sreya · Oct 25, 2024 · Oct 5, 2024 · Oct 5, 2024 · Oct 5, 2024
commit 0646b306309e30645ef482b10e8fb148890e92c3
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -616,7 +616,10 @@ func New(options *Options) *API {
 		api.Logger.Fatal(api.ctx, "start key rotator", slog.Error(err))
 	}
 
-	api.oauthConvertKeycache = cryptokeys.NewDBCache(api.Logger.Named("oauth_convert_keycache"), api.Database, database.CryptoKeyFeatureOidcConvert)
+	api.oauthConvertKeycache, err = cryptokeys.NewSigningCache(api.Logger.Named("oauth_convert_keycache"), api.Database, database.CryptoKeyFeatureOIDCConvert)
+	if err != nil {
+		api.Logger.Fatal(api.ctx, "failed to initialize oauth convert key cache", slog.Error(err))
+	}
 
 	api.statsReporter = workspacestats.NewReporter(workspacestats.ReporterOptions{
 		Database:              options.Database,
@@ -1402,7 +1405,7 @@ type API struct {
 	// resumeTokenKeycache is used to fetch and cache keys used for signing JWTs
 	// oauthConvertKeycache is used to fetch and cache keys used for signing JWTs
 	// during OAuth conversions. See userauth.go.convertUserToOauth.
-	oauthConvertKeycache cryptokeys.Keycache
+	oauthConvertKeycache cryptokeys.SigningKeycache
 }
 
 // Close waits for all WebSocket connections to drain before returning.

diff --git a/coderd/cryptokeys/rotate.go b/coderd/cryptokeys/rotate.go
@@ -229,9 +229,9 @@ func (k *rotator) rotateKey(ctx context.Context, tx database.Store, key database
 
 func generateNewSecret(feature database.CryptoKeyFeature) (string, error) {
 	switch feature {
-	case database.CryptoKeyFeatureWorkspaceApps:
+	case database.CryptoKeyFeatureWorkspaceAppsAPIKey:
 		return generateKey(32)
-	case database.CryptoKeyFeatureOidcConvert:
+	case database.CryptoKeyFeatureOIDCConvert:
 		return generateKey(64)
 	case database.CryptoKeyFeatureTailnetResume:
 		return generateKey(64)
@@ -250,9 +250,9 @@ func generateKey(length int) (string, error) {
 
 func tokenDuration(feature database.CryptoKeyFeature) time.Duration {
 	switch feature {
-	case database.CryptoKeyFeatureWorkspaceApps:
+	case database.CryptoKeyFeatureWorkspaceAppsAPIKey:
 		return WorkspaceAppsTokenDuration
-	case database.CryptoKeyFeatureOidcConvert:
+	case database.CryptoKeyFeatureOIDCConvert:
 		return OIDCConvertTokenDuration
 	case database.CryptoKeyFeatureTailnetResume:
 		return TailnetResumeTokenDuration

diff --git a/coderd/cryptokeys/rotate_internal_test.go b/coderd/cryptokeys/rotate_internal_test.go
@@ -38,15 +38,15 @@ func Test_rotateKeys(t *testing.T) {
 			clock:       clock,
 			logger:      logger,
 			features: []database.CryptoKeyFeature{
-				database.CryptoKeyFeatureWorkspaceApps,
+				database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			},
 		}
 
 		now := dbnow(clock)
 
 		// Seed the database with an existing key.
 		oldKey := dbgen.CryptoKey(t, db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			StartsAt: now,
 			Sequence: 15,
 		})
@@ -69,11 +69,11 @@ func Test_rotateKeys(t *testing.T) {
 
 		// The new key should be created and have a starts_at of the old key's expires_at.
 		newKey, err := db.GetCryptoKeyByFeatureAndSequence(ctx, database.GetCryptoKeyByFeatureAndSequenceParams{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			Sequence: oldKey.Sequence + 1,
 		})
 		require.NoError(t, err)
-		requireKey(t, newKey, database.CryptoKeyFeatureWorkspaceApps, oldKey.ExpiresAt(keyDuration), nullTime, oldKey.Sequence+1)
+		requireKey(t, newKey, database.CryptoKeyFeatureWorkspaceAppsAPIKey, oldKey.ExpiresAt(keyDuration), nullTime, oldKey.Sequence+1)
 
 		// Advance the clock just before the keys delete time.
 		clock.Advance(oldKey.DeletesAt.Time.UTC().Sub(now) - time.Second)
@@ -123,15 +123,15 @@ func Test_rotateKeys(t *testing.T) {
 			clock:       clock,
 			logger:      logger,
 			features: []database.CryptoKeyFeature{
-				database.CryptoKeyFeatureWorkspaceApps,
+				database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			},
 		}
 
 		now := dbnow(clock)
 
 		// Seed the database with an existing key
 		existingKey := dbgen.CryptoKey(t, db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			StartsAt: now,
 			Sequence: 123,
 		})
@@ -179,15 +179,15 @@ func Test_rotateKeys(t *testing.T) {
 			clock:       clock,
 			logger:      logger,
 			features: []database.CryptoKeyFeature{
-				database.CryptoKeyFeatureWorkspaceApps,
+				database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			},
 		}
 
 		now := dbnow(clock)
 
 		// Seed the database with an existing key
 		deletingKey := dbgen.CryptoKey(t, db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			StartsAt: now.Add(-keyDuration),
 			Sequence: 789,
 			DeletesAt: sql.NullTime{
@@ -232,15 +232,15 @@ func Test_rotateKeys(t *testing.T) {
 			clock:       clock,
 			logger:      logger,
 			features: []database.CryptoKeyFeature{
-				database.CryptoKeyFeatureWorkspaceApps,
+				database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			},
 		}
 
 		now := dbnow(clock)
 
 		// Seed the database with an existing key
 		deletingKey := dbgen.CryptoKey(t, db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			StartsAt: now,
 			Sequence: 456,
 			DeletesAt: sql.NullTime{
@@ -281,7 +281,7 @@ func Test_rotateKeys(t *testing.T) {
 			clock:       clock,
 			logger:      logger,
 			features: []database.CryptoKeyFeature{
-				database.CryptoKeyFeatureWorkspaceApps,
+				database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			},
 		}
 
@@ -291,7 +291,7 @@ func Test_rotateKeys(t *testing.T) {
 		keys, err := db.GetCryptoKeys(ctx)
 		require.NoError(t, err)
 		require.Len(t, keys, 1)
-		requireKey(t, keys[0], database.CryptoKeyFeatureWorkspaceApps, clock.Now().UTC(), nullTime, 1)
+		requireKey(t, keys[0], database.CryptoKeyFeatureWorkspaceAppsAPIKey, clock.Now().UTC(), nullTime, 1)
 	})
 
 	// Assert we insert a new key when the only key was manually deleted.
@@ -312,14 +312,14 @@ func Test_rotateKeys(t *testing.T) {
 			clock:       clock,
 			logger:      logger,
 			features: []database.CryptoKeyFeature{
-				database.CryptoKeyFeatureWorkspaceApps,
+				database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			},
 		}
 
 		now := dbnow(clock)
 
 		deletedkey := dbgen.CryptoKey(t, db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			StartsAt: now,
 			Sequence: 19,
 			DeletesAt: sql.NullTime{
@@ -338,7 +338,7 @@ func Test_rotateKeys(t *testing.T) {
 		keys, err := db.GetCryptoKeys(ctx)
 		require.NoError(t, err)
 		require.Len(t, keys, 1)
-		requireKey(t, keys[0], database.CryptoKeyFeatureWorkspaceApps, now, nullTime, deletedkey.Sequence+1)
+		requireKey(t, keys[0], database.CryptoKeyFeatureWorkspaceAppsAPIKey, now, nullTime, deletedkey.Sequence+1)
 	})
 
 	// This tests ensures that rotation works with multiple
@@ -389,14 +389,14 @@ func Test_rotateKeys(t *testing.T) {
 
 		// Insert a key that should be rotated.
 		rotatedKey := dbgen.CryptoKey(t, db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			StartsAt: now.Add(-keyDuration + time.Hour),
 			Sequence: 42,
 		})
 
 		// Insert a key that should not trigger an action.
 		validKey := dbgen.CryptoKey(t, db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureOidcConvert,
+			Feature:  database.CryptoKeyFeatureOIDCConvert,
 			StartsAt: now,
 			Sequence: 17,
 		})
@@ -412,29 +412,29 @@ func Test_rotateKeys(t *testing.T) {
 		require.NoError(t, err)
 
 		// No actions on OIDC convert.
-		require.Len(t, kbf[database.CryptoKeyFeatureOidcConvert], 1)
+		require.Len(t, kbf[database.CryptoKeyFeatureOIDCConvert], 1)
 		// Workspace apps should have been rotated.
-		require.Len(t, kbf[database.CryptoKeyFeatureWorkspaceApps], 2)
+		require.Len(t, kbf[database.CryptoKeyFeatureWorkspaceAppsAPIKey], 2)
 		// No existing key for tailnet resume should've
 		// caused a key to be inserted.
 		require.Len(t, kbf[database.CryptoKeyFeatureTailnetResume], 1)
 
-		oidcKey := kbf[database.CryptoKeyFeatureOidcConvert][0]
+		oidcKey := kbf[database.CryptoKeyFeatureOIDCConvert][0]
 		tailnetKey := kbf[database.CryptoKeyFeatureTailnetResume][0]
-		requireKey(t, oidcKey, database.CryptoKeyFeatureOidcConvert, now, nullTime, validKey.Sequence)
+		requireKey(t, oidcKey, database.CryptoKeyFeatureOIDCConvert, now, nullTime, validKey.Sequence)
 		requireKey(t, tailnetKey, database.CryptoKeyFeatureTailnetResume, now, nullTime, deletedKey.Sequence+1)
 
-		newKey := kbf[database.CryptoKeyFeatureWorkspaceApps][0]
-		oldKey := kbf[database.CryptoKeyFeatureWorkspaceApps][1]
+		newKey := kbf[database.CryptoKeyFeatureWorkspaceAppsAPIKey][0]
+		oldKey := kbf[database.CryptoKeyFeatureWorkspaceAppsAPIKey][1]
 		if newKey.Sequence == rotatedKey.Sequence {
 			oldKey, newKey = newKey, oldKey
 		}
 		deletesAt := sql.NullTime{
 			Time:  rotatedKey.ExpiresAt(keyDuration).Add(WorkspaceAppsTokenDuration + time.Hour),
 			Valid: true,
 		}
-		requireKey(t, oldKey, database.CryptoKeyFeatureWorkspaceApps, rotatedKey.StartsAt.UTC(), deletesAt, rotatedKey.Sequence)
-		requireKey(t, newKey, database.CryptoKeyFeatureWorkspaceApps, rotatedKey.ExpiresAt(keyDuration), nullTime, rotatedKey.Sequence+1)
+		requireKey(t, oldKey, database.CryptoKeyFeatureWorkspaceAppsAPIKey, rotatedKey.StartsAt.UTC(), deletesAt, rotatedKey.Sequence)
+		requireKey(t, newKey, database.CryptoKeyFeatureWorkspaceAppsAPIKey, rotatedKey.ExpiresAt(keyDuration), nullTime, rotatedKey.Sequence+1)
 	})
 
 	t.Run("UnknownFeature", func(t *testing.T) {
@@ -478,11 +478,11 @@ func Test_rotateKeys(t *testing.T) {
 			keyDuration: keyDuration,
 			clock:       clock,
 			logger:      logger,
-			features:    []database.CryptoKeyFeature{database.CryptoKeyFeatureWorkspaceApps},
+			features:    []database.CryptoKeyFeature{database.CryptoKeyFeatureWorkspaceAppsAPIKey},
 		}
 
 		expiringKey := dbgen.CryptoKey(t, db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			StartsAt: now.Add(-keyDuration),
 			Sequence: 345,
 		})
@@ -522,19 +522,19 @@ func Test_rotateKeys(t *testing.T) {
 			keyDuration: keyDuration,
 			clock:       clock,
 			logger:      logger,
-			features:    []database.CryptoKeyFeature{database.CryptoKeyFeatureWorkspaceApps},
+			features:    []database.CryptoKeyFeature{database.CryptoKeyFeatureWorkspaceAppsAPIKey},
 		}
 
 		now := dbnow(clock)
 
 		expiredKey := dbgen.CryptoKey(t, db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			StartsAt: now.Add(-keyDuration - 2*time.Hour),
 			Sequence: 19,
 		})
 
 		deletedKey := dbgen.CryptoKey(t, db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			StartsAt: now,
 			Sequence: 20,
 			Secret: sql.NullString{
@@ -587,9 +587,9 @@ func requireKey(t *testing.T, key database.CryptoKey, feature database.CryptoKey
 	require.NoError(t, err)
 
 	switch key.Feature {
-	case database.CryptoKeyFeatureOidcConvert:
+	case database.CryptoKeyFeatureOIDCConvert:
 		require.Len(t, secret, 64)
-	case database.CryptoKeyFeatureWorkspaceApps:
+	case database.CryptoKeyFeatureWorkspaceAppsAPIKey:
 		require.Len(t, secret, 32)
 	case database.CryptoKeyFeatureTailnetResume:
 		require.Len(t, secret, 64)

diff --git a/coderd/cryptokeys/rotate_test.go b/coderd/cryptokeys/rotate_test.go
@@ -58,7 +58,7 @@ func TestRotator(t *testing.T) {
 		now := clock.Now().UTC()
 
 		rotatingKey := dbgen.CryptoKey(t, db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			StartsAt: now.Add(-cryptokeys.DefaultKeyDuration + time.Hour + time.Minute),
 			Sequence: 12345,
 		})
@@ -85,7 +85,7 @@ func TestRotator(t *testing.T) {
 		require.NoError(t, err)
 		require.Len(t, keys, initialKeyLen+1)
 
-		newKey, err := db.GetLatestCryptoKeyByFeature(ctx, database.CryptoKeyFeatureWorkspaceApps)
+		newKey, err := db.GetLatestCryptoKeyByFeature(ctx, database.CryptoKeyFeatureWorkspaceAppsAPIKey)
 		require.NoError(t, err)
 		require.Equal(t, rotatingKey.Sequence+1, newKey.Sequence)
 		require.Equal(t, rotatingKey.ExpiresAt(cryptokeys.DefaultKeyDuration), newKey.StartsAt.UTC())

diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -2246,13 +2246,13 @@ func (s *MethodTestSuite) TestCryptoKeys() {
 	}))
 	s.Run("InsertCryptoKey", s.Subtest(func(db database.Store, check *expects) {
 		check.Args(database.InsertCryptoKeyParams{
-			Feature: database.CryptoKeyFeatureWorkspaceApps,
+			Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 		}).
 			Asserts(rbac.ResourceCryptoKey, policy.ActionCreate)
 	}))
 	s.Run("DeleteCryptoKey", s.Subtest(func(db database.Store, check *expects) {
 		key := dbgen.CryptoKey(s.T(), db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			Sequence: 4,
 		})
 		check.Args(database.DeleteCryptoKeyParams{
@@ -2262,7 +2262,7 @@ func (s *MethodTestSuite) TestCryptoKeys() {
 	}))
 	s.Run("GetCryptoKeyByFeatureAndSequence", s.Subtest(func(db database.Store, check *expects) {
 		key := dbgen.CryptoKey(s.T(), db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			Sequence: 4,
 		})
 		check.Args(database.GetCryptoKeyByFeatureAndSequenceParams{
@@ -2272,14 +2272,14 @@ func (s *MethodTestSuite) TestCryptoKeys() {
 	}))
 	s.Run("GetLatestCryptoKeyByFeature", s.Subtest(func(db database.Store, check *expects) {
 		dbgen.CryptoKey(s.T(), db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			Sequence: 4,
 		})
-		check.Args(database.CryptoKeyFeatureWorkspaceApps).Asserts(rbac.ResourceCryptoKey, policy.ActionRead)
+		check.Args(database.CryptoKeyFeatureWorkspaceAppsAPIKey).Asserts(rbac.ResourceCryptoKey, policy.ActionRead)
 	}))
 	s.Run("UpdateCryptoKeyDeletesAt", s.Subtest(func(db database.Store, check *expects) {
 		key := dbgen.CryptoKey(s.T(), db, database.CryptoKey{
-			Feature:  database.CryptoKeyFeatureWorkspaceApps,
+			Feature:  database.CryptoKeyFeatureWorkspaceAppsAPIKey,
 			Sequence: 4,
 		})
 		check.Args(database.UpdateCryptoKeyDeletesAtParams{
@@ -2289,7 +2289,7 @@ func (s *MethodTestSuite) TestCryptoKeys() {
 		}).Asserts(rbac.ResourceCryptoKey, policy.ActionUpdate)
 	}))
 	s.Run("GetCryptoKeysByFeature", s.Subtest(func(db database.Store, check *expects) {
-		check.Args(database.CryptoKeyFeatureWorkspaceApps).
+		check.Args(database.CryptoKeyFeatureWorkspaceAppsAPIKey).
 			Asserts(rbac.ResourceCryptoKey, policy.ActionRead)
 	}))
 }

diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go
@@ -943,7 +943,7 @@ func CustomRole(t testing.TB, db database.Store, seed database.CustomRole) datab
 func CryptoKey(t testing.TB, db database.Store, seed database.CryptoKey) database.CryptoKey {
 	t.Helper()
 
-	seed.Feature = takeFirst(seed.Feature, database.CryptoKeyFeatureWorkspaceApps)
+	seed.Feature = takeFirst(seed.Feature, database.CryptoKeyFeatureWorkspaceAppsAPIKey)
 
 	// An empty string for the secret is interpreted as
 	// a caller wanting a new secret to be generated.
@@ -1048,9 +1048,9 @@ func takeFirst[Value comparable](values ...Value) Value {
 
 func newCryptoKeySecret(feature database.CryptoKeyFeature) (string, error) {
 	switch feature {
-	case database.CryptoKeyFeatureWorkspaceApps:
+	case database.CryptoKeyFeatureWorkspaceAppsAPIKey:
 		return generateCryptoKey(32)
-	case database.CryptoKeyFeatureOidcConvert:
+	case database.CryptoKeyFeatureOIDCConvert:
 		return generateCryptoKey(64)
 	case database.CryptoKeyFeatureTailnetResume:
 		return generateCryptoKey(64)

diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
diff --git a/coderd/database/migrations/000262_site_to_crypto_key.down.sql b/coderd/database/migrations/000262_site_to_crypto_key.down.sql
@@ -0,0 +1,19 @@
+-- Step 1: Remove the new entries from crypto_keys table
+DELETE FROM crypto_keys
+WHERE feature IN ('workspace_apps_token', 'workspace_apps_api_key', 'tailnet_resume')
+  AND sequence = 1;
+
+CREATE TYPE crypto_key_feature_old AS ENUM (
+    'workspace_apps',
+    'oidc_convert',
+    'tailnet_resume'
+);
+
+ALTER TABLE crypto_keys
+    ALTER COLUMN feature TYPE crypto_key_feature_old
+    USING (feature::text::crypto_key_feature_old);
+
+DROP TYPE crypto_key_feature;
+
+ALTER TYPE crypto_key_feature_old RENAME TO crypto_key_feature;
+
diff --git a/...igrations/000262_site_to_crypto_key.u.sql → ...grations/000262_site_to_crypto_key.up.sql b/...igrations/000262_site_to_crypto_key.u.sql → ...grations/000262_site_to_crypto_key.up.sql