Compare commits

..

5 Commits

Author SHA1 Message Date
Kristoffer Dalby
1e2419bc03 update changelog
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
2025-02-07 10:42:18 +01:00
Kristoffer Dalby
3b55dacda1 hscontrol/db: add migration setting non existing pak on nodes to null (#2412)
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
2025-02-07 10:25:35 +01:00
Kristoffer Dalby
ffe2f299cb do not allow preauth keys to be deleted if assigned to node (#2396)
* do not allow preauth keys to be deleted if assigned to node

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* update changelog

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

---------

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
2025-02-07 10:25:20 +01:00
nblock
341a3d3005 Remove routes without a node_id (#2386)
The routes table has a NOT NULL constraint on node_id.

Fixes: #2376
2025-01-30 14:59:30 +01:00
Kristoffer Dalby
46b82269e0 simplify findUserByToken in ACL, add missing testcases (#2388)
* update users doc on unique constraints

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* simplify finduser func

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* add initial tests for findUserFromToken

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* add changelog

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

---------

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
2025-01-30 14:58:54 +01:00
10 changed files with 454 additions and 29 deletions

View File

@@ -1,5 +1,24 @@
# CHANGELOG
## 0.24.3 (2025-02-07)
### Changes
- Fix migration error caused by nodes having invalid auth keys
[#2412](https://github.com/juanfont/headscale/pull/2412)
- Pre auth keys belonging to a user are no longer deleted with the user
[#2396](https://github.com/juanfont/headscale/pull/2396)
- Pre auth keys that are used by a node can no longer be deleted
[#2396](https://github.com/juanfont/headscale/pull/2396)
## 0.24.2 (2025-01-30)
### Changes
- Fix issue where email and username being equal fails to match in Policy
[#2388](https://github.com/juanfont/headscale/pull/2388)
- Delete invalid routes before adding a NOT NULL constraint on node_id
[#2386](https://github.com/juanfont/headscale/pull/2386)
## 0.24.1 (2025-01-23)
### Changes

View File

@@ -512,7 +512,7 @@ COMMIT;
err := tx.AutoMigrate(&types.User{})
if err != nil {
return err
return fmt.Errorf("automigrating types.User: %w", err)
}
return nil
@@ -527,7 +527,7 @@ COMMIT;
Migrate: func(tx *gorm.DB) error {
err := tx.AutoMigrate(&types.User{})
if err != nil {
return err
return fmt.Errorf("automigrating types.User: %w", err)
}
// Set up indexes and unique constraints outside of GORM, it does not support
@@ -565,9 +565,57 @@ COMMIT;
}
}
// Remove any invalid routes without a node_id.
if tx.Migrator().HasTable(&types.Route{}) {
err := tx.Exec("delete from routes where node_id is null").Error
if err != nil {
return err
}
}
err := tx.AutoMigrate(&types.Route{})
if err != nil {
return err
return fmt.Errorf("automigrating types.Route: %w", err)
}
return nil
},
Rollback: func(db *gorm.DB) error { return nil },
},
// Add back constraint so you cannot delete preauth keys that
// is still used by a node.
{
ID: "202501311657",
Migrate: func(tx *gorm.DB) error {
err := tx.AutoMigrate(&types.PreAuthKey{})
if err != nil {
return fmt.Errorf("automigrating types.PreAuthKey: %w", err)
}
err = tx.AutoMigrate(&types.Node{})
if err != nil {
return fmt.Errorf("automigrating types.Node: %w", err)
}
return nil
},
Rollback: func(db *gorm.DB) error { return nil },
},
// Ensure there are no nodes refering to a deleted preauthkey.
{
ID: "202502070949",
Migrate: func(tx *gorm.DB) error {
if tx.Migrator().HasTable(&types.PreAuthKey{}) {
err := tx.Exec(`
UPDATE nodes
SET auth_key_id = NULL
WHERE auth_key_id IS NOT NULL
AND auth_key_id NOT IN (
SELECT id FROM pre_auth_keys
);
`).Error
if err != nil {
return fmt.Errorf("setting auth_key to null on nodes with non-existing keys: %w", err)
}
}
return nil

View File

@@ -201,6 +201,26 @@ func TestMigrationsSQLite(t *testing.T) {
}
},
},
{
dbPath: "testdata/failing-node-preauth-constraint.sqlite",
wantFunc: func(t *testing.T, h *HSDatabase) {
nodes, err := Read(h.DB, func(rx *gorm.DB) (types.Nodes, error) {
return ListNodes(rx)
})
require.NoError(t, err)
for _, node := range nodes {
assert.Falsef(t, node.MachineKey.IsZero(), "expected non zero machinekey")
assert.Contains(t, node.MachineKey.String(), "mkey:")
assert.Falsef(t, node.NodeKey.IsZero(), "expected non zero nodekey")
assert.Contains(t, node.NodeKey.String(), "nodekey:")
assert.Falsef(t, node.DiscoKey.IsZero(), "expected non zero discokey")
assert.Contains(t, node.DiscoKey.String(), "discokey:")
assert.Nil(t, node.AuthKey)
assert.Nil(t, node.AuthKeyID)
}
},
},
}
for _, tt := range tests {

View File

@@ -2,10 +2,13 @@ package db
import (
"sort"
"testing"
"time"
"github.com/juanfont/headscale/hscontrol/types"
"github.com/juanfont/headscale/hscontrol/util"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"gopkg.in/check.v1"
"tailscale.com/types/ptr"
)
@@ -175,3 +178,25 @@ func (*Suite) TestPreAuthKeyACLTags(c *check.C) {
sort.Sort(sort.StringSlice(gotTags))
c.Assert(gotTags, check.DeepEquals, tags)
}
func TestCannotDeleteAssignedPreAuthKey(t *testing.T) {
db, err := newSQLiteTestDB()
require.NoError(t, err)
user, err := db.CreateUser(types.User{Name: "test8"})
assert.NoError(t, err)
key, err := db.CreatePreAuthKey(types.UserID(user.ID), false, false, nil, []string{"tag:good"})
assert.NoError(t, err)
node := types.Node{
ID: 0,
Hostname: "testest",
UserID: user.ID,
RegisterMethod: util.RegisterMethodAuthKey,
AuthKeyID: ptr.To(key.ID),
}
db.DB.Save(&node)
err = db.DB.Delete(key).Error
require.ErrorContains(t, err, "constraint failed: FOREIGN KEY constraint failed")
}

Binary file not shown.

View File

@@ -381,7 +381,7 @@ func (pol *ACLPolicy) CompileSSHPolicy(
}
for _, userStr := range usersFromGroup {
user, err := findUserFromTokenOrErr(users, userStr)
user, err := findUserFromToken(users, userStr)
if err != nil {
log.Trace().Err(err).Msg("user not found")
continue
@@ -400,7 +400,7 @@ func (pol *ACLPolicy) CompileSSHPolicy(
// corresponds with the User info in the netmap.
// TODO(kradalby): This is a bit of a hack, and it should go
// away with the new policy where users can be reliably determined.
if user, err := findUserFromTokenOrErr(users, srcToken); err == nil {
if user, err := findUserFromToken(users, srcToken); err == nil {
principals = append(principals, &tailcfg.SSHPrincipal{
UserLogin: user.Username(),
})
@@ -1001,7 +1001,7 @@ func (pol *ACLPolicy) TagsOfNode(
}
var found bool
for _, owner := range owners {
user, err := findUserFromTokenOrErr(users, owner)
user, err := findUserFromToken(users, owner)
if err != nil {
log.Trace().Caller().Err(err).Msg("could not determine user to filter tags by")
}
@@ -1038,7 +1038,7 @@ func (pol *ACLPolicy) TagsOfNode(
func filterNodesByUser(nodes types.Nodes, users []types.User, userToken string) types.Nodes {
var out types.Nodes
user, err := findUserFromTokenOrErr(users, userToken)
user, err := findUserFromToken(users, userToken)
if err != nil {
log.Trace().Caller().Err(err).Msg("could not determine user to filter nodes by")
return out
@@ -1058,24 +1058,19 @@ var (
ErrorMultipleUserMatching = errors.New("multiple users matching")
)
func findUserFromTokenOrErr(
users []types.User,
token string,
) (types.User, error) {
// findUserFromToken finds and returns a user based on the given token, prioritizing matches by ProviderIdentifier, followed by email or name.
// If no matching user is found, it returns an error of type ErrorNoUserMatching.
// If multiple users match the token, it returns an error indicating multiple matches.
func findUserFromToken(users []types.User, token string) (types.User, error) {
var potentialUsers []types.User
for _, user := range users {
if user.ProviderIdentifier.Valid && user.ProviderIdentifier.String == token {
// If a user is matching with a known unique field,
// disgard all other users and only keep the current
// user.
potentialUsers = []types.User{user}
// Prioritize ProviderIdentifier match and exit early
return user, nil
}
break
}
if user.Email == token {
potentialUsers = append(potentialUsers, user)
}
if user.Name == token {
if user.Email == token || user.Name == token {
potentialUsers = append(potentialUsers, user)
}
}

View File

@@ -4046,3 +4046,315 @@ func TestValidTagInvalidUser(t *testing.T) {
t.Errorf("TestValidTagInvalidUser() unexpected result (-want +got):\n%s", diff)
}
}
func TestFindUserByToken(t *testing.T) {
tests := []struct {
name string
users []types.User
token string
want types.User
wantErr bool
}{
{
name: "exact match by ProviderIdentifier",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "token1"}},
{Email: "user2@example.com"},
},
token: "token1",
want: types.User{ProviderIdentifier: sql.NullString{Valid: true, String: "token1"}},
wantErr: false,
},
{
name: "no matches found",
users: []types.User{
{Email: "user1@example.com"},
{Name: "username"},
},
token: "nonexistent-token",
want: types.User{},
wantErr: true,
},
{
name: "multiple matches by email and name",
users: []types.User{
{Email: "token2", Name: "notoken"},
{Name: "token2", Email: "notoken@example.com"},
},
token: "token2",
want: types.User{},
wantErr: true,
},
{
name: "match by email",
users: []types.User{
{Email: "token3@example.com"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "othertoken"}},
},
token: "token3@example.com",
want: types.User{Email: "token3@example.com"},
wantErr: false,
},
{
name: "match by name",
users: []types.User{
{Name: "token4"},
{Email: "user5@example.com"},
},
token: "token4",
want: types.User{Name: "token4"},
wantErr: false,
},
{
name: "provider identifier takes precedence over email and name matches",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "token5"}},
{Email: "token5@example.com", Name: "token5"},
},
token: "token5",
want: types.User{ProviderIdentifier: sql.NullString{Valid: true, String: "token5"}},
wantErr: false,
},
{
name: "empty token finds no users",
users: []types.User{
{Email: "user6@example.com"},
{Name: "username6"},
},
token: "",
want: types.User{},
wantErr: true,
},
// Test case 1: Duplicate Emails with Unique ProviderIdentifiers
{
name: "duplicate emails with unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid1"}, Email: "user@example.com"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid2"}, Email: "user@example.com"},
},
token: "user@example.com",
want: types.User{},
wantErr: true,
},
// Test case 2: Duplicate Names with Unique ProviderIdentifiers
{
name: "duplicate names with unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid3"}, Name: "John Doe"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid4"}, Name: "John Doe"},
},
token: "John Doe",
want: types.User{},
wantErr: true,
},
// Test case 3: Duplicate Emails and Names with Unique ProviderIdentifiers
{
name: "duplicate emails and names with unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid5"}, Email: "user@example.com", Name: "John Doe"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid6"}, Email: "user@example.com", Name: "John Doe"},
},
token: "user@example.com",
want: types.User{},
wantErr: true,
},
// Test case 4: Unique Names without ProviderIdentifiers
{
name: "unique names without provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "johndoe@example.com"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "janesmith@example.com"},
},
token: "John Doe",
want: types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "johndoe@example.com"},
wantErr: false,
},
// Test case 5: Duplicate Emails without ProviderIdentifiers but Unique Names
{
name: "duplicate emails without provider identifiers but unique names",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "user@example.com"},
},
token: "John Doe",
want: types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
wantErr: false,
},
// Test case 6: Duplicate Names and Emails without ProviderIdentifiers
{
name: "duplicate names and emails without provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
},
token: "John Doe",
want: types.User{},
wantErr: true,
},
// Test case 7: Multiple Users with the Same Email but Different Names and Unique ProviderIdentifiers
{
name: "multiple users with same email, different names, unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid7"}, Email: "user@example.com", Name: "John Doe"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid8"}, Email: "user@example.com", Name: "Jane Smith"},
},
token: "user@example.com",
want: types.User{},
wantErr: true,
},
// Test case 8: Multiple Users with the Same Name but Different Emails and Unique ProviderIdentifiers
{
name: "multiple users with same name, different emails, unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid9"}, Email: "johndoe@example.com", Name: "John Doe"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid10"}, Email: "janedoe@example.com", Name: "John Doe"},
},
token: "John Doe",
want: types.User{},
wantErr: true,
},
// Test case 9: Multiple Users with Same Email and Name but Unique ProviderIdentifiers
{
name: "multiple users with same email and name, unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid11"}, Email: "user@example.com", Name: "John Doe"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid12"}, Email: "user@example.com", Name: "John Doe"},
},
token: "user@example.com",
want: types.User{},
wantErr: true,
},
// Test case 10: Multiple Users without ProviderIdentifiers but with Unique Names and Emails
{
name: "multiple users without provider identifiers, unique names and emails",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "johndoe@example.com"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "janesmith@example.com"},
},
token: "John Doe",
want: types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "johndoe@example.com"},
wantErr: false,
},
// Test case 11: Multiple Users without ProviderIdentifiers and Duplicate Emails but Unique Names
{
name: "multiple users without provider identifiers, duplicate emails but unique names",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "user@example.com"},
},
token: "John Doe",
want: types.User{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
wantErr: false,
},
// Test case 12: Multiple Users without ProviderIdentifiers and Duplicate Names but Unique Emails
{
name: "multiple users without provider identifiers, duplicate names but unique emails",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "johndoe@example.com"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "janedoe@example.com"},
},
token: "John Doe",
want: types.User{},
wantErr: true,
},
// Test case 13: Multiple Users without ProviderIdentifiers and Duplicate Both Names and Emails
{
name: "multiple users without provider identifiers, duplicate names and emails",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
},
token: "John Doe",
want: types.User{},
wantErr: true,
},
// Test case 14: Multiple Users with Same Email Without ProviderIdentifiers
{
name: "multiple users with same email without provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "user@example.com"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "Jane Smith", Email: "user@example.com"},
},
token: "user@example.com",
want: types.User{},
wantErr: true,
},
// Test case 15: Multiple Users with Same Name Without ProviderIdentifiers
{
name: "multiple users with same name without provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "johndoe@example.com"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "John Doe", Email: "janedoe@example.com"},
},
token: "John Doe",
want: types.User{},
wantErr: true,
},
{
name: "Name field used as email address match",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid3"}, Name: "user@example.com", Email: "another@example.com"},
},
token: "user@example.com",
want: types.User{ProviderIdentifier: sql.NullString{Valid: true, String: "pid3"}, Name: "user@example.com", Email: "another@example.com"},
wantErr: false,
},
{
name: "multiple users with same name as email and unique provider identifiers",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid4"}, Name: "user@example.com", Email: "user1@example.com"},
{ProviderIdentifier: sql.NullString{Valid: true, String: "pid5"}, Name: "user@example.com", Email: "user2@example.com"},
},
token: "user@example.com",
want: types.User{},
wantErr: true,
},
{
name: "no provider identifier and duplicate names as emails",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "user@example.com", Email: "another1@example.com"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "user@example.com", Email: "another2@example.com"},
},
token: "user@example.com",
want: types.User{},
wantErr: true,
},
{
name: "name as email with multiple matches when provider identifier is not set",
users: []types.User{
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "user@example.com", Email: "another1@example.com"},
{ProviderIdentifier: sql.NullString{Valid: false, String: ""}, Name: "user@example.com", Email: "another2@example.com"},
},
token: "user@example.com",
want: types.User{},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
gotUser, err := findUserFromToken(tt.users, tt.token)
if (err != nil) != tt.wantErr {
t.Errorf("findUserFromToken() error = %v, wantErr %v", err, tt.wantErr)
return
}
if diff := cmp.Diff(tt.want, gotUser, util.Comparers...); diff != "" {
t.Errorf("findUserFromToken() unexpected result (-want +got):\n%s", diff)
}
})
}
}

View File

@@ -77,9 +77,12 @@ type Node struct {
ForcedTags []string `gorm:"serializer:json"`
// TODO(kradalby): This seems like irrelevant information?
AuthKeyID *uint64 `sql:"DEFAULT:NULL"`
AuthKey *PreAuthKey `gorm:"constraint:OnDelete:SET NULL;"`
// When a node has been created with a PreAuthKey, we need to
// prevent the preauthkey from being deleted before the node.
// The preauthkey can define "tags" of the node so we need it
// around.
AuthKeyID *uint64 `sql:"DEFAULT:NULL"`
AuthKey *PreAuthKey
LastSeen *time.Time
Expiry *time.Time

View File

@@ -14,7 +14,7 @@ type PreAuthKey struct {
ID uint64 `gorm:"primary_key"`
Key string
UserID uint
User User `gorm:"constraint:OnDelete:CASCADE;"`
User User `gorm:"constraint:OnDelete:SET NULL;"`
Reusable bool
Ephemeral bool `gorm:"default:false"`
Used bool `gorm:"default:false"`

View File

@@ -29,8 +29,9 @@ type User struct {
// you can have multiple users with the same name in OIDC,
// but not if you only run with CLI users.
// Username for the user, is used if email is empty
// Name (username) for the user, is used if email is empty
// Should not be used, please use Username().
// It is unique if ProviderIdentifier is not set.
Name string
// Typically the full name of the user
@@ -40,9 +41,11 @@ type User struct {
// Should not be used, please use Username().
Email string
// Unique identifier of the user from OIDC,
// comes from `sub` claim in the OIDC token
// and is used to lookup the user.
// ProviderIdentifier is a unique or not set identifier of the
// user from OIDC. It is the combination of `iss`
// and `sub` claim in the OIDC token.
// It is unique if set.
// It is unique together with Name.
ProviderIdentifier sql.NullString
// Provider is the origin of the user account,