Skip to content

Commit fa9a75d

Browse files
sreyasreya
committed
Update cryptokey feature test and migration logic
- Enhance testing scenarios for cryptokey features including cases with no keys and specific key states. - Update tests to ensure new cryptokey features are handled correctly. - Remove outdated migration scripts for cryptokey features as they are not required anymore. - Refactor workspace proxy keys to only allow whitelisted cryptokey features, improving security and stability.
1 parent 7557ed2 commit fa9a75d

File tree

6 files changed

+84
-59
lines changed

6 files changed

+84
-59
lines changed

‎coderd/cryptokeys/rotate_internal_test.go

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -365,9 +365,11 @@ func Test_rotateKeys(t *testing.T) {
365365

366366
now := dbnow(clock)
367367

368-
// We'll test a scenario where one feature has no valid keys.
369-
// Another has a key that should be rotate. And one that
370-
// has a valid key that shouldn't trigger an action.
368+
// We'll test a scenario where:
369+
// - One feature has no valid keys.
370+
// - One has a key that should be rotated.
371+
// - One has a valid key that shouldn't trigger an action.
372+
// - One has no keys at all.
371373
_ = dbgen.CryptoKey(t, db, database.CryptoKey{
372374
Feature: database.CryptoKeyFeatureTailnetResume,
373375
StartsAt: now.Add(-keyDuration),
@@ -377,6 +379,7 @@ func Test_rotateKeys(t *testing.T) {
377379
Valid: false,
378380
},
379381
})
382+
// Generate another deleted key to ensure we insert after the latest sequence.
380383
deletedKey := dbgen.CryptoKey(t, db, database.CryptoKey{
381384
Feature: database.CryptoKeyFeatureTailnetResume,
382385
StartsAt: now.Add(-keyDuration),
@@ -406,7 +409,7 @@ func Test_rotateKeys(t *testing.T) {
406409

407410
keys, err := db.GetCryptoKeys(ctx)
408411
require.NoError(t, err)
409-
require.Len(t, keys, 4)
412+
require.Len(t, keys, 5)
410413

411414
kbf, err := keysByFeature(keys, database.AllCryptoKeyFeatureValues())
412415
require.NoError(t, err)
@@ -418,12 +421,14 @@ func Test_rotateKeys(t *testing.T) {
418421
// No existing key for tailnet resume should've
419422
// caused a key to be inserted.
420423
require.Len(t, kbf[database.CryptoKeyFeatureTailnetResume], 1)
424+
require.Len(t, kbf[database.CryptoKeyFeatureWorkspaceAppsToken], 1)
421425

422426
oidcKey := kbf[database.CryptoKeyFeatureOIDCConvert][0]
423427
tailnetKey := kbf[database.CryptoKeyFeatureTailnetResume][0]
428+
appTokenKey := kbf[database.CryptoKeyFeatureWorkspaceAppsToken][0]
424429
requireKey(t, oidcKey, database.CryptoKeyFeatureOIDCConvert, now, nullTime, validKey.Sequence)
425430
requireKey(t, tailnetKey, database.CryptoKeyFeatureTailnetResume, now, nullTime, deletedKey.Sequence+1)
426-
431+
requireKey(t, appTokenKey, database.CryptoKeyFeatureWorkspaceAppsToken, now, nullTime, 1)
427432
newKey := kbf[database.CryptoKeyFeatureWorkspaceAppsAPIKey][0]
428433
oldKey := kbf[database.CryptoKeyFeatureWorkspaceAppsAPIKey][1]
429434
if newKey.Sequence == rotatedKey.Sequence {
@@ -589,6 +594,8 @@ func requireKey(t *testing.T, key database.CryptoKey, feature database.CryptoKey
589594
switch key.Feature {
590595
case database.CryptoKeyFeatureOIDCConvert:
591596
require.Len(t, secret, 64)
597+
case database.CryptoKeyFeatureWorkspaceAppsToken:
598+
require.Len(t, secret, 64)
592599
case database.CryptoKeyFeatureWorkspaceAppsAPIKey:
593600
require.Len(t, secret, 32)
594601
case database.CryptoKeyFeatureTailnetResume:

‎coderd/database/dbgen/dbgen.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1050,6 +1050,8 @@ func newCryptoKeySecret(feature database.CryptoKeyFeature) (string, error) {
10501050
switch feature {
10511051
case database.CryptoKeyFeatureWorkspaceAppsAPIKey:
10521052
return generateCryptoKey(32)
1053+
case database.CryptoKeyFeatureWorkspaceAppsToken:
1054+
return generateCryptoKey(64)
10531055
case database.CryptoKeyFeatureOIDCConvert:
10541056
return generateCryptoKey(64)
10551057
case database.CryptoKeyFeatureTailnetResume:

‎coderd/database/migrations/000264_cryptokey_features.down.sql

Lines changed: 0 additions & 18 deletions
This file was deleted.

‎coderd/database/migrations/000264_cryptokey_features.up.sql

Lines changed: 0 additions & 18 deletions
This file was deleted.

‎enterprise/coderd/workspaceproxy.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,13 @@ import (
3434
"github.com/coder/coder/v2/enterprise/wsproxy/wsproxysdk"
3535
)
3636

37+
// whitelistedCryptoKeyFeatures is a list of crypto key features that are
38+
// allowed to be queried with workspace proxies.
39+
var whitelistedCryptoKeyFeatures = []database.CryptoKeyFeature{
40+
database.CryptoKeyFeatureWorkspaceAppsToken,
41+
database.CryptoKeyFeatureWorkspaceAppsAPIKey,
42+
}
43+
3744
// forceWorkspaceProxyHealthUpdate forces an update of the proxy health.
3845
// This is useful when a proxy is created or deleted. Errors will be logged.
3946
func (api *API) forceWorkspaceProxyHealthUpdate(ctx context.Context) {
@@ -736,7 +743,7 @@ func (api *API) workspaceProxyCryptoKeys(rw http.ResponseWriter, r *http.Request
736743
return
737744
}
738745

739-
if !slices.Contains(database.AllCryptoKeyFeatureValues(), feature) {
746+
if !slices.Contains(whitelistedCryptoKeyFeatures, feature) {
740747
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
741748
Message: fmt.Sprintf("Invalid feature: %q", feature),
742749
})

‎enterprise/coderd/workspaceproxy_test.go

Lines changed: 62 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -918,14 +918,14 @@ func TestGetCryptoKeys(t *testing.T) {
918918
StartsAt: now.Add(-time.Hour),
919919
Sequence: 2,
920920
})
921-
key1 := db2sdk.CryptoKey(expectedKey1)
921+
encryptionKey := db2sdk.CryptoKey(expectedKey1)
922922

923923
expectedKey2 := dbgen.CryptoKey(t, db, database.CryptoKey{
924-
Feature: database.CryptoKeyFeatureWorkspaceAppsAPIKey,
924+
Feature: database.CryptoKeyFeatureWorkspaceAppsToken,
925925
StartsAt: now,
926926
Sequence: 3,
927927
})
928-
key2 := db2sdk.CryptoKey(expectedKey2)
928+
signingKey := db2sdk.CryptoKey(expectedKey2)
929929

930930
// Create a deleted key.
931931
_ = dbgen.CryptoKey(t, db, database.CryptoKey{
@@ -935,19 +935,7 @@ func TestGetCryptoKeys(t *testing.T) {
935935
String: "secret1",
936936
Valid: false,
937937
},
938-
Sequence: 1,
939-
})
940-
941-
// Create a key with different features.
942-
_ = dbgen.CryptoKey(t, db, database.CryptoKey{
943-
Feature: database.CryptoKeyFeatureTailnetResume,
944-
StartsAt: now.Add(-time.Hour),
945-
Sequence: 1,
946-
})
947-
_ = dbgen.CryptoKey(t, db, database.CryptoKey{
948-
Feature: database.CryptoKeyFeatureOIDCConvert,
949-
StartsAt: now.Add(-time.Hour),
950-
Sequence: 1,
938+
Sequence: 4,
951939
})
952940

953941
proxy := coderdenttest.NewWorkspaceProxyReplica(t, api, cclient, &coderdenttest.ProxyOptions{
@@ -957,8 +945,53 @@ func TestGetCryptoKeys(t *testing.T) {
957945
keys, err := proxy.SDKClient.CryptoKeys(ctx, codersdk.CryptoKeyFeatureWorkspaceAppsAPIKey)
958946
require.NoError(t, err)
959947
require.NotEmpty(t, keys)
948+
// 1 key is generated on startup, the other is the one we generated for our test.
960949
require.Equal(t, 2, len(keys.CryptoKeys))
961-
requireContainsKeys(t, keys.CryptoKeys, key1, key2)
950+
requireContainsKeys(t, keys.CryptoKeys, encryptionKey)
951+
requireNotContainsKeys(t, keys.CryptoKeys, signingKey)
952+
953+
keys, err = proxy.SDKClient.CryptoKeys(ctx, codersdk.CryptoKeyFeatureWorkspaceAppsToken)
954+
require.NoError(t, err)
955+
require.NotEmpty(t, keys)
956+
requireContainsKeys(t, keys.CryptoKeys, signingKey)
957+
requireNotContainsKeys(t, keys.CryptoKeys, encryptionKey)
958+
})
959+
960+
t.Run("InvalidFeature", func(t *testing.T) {
961+
t.Parallel()
962+
963+
ctx := testutil.Context(t, testutil.WaitMedium)
964+
db, pubsub := dbtestutil.NewDB(t)
965+
cclient, _, api, _ := coderdenttest.NewWithAPI(t, &coderdenttest.Options{
966+
Options: &coderdtest.Options{
967+
Database: db,
968+
Pubsub: pubsub,
969+
IncludeProvisionerDaemon: true,
970+
},
971+
LicenseOptions: &coderdenttest.LicenseOptions{
972+
Features: license.Features{
973+
codersdk.FeatureWorkspaceProxy: 1,
974+
},
975+
},
976+
})
977+
978+
proxy := coderdenttest.NewWorkspaceProxyReplica(t, api, cclient, &coderdenttest.ProxyOptions{
979+
Name: testutil.GetRandomName(t),
980+
})
981+
982+
_, err := proxy.SDKClient.CryptoKeys(ctx, codersdk.CryptoKeyFeatureOIDCConvert)
983+
require.Error(t, err)
984+
var sdkErr *codersdk.Error
985+
require.ErrorAs(t, err, &sdkErr)
986+
require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode())
987+
_, err = proxy.SDKClient.CryptoKeys(ctx, codersdk.CryptoKeyFeatureTailnetResume)
988+
require.Error(t, err)
989+
require.ErrorAs(t, err, &sdkErr)
990+
require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode())
991+
_, err = proxy.SDKClient.CryptoKeys(ctx, "invalid")
992+
require.Error(t, err)
993+
require.ErrorAs(t, err, &sdkErr)
994+
require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode())
962995
})
963996

964997
t.Run("Unauthorized", func(t *testing.T) {
@@ -994,6 +1027,18 @@ func TestGetCryptoKeys(t *testing.T) {
9941027
})
9951028
}
9961029

1030+
func requireNotContainsKeys(t *testing.T, keys []codersdk.CryptoKey, unexpected ...codersdk.CryptoKey) {
1031+
t.Helper()
1032+
1033+
for _, expectedKey := range unexpected {
1034+
for _, key := range keys {
1035+
if key.Feature == expectedKey.Feature && key.Sequence == expectedKey.Sequence {
1036+
t.Fatalf("unexpected key %+v found", expectedKey)
1037+
}
1038+
}
1039+
}
1040+
}
1041+
9971042
func requireContainsKeys(t *testing.T, keys []codersdk.CryptoKey, expected ...codersdk.CryptoKey) {
9981043
t.Helper()
9991044

0 commit comments

Comments
 (0)