chore!: ensure consistent secret token generation and hashing (coder#20388)

This PR uses the same sha256 hashing technique as we use for APIKeys. So
now all randomly generated secrets will be hashed with sha256 for
consistency.

This is a breaking change for the oauth tokens. Since oauth is only
allowed for dev builds and experimental, this is ok.

Author: Emyrk

coderd/apidoc/docs.go

@@ -2,6 +2,7 @@ package apikey
 
 import (
 	"crypto/sha256"
+	"crypto/subtle"
 	"fmt"
 	"net"
 	"time"
@@ -44,12 +45,17 @@ type CreateParams struct {
 // database representation. It is the responsibility of the caller to insert it
 // into the database.
 func Generate(params CreateParams) (database.InsertAPIKeyParams, string, error) {
-	keyID, keySecret, err := generateKey()
+	// Length of an API Key ID.
+	keyID, err := cryptorand.String(10)
 	if err != nil {
-		return database.InsertAPIKeyParams{}, "", xerrors.Errorf("generate API key: %w", err)
+		return database.InsertAPIKeyParams{}, "", xerrors.Errorf("generate API key ID: %w", err)
 	}
 
-	hashed := sha256.Sum256([]byte(keySecret))
+	// Length of an API Key secret.
+	keySecret, hashedSecret, err := GenerateSecret(22)
+	if err != nil {
+		return database.InsertAPIKeyParams{}, "", xerrors.Errorf("generate API key secret: %w", err)
+	}
 
 	// Default expires at to now+lifetime, or use the configured value if not
 	// set.
@@ -120,25 +126,32 @@ func Generate(params CreateParams) (database.InsertAPIKeyParams, string, error)
 		ExpiresAt:    params.ExpiresAt.UTC(),
 		CreatedAt:    dbtime.Now(),
 		UpdatedAt:    dbtime.Now(),
-		HashedSecret: hashed[:],
+		HashedSecret: hashedSecret,
 		LoginType:    params.LoginType,
 		Scopes:       scopes,
 		AllowList:    params.AllowList,
 		TokenName:    params.TokenName,
 	}, token, nil
 }
 
-// generateKey a new ID and secret for an API key.
-func generateKey() (id string, secret string, err error) {
-	// Length of an API Key ID.
-	id, err = cryptorand.String(10)
-	if err != nil {
-		return "", "", err
-	}
-	// Length of an API Key secret.
-	secret, err = cryptorand.String(22)
+func GenerateSecret(length int) (secret string, hashed []byte, err error) {
+	secret, err = cryptorand.String(length)
 	if err != nil {
-		return "", "", err
+		return "", nil, err
 	}
-	return id, secret, nil
+	hash := HashSecret(secret)
+	return secret, hash, nil
+}
+
+// ValidateHash compares a secret against an expected hashed secret.
+func ValidateHash(hashedSecret []byte, secret string) bool {
+	hash := HashSecret(secret)
+	return subtle.ConstantTimeCompare(hashedSecret, hash) == 1
+}
+
+// HashSecret is the single function used to hash API key secrets.
+// Use this to ensure a consistent hashing algorithm.
+func HashSecret(secret string) []byte {
+	hash := sha256.Sum256([]byte(secret))
+	return hash[:]
 }

coderd/apidoc/swagger.json

@@ -1,7 +1,6 @@
 package apikey_test
 
 import (
-	"crypto/sha256"
 	"strings"
 	"testing"
 	"time"
@@ -126,8 +125,8 @@ func TestGenerate(t *testing.T) {
 			require.Equal(t, key.ID, keytokens[0])
 
 			// Assert that the hashed secret is correct.
-			hashed := sha256.Sum256([]byte(keytokens[1]))
-			assert.ElementsMatch(t, hashed, key.HashedSecret)
+			equal := apikey.ValidateHash(key.HashedSecret, keytokens[1])
+			require.True(t, equal, "valid secret")
 
 			assert.Equal(t, tc.params.UserID, key.UserID)
 			assert.WithinDuration(t, dbtime.Now(), key.CreatedAt, time.Second*5)
@@ -173,3 +172,17 @@ func TestGenerate(t *testing.T) {
 		})
 	}
 }
+
+// TestInvalid just ensures the false case is asserted by some tests.
+// Otherwise, a function that just `returns true` might pass all tests incorrectly.
+func TestInvalid(t *testing.T) {
+	t.Parallel()
+
+	require.Falsef(t, apikey.ValidateHash([]byte{}, "secret"), "empty hash")
+
+	secret, hash, err := apikey.GenerateSecret(10)
+	require.NoError(t, err)
+
+	require.Falsef(t, apikey.ValidateHash(hash, secret+"_"), "different secret")
+	require.Falsef(t, apikey.ValidateHash(hash[:len(hash)-1], secret), "different hash length")
+}

coderd/apikey/apikey.go

@@ -2475,7 +2475,7 @@ func (q *querier) GetOAuth2ProviderAppByID(ctx context.Context, id uuid.UUID) (d
 	return q.db.GetOAuth2ProviderAppByID(ctx, id)
 }
 
-func (q *querier) GetOAuth2ProviderAppByRegistrationToken(ctx context.Context, registrationAccessToken sql.NullString) (database.OAuth2ProviderApp, error) {
+func (q *querier) GetOAuth2ProviderAppByRegistrationToken(ctx context.Context, registrationAccessToken []byte) (database.OAuth2ProviderApp, error) {
 	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceOauth2App); err != nil {
 		return database.OAuth2ProviderApp{}, err
 	}

coderd/apikey/apikey_test.go

@@ -3925,9 +3925,9 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() {
 	}))
 	s.Run("GetOAuth2ProviderAppByRegistrationToken", s.Subtest(func(db database.Store, check *expects) {
 		app := dbgen.OAuth2ProviderApp(s.T(), db, database.OAuth2ProviderApp{
-			RegistrationAccessToken: sql.NullString{String: "test-token", Valid: true},
+			RegistrationAccessToken: []byte("test-token"),
 		})
-		check.Args(sql.NullString{String: "test-token", Valid: true}).Asserts(rbac.ResourceOauth2App, policy.ActionRead).Returns(app)
+		check.Args([]byte("test-token")).Asserts(rbac.ResourceOauth2App, policy.ActionRead).Returns(app)
 	}))
 }
 

coderd/database/dbauthz/dbauthz.go

@@ -3,7 +3,6 @@ package dbgen
 import (
 	"context"
 	"crypto/rand"
-	"crypto/sha256"
 	"database/sql"
 	"encoding/hex"
 	"encoding/json"
@@ -20,6 +19,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"golang.org/x/xerrors"
 
+	"github.com/coder/coder/v2/coderd/apikey"
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/db2sdk"
 	"github.com/coder/coder/v2/coderd/database/dbauthz"
@@ -161,8 +161,8 @@ func Template(t testing.TB, db database.Store, seed database.Template) database.
 
 func APIKey(t testing.TB, db database.Store, seed database.APIKey, munge ...func(*database.InsertAPIKeyParams)) (key database.APIKey, token string) {
 	id, _ := cryptorand.String(10)
-	secret, _ := cryptorand.String(22)
-	hashed := sha256.Sum256([]byte(secret))
+	secret, hashed, err := apikey.GenerateSecret(22)
+	require.NoError(t, err)
 
 	ip := seed.IPAddress
 	if !ip.Valid {
@@ -179,7 +179,7 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey, munge ...func
 		ID: takeFirst(seed.ID, id),
 		// 0 defaults to 86400 at the db layer
 		LifetimeSeconds: takeFirst(seed.LifetimeSeconds, 0),
-		HashedSecret:    takeFirstSlice(seed.HashedSecret, hashed[:]),
+		HashedSecret:    takeFirstSlice(seed.HashedSecret, hashed),
 		IPAddress:       ip,
 		UserID:          takeFirst(seed.UserID, uuid.New()),
 		LastUsed:        takeFirst(seed.LastUsed, dbtime.Now()),
@@ -194,7 +194,7 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey, munge ...func
 	for _, fn := range munge {
 		fn(&params)
 	}
-	key, err := db.InsertAPIKey(genCtx, params)
+	key, err = db.InsertAPIKey(genCtx, params)
 	require.NoError(t, err, "insert api key")
 	return key, fmt.Sprintf("%s-%s", key.ID, secret)
 }
@@ -980,16 +980,15 @@ func WorkspaceResourceMetadatums(t testing.TB, db database.Store, seed database.
 }
 
 func WorkspaceProxy(t testing.TB, db database.Store, orig database.WorkspaceProxy) (database.WorkspaceProxy, string) {
-	secret, err := cryptorand.HexString(64)
+	secret, hashedSecret, err := apikey.GenerateSecret(64)
 	require.NoError(t, err, "generate secret")
-	hashedSecret := sha256.Sum256([]byte(secret))
 
 	proxy, err := db.InsertWorkspaceProxy(genCtx, database.InsertWorkspaceProxyParams{
 		ID:                takeFirst(orig.ID, uuid.New()),
 		Name:              takeFirst(orig.Name, testutil.GetRandomName(t)),
 		DisplayName:       takeFirst(orig.DisplayName, testutil.GetRandomName(t)),
 		Icon:              takeFirst(orig.Icon, testutil.GetRandomName(t)),
-		TokenHashedSecret: hashedSecret[:],
+		TokenHashedSecret: hashedSecret,
 		CreatedAt:         takeFirst(orig.CreatedAt, dbtime.Now()),
 		UpdatedAt:         takeFirst(orig.UpdatedAt, dbtime.Now()),
 		DerpEnabled:       takeFirst(orig.DerpEnabled, false),
@@ -1259,7 +1258,7 @@ func OAuth2ProviderApp(t testing.TB, db database.Store, seed database.OAuth2Prov
 		Jwks:                    seed.Jwks, // pqtype.NullRawMessage{} is not comparable, use existing value
 		SoftwareID:              takeFirst(seed.SoftwareID, sql.NullString{}),
 		SoftwareVersion:         takeFirst(seed.SoftwareVersion, sql.NullString{}),
-		RegistrationAccessToken: takeFirst(seed.RegistrationAccessToken, sql.NullString{}),
+		RegistrationAccessToken: seed.RegistrationAccessToken,
 		RegistrationClientUri:   takeFirst(seed.RegistrationClientUri, sql.NullString{}),
 	})
 	require.NoError(t, err, "insert oauth2 app")