From d49ae71a805e9488bed75ce33044a02b248012d4 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sun, 2 Nov 2025 11:25:24 +0100 Subject: [PATCH] db: use partial unique index for pre_auth_keys.prefix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes the UNIQUE constraint on the pre_auth_keys.prefix column to allow multiple legacy keys (with NULL or empty prefix) while still enforcing uniqueness for new bcrypt-based keys. Changes: - Modified migration to create partial unique index with WHERE clause - Updated schema.sql to match migration - Added comprehensive test (TestMultipleLegacyKeysAllowed) to verify: * Multiple legacy keys with empty prefix can coexist * New bcrypt keys have unique prefixes * Duplicate non-empty prefixes are rejected - Fixed lint issues (errcheck, intrange) - Fixed hardening: position-based parsing instead of separator-based - Added validation for empty string, length, separator, and character set The partial index uses: WHERE prefix IS NOT NULL AND prefix != '' This allows unlimited legacy keys (backward compatibility) while preventing duplicate prefixes for new keys (security). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- hscontrol/db/db.go | 12 ++- hscontrol/db/preauth_keys.go | 13 +-- hscontrol/db/preauth_keys_test.go | 127 ++++++++++++++++++++++++------ hscontrol/db/schema.sql | 2 +- hscontrol/db/users_test.go | 14 ++-- 5 files changed, 125 insertions(+), 43 deletions(-) diff --git a/hscontrol/db/db.go b/hscontrol/db/db.go index 7a0ef422..5633221f 100644 --- a/hscontrol/db/db.go +++ b/hscontrol/db/db.go @@ -961,10 +961,16 @@ AND auth_key_id NOT IN ( // them with the same security model as api keys. ID: "202511011637-preauthkey-bcrypt", Migrate: func(tx *gorm.DB) error { - tx.Migrator().AddColumn(&types.PreAuthKey{}, "prefix") - tx.Migrator().AddColumn(&types.PreAuthKey{}, "hash") + if err := tx.Migrator().AddColumn(&types.PreAuthKey{}, "prefix"); err != nil { + return fmt.Errorf("adding prefix column: %w", err) + } + if err := tx.Migrator().AddColumn(&types.PreAuthKey{}, "hash"); err != nil { + return fmt.Errorf("adding hash column: %w", err) + } - if err := tx.Exec("CREATE UNIQUE INDEX IF NOT EXISTS idx_pre_auth_keys_prefix ON pre_auth_keys(prefix)").Error; err != nil { + // Create partial unique index to allow multiple legacy keys (NULL/empty prefix) + // while enforcing uniqueness for new bcrypt-based keys + if err := tx.Exec("CREATE UNIQUE INDEX IF NOT EXISTS idx_pre_auth_keys_prefix ON pre_auth_keys(prefix) WHERE prefix IS NOT NULL AND prefix != ''").Error; err != nil { return fmt.Errorf("creating prefix index: %w", err) } diff --git a/hscontrol/db/preauth_keys.go b/hscontrol/db/preauth_keys.go index 4faca955..c80a764c 100644 --- a/hscontrol/db/preauth_keys.go +++ b/hscontrol/db/preauth_keys.go @@ -79,10 +79,10 @@ func CreatePreAuthKey( // Validate generated prefix (should always be valid, but be defensive) if len(prefix) != authKeyPrefixLength { - return nil, fmt.Errorf("generated prefix has invalid length: expected %d, got %d", authKeyPrefixLength, len(prefix)) + return nil, fmt.Errorf("%w: generated prefix has invalid length: expected %d, got %d", ErrPreAuthKeyFailedToParse, authKeyPrefixLength, len(prefix)) } if !isValidBase64URLSafe(prefix) { - return nil, fmt.Errorf("generated prefix contains invalid characters") + return nil, fmt.Errorf("%w: generated prefix contains invalid characters", ErrPreAuthKeyFailedToParse) } toBeHashed, err := util.GenerateRandomStringURLSafe(authKeyLength) @@ -92,10 +92,10 @@ func CreatePreAuthKey( // Validate generated hash (should always be valid, but be defensive) if len(toBeHashed) != authKeyLength { - return nil, fmt.Errorf("generated hash has invalid length: expected %d, got %d", authKeyLength, len(toBeHashed)) + return nil, fmt.Errorf("%w: generated hash has invalid length: expected %d, got %d", ErrPreAuthKeyFailedToParse, authKeyLength, len(toBeHashed)) } if !isValidBase64URLSafe(toBeHashed) { - return nil, fmt.Errorf("generated hash contains invalid characters") + return nil, fmt.Errorf("%w: generated hash contains invalid characters", ErrPreAuthKeyFailedToParse) } keyStr := authKeyPrefix + prefix + "-" + toBeHashed @@ -234,14 +234,15 @@ func findAuthKey(tx *gorm.DB, keyStr string) (*types.PreAuthKey, error) { return &pak, nil } -// isValidBase64URLSafe checks if a string contains only base64 URL-safe characters +// isValidBase64URLSafe checks if a string contains only base64 URL-safe characters. func isValidBase64URLSafe(s string) bool { for _, c := range s { if !((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || - (c >= '0' && c <= '9') || c == '-' || c == '_') { + (c >= '0' && c <= '9') || c == '-' || c == '_') { return false } } + return true } diff --git a/hscontrol/db/preauth_keys_test.go b/hscontrol/db/preauth_keys_test.go index 2b475f5f..4d029496 100644 --- a/hscontrol/db/preauth_keys_test.go +++ b/hscontrol/db/preauth_keys_test.go @@ -1,6 +1,7 @@ package db import ( + "fmt" "slices" "strings" "testing" @@ -15,8 +16,8 @@ import ( func TestCreatePreAuthKey(t *testing.T) { tests := []struct { - name string - test func(*testing.T, *HSDatabase) + name string + test func(*testing.T, *HSDatabase) }{ { name: "error_invalid_user_id", @@ -65,8 +66,8 @@ func TestCreatePreAuthKey(t *testing.T) { func TestPreAuthKeyACLTags(t *testing.T) { tests := []struct { - name string - test func(*testing.T, *HSDatabase) + name string + test func(*testing.T, *HSDatabase) }{ { name: "reject_malformed_tags", @@ -115,10 +116,10 @@ func TestCannotDeleteAssignedPreAuthKey(t *testing.T) { db, err := newSQLiteTestDB() require.NoError(t, err) user, err := db.CreateUser(types.User{Name: "test8"}) - assert.NoError(t, err) + require.NoError(t, err) key, err := db.CreatePreAuthKey(types.UserID(user.ID), false, false, nil, []string{"tag:good"}) - assert.NoError(t, err) + require.NoError(t, err) node := types.Node{ ID: 0, @@ -139,11 +140,11 @@ func TestPreAuthKeyAuthentication(t *testing.T) { user := db.CreateUserForTest("test-user") tests := []struct { - name string - setupKey func() string // Returns key string to test - wantFindErr bool // Error when finding the key - wantValidateErr bool // Error when validating the key - validateResult func(*testing.T, *types.PreAuthKey) + name string + setupKey func() string // Returns key string to test + wantFindErr bool // Error when finding the key + wantValidateErr bool // Error when validating the key + validateResult func(*testing.T, *types.PreAuthKey) }{ { name: "legacy_key_plaintext", @@ -160,9 +161,10 @@ func TestPreAuthKeyAuthentication(t *testing.T) { VALUES (?, ?, ?, ?, ?, ?) `, legacyKey, user.ID, true, false, false, now).Error require.NoError(t, err) + return legacyKey }, - wantFindErr: false, + wantFindErr: false, wantValidateErr: false, validateResult: func(t *testing.T, pak *types.PreAuthKey) { assert.Equal(t, user.ID, pak.UserID) @@ -180,9 +182,10 @@ func TestPreAuthKeyAuthentication(t *testing.T) { true, false, nil, []string{"tag:test"}, ) require.NoError(t, err) + return keyStr.Key }, - wantFindErr: false, + wantFindErr: false, wantValidateErr: false, validateResult: func(t *testing.T, pak *types.PreAuthKey) { assert.Equal(t, user.ID, pak.UserID) @@ -208,17 +211,17 @@ func TestPreAuthKeyAuthentication(t *testing.T) { // Extract prefix and hash using fixed-length parsing like the real code does _, prefixAndHash, found := strings.Cut(keyStr.Key, "hskey-auth-") assert.True(t, found) - assert.True(t, len(prefixAndHash) >= 12+1+64) // prefix + '-' + hash minimum + assert.GreaterOrEqual(t, len(prefixAndHash), 12+1+64) // prefix + '-' + hash minimum prefix := prefixAndHash[:12] - assert.Len(t, prefix, 12) // Prefix is 12 chars + assert.Len(t, prefix, 12) // Prefix is 12 chars assert.Equal(t, byte('-'), prefixAndHash[12]) // Separator hash := prefixAndHash[13:] assert.Len(t, hash, 64) // Hash is 64 chars return keyStr.Key }, - wantFindErr: false, + wantFindErr: false, wantValidateErr: false, }, { @@ -235,9 +238,10 @@ func TestPreAuthKeyAuthentication(t *testing.T) { // Return key with tampered hash using fixed-length parsing _, prefixAndHash, _ := strings.Cut(keyStr, "hskey-auth-") prefix := prefixAndHash[:12] + return "hskey-auth-" + prefix + "-" + "wrong_hash_here_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" }, - wantFindErr: true, + wantFindErr: true, wantValidateErr: false, }, { @@ -245,7 +249,7 @@ func TestPreAuthKeyAuthentication(t *testing.T) { setupKey: func() string { return "" }, - wantFindErr: true, + wantFindErr: true, wantValidateErr: false, }, { @@ -253,7 +257,7 @@ func TestPreAuthKeyAuthentication(t *testing.T) { setupKey: func() string { return "hskey-auth-short" }, - wantFindErr: true, + wantFindErr: true, wantValidateErr: false, }, { @@ -261,7 +265,7 @@ func TestPreAuthKeyAuthentication(t *testing.T) { setupKey: func() string { return "hskey-auth-ABCDEFGHIJKLabcdefghijklmnopqrstuvwxyz1234567890ABCDEFGHIJKLMNOPQRSTUVWXYZ" }, - wantFindErr: true, + wantFindErr: true, wantValidateErr: false, }, { @@ -269,7 +273,7 @@ func TestPreAuthKeyAuthentication(t *testing.T) { setupKey: func() string { return "hskey-auth-ABCDEFGHIJKL-short" }, - wantFindErr: true, + wantFindErr: true, wantValidateErr: false, }, { @@ -277,7 +281,7 @@ func TestPreAuthKeyAuthentication(t *testing.T) { setupKey: func() string { return "hskey-auth-ABC$EF@HIJKL-" + strings.Repeat("a", 64) }, - wantFindErr: true, + wantFindErr: true, wantValidateErr: false, }, { @@ -285,7 +289,7 @@ func TestPreAuthKeyAuthentication(t *testing.T) { setupKey: func() string { return "hskey-auth-ABCDEFGHIJKL-" + "invalid$chars" + strings.Repeat("a", 54) }, - wantFindErr: true, + wantFindErr: true, wantValidateErr: false, }, { @@ -294,7 +298,7 @@ func TestPreAuthKeyAuthentication(t *testing.T) { // Create a validly formatted key but with a prefix that doesn't exist return "hskey-auth-NotInDB12345-" + strings.Repeat("a", 64) }, - wantFindErr: true, + wantFindErr: true, wantValidateErr: false, }, { @@ -310,9 +314,10 @@ func TestPreAuthKeyAuthentication(t *testing.T) { VALUES (?, ?, ?, ?, ?, ?, ?) `, legacyKey, user.ID, true, false, false, now, expiration).Error require.NoError(t, err) + return legacyKey }, - wantFindErr: false, + wantFindErr: false, wantValidateErr: true, }, { @@ -327,9 +332,10 @@ func TestPreAuthKeyAuthentication(t *testing.T) { VALUES (?, ?, ?, ?, ?, ?) `, legacyKey, user.ID, false, false, true, now).Error require.NoError(t, err) + return legacyKey }, - wantFindErr: false, + wantFindErr: false, wantValidateErr: true, }, } @@ -361,3 +367,72 @@ func TestPreAuthKeyAuthentication(t *testing.T) { }) } } + +func TestMultipleLegacyKeysAllowed(t *testing.T) { + db, err := newSQLiteTestDB() + require.NoError(t, err) + + user, err := db.CreateUser(types.User{Name: "test-legacy"}) + require.NoError(t, err) + + // Create multiple legacy keys by directly inserting with empty prefix + // This simulates the migration scenario where existing databases have multiple + // plaintext keys without prefix/hash fields + now := time.Now() + + for i := range 5 { + legacyKey := fmt.Sprintf("legacy_key_%d_%s", i, strings.Repeat("x", 40)) + + err := db.DB.Exec(` + INSERT INTO pre_auth_keys (key, prefix, hash, user_id, reusable, ephemeral, used, created_at) + VALUES (?, '', NULL, ?, ?, ?, ?, ?) + `, legacyKey, user.ID, true, false, false, now).Error + require.NoError(t, err, "should allow multiple legacy keys with empty prefix") + } + + // Verify all legacy keys can be retrieved + var legacyKeys []types.PreAuthKey + err = db.DB.Where("prefix = '' OR prefix IS NULL").Find(&legacyKeys).Error + require.NoError(t, err) + assert.Len(t, legacyKeys, 5, "should have created 5 legacy keys") + + // Now create new bcrypt-based keys - these should have unique prefixes + key1, err := db.CreatePreAuthKey(types.UserID(user.ID), true, false, nil, nil) + require.NoError(t, err) + assert.NotEmpty(t, key1.Key) + + key2, err := db.CreatePreAuthKey(types.UserID(user.ID), true, false, nil, nil) + require.NoError(t, err) + assert.NotEmpty(t, key2.Key) + + // Verify the new keys have different prefixes + pak1, err := db.GetPreAuthKey(key1.Key) + require.NoError(t, err) + assert.NotEmpty(t, pak1.Prefix) + + pak2, err := db.GetPreAuthKey(key2.Key) + require.NoError(t, err) + assert.NotEmpty(t, pak2.Prefix) + + assert.NotEqual(t, pak1.Prefix, pak2.Prefix, "new keys should have unique prefixes") + + // Verify we cannot manually insert duplicate non-empty prefixes + duplicatePrefix := "test_prefix1" + hash1 := []byte("hash1") + hash2 := []byte("hash2") + + // First insert should succeed + err = db.DB.Exec(` + INSERT INTO pre_auth_keys (key, prefix, hash, user_id, reusable, ephemeral, used, created_at) + VALUES ('', ?, ?, ?, ?, ?, ?, ?) + `, duplicatePrefix, hash1, user.ID, true, false, false, now).Error + require.NoError(t, err, "first key with prefix should succeed") + + // Second insert with same prefix should fail + err = db.DB.Exec(` + INSERT INTO pre_auth_keys (key, prefix, hash, user_id, reusable, ephemeral, used, created_at) + VALUES ('', ?, ?, ?, ?, ?, ?, ?) + `, duplicatePrefix, hash2, user.ID, true, false, false, now).Error + assert.Error(t, err, "duplicate non-empty prefix should be rejected") + assert.Contains(t, err.Error(), "UNIQUE constraint failed", "should fail with UNIQUE constraint error") +} diff --git a/hscontrol/db/schema.sql b/hscontrol/db/schema.sql index 792559ab..075c9d4d 100644 --- a/hscontrol/db/schema.sql +++ b/hscontrol/db/schema.sql @@ -61,7 +61,7 @@ CREATE TABLE pre_auth_keys( CONSTRAINT fk_pre_auth_keys_user FOREIGN KEY(user_id) REFERENCES users(id) ON DELETE SET NULL ); -CREATE UNIQUE INDEX idx_pre_auth_keys_prefix ON pre_auth_keys(prefix); +CREATE UNIQUE INDEX idx_pre_auth_keys_prefix ON pre_auth_keys(prefix) WHERE prefix IS NOT NULL AND prefix != ''; CREATE TABLE api_keys( id integer PRIMARY KEY AUTOINCREMENT, diff --git a/hscontrol/db/users_test.go b/hscontrol/db/users_test.go index ed108e03..91cd8daf 100644 --- a/hscontrol/db/users_test.go +++ b/hscontrol/db/users_test.go @@ -31,8 +31,8 @@ func TestCreateAndDestroyUser(t *testing.T) { func TestDestroyUserErrors(t *testing.T) { tests := []struct { - name string - test func(*testing.T, *HSDatabase) + name string + test func(*testing.T, *HSDatabase) }{ { name: "error_user_not_found", @@ -95,8 +95,8 @@ func TestDestroyUserErrors(t *testing.T) { func TestRenameUser(t *testing.T) { tests := []struct { - name string - test func(*testing.T, *HSDatabase) + name string + test func(*testing.T, *HSDatabase) }{ { name: "success_rename", @@ -113,7 +113,7 @@ func TestRenameUser(t *testing.T) { users, err = db.ListUsers(&types.User{Name: "test"}) require.NoError(t, err) - assert.Len(t, users, 0) + assert.Empty(t, users) users, err = db.ListUsers(&types.User{Name: "test-renamed"}) require.NoError(t, err) @@ -155,8 +155,8 @@ func TestRenameUser(t *testing.T) { func TestAssignNodeToUser(t *testing.T) { tests := []struct { - name string - test func(*testing.T, *HSDatabase) + name string + test func(*testing.T, *HSDatabase) }{ { name: "success_reassign_node",