From a52248669907c989c046bbab7e2fb68dbf62eb48 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Wed, 23 Oct 2024 17:59:57 +0200 Subject: [PATCH 01/14] migrate setup key to hashed + add delete endpoint + allow not expiring setup keys --- management/server/account.go | 1 + management/server/http/api/openapi.yml | 26 ++++++ management/server/http/handler.go | 1 + management/server/http/setupkeys_handler.go | 38 ++++++-- management/server/metrics/selfhosted.go | 2 +- management/server/migration/migration.go | 86 +++++++++++++++++++ management/server/migration/migration_test.go | 45 ++++++++++ management/server/mock_server/account_mock.go | 4 +- management/server/peer.go | 9 +- management/server/peer/peer.go | 35 ++++---- management/server/setupkey.go | 79 ++++++++++------- management/server/setupkey_test.go | 14 +-- management/server/sql_store.go | 26 +++++- management/server/sql_store_test.go | 29 +++++++ management/server/store.go | 4 + 15 files changed, 330 insertions(+), 69 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index a8a244bdf1f..1810c6b41ec 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -153,6 +153,7 @@ type AccountManager interface { FindExistingPostureCheck(accountID string, checks *posture.ChecksDefinition) (*posture.Checks, error) GetAccountIDForPeerKey(ctx context.Context, peerKey string) (string, error) GetAccountSettings(ctx context.Context, accountID string, userID string) (*Settings, error) + DeleteSetupKey(ctx context.Context, accountID, userID, keyID string) error } type DefaultAccountManager struct { diff --git a/management/server/http/api/openapi.yml b/management/server/http/api/openapi.yml index 9d51482481a..e8ebe6b86d2 100644 --- a/management/server/http/api/openapi.yml +++ b/management/server/http/api/openapi.yml @@ -2018,6 +2018,32 @@ paths: "$ref": "#/components/responses/forbidden" '500': "$ref": "#/components/responses/internal_error" + delete: + summary: Delete a Setup Key + description: Delete a Setup Key + tags: [ Setup Keys ] + security: + - BearerAuth: [ ] + - TokenAuth: [ ] + parameters: + - in: path + name: keyId + required: true + schema: + type: string + description: The unique identifier of a setup key + responses: + '200': + description: Delete status code + content: { } + '400': + "$ref": "#/components/responses/bad_request" + '401': + "$ref": "#/components/responses/requires_authentication" + '403': + "$ref": "#/components/responses/forbidden" + '500': + "$ref": "#/components/responses/internal_error" /api/groups: get: summary: List all Groups diff --git a/management/server/http/handler.go b/management/server/http/handler.go index 3f8a8554d07..c3928bff681 100644 --- a/management/server/http/handler.go +++ b/management/server/http/handler.go @@ -141,6 +141,7 @@ func (apiHandler *apiHandler) addSetupKeysEndpoint() { apiHandler.Router.HandleFunc("/setup-keys", keysHandler.CreateSetupKey).Methods("POST", "OPTIONS") apiHandler.Router.HandleFunc("/setup-keys/{keyId}", keysHandler.GetSetupKey).Methods("GET", "OPTIONS") apiHandler.Router.HandleFunc("/setup-keys/{keyId}", keysHandler.UpdateSetupKey).Methods("PUT", "OPTIONS") + apiHandler.Router.HandleFunc("/setup-keys/{keyId}", keysHandler.DeleteSetupKey).Methods("DELETE", "OPTIONS") } func (apiHandler *apiHandler) addPoliciesEndpoint() { diff --git a/management/server/http/setupkeys_handler.go b/management/server/http/setupkeys_handler.go index 8514f0b556b..020b1581403 100644 --- a/management/server/http/setupkeys_handler.go +++ b/management/server/http/setupkeys_handler.go @@ -62,9 +62,8 @@ func (h *SetupKeysHandler) CreateSetupKey(w http.ResponseWriter, r *http.Request expiresIn := time.Duration(req.ExpiresIn) * time.Second day := time.Hour * 24 - year := day * 365 - if expiresIn < day || expiresIn > year { - util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "expiresIn should be between 1 day and 365 days"), w) + if expiresIn < day || expiresIn == 0 { + util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "expiresIn should be at least 1 day"), w) return } @@ -76,6 +75,7 @@ func (h *SetupKeysHandler) CreateSetupKey(w http.ResponseWriter, r *http.Request if req.Ephemeral != nil { ephemeral = *req.Ephemeral } + setupKey, err := h.accountManager.CreateSetupKey(r.Context(), accountID, req.Name, server.SetupKeyType(req.Type), expiresIn, req.AutoGroups, req.UsageLimit, userID, ephemeral) if err != nil { @@ -83,7 +83,11 @@ func (h *SetupKeysHandler) CreateSetupKey(w http.ResponseWriter, r *http.Request return } - writeSuccess(r.Context(), w, setupKey) + apiSetupKeys := toResponseBody(setupKey) + // for the creation we need to send the plain key + apiSetupKeys.Key = setupKey.Key + + util.WriteJSONObject(r.Context(), w, apiSetupKeys) } // GetSetupKey is a GET request to get a SetupKey by ID @@ -181,6 +185,30 @@ func (h *SetupKeysHandler) GetAllSetupKeys(w http.ResponseWriter, r *http.Reques util.WriteJSONObject(r.Context(), w, apiSetupKeys) } +func (h *SetupKeysHandler) DeleteSetupKey(w http.ResponseWriter, r *http.Request) { + claims := h.claimsExtractor.FromRequestContext(r) + accountID, userID, err := h.accountManager.GetAccountIDFromToken(r.Context(), claims) + if err != nil { + util.WriteError(r.Context(), err, w) + return + } + + vars := mux.Vars(r) + keyID := vars["keyId"] + if len(keyID) == 0 { + util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid key ID"), w) + return + } + + err = h.accountManager.DeleteSetupKey(r.Context(), accountID, userID, keyID) + if err != nil { + util.WriteError(r.Context(), err, w) + return + } + + util.WriteJSONObject(r.Context(), w, emptyObject{}) +} + func writeSuccess(ctx context.Context, w http.ResponseWriter, key *server.SetupKey) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(200) @@ -206,7 +234,7 @@ func toResponseBody(key *server.SetupKey) *api.SetupKey { return &api.SetupKey{ Id: key.Id, - Key: key.Key, + Key: key.KeySecret, Name: key.Name, Expires: key.ExpiresAt, Type: string(key.Type), diff --git a/management/server/metrics/selfhosted.go b/management/server/metrics/selfhosted.go index bdf744d211e..843fa575e83 100644 --- a/management/server/metrics/selfhosted.go +++ b/management/server/metrics/selfhosted.go @@ -267,7 +267,7 @@ func (w *Worker) generateProperties(ctx context.Context) properties { peersSSHEnabled++ } - if peer.SetupKey == "" { + if peer.UserID != "" { userPeers++ } diff --git a/management/server/migration/migration.go b/management/server/migration/migration.go index 4c8baea5e87..1cfe041e945 100644 --- a/management/server/migration/migration.go +++ b/management/server/migration/migration.go @@ -2,13 +2,16 @@ package migration import ( "context" + "crypto/sha256" "database/sql" + b64 "encoding/base64" "encoding/gob" "encoding/json" "errors" "fmt" "net" "strings" + "unicode/utf8" log "github.com/sirupsen/logrus" "gorm.io/gorm" @@ -205,3 +208,86 @@ func MigrateNetIPFieldFromBlobToJSON[T any](ctx context.Context, db *gorm.DB, fi return nil } + +func MigrateSetupKeyToHashedSetupKey[T any](ctx context.Context, db *gorm.DB) error { + oldColumnName := "key" + newColumnName := "key_secret" + + var model T + + if !db.Migrator().HasTable(&model) { + log.WithContext(ctx).Debugf("Table for %T does not exist, no migration needed", model) + return nil + } + + stmt := &gorm.Statement{DB: db} + err := stmt.Parse(&model) + if err != nil { + return fmt.Errorf("parse model: %w", err) + } + tableName := stmt.Schema.Table + + if err := db.Transaction(func(tx *gorm.DB) error { + if !tx.Migrator().HasColumn(&model, newColumnName) { + log.WithContext(ctx).Infof("Column %s does not exist in table %s, adding it", newColumnName, tableName) + if err := tx.Migrator().AddColumn(&model, newColumnName); err != nil { + return fmt.Errorf("add column %s: %w", newColumnName, err) + } + } + + var rows []map[string]any + if err := tx.Table(tableName).Select("id", oldColumnName, newColumnName).Where(newColumnName + " IS NULL OR " + newColumnName + " = ''").Find(&rows).Error; err != nil { + return fmt.Errorf("find rows with empty secret key: %w", err) + } + + if len(rows) == 0 { + log.WithContext(ctx).Infof("No plain setup keys found in table %s, no migration needed", tableName) + return nil + } + + for _, row := range rows { + var plainKey string + if columnValue := row[oldColumnName]; columnValue != nil { + value, ok := columnValue.(string) + if !ok { + return fmt.Errorf("type assertion failed") + } + plainKey = value + } + + secretKey := hiddenKey(plainKey, 4) + + hashedKey := sha256.Sum256([]byte(plainKey)) + encodedHashedKey := b64.StdEncoding.EncodeToString(hashedKey[:]) + + if err := tx.Table(tableName).Where("id = ?", row["id"]).Update(newColumnName, secretKey).Error; err != nil { + return fmt.Errorf("update row with secret key: %w", err) + } + + if err := tx.Table(tableName).Where("id = ?", row["id"]).Update(oldColumnName, encodedHashedKey).Error; err != nil { + return fmt.Errorf("update row with hashed key: %w", err) + } + } + + if err := tx.Exec(fmt.Sprintf("ALTER TABLE %s DROP COLUMN %s", "peers", "setup_key")).Error; err != nil { + return fmt.Errorf("drop column %s: %w", oldColumnName, err) + } + + return nil + }); err != nil { + return err + } + + log.Printf("Migration of plain setup key to hashed setup key completed") + return nil +} + +// hiddenKey returns the Key value hidden with "*" and a 5 character prefix. +// E.g., "831F6*******************************" +func hiddenKey(key string, length int) string { + prefix := key[0:5] + if length > utf8.RuneCountInString(key) { + length = utf8.RuneCountInString(key) - len(prefix) + } + return prefix + strings.Repeat("*", length) +} diff --git a/management/server/migration/migration_test.go b/management/server/migration/migration_test.go index 5a192664169..76861c02f3d 100644 --- a/management/server/migration/migration_test.go +++ b/management/server/migration/migration_test.go @@ -160,3 +160,48 @@ func TestMigrateNetIPFieldFromBlobToJSON_WithJSONData(t *testing.T) { db.Model(&nbpeer.Peer{}).Select("location_connection_ip").First(&jsonStr) assert.JSONEq(t, `"10.0.0.1"`, jsonStr, "Data should be unchanged") } + +func TestMigrateSetupKeyToHashedSetupKey_ForPlainKey(t *testing.T) { + db := setupDatabase(t) + + err := db.AutoMigrate(&server.SetupKey{}) + require.NoError(t, err, "Failed to auto-migrate tables") + + err = db.Save(&server.SetupKey{ + Key: "EEFDAB47-C1A5-4472-8C05-71DE9A1E8382", + }).Error + require.NoError(t, err, "Failed to insert setup key") + + err = migration.MigrateSetupKeyToHashedSetupKey[server.SetupKey](context.Background(), db) + require.NoError(t, err, "Migration should not fail to migrate setup key") + + var key server.SetupKey + err = db.Model(&server.SetupKey{}).First(&key).Error + assert.NoError(t, err, "Failed to fetch setup key") + + assert.Equal(t, "EEFDA*******************************", key.KeySecret, "Key should be secret") + assert.Equal(t, "9+FQcmNd2GCxIK+SvHmtp6PPGV4MKEicDS+xuSQmvlE=", key.Key, "Key should be hashed") +} + +func TestMigrateSetupKeyToHashedSetupKey_ForAlreadyMigratedKey(t *testing.T) { + db := setupDatabase(t) + + err := db.AutoMigrate(&server.SetupKey{}) + require.NoError(t, err, "Failed to auto-migrate tables") + + err = db.Save(&server.SetupKey{ + Key: "9+FQcmNd2GCxIK+SvHmtp6PPGV4MKEicDS+xuSQmvlE=", + KeySecret: "EEFDA*******************************", + }).Error + require.NoError(t, err, "Failed to insert setup key") + + err = migration.MigrateSetupKeyToHashedSetupKey[server.SetupKey](context.Background(), db) + require.NoError(t, err, "Migration should not fail to migrate setup key") + + var key server.SetupKey + err = db.Model(&server.SetupKey{}).First(&key).Error + assert.NoError(t, err, "Failed to fetch setup key") + + assert.Equal(t, "EEFDA*******************************", key.KeySecret, "Key should be secret") + assert.Equal(t, "9+FQcmNd2GCxIK+SvHmtp6PPGV4MKEicDS+xuSQmvlE=", key.Key, "Key should be hashed") +} diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index 681bf533ae4..e41f63b9159 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -187,11 +187,11 @@ func (am *MockAccountManager) CreateSetupKey( usageLimit int, userID string, ephemeral bool, -) (*server.SetupKey, error) { +) (*server.SetupKey, string, error) { if am.CreateSetupKeyFunc != nil { return am.CreateSetupKeyFunc(ctx, accountID, keyName, keyType, expiresIn, autoGroups, usageLimit, userID, ephemeral) } - return nil, status.Errorf(codes.Unimplemented, "method CreateSetupKey is not implemented") + return nil, "", status.Errorf(codes.Unimplemented, "method CreateSetupKey is not implemented") } // AccountExists mock implementation of AccountExists from server.AccountManager interface diff --git a/management/server/peer.go b/management/server/peer.go index 80d43497a70..96ede151158 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -2,6 +2,8 @@ package server import ( "context" + "crypto/sha256" + b64 "encoding/base64" "fmt" "net" "slices" @@ -396,6 +398,8 @@ func (am *DefaultAccountManager) AddPeer(ctx context.Context, setupKey, userID s } upperKey := strings.ToUpper(setupKey) + hashedKey := sha256.Sum256([]byte(upperKey)) + encodedHashedKey := b64.StdEncoding.EncodeToString(hashedKey[:]) var accountID string var err error addedByUser := false @@ -403,7 +407,7 @@ func (am *DefaultAccountManager) AddPeer(ctx context.Context, setupKey, userID s addedByUser = true accountID, err = am.Store.GetAccountIDByUserID(userID) } else { - accountID, err = am.Store.GetAccountIDBySetupKey(ctx, setupKey) + accountID, err = am.Store.GetAccountIDBySetupKey(ctx, encodedHashedKey) } if err != nil { return nil, nil, nil, status.Errorf(status.NotFound, "failed adding new peer: account not found") @@ -448,7 +452,7 @@ func (am *DefaultAccountManager) AddPeer(ctx context.Context, setupKey, userID s opEvent.Activity = activity.PeerAddedByUser } else { // Validate the setup key - sk, err := transaction.GetSetupKeyBySecret(ctx, LockingStrengthUpdate, upperKey) + sk, err := transaction.GetSetupKeyBySecret(ctx, LockingStrengthUpdate, encodedHashedKey) if err != nil { return fmt.Errorf("failed to get setup key: %w", err) } @@ -489,7 +493,6 @@ func (am *DefaultAccountManager) AddPeer(ctx context.Context, setupKey, userID s ID: xid.New().String(), AccountID: accountID, Key: peer.Key, - SetupKey: upperKey, IP: freeIP, Meta: peer.Meta, Name: peer.Meta.Hostname, diff --git a/management/server/peer/peer.go b/management/server/peer/peer.go index ef96bce7dd8..1ff67da1231 100644 --- a/management/server/peer/peer.go +++ b/management/server/peer/peer.go @@ -16,8 +16,6 @@ type Peer struct { AccountID string `json:"-" gorm:"index"` // WireGuard public key Key string `gorm:"index"` - // A setup key this peer was registered with - SetupKey string `diff:"-"` // IP address of the Peer IP net.IP `gorm:"serializer:json"` // Meta is a Peer system meta data @@ -172,23 +170,22 @@ func (p *Peer) Copy() *Peer { peerStatus = p.Status.Copy() } return &Peer{ - ID: p.ID, - AccountID: p.AccountID, - Key: p.Key, - SetupKey: p.SetupKey, - IP: p.IP, - Meta: p.Meta, - Name: p.Name, - DNSLabel: p.DNSLabel, - Status: peerStatus, - UserID: p.UserID, - SSHKey: p.SSHKey, - SSHEnabled: p.SSHEnabled, - LoginExpirationEnabled: p.LoginExpirationEnabled, - LastLogin: p.LastLogin, - CreatedAt: p.CreatedAt, - Ephemeral: p.Ephemeral, - Location: p.Location, + ID: p.ID, + AccountID: p.AccountID, + Key: p.Key, + IP: p.IP, + Meta: p.Meta, + Name: p.Name, + DNSLabel: p.DNSLabel, + Status: peerStatus, + UserID: p.UserID, + SSHKey: p.SSHKey, + SSHEnabled: p.SSHEnabled, + LoginExpirationEnabled: p.LoginExpirationEnabled, + LastLogin: p.LastLogin, + CreatedAt: p.CreatedAt, + Ephemeral: p.Ephemeral, + Location: p.Location, InactivityExpirationEnabled: p.InactivityExpirationEnabled, } } diff --git a/management/server/setupkey.go b/management/server/setupkey.go index e84f8fcd687..224938920e8 100644 --- a/management/server/setupkey.go +++ b/management/server/setupkey.go @@ -2,6 +2,9 @@ package server import ( "context" + "crypto/sha256" + b64 "encoding/base64" + "fmt" "hash/fnv" "strconv" "strings" @@ -73,6 +76,7 @@ type SetupKey struct { // AccountID is a reference to Account that this object belongs AccountID string `json:"-" gorm:"index"` Key string + KeySecret string Name string Type SetupKeyType CreatedAt time.Time @@ -120,19 +124,17 @@ func (key *SetupKey) Copy() *SetupKey { // EventMeta returns activity event meta related to the setup key func (key *SetupKey) EventMeta() map[string]any { - return map[string]any{"name": key.Name, "type": key.Type, "key": key.HiddenCopy(1).Key} + return map[string]any{"name": key.Name, "type": key.Type, "key": key.KeySecret} } -// HiddenCopy returns a copy of the key with a Key value hidden with "*" and a 5 character prefix. +// hiddenKey returns the Key value hidden with "*" and a 5 character prefix. // E.g., "831F6*******************************" -func (key *SetupKey) HiddenCopy(length int) *SetupKey { - k := key.Copy() - prefix := k.Key[0:5] - if length > utf8.RuneCountInString(key.Key) { - length = utf8.RuneCountInString(key.Key) - len(prefix) +func hiddenKey(key string, length int) string { + prefix := key[0:5] + if length > utf8.RuneCountInString(key) { + length = utf8.RuneCountInString(key) - len(prefix) } - k.Key = prefix + strings.Repeat("*", length) - return k + return prefix + strings.Repeat("*", length) } // IncrementUsage makes a copy of a key, increments the UsedTimes by 1 and sets LastUsed to now @@ -155,6 +157,9 @@ func (key *SetupKey) IsRevoked() bool { // IsExpired if key was expired func (key *SetupKey) IsExpired() bool { + if key.ExpiresAt.IsZero() { + return false + } return time.Now().After(key.ExpiresAt) } @@ -169,32 +174,44 @@ func (key *SetupKey) IsOverUsed() bool { // GenerateSetupKey generates a new setup key func GenerateSetupKey(name string, t SetupKeyType, validFor time.Duration, autoGroups []string, - usageLimit int, ephemeral bool) *SetupKey { + usageLimit int, ephemeral bool) (*SetupKey, string) { key := strings.ToUpper(uuid.New().String()) limit := usageLimit if t == SetupKeyOneOff { limit = 1 } + + expiresAt := time.Time{} + if validFor != 0 { + expiresAt = time.Now().UTC().Add(validFor) + } + + hashedKey := sha256.Sum256([]byte(key)) + encodedHashedKey := b64.StdEncoding.EncodeToString(hashedKey[:]) + return &SetupKey{ Id: strconv.Itoa(int(Hash(key))), - Key: key, + Key: encodedHashedKey, + KeySecret: hiddenKey(key, 4), Name: name, Type: t, CreatedAt: time.Now().UTC(), - ExpiresAt: time.Now().UTC().Add(validFor), + ExpiresAt: expiresAt, UpdatedAt: time.Now().UTC(), Revoked: false, UsedTimes: 0, AutoGroups: autoGroups, UsageLimit: limit, Ephemeral: ephemeral, - } + }, key } // GenerateDefaultSetupKey generates a default reusable setup key with an unlimited usage and 30 days expiration func GenerateDefaultSetupKey() *SetupKey { - return GenerateSetupKey(DefaultSetupKeyName, SetupKeyReusable, DefaultSetupKeyDuration, []string{}, + key, plainKey := GenerateSetupKey(DefaultSetupKeyName, SetupKeyReusable, DefaultSetupKeyDuration, []string{}, SetupKeyUnlimitedUsage, false) + key.Key = plainKey + return key } func Hash(s string) uint32 { @@ -227,7 +244,7 @@ func (am *DefaultAccountManager) CreateSetupKey(ctx context.Context, accountID s return nil, err } - setupKey := GenerateSetupKey(keyName, keyType, keyDuration, autoGroups, usageLimit, ephemeral) + setupKey, plainKey := GenerateSetupKey(keyName, keyType, keyDuration, autoGroups, usageLimit, ephemeral) account.SetupKeys[setupKey.Key] = setupKey err = am.Store.SaveAccount(ctx, account) if err != nil { @@ -246,6 +263,9 @@ func (am *DefaultAccountManager) CreateSetupKey(ctx context.Context, accountID s } } + // for the creation return the plain key to the caller + setupKey.Key = plainKey + return setupKey, nil } @@ -342,18 +362,7 @@ func (am *DefaultAccountManager) ListSetupKeys(ctx context.Context, accountID, u return nil, err } - keys := make([]*SetupKey, 0, len(setupKeys)) - for _, key := range setupKeys { - var k *SetupKey - if !user.IsAdminOrServiceUser() { - k = key.HiddenCopy(999) - } else { - k = key.Copy() - } - keys = append(keys, k) - } - - return keys, nil + return setupKeys, nil } // GetSetupKey looks up a SetupKey by KeyID, returns NotFound error if not found. @@ -377,11 +386,21 @@ func (am *DefaultAccountManager) GetSetupKey(ctx context.Context, accountID, use setupKey.UpdatedAt = setupKey.CreatedAt } - if !user.IsAdminOrServiceUser() { - setupKey = setupKey.HiddenCopy(999) + return setupKey, nil +} + +// DeleteSetupKey removes the setup key from the account +func (am *DefaultAccountManager) DeleteSetupKey(ctx context.Context, accountID, userID, keyID string) error { + user, err := am.Store.GetUserByUserID(ctx, LockingStrengthShare, userID) + if err != nil { + return fmt.Errorf("failed to get user: %w", err) + } + + if !user.IsAdminOrServiceUser() || user.AccountID != accountID { + return status.Errorf(status.Unauthorized, "only users with admin power can view setup keys") } - return setupKey, nil + return am.Store.DeleteSetupKey(ctx, accountID, keyID) } func validateSetupKeyAutoGroups(account *Account, autoGroups []string) error { diff --git a/management/server/setupkey_test.go b/management/server/setupkey_test.go index 651b5401047..5b01b44954a 100644 --- a/management/server/setupkey_test.go +++ b/management/server/setupkey_test.go @@ -256,7 +256,7 @@ func TestGenerateSetupKey(t *testing.T) { expectedUpdatedAt := time.Now().UTC() var expectedAutoGroups []string - key := GenerateSetupKey(expectedName, SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) + key, _ := GenerateSetupKey(expectedName, SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) assertKey(t, key, expectedName, expectedRevoke, expectedType, expectedUsedTimes, expectedCreatedAt, expectedExpiresAt, strconv.Itoa(int(Hash(key.Key))), expectedUpdatedAt, expectedAutoGroups) @@ -264,33 +264,33 @@ func TestGenerateSetupKey(t *testing.T) { } func TestSetupKey_IsValid(t *testing.T) { - validKey := GenerateSetupKey("valid key", SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) + validKey, _ := GenerateSetupKey("valid key", SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) if !validKey.IsValid() { t.Errorf("expected key to be valid, got invalid %v", validKey) } // expired - expiredKey := GenerateSetupKey("invalid key", SetupKeyOneOff, -time.Hour, []string{}, SetupKeyUnlimitedUsage, false) + expiredKey, _ := GenerateSetupKey("invalid key", SetupKeyOneOff, -time.Hour, []string{}, SetupKeyUnlimitedUsage, false) if expiredKey.IsValid() { t.Errorf("expected key to be invalid due to expiration, got valid %v", expiredKey) } // revoked - revokedKey := GenerateSetupKey("invalid key", SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) + revokedKey, _ := GenerateSetupKey("invalid key", SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) revokedKey.Revoked = true if revokedKey.IsValid() { t.Errorf("expected revoked key to be invalid, got valid %v", revokedKey) } // overused - overUsedKey := GenerateSetupKey("invalid key", SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) + overUsedKey, _ := GenerateSetupKey("invalid key", SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) overUsedKey.UsedTimes = 1 if overUsedKey.IsValid() { t.Errorf("expected overused key to be invalid, got valid %v", overUsedKey) } // overused - reusableKey := GenerateSetupKey("valid key", SetupKeyReusable, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) + reusableKey, _ := GenerateSetupKey("valid key", SetupKeyReusable, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) reusableKey.UsedTimes = 99 if !reusableKey.IsValid() { t.Errorf("expected reusable key to be valid when used many times, got valid %v", reusableKey) @@ -346,7 +346,7 @@ func assertKey(t *testing.T, key *SetupKey, expectedName string, expectedRevoke func TestSetupKey_Copy(t *testing.T) { - key := GenerateSetupKey("key name", SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) + key, _ := GenerateSetupKey("key name", SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) keyCopy := key.Copy() assertKey(t, keyCopy, key.Name, key.Revoked, string(key.Type), key.UsedTimes, key.CreatedAt, key.ExpiresAt, key.Id, diff --git a/management/server/sql_store.go b/management/server/sql_store.go index 47395f51109..06591865cff 100644 --- a/management/server/sql_store.go +++ b/management/server/sql_store.go @@ -741,7 +741,7 @@ func (s *SqlStore) GetAccountIDByUserID(userID string) (string, error) { func (s *SqlStore) GetAccountIDBySetupKey(ctx context.Context, setupKey string) (string, error) { var accountID string - result := s.db.WithContext(ctx).Model(&SetupKey{}).Select("account_id").Where(keyQueryCondition, strings.ToUpper(setupKey)).First(&accountID) + result := s.db.WithContext(ctx).Model(&SetupKey{}).Select("account_id").Where(keyQueryCondition, setupKey).First(&accountID) if result.Error != nil { if errors.Is(result.Error, gorm.ErrRecordNotFound) { return "", status.Errorf(status.NotFound, "account not found: index lookup failed") @@ -973,7 +973,7 @@ func NewPostgresqlStoreFromSqlStore(ctx context.Context, sqliteStore *SqlStore, func (s *SqlStore) GetSetupKeyBySecret(ctx context.Context, lockStrength LockingStrength, key string) (*SetupKey, error) { var setupKey SetupKey result := s.db.WithContext(ctx).Clauses(clause.Locking{Strength: string(lockStrength)}). - First(&setupKey, keyQueryCondition, strings.ToUpper(key)) + First(&setupKey, keyQueryCondition, key) if result.Error != nil { if errors.Is(result.Error, gorm.ErrRecordNotFound) { return nil, status.Errorf(status.NotFound, "setup key not found") @@ -1232,6 +1232,10 @@ func (s *SqlStore) GetNameServerGroupByID(ctx context.Context, lockStrength Lock return getRecordByID[nbdns.NameServerGroup](s.db.WithContext(ctx), lockStrength, nsGroupID, accountID) } +func (s *SqlStore) DeleteSetupKey(ctx context.Context, accountID, keyID string) error { + return deleteRecordByID[SetupKey](s.db.WithContext(ctx), LockingStrengthUpdate, keyID, accountID) +} + // getRecords retrieves records from the database based on the account ID. func getRecords[T any](db *gorm.DB, lockStrength LockingStrength, accountID string) ([]T, error) { var record []T @@ -1264,3 +1268,21 @@ func getRecordByID[T any](db *gorm.DB, lockStrength LockingStrength, recordID, a } return &record, nil } + +// deleteRecordByID deletes a record by its ID and account ID from the database. +func deleteRecordByID[T any](db *gorm.DB, lockStrength LockingStrength, recordID, accountID string) error { + var record T + result := db.Clauses(clause.Locking{Strength: string(lockStrength)}).Delete(record, accountAndIDQueryCondition, accountID, recordID) + if err := result.Error; err != nil { + parts := strings.Split(fmt.Sprintf("%T", record), ".") + recordType := parts[len(parts)-1] + + return status.Errorf(status.Internal, "failed to delete %s from store: %v", recordType, err) + } + + if result.RowsAffected == 0 { + return status.Errorf(status.NotFound, "record not found") + } + + return nil +} diff --git a/management/server/sql_store_test.go b/management/server/sql_store_test.go index 000eb1b11b2..fad5e81ea42 100644 --- a/management/server/sql_store_test.go +++ b/management/server/sql_store_test.go @@ -1264,3 +1264,32 @@ func TestSqlite_GetGroupByName(t *testing.T) { require.NoError(t, err) require.Equal(t, "All", group.Name) } + +func Test_DeleteSetupKeySuccessfully(t *testing.T) { + t.Setenv("NETBIRD_STORE_ENGINE", string(SqliteStoreEngine)) + store, cleanup, err := NewTestStoreFromSQL(context.Background(), "testdata/extended-store.sql", t.TempDir()) + t.Cleanup(cleanup) + require.NoError(t, err) + + accountID := "bf1c8084-ba50-4ce7-9439-34653001fc3b" + setupKeyID := "A2C8E62B-38F5-4553-B31E-DD66C696CEBB" + + err = store.DeleteSetupKey(context.Background(), accountID, setupKeyID) + require.NoError(t, err) + + _, err = store.GetSetupKeyByID(context.Background(), LockingStrengthShare, setupKeyID, accountID) + require.Error(t, err) +} + +func Test_DeleteSetupKeyFailsForNonExistingKey(t *testing.T) { + t.Setenv("NETBIRD_STORE_ENGINE", string(SqliteStoreEngine)) + store, cleanup, err := NewTestStoreFromSQL(context.Background(), "testdata/extended-store.sql", t.TempDir()) + t.Cleanup(cleanup) + require.NoError(t, err) + + accountID := "bf1c8084-ba50-4ce7-9439-34653001fc3b" + nonExistingKeyID := "non-existing-key-id" + + err = store.DeleteSetupKey(context.Background(), accountID, nonExistingKeyID) + require.Error(t, err) +} diff --git a/management/server/store.go b/management/server/store.go index 131fd8aaab6..087c9884763 100644 --- a/management/server/store.go +++ b/management/server/store.go @@ -124,6 +124,7 @@ type Store interface { // This is also a method of metrics.DataSource interface. GetStoreEngine() StoreEngine ExecuteInTransaction(ctx context.Context, f func(store Store) error) error + DeleteSetupKey(ctx context.Context, accountID, keyID string) error } type StoreEngine string @@ -241,6 +242,9 @@ func getMigrations(ctx context.Context) []migrationFunc { func(db *gorm.DB) error { return migration.MigrateNetIPFieldFromBlobToJSON[nbpeer.Peer](ctx, db, "ip", "idx_peers_account_id_ip") }, + func(db *gorm.DB) error { + return migration.MigrateSetupKeyToHashedSetupKey[SetupKey](ctx, db) + }, } } From 682b2c8687c2a0c86f0b7b16e8d5d24195ba887b Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Wed, 23 Oct 2024 18:29:21 +0200 Subject: [PATCH 02/14] fix migration --- management/server/migration/migration.go | 12 +++++++++--- management/server/setupkey.go | 1 + 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/management/server/migration/migration.go b/management/server/migration/migration.go index 1cfe041e945..b3a04560310 100644 --- a/management/server/migration/migration.go +++ b/management/server/migration/migration.go @@ -235,9 +235,15 @@ func MigrateSetupKeyToHashedSetupKey[T any](ctx context.Context, db *gorm.DB) er } } + pattern := "[A-F0-9]{8}-[A-F0-9]{4}-[A-F0-9]{4}-[A-F0-9]{4}-[A-F0-9]{12}" + var rows []map[string]any - if err := tx.Table(tableName).Select("id", oldColumnName, newColumnName).Where(newColumnName + " IS NULL OR " + newColumnName + " = ''").Find(&rows).Error; err != nil { - return fmt.Errorf("find rows with empty secret key: %w", err) + if err := tx.Table(tableName). + Select("id", oldColumnName, newColumnName). + Where(newColumnName+" IS NULL OR "+newColumnName+" = ''"). + Where(oldColumnName+" ~ ?", pattern). + Find(&rows).Error; err != nil { + return fmt.Errorf("find rows with empty secret key and matching pattern: %w", err) } if len(rows) == 0 { @@ -270,7 +276,7 @@ func MigrateSetupKeyToHashedSetupKey[T any](ctx context.Context, db *gorm.DB) er } if err := tx.Exec(fmt.Sprintf("ALTER TABLE %s DROP COLUMN %s", "peers", "setup_key")).Error; err != nil { - return fmt.Errorf("drop column %s: %w", oldColumnName, err) + log.WithContext(ctx).Errorf("Failed to drop column %s: %v", "setup_key", err) } return nil diff --git a/management/server/setupkey.go b/management/server/setupkey.go index 224938920e8..560b4ea0379 100644 --- a/management/server/setupkey.go +++ b/management/server/setupkey.go @@ -108,6 +108,7 @@ func (key *SetupKey) Copy() *SetupKey { Id: key.Id, AccountID: key.AccountID, Key: key.Key, + KeySecret: key.KeySecret, Name: key.Name, Type: key.Type, CreatedAt: key.CreatedAt, From be6c42fd683c6a8a50cdb264d106f754401db8ba Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Wed, 23 Oct 2024 18:49:43 +0200 Subject: [PATCH 03/14] fix migration --- management/server/migration/migration.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/management/server/migration/migration.go b/management/server/migration/migration.go index b3a04560310..6f12d94b401 100644 --- a/management/server/migration/migration.go +++ b/management/server/migration/migration.go @@ -235,13 +235,11 @@ func MigrateSetupKeyToHashedSetupKey[T any](ctx context.Context, db *gorm.DB) er } } - pattern := "[A-F0-9]{8}-[A-F0-9]{4}-[A-F0-9]{4}-[A-F0-9]{4}-[A-F0-9]{12}" - var rows []map[string]any if err := tx.Table(tableName). Select("id", oldColumnName, newColumnName). - Where(newColumnName+" IS NULL OR "+newColumnName+" = ''"). - Where(oldColumnName+" ~ ?", pattern). + Where(newColumnName + " IS NULL OR " + newColumnName + " = ''"). + Where("SUBSTR(" + oldColumnName + ", 9, 1) = '-'"). Find(&rows).Error; err != nil { return fmt.Errorf("find rows with empty secret key and matching pattern: %w", err) } From 7721334bbad1cd02886a6333932b66cc509cc944 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Wed, 23 Oct 2024 18:57:48 +0200 Subject: [PATCH 04/14] fix migration test --- management/server/migration/migration_test.go | 29 ++++++++++++++++--- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/management/server/migration/migration_test.go b/management/server/migration/migration_test.go index 76861c02f3d..b9342726284 100644 --- a/management/server/migration/migration_test.go +++ b/management/server/migration/migration_test.go @@ -179,11 +179,11 @@ func TestMigrateSetupKeyToHashedSetupKey_ForPlainKey(t *testing.T) { err = db.Model(&server.SetupKey{}).First(&key).Error assert.NoError(t, err, "Failed to fetch setup key") - assert.Equal(t, "EEFDA*******************************", key.KeySecret, "Key should be secret") + assert.Equal(t, "EEFDA****", key.KeySecret, "Key should be secret") assert.Equal(t, "9+FQcmNd2GCxIK+SvHmtp6PPGV4MKEicDS+xuSQmvlE=", key.Key, "Key should be hashed") } -func TestMigrateSetupKeyToHashedSetupKey_ForAlreadyMigratedKey(t *testing.T) { +func TestMigrateSetupKeyToHashedSetupKey_ForAlreadyMigratedKey_Case1(t *testing.T) { db := setupDatabase(t) err := db.AutoMigrate(&server.SetupKey{}) @@ -191,7 +191,29 @@ func TestMigrateSetupKeyToHashedSetupKey_ForAlreadyMigratedKey(t *testing.T) { err = db.Save(&server.SetupKey{ Key: "9+FQcmNd2GCxIK+SvHmtp6PPGV4MKEicDS+xuSQmvlE=", - KeySecret: "EEFDA*******************************", + KeySecret: "EEFDA****", + }).Error + require.NoError(t, err, "Failed to insert setup key") + + err = migration.MigrateSetupKeyToHashedSetupKey[server.SetupKey](context.Background(), db) + require.NoError(t, err, "Migration should not fail to migrate setup key") + + var key server.SetupKey + err = db.Model(&server.SetupKey{}).First(&key).Error + assert.NoError(t, err, "Failed to fetch setup key") + + assert.Equal(t, "EEFDA****", key.KeySecret, "Key should be secret") + assert.Equal(t, "9+FQcmNd2GCxIK+SvHmtp6PPGV4MKEicDS+xuSQmvlE=", key.Key, "Key should be hashed") +} + +func TestMigrateSetupKeyToHashedSetupKey_ForAlreadyMigratedKey_Case2(t *testing.T) { + db := setupDatabase(t) + + err := db.AutoMigrate(&server.SetupKey{}) + require.NoError(t, err, "Failed to auto-migrate tables") + + err = db.Save(&server.SetupKey{ + Key: "9+FQcmNd2GCxIK+SvHmtp6PPGV4MKEicDS+xuSQmvlE=", }).Error require.NoError(t, err, "Failed to insert setup key") @@ -202,6 +224,5 @@ func TestMigrateSetupKeyToHashedSetupKey_ForAlreadyMigratedKey(t *testing.T) { err = db.Model(&server.SetupKey{}).First(&key).Error assert.NoError(t, err, "Failed to fetch setup key") - assert.Equal(t, "EEFDA*******************************", key.KeySecret, "Key should be secret") assert.Equal(t, "9+FQcmNd2GCxIK+SvHmtp6PPGV4MKEicDS+xuSQmvlE=", key.Key, "Key should be hashed") } From c36bb338bea6fc6456a2038a10aa537337eb0c96 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 24 Oct 2024 12:13:17 +0200 Subject: [PATCH 05/14] fix tests --- management/server/account_test.go | 9 -- management/server/http/peers_handler_test.go | 4 +- .../server/http/setupkeys_handler_test.go | 9 +- management/server/migration/migration_test.go | 3 + management/server/mock_server/account_mock.go | 9 +- management/server/peer_test.go | 20 +-- management/server/setupkey.go | 6 +- management/server/setupkey_test.go | 52 +++++-- management/server/sql_store.go | 2 +- management/server/sql_store_test.go | 135 ++++++++---------- 10 files changed, 134 insertions(+), 115 deletions(-) diff --git a/management/server/account_test.go b/management/server/account_test.go index 3c3fcebc67f..1cd4ae449db 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -1010,7 +1010,6 @@ func TestAccountManager_AddPeer(t *testing.T) { return } expectedPeerKey := key.PublicKey().String() - expectedSetupKey := setupKey.Key peer, _, _, err := manager.AddPeer(context.Background(), setupKey.Key, "", &nbpeer.Peer{ Key: expectedPeerKey, @@ -1035,10 +1034,6 @@ func TestAccountManager_AddPeer(t *testing.T) { t.Errorf("expecting just added peer's IP %s to be in a network range %s", peer.IP.String(), account.Network.Net.String()) } - if peer.SetupKey != expectedSetupKey { - t.Errorf("expecting just added peer to have SetupKey = %s, got %s", expectedSetupKey, peer.SetupKey) - } - if account.Network.CurrentSerial() != 1 { t.Errorf("expecting Network Serial=%d to be incremented by 1 and be equal to %d when adding new peer to account", serial, account.Network.CurrentSerial()) } @@ -2367,7 +2362,6 @@ func TestAccount_GetNextPeerExpiration(t *testing.T) { LoginExpired: false, }, LoginExpirationEnabled: true, - SetupKey: "key", }, "peer-2": { Status: &nbpeer.PeerStatus{ @@ -2375,7 +2369,6 @@ func TestAccount_GetNextPeerExpiration(t *testing.T) { LoginExpired: false, }, LoginExpirationEnabled: true, - SetupKey: "key", }, }, expiration: time.Second, @@ -2529,7 +2522,6 @@ func TestAccount_GetNextInactivePeerExpiration(t *testing.T) { LoginExpired: false, }, InactivityExpirationEnabled: true, - SetupKey: "key", }, "peer-2": { Status: &nbpeer.PeerStatus{ @@ -2537,7 +2529,6 @@ func TestAccount_GetNextInactivePeerExpiration(t *testing.T) { LoginExpired: false, }, InactivityExpirationEnabled: true, - SetupKey: "key", }, }, expiration: time.Second, diff --git a/management/server/http/peers_handler_test.go b/management/server/http/peers_handler_test.go index f933eee1497..dd49c03b848 100644 --- a/management/server/http/peers_handler_test.go +++ b/management/server/http/peers_handler_test.go @@ -13,12 +13,13 @@ import ( "time" "github.com/gorilla/mux" + "golang.org/x/exp/maps" + "github.com/netbirdio/netbird/management/server" nbgroup "github.com/netbirdio/netbird/management/server/group" "github.com/netbirdio/netbird/management/server/http/api" "github.com/netbirdio/netbird/management/server/jwtclaims" nbpeer "github.com/netbirdio/netbird/management/server/peer" - "golang.org/x/exp/maps" "github.com/stretchr/testify/assert" @@ -168,7 +169,6 @@ func TestGetPeers(t *testing.T) { peer := &nbpeer.Peer{ ID: testPeerID, Key: "key", - SetupKey: "setupkey", IP: net.ParseIP("100.64.0.1"), Status: &nbpeer.PeerStatus{Connected: true}, Name: "PeerName", diff --git a/management/server/http/setupkeys_handler_test.go b/management/server/http/setupkeys_handler_test.go index 2d15287af25..c38fce4e83b 100644 --- a/management/server/http/setupkeys_handler_test.go +++ b/management/server/http/setupkeys_handler_test.go @@ -81,18 +81,21 @@ func initSetupKeysTestMetaData(defaultKey *server.SetupKey, newKey *server.Setup } func TestSetupKeysHandlers(t *testing.T) { - defaultSetupKey := server.GenerateDefaultSetupKey() + defaultSetupKey, _ := server.GenerateDefaultSetupKey() defaultSetupKey.Id = existingSetupKeyID adminUser := server.NewAdminUser("test_user") - newSetupKey := server.GenerateSetupKey(newSetupKeyName, server.SetupKeyReusable, 0, []string{"group-1"}, + newSetupKey, plainKey := server.GenerateSetupKey(newSetupKeyName, server.SetupKeyReusable, 0, []string{"group-1"}, server.SetupKeyUnlimitedUsage, true) + newSetupKey.Key = plainKey updatedDefaultSetupKey := defaultSetupKey.Copy() updatedDefaultSetupKey.AutoGroups = []string{"group-1"} updatedDefaultSetupKey.Name = updatedSetupKeyName updatedDefaultSetupKey.Revoked = true + expectedNewKey := toResponseBody(newSetupKey) + expectedNewKey.Key = plainKey tt := []struct { name string requestType string @@ -134,7 +137,7 @@ func TestSetupKeysHandlers(t *testing.T) { []byte(fmt.Sprintf("{\"name\":\"%s\",\"type\":\"%s\",\"expires_in\":86400, \"ephemeral\":true}", newSetupKey.Name, newSetupKey.Type))), expectedStatus: http.StatusOK, expectedBody: true, - expectedSetupKey: toResponseBody(newSetupKey), + expectedSetupKey: expectedNewKey, }, { name: "Update Setup Key", diff --git a/management/server/migration/migration_test.go b/management/server/migration/migration_test.go index b9342726284..51358c7ad67 100644 --- a/management/server/migration/migration_test.go +++ b/management/server/migration/migration_test.go @@ -168,6 +168,7 @@ func TestMigrateSetupKeyToHashedSetupKey_ForPlainKey(t *testing.T) { require.NoError(t, err, "Failed to auto-migrate tables") err = db.Save(&server.SetupKey{ + Id: "1", Key: "EEFDAB47-C1A5-4472-8C05-71DE9A1E8382", }).Error require.NoError(t, err, "Failed to insert setup key") @@ -190,6 +191,7 @@ func TestMigrateSetupKeyToHashedSetupKey_ForAlreadyMigratedKey_Case1(t *testing. require.NoError(t, err, "Failed to auto-migrate tables") err = db.Save(&server.SetupKey{ + Id: "1", Key: "9+FQcmNd2GCxIK+SvHmtp6PPGV4MKEicDS+xuSQmvlE=", KeySecret: "EEFDA****", }).Error @@ -213,6 +215,7 @@ func TestMigrateSetupKeyToHashedSetupKey_ForAlreadyMigratedKey_Case2(t *testing. require.NoError(t, err, "Failed to auto-migrate tables") err = db.Save(&server.SetupKey{ + Id: "1", Key: "9+FQcmNd2GCxIK+SvHmtp6PPGV4MKEicDS+xuSQmvlE=", }).Error require.NoError(t, err, "Failed to insert setup key") diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index e41f63b9159..05163f51493 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -111,6 +111,11 @@ type MockAccountManager struct { GetAccountSettingsFunc func(ctx context.Context, accountID string, userID string) (*server.Settings, error) } +func (am *MockAccountManager) DeleteSetupKey(ctx context.Context, accountID, userID, keyID string) error { + // TODO implement me + panic("implement me") +} + func (am *MockAccountManager) SyncAndMarkPeer(ctx context.Context, accountID string, peerPubKey string, meta nbpeer.PeerSystemMeta, realIP net.IP) (*nbpeer.Peer, *server.NetworkMap, []*posture.Checks, error) { if am.SyncAndMarkPeerFunc != nil { return am.SyncAndMarkPeerFunc(ctx, accountID, peerPubKey, meta, realIP) @@ -187,11 +192,11 @@ func (am *MockAccountManager) CreateSetupKey( usageLimit int, userID string, ephemeral bool, -) (*server.SetupKey, string, error) { +) (*server.SetupKey, error) { if am.CreateSetupKeyFunc != nil { return am.CreateSetupKeyFunc(ctx, accountID, keyName, keyType, expiresIn, autoGroups, usageLimit, userID, ephemeral) } - return nil, "", status.Errorf(codes.Unimplemented, "method CreateSetupKey is not implemented") + return nil, status.Errorf(codes.Unimplemented, "method CreateSetupKey is not implemented") } // AccountExists mock implementation of AccountExists from server.AccountManager interface diff --git a/management/server/peer_test.go b/management/server/peer_test.go index 7b2180bf019..5127f77fbe6 100644 --- a/management/server/peer_test.go +++ b/management/server/peer_test.go @@ -2,6 +2,8 @@ package server import ( "context" + "crypto/sha256" + b64 "encoding/base64" "fmt" "io" "net" @@ -1090,7 +1092,6 @@ func Test_RegisterPeerByUser(t *testing.T) { ID: xid.New().String(), AccountID: existingAccountID, Key: "newPeerKey", - SetupKey: "", IP: net.IP{123, 123, 123, 123}, Meta: nbpeer.PeerSystemMeta{ Hostname: "newPeer", @@ -1155,7 +1156,6 @@ func Test_RegisterPeerBySetupKey(t *testing.T) { ID: xid.New().String(), AccountID: existingAccountID, Key: "newPeerKey", - SetupKey: "existingSetupKey", UserID: "", IP: net.IP{123, 123, 123, 123}, Meta: nbpeer.PeerSystemMeta{ @@ -1175,7 +1175,6 @@ func Test_RegisterPeerBySetupKey(t *testing.T) { peer, err := store.GetPeerByPeerPubKey(context.Background(), LockingStrengthShare, newPeer.Key) require.NoError(t, err) assert.Equal(t, peer.AccountID, existingAccountID) - assert.Equal(t, peer.SetupKey, existingSetupKeyID) account, err := store.GetAccount(context.Background(), existingAccountID) require.NoError(t, err) @@ -1187,8 +1186,11 @@ func Test_RegisterPeerBySetupKey(t *testing.T) { lastUsed, err := time.Parse("2006-01-02T15:04:05Z", "0001-01-01T00:00:00Z") assert.NoError(t, err) - assert.NotEqual(t, lastUsed, account.SetupKeys[existingSetupKeyID].LastUsed) - assert.Equal(t, 1, account.SetupKeys[existingSetupKeyID].UsedTimes) + + hashedKey := sha256.Sum256([]byte(existingSetupKeyID)) + encodedHashedKey := b64.StdEncoding.EncodeToString(hashedKey[:]) + assert.NotEqual(t, lastUsed, account.SetupKeys[encodedHashedKey].LastUsed) + assert.Equal(t, 1, account.SetupKeys[encodedHashedKey].UsedTimes) } @@ -1221,7 +1223,6 @@ func Test_RegisterPeerRollbackOnFailure(t *testing.T) { ID: xid.New().String(), AccountID: existingAccountID, Key: "newPeerKey", - SetupKey: "existingSetupKey", UserID: "", IP: net.IP{123, 123, 123, 123}, Meta: nbpeer.PeerSystemMeta{ @@ -1250,8 +1251,11 @@ func Test_RegisterPeerRollbackOnFailure(t *testing.T) { lastUsed, err := time.Parse("2006-01-02T15:04:05Z", "0001-01-01T00:00:00Z") assert.NoError(t, err) - assert.Equal(t, lastUsed, account.SetupKeys[faultyKey].LastUsed.UTC()) - assert.Equal(t, 0, account.SetupKeys[faultyKey].UsedTimes) + + hashedKey := sha256.Sum256([]byte(faultyKey)) + encodedHashedKey := b64.StdEncoding.EncodeToString(hashedKey[:]) + assert.Equal(t, lastUsed, account.SetupKeys[encodedHashedKey].LastUsed.UTC()) + assert.Equal(t, 0, account.SetupKeys[encodedHashedKey].UsedTimes) } func TestPeerAccountPeersUpdate(t *testing.T) { diff --git a/management/server/setupkey.go b/management/server/setupkey.go index 560b4ea0379..53c18adacbb 100644 --- a/management/server/setupkey.go +++ b/management/server/setupkey.go @@ -208,11 +208,9 @@ func GenerateSetupKey(name string, t SetupKeyType, validFor time.Duration, autoG } // GenerateDefaultSetupKey generates a default reusable setup key with an unlimited usage and 30 days expiration -func GenerateDefaultSetupKey() *SetupKey { - key, plainKey := GenerateSetupKey(DefaultSetupKeyName, SetupKeyReusable, DefaultSetupKeyDuration, []string{}, +func GenerateDefaultSetupKey() (*SetupKey, string) { + return GenerateSetupKey(DefaultSetupKeyName, SetupKeyReusable, DefaultSetupKeyDuration, []string{}, SetupKeyUnlimitedUsage, false) - key.Key = plainKey - return key } func Hash(s string) uint32 { diff --git a/management/server/setupkey_test.go b/management/server/setupkey_test.go index 5b01b44954a..224e3f4045a 100644 --- a/management/server/setupkey_test.go +++ b/management/server/setupkey_test.go @@ -2,8 +2,11 @@ package server import ( "context" + "crypto/sha256" + "encoding/base64" "fmt" "strconv" + "strings" "testing" "time" @@ -66,7 +69,7 @@ func TestDefaultAccountManager_SaveSetupKey(t *testing.T) { } assertKey(t, newKey, newKeyName, revoked, "reusable", 0, key.CreatedAt, key.ExpiresAt, - key.Id, time.Now().UTC(), autoGroups) + key.Id, time.Now().UTC(), autoGroups, false) // check the corresponding events that should have been generated ev := getEvent(t, account.Id, manager, activity.SetupKeyRevoked) @@ -183,7 +186,7 @@ func TestDefaultAccountManager_CreateSetupKey(t *testing.T) { assertKey(t, key, tCase.expectedKeyName, false, tCase.expectedType, tCase.expectedUsedTimes, tCase.expectedCreatedAt, tCase.expectedExpiresAt, strconv.Itoa(int(Hash(key.Key))), - tCase.expectedUpdatedAt, tCase.expectedGroups) + tCase.expectedUpdatedAt, tCase.expectedGroups, true) // check the corresponding events that should have been generated ev := getEvent(t, account.Id, manager, activity.SetupKeyCreated) @@ -239,10 +242,10 @@ func TestGenerateDefaultSetupKey(t *testing.T) { expectedExpiresAt := time.Now().UTC().Add(24 * 30 * time.Hour) var expectedAutoGroups []string - key := GenerateDefaultSetupKey() + key, plainKey := GenerateDefaultSetupKey() assertKey(t, key, expectedName, expectedRevoke, expectedType, expectedUsedTimes, expectedCreatedAt, - expectedExpiresAt, strconv.Itoa(int(Hash(key.Key))), expectedUpdatedAt, expectedAutoGroups) + expectedExpiresAt, strconv.Itoa(int(Hash(plainKey))), expectedUpdatedAt, expectedAutoGroups, false) } @@ -256,10 +259,10 @@ func TestGenerateSetupKey(t *testing.T) { expectedUpdatedAt := time.Now().UTC() var expectedAutoGroups []string - key, _ := GenerateSetupKey(expectedName, SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) + key, plain := GenerateSetupKey(expectedName, SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) assertKey(t, key, expectedName, expectedRevoke, expectedType, expectedUsedTimes, expectedCreatedAt, - expectedExpiresAt, strconv.Itoa(int(Hash(key.Key))), expectedUpdatedAt, expectedAutoGroups) + expectedExpiresAt, strconv.Itoa(int(Hash(plain))), expectedUpdatedAt, expectedAutoGroups, false) } @@ -299,7 +302,7 @@ func TestSetupKey_IsValid(t *testing.T) { func assertKey(t *testing.T, key *SetupKey, expectedName string, expectedRevoke bool, expectedType string, expectedUsedTimes int, expectedCreatedAt time.Time, expectedExpiresAt time.Time, expectedID string, - expectedUpdatedAt time.Time, expectedAutoGroups []string) { + expectedUpdatedAt time.Time, expectedAutoGroups []string, expectHashedKey bool) { t.Helper() if key.Name != expectedName { t.Errorf("expected setup key to have Name %v, got %v", expectedName, key.Name) @@ -329,13 +332,23 @@ func assertKey(t *testing.T, key *SetupKey, expectedName string, expectedRevoke t.Errorf("expected setup key to have CreatedAt ~ %v, got %v", expectedCreatedAt, key.CreatedAt) } - _, err := uuid.Parse(key.Key) - if err != nil { - t.Errorf("expected key to be a valid UUID, got %v, %v", key.Key, err) + if expectHashedKey { + _, err := uuid.Parse(key.Key) + if err != nil { + t.Errorf("expected new key to be a valid UUID, got %v, %v", key.Key, err) + } + } else { + if !isValidBase64SHA256(key.Key) { + t.Errorf("expected existing key to be hashed, got %v", key.Key) + } } - if key.Id != strconv.Itoa(int(Hash(key.Key))) { - t.Errorf("expected key Id t= %v, got %v", expectedID, key.Id) + if !strings.HasSuffix(key.KeySecret, "****") { + t.Errorf("expected key secret to be secure, got %v", key.Key) + } + + if key.Id != expectedID { + t.Errorf("expected key Id %v, got %v", expectedID, key.Id) } if len(key.AutoGroups) != len(expectedAutoGroups) { @@ -344,13 +357,26 @@ func assertKey(t *testing.T, key *SetupKey, expectedName string, expectedRevoke assert.ElementsMatch(t, key.AutoGroups, expectedAutoGroups, "expected key AutoGroups to be equal") } +func isValidBase64SHA256(encodedKey string) bool { + decoded, err := base64.StdEncoding.DecodeString(encodedKey) + if err != nil { + return false + } + + if len(decoded) != sha256.Size { + return false + } + + return true +} + func TestSetupKey_Copy(t *testing.T) { key, _ := GenerateSetupKey("key name", SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) keyCopy := key.Copy() assertKey(t, keyCopy, key.Name, key.Revoked, string(key.Type), key.UsedTimes, key.CreatedAt, key.ExpiresAt, key.Id, - key.UpdatedAt, key.AutoGroups) + key.UpdatedAt, key.AutoGroups, false) } diff --git a/management/server/sql_store.go b/management/server/sql_store.go index 06591865cff..27238d28e8a 100644 --- a/management/server/sql_store.go +++ b/management/server/sql_store.go @@ -469,7 +469,7 @@ func (s *SqlStore) GetAccountIDByPrivateDomain(ctx context.Context, lockStrength func (s *SqlStore) GetAccountBySetupKey(ctx context.Context, setupKey string) (*Account, error) { var key SetupKey - result := s.db.WithContext(ctx).Select("account_id").First(&key, keyQueryCondition, strings.ToUpper(setupKey)) + result := s.db.WithContext(ctx).Select("account_id").First(&key, keyQueryCondition, setupKey) if result.Error != nil { if errors.Is(result.Error, gorm.ErrRecordNotFound) { return nil, status.Errorf(status.NotFound, "account not found: index lookup failed") diff --git a/management/server/sql_store_test.go b/management/server/sql_store_test.go index fad5e81ea42..a1b5a1c06c0 100644 --- a/management/server/sql_store_test.go +++ b/management/server/sql_store_test.go @@ -71,7 +71,7 @@ func runLargeTest(t *testing.T, store Store) { if err != nil { t.Fatal(err) } - setupKey := GenerateDefaultSetupKey() + setupKey, _ := GenerateDefaultSetupKey() account.SetupKeys[setupKey.Key] = setupKey const numPerAccount = 6000 for n := 0; n < numPerAccount; n++ { @@ -81,7 +81,6 @@ func runLargeTest(t *testing.T, store Store) { peer := &nbpeer.Peer{ ID: peerID, Key: peerID, - SetupKey: "", IP: netIP, Name: peerID, DNSLabel: peerID, @@ -133,7 +132,7 @@ func runLargeTest(t *testing.T, store Store) { } account.NameServerGroups[nameserver.ID] = nameserver - setupKey := GenerateDefaultSetupKey() + setupKey, _ := GenerateDefaultSetupKey() account.SetupKeys[setupKey.Key] = setupKey } @@ -215,30 +214,28 @@ func TestSqlite_SaveAccount(t *testing.T) { assert.NoError(t, err) account := newAccountWithId(context.Background(), "account_id", "testuser", "") - setupKey := GenerateDefaultSetupKey() + setupKey, _ := GenerateDefaultSetupKey() account.SetupKeys[setupKey.Key] = setupKey account.Peers["testpeer"] = &nbpeer.Peer{ - Key: "peerkey", - SetupKey: "peerkeysetupkey", - IP: net.IP{127, 0, 0, 1}, - Meta: nbpeer.PeerSystemMeta{}, - Name: "peer name", - Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, + Key: "peerkey", + IP: net.IP{127, 0, 0, 1}, + Meta: nbpeer.PeerSystemMeta{}, + Name: "peer name", + Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, } err = store.SaveAccount(context.Background(), account) require.NoError(t, err) account2 := newAccountWithId(context.Background(), "account_id2", "testuser2", "") - setupKey = GenerateDefaultSetupKey() + setupKey, _ = GenerateDefaultSetupKey() account2.SetupKeys[setupKey.Key] = setupKey account2.Peers["testpeer2"] = &nbpeer.Peer{ - Key: "peerkey2", - SetupKey: "peerkeysetupkey2", - IP: net.IP{127, 0, 0, 2}, - Meta: nbpeer.PeerSystemMeta{}, - Name: "peer name 2", - Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, + Key: "peerkey2", + IP: net.IP{127, 0, 0, 2}, + Meta: nbpeer.PeerSystemMeta{}, + Name: "peer name 2", + Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, } err = store.SaveAccount(context.Background(), account2) @@ -297,15 +294,14 @@ func TestSqlite_DeleteAccount(t *testing.T) { }} account := newAccountWithId(context.Background(), "account_id", testUserID, "") - setupKey := GenerateDefaultSetupKey() + setupKey, _ := GenerateDefaultSetupKey() account.SetupKeys[setupKey.Key] = setupKey account.Peers["testpeer"] = &nbpeer.Peer{ - Key: "peerkey", - SetupKey: "peerkeysetupkey", - IP: net.IP{127, 0, 0, 1}, - Meta: nbpeer.PeerSystemMeta{}, - Name: "peer name", - Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, + Key: "peerkey", + IP: net.IP{127, 0, 0, 1}, + Meta: nbpeer.PeerSystemMeta{}, + Name: "peer name", + Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, } account.Users[testUserID] = user @@ -394,13 +390,12 @@ func TestSqlite_SavePeer(t *testing.T) { // save status of non-existing peer peer := &nbpeer.Peer{ - Key: "peerkey", - ID: "testpeer", - SetupKey: "peerkeysetupkey", - IP: net.IP{127, 0, 0, 1}, - Meta: nbpeer.PeerSystemMeta{Hostname: "testingpeer"}, - Name: "peer name", - Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, + Key: "peerkey", + ID: "testpeer", + IP: net.IP{127, 0, 0, 1}, + Meta: nbpeer.PeerSystemMeta{Hostname: "testingpeer"}, + Name: "peer name", + Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, } ctx := context.Background() err = store.SavePeer(ctx, account.Id, peer) @@ -453,13 +448,12 @@ func TestSqlite_SavePeerStatus(t *testing.T) { // save new status of existing peer account.Peers["testpeer"] = &nbpeer.Peer{ - Key: "peerkey", - ID: "testpeer", - SetupKey: "peerkeysetupkey", - IP: net.IP{127, 0, 0, 1}, - Meta: nbpeer.PeerSystemMeta{}, - Name: "peer name", - Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, + Key: "peerkey", + ID: "testpeer", + IP: net.IP{127, 0, 0, 1}, + Meta: nbpeer.PeerSystemMeta{}, + Name: "peer name", + Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, } err = store.SaveAccount(context.Background(), account) @@ -720,15 +714,14 @@ func newSqliteStore(t *testing.T) *SqlStore { func newAccount(store Store, id int) error { str := fmt.Sprintf("%s-%d", uuid.New().String(), id) account := newAccountWithId(context.Background(), str, str+"-testuser", "example.com") - setupKey := GenerateDefaultSetupKey() + setupKey, _ := GenerateDefaultSetupKey() account.SetupKeys[setupKey.Key] = setupKey account.Peers["p"+str] = &nbpeer.Peer{ - Key: "peerkey" + str, - SetupKey: "peerkeysetupkey", - IP: net.IP{127, 0, 0, 1}, - Meta: nbpeer.PeerSystemMeta{}, - Name: "peer name", - Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, + Key: "peerkey" + str, + IP: net.IP{127, 0, 0, 1}, + Meta: nbpeer.PeerSystemMeta{}, + Name: "peer name", + Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, } return store.SaveAccount(context.Background(), account) @@ -760,30 +753,28 @@ func TestPostgresql_SaveAccount(t *testing.T) { assert.NoError(t, err) account := newAccountWithId(context.Background(), "account_id", "testuser", "") - setupKey := GenerateDefaultSetupKey() + setupKey, _ := GenerateDefaultSetupKey() account.SetupKeys[setupKey.Key] = setupKey account.Peers["testpeer"] = &nbpeer.Peer{ - Key: "peerkey", - SetupKey: "peerkeysetupkey", - IP: net.IP{127, 0, 0, 1}, - Meta: nbpeer.PeerSystemMeta{}, - Name: "peer name", - Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, + Key: "peerkey", + IP: net.IP{127, 0, 0, 1}, + Meta: nbpeer.PeerSystemMeta{}, + Name: "peer name", + Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, } err = store.SaveAccount(context.Background(), account) require.NoError(t, err) account2 := newAccountWithId(context.Background(), "account_id2", "testuser2", "") - setupKey = GenerateDefaultSetupKey() + setupKey, _ = GenerateDefaultSetupKey() account2.SetupKeys[setupKey.Key] = setupKey account2.Peers["testpeer2"] = &nbpeer.Peer{ - Key: "peerkey2", - SetupKey: "peerkeysetupkey2", - IP: net.IP{127, 0, 0, 2}, - Meta: nbpeer.PeerSystemMeta{}, - Name: "peer name 2", - Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, + Key: "peerkey2", + IP: net.IP{127, 0, 0, 2}, + Meta: nbpeer.PeerSystemMeta{}, + Name: "peer name 2", + Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, } err = store.SaveAccount(context.Background(), account2) @@ -842,15 +833,14 @@ func TestPostgresql_DeleteAccount(t *testing.T) { }} account := newAccountWithId(context.Background(), "account_id", testUserID, "") - setupKey := GenerateDefaultSetupKey() + setupKey, _ := GenerateDefaultSetupKey() account.SetupKeys[setupKey.Key] = setupKey account.Peers["testpeer"] = &nbpeer.Peer{ - Key: "peerkey", - SetupKey: "peerkeysetupkey", - IP: net.IP{127, 0, 0, 1}, - Meta: nbpeer.PeerSystemMeta{}, - Name: "peer name", - Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, + Key: "peerkey", + IP: net.IP{127, 0, 0, 1}, + Meta: nbpeer.PeerSystemMeta{}, + Name: "peer name", + Status: &nbpeer.PeerStatus{Connected: true, LastSeen: time.Now().UTC()}, } account.Users[testUserID] = user @@ -921,13 +911,12 @@ func TestPostgresql_SavePeerStatus(t *testing.T) { // save new status of existing peer account.Peers["testpeer"] = &nbpeer.Peer{ - Key: "peerkey", - ID: "testpeer", - SetupKey: "peerkeysetupkey", - IP: net.IP{127, 0, 0, 1}, - Meta: nbpeer.PeerSystemMeta{}, - Name: "peer name", - Status: &nbpeer.PeerStatus{Connected: false, LastSeen: time.Now().UTC()}, + Key: "peerkey", + ID: "testpeer", + IP: net.IP{127, 0, 0, 1}, + Meta: nbpeer.PeerSystemMeta{}, + Name: "peer name", + Status: &nbpeer.PeerStatus{Connected: false, LastSeen: time.Now().UTC()}, } err = store.SaveAccount(context.Background(), account) From 926d224b7b5259ea3a338b6e2c58670062992793 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 24 Oct 2024 12:30:20 +0200 Subject: [PATCH 06/14] linter --- management/server/setupkey_test.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/management/server/setupkey_test.go b/management/server/setupkey_test.go index 224e3f4045a..3e74023e87b 100644 --- a/management/server/setupkey_test.go +++ b/management/server/setupkey_test.go @@ -332,14 +332,12 @@ func assertKey(t *testing.T, key *SetupKey, expectedName string, expectedRevoke t.Errorf("expected setup key to have CreatedAt ~ %v, got %v", expectedCreatedAt, key.CreatedAt) } - if expectHashedKey { + if expectHashedKey && !isValidBase64SHA256(key.Key) { + t.Errorf("expected key to be hashed, got %v", key.Key) + } else { _, err := uuid.Parse(key.Key) if err != nil { - t.Errorf("expected new key to be a valid UUID, got %v, %v", key.Key, err) - } - } else { - if !isValidBase64SHA256(key.Key) { - t.Errorf("expected existing key to be hashed, got %v", key.Key) + t.Errorf("expected key to be a valid UUID, got %v, %v", key.Key, err) } } From 1171afbc91ca77a70b106e59697559395bbb20a0 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 24 Oct 2024 12:44:15 +0200 Subject: [PATCH 07/14] create custom error types --- management/server/http/setupkeys_handler.go | 6 +++--- management/server/setupkey.go | 6 +++--- management/server/status/error.go | 10 ++++++++++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/management/server/http/setupkeys_handler.go b/management/server/http/setupkeys_handler.go index 020b1581403..f3e8bdbbcc5 100644 --- a/management/server/http/setupkeys_handler.go +++ b/management/server/http/setupkeys_handler.go @@ -102,7 +102,7 @@ func (h *SetupKeysHandler) GetSetupKey(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) keyID := vars["keyId"] if len(keyID) == 0 { - util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid key ID"), w) + util.WriteError(r.Context(), status.NewInvalidKeyIDError(), w) return } @@ -127,7 +127,7 @@ func (h *SetupKeysHandler) UpdateSetupKey(w http.ResponseWriter, r *http.Request vars := mux.Vars(r) keyID := vars["keyId"] if len(keyID) == 0 { - util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid key ID"), w) + util.WriteError(r.Context(), status.NewInvalidKeyIDError(), w) return } @@ -196,7 +196,7 @@ func (h *SetupKeysHandler) DeleteSetupKey(w http.ResponseWriter, r *http.Request vars := mux.Vars(r) keyID := vars["keyId"] if len(keyID) == 0 { - util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid key ID"), w) + util.WriteError(r.Context(), status.NewInvalidKeyIDError(), w) return } diff --git a/management/server/setupkey.go b/management/server/setupkey.go index 53c18adacbb..9de143383f1 100644 --- a/management/server/setupkey.go +++ b/management/server/setupkey.go @@ -353,7 +353,7 @@ func (am *DefaultAccountManager) ListSetupKeys(ctx context.Context, accountID, u } if !user.IsAdminOrServiceUser() || user.AccountID != accountID { - return nil, status.Errorf(status.Unauthorized, "only users with admin power can view setup keys") + return nil, status.NewUnauthorizedToViewSetupKeysError() } setupKeys, err := am.Store.GetAccountSetupKeys(ctx, LockingStrengthShare, accountID) @@ -372,7 +372,7 @@ func (am *DefaultAccountManager) GetSetupKey(ctx context.Context, accountID, use } if !user.IsAdminOrServiceUser() || user.AccountID != accountID { - return nil, status.Errorf(status.Unauthorized, "only users with admin power can view setup keys") + return nil, status.NewUnauthorizedToViewSetupKeysError() } setupKey, err := am.Store.GetSetupKeyByID(ctx, LockingStrengthShare, keyID, accountID) @@ -396,7 +396,7 @@ func (am *DefaultAccountManager) DeleteSetupKey(ctx context.Context, accountID, } if !user.IsAdminOrServiceUser() || user.AccountID != accountID { - return status.Errorf(status.Unauthorized, "only users with admin power can view setup keys") + return status.NewUnauthorizedToViewSetupKeysError() } return am.Store.DeleteSetupKey(ctx, accountID, keyID) diff --git a/management/server/status/error.go b/management/server/status/error.go index 29d185216d8..e9fc8c15ef9 100644 --- a/management/server/status/error.go +++ b/management/server/status/error.go @@ -114,3 +114,13 @@ func NewGetAccountFromStoreError(err error) error { func NewGetUserFromStoreError() error { return Errorf(Internal, "issue getting user from store") } + +// NewInvalidKeyIDError creates a new Error with InvalidArgument type for an issue getting a setup key +func NewInvalidKeyIDError() error { + return Errorf(InvalidArgument, "invalid key ID") +} + +// NewUnauthorizedToViewSetupKeysError creates a new Error with Unauthorized type for an issue getting a setup key +func NewUnauthorizedToViewSetupKeysError() error { + return Errorf(Unauthorized, "only users with admin power can view setup keys") +} From 93927bc343cb2eab0452e2476801d1ba9aa3cb15 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 24 Oct 2024 13:03:59 +0200 Subject: [PATCH 08/14] fix store tests --- management/server/sql_store_test.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/management/server/sql_store_test.go b/management/server/sql_store_test.go index a1b5a1c06c0..b371e231319 100644 --- a/management/server/sql_store_test.go +++ b/management/server/sql_store_test.go @@ -2,6 +2,8 @@ package server import ( "context" + "crypto/sha256" + b64 "encoding/base64" "fmt" "math/rand" "net" @@ -1107,12 +1109,17 @@ func TestSqlite_GetSetupKeyBySecret(t *testing.T) { existingAccountID := "bf1c8084-ba50-4ce7-9439-34653001fc3b" + plainKey := "A2C8E62B-38F5-4553-B31E-DD66C696CEBB" + hashedKey := sha256.Sum256([]byte(plainKey)) + encodedHashedKey := b64.StdEncoding.EncodeToString(hashedKey[:]) + _, err = store.GetAccount(context.Background(), existingAccountID) require.NoError(t, err) - setupKey, err := store.GetSetupKeyBySecret(context.Background(), LockingStrengthShare, "A2C8E62B-38F5-4553-B31E-DD66C696CEBB") + setupKey, err := store.GetSetupKeyBySecret(context.Background(), LockingStrengthShare, encodedHashedKey) require.NoError(t, err) - assert.Equal(t, "A2C8E62B-38F5-4553-B31E-DD66C696CEBB", setupKey.Key) + assert.Equal(t, encodedHashedKey, setupKey.Key) + assert.Equal(t, hiddenKey(plainKey, 4), setupKey.KeySecret) assert.Equal(t, "bf1c8084-ba50-4ce7-9439-34653001fc3b", setupKey.AccountID) assert.Equal(t, "Default key", setupKey.Name) } @@ -1127,24 +1134,28 @@ func TestSqlite_incrementSetupKeyUsage(t *testing.T) { existingAccountID := "bf1c8084-ba50-4ce7-9439-34653001fc3b" + plainKey := "A2C8E62B-38F5-4553-B31E-DD66C696CEBB" + hashedKey := sha256.Sum256([]byte(plainKey)) + encodedHashedKey := b64.StdEncoding.EncodeToString(hashedKey[:]) + _, err = store.GetAccount(context.Background(), existingAccountID) require.NoError(t, err) - setupKey, err := store.GetSetupKeyBySecret(context.Background(), LockingStrengthShare, "A2C8E62B-38F5-4553-B31E-DD66C696CEBB") + setupKey, err := store.GetSetupKeyBySecret(context.Background(), LockingStrengthShare, encodedHashedKey) require.NoError(t, err) assert.Equal(t, 0, setupKey.UsedTimes) err = store.IncrementSetupKeyUsage(context.Background(), setupKey.Id) require.NoError(t, err) - setupKey, err = store.GetSetupKeyBySecret(context.Background(), LockingStrengthShare, "A2C8E62B-38F5-4553-B31E-DD66C696CEBB") + setupKey, err = store.GetSetupKeyBySecret(context.Background(), LockingStrengthShare, encodedHashedKey) require.NoError(t, err) assert.Equal(t, 1, setupKey.UsedTimes) err = store.IncrementSetupKeyUsage(context.Background(), setupKey.Id) require.NoError(t, err) - setupKey, err = store.GetSetupKeyBySecret(context.Background(), LockingStrengthShare, "A2C8E62B-38F5-4553-B31E-DD66C696CEBB") + setupKey, err = store.GetSetupKeyBySecret(context.Background(), LockingStrengthShare, encodedHashedKey) require.NoError(t, err) assert.Equal(t, 2, setupKey.UsedTimes) } From ec9af45938dd01b304198562285e9d8897479222 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 24 Oct 2024 13:26:10 +0200 Subject: [PATCH 09/14] fix setup key tests --- management/server/setupkey_test.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/management/server/setupkey_test.go b/management/server/setupkey_test.go index 3e74023e87b..2ed8aef95c6 100644 --- a/management/server/setupkey_test.go +++ b/management/server/setupkey_test.go @@ -69,7 +69,7 @@ func TestDefaultAccountManager_SaveSetupKey(t *testing.T) { } assertKey(t, newKey, newKeyName, revoked, "reusable", 0, key.CreatedAt, key.ExpiresAt, - key.Id, time.Now().UTC(), autoGroups, false) + key.Id, time.Now().UTC(), autoGroups, true) // check the corresponding events that should have been generated ev := getEvent(t, account.Id, manager, activity.SetupKeyRevoked) @@ -186,7 +186,7 @@ func TestDefaultAccountManager_CreateSetupKey(t *testing.T) { assertKey(t, key, tCase.expectedKeyName, false, tCase.expectedType, tCase.expectedUsedTimes, tCase.expectedCreatedAt, tCase.expectedExpiresAt, strconv.Itoa(int(Hash(key.Key))), - tCase.expectedUpdatedAt, tCase.expectedGroups, true) + tCase.expectedUpdatedAt, tCase.expectedGroups, false) // check the corresponding events that should have been generated ev := getEvent(t, account.Id, manager, activity.SetupKeyCreated) @@ -245,7 +245,7 @@ func TestGenerateDefaultSetupKey(t *testing.T) { key, plainKey := GenerateDefaultSetupKey() assertKey(t, key, expectedName, expectedRevoke, expectedType, expectedUsedTimes, expectedCreatedAt, - expectedExpiresAt, strconv.Itoa(int(Hash(plainKey))), expectedUpdatedAt, expectedAutoGroups, false) + expectedExpiresAt, strconv.Itoa(int(Hash(plainKey))), expectedUpdatedAt, expectedAutoGroups, true) } @@ -262,7 +262,7 @@ func TestGenerateSetupKey(t *testing.T) { key, plain := GenerateSetupKey(expectedName, SetupKeyOneOff, time.Hour, []string{}, SetupKeyUnlimitedUsage, false) assertKey(t, key, expectedName, expectedRevoke, expectedType, expectedUsedTimes, expectedCreatedAt, - expectedExpiresAt, strconv.Itoa(int(Hash(plain))), expectedUpdatedAt, expectedAutoGroups, false) + expectedExpiresAt, strconv.Itoa(int(Hash(plain))), expectedUpdatedAt, expectedAutoGroups, true) } @@ -332,8 +332,10 @@ func assertKey(t *testing.T, key *SetupKey, expectedName string, expectedRevoke t.Errorf("expected setup key to have CreatedAt ~ %v, got %v", expectedCreatedAt, key.CreatedAt) } - if expectHashedKey && !isValidBase64SHA256(key.Key) { - t.Errorf("expected key to be hashed, got %v", key.Key) + if expectHashedKey { + if !isValidBase64SHA256(key.Key) { + t.Errorf("expected key to be hashed, got %v", key.Key) + } } else { _, err := uuid.Parse(key.Key) if err != nil { @@ -374,7 +376,7 @@ func TestSetupKey_Copy(t *testing.T) { keyCopy := key.Copy() assertKey(t, keyCopy, key.Name, key.Revoked, string(key.Type), key.UsedTimes, key.CreatedAt, key.ExpiresAt, key.Id, - key.UpdatedAt, key.AutoGroups, false) + key.UpdatedAt, key.AutoGroups, true) } From 348c33feaaa4f1f2bb9c4e390a6a316e1c28fa8a Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 24 Oct 2024 15:55:30 +0200 Subject: [PATCH 10/14] update api docs --- management/server/http/api/openapi.yml | 5 ++--- management/server/http/api/types.gen.go | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/management/server/http/api/openapi.yml b/management/server/http/api/openapi.yml index e8ebe6b86d2..9b4592ccf10 100644 --- a/management/server/http/api/openapi.yml +++ b/management/server/http/api/openapi.yml @@ -530,10 +530,9 @@ components: type: string example: reusable expires_in: - description: Expiration time in seconds + description: Expiration time in seconds, 0 will mean the key never expires type: integer - minimum: 86400 - maximum: 31536000 + minimum: 0 example: 86400 revoked: description: Setup key revocation status diff --git a/management/server/http/api/types.gen.go b/management/server/http/api/types.gen.go index e2870d5d8ef..c1ef1ba2122 100644 --- a/management/server/http/api/types.gen.go +++ b/management/server/http/api/types.gen.go @@ -1101,7 +1101,7 @@ type SetupKeyRequest struct { // Ephemeral Indicate that the peer will be ephemeral or not Ephemeral *bool `json:"ephemeral,omitempty"` - // ExpiresIn Expiration time in seconds + // ExpiresIn Expiration time in seconds, 0 will mean the key never expires ExpiresIn int `json:"expires_in"` // Name Setup Key name From 4d65752b35d2aae79d9519bf4ccf6f9ac0d629c0 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 24 Oct 2024 17:23:51 +0200 Subject: [PATCH 11/14] add setup key delete test --- management/server/http/setupkeys_handler_test.go | 16 ++++++++++++++++ management/server/mock_server/account_mock.go | 7 +++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/management/server/http/setupkeys_handler_test.go b/management/server/http/setupkeys_handler_test.go index c38fce4e83b..09256d0ea5e 100644 --- a/management/server/http/setupkeys_handler_test.go +++ b/management/server/http/setupkeys_handler_test.go @@ -67,6 +67,13 @@ func initSetupKeysTestMetaData(defaultKey *server.SetupKey, newKey *server.Setup ListSetupKeysFunc: func(_ context.Context, accountID, userID string) ([]*server.SetupKey, error) { return []*server.SetupKey{defaultKey}, nil }, + + DeleteSetupKeyFunc: func(_ context.Context, accountID, userID, keyID string) error { + if keyID == defaultKey.Id { + return nil + } + return status.Errorf(status.NotFound, "key %s not found", keyID) + }, }, claimsExtractor: jwtclaims.NewClaimsExtractor( jwtclaims.WithFromRequestContext(func(r *http.Request) jwtclaims.AuthorizationClaims { @@ -153,6 +160,14 @@ func TestSetupKeysHandlers(t *testing.T) { expectedBody: true, expectedSetupKey: toResponseBody(updatedDefaultSetupKey), }, + { + name: "Delete Setup Key", + requestType: http.MethodDelete, + requestPath: "/api/setup-keys/" + defaultSetupKey.Id, + requestBody: bytes.NewBuffer([]byte("")), + expectedStatus: http.StatusOK, + expectedBody: false, + }, } handler := initSetupKeysTestMetaData(defaultSetupKey, newSetupKey, updatedDefaultSetupKey, adminUser) @@ -167,6 +182,7 @@ func TestSetupKeysHandlers(t *testing.T) { router.HandleFunc("/api/setup-keys", handler.CreateSetupKey).Methods("POST", "OPTIONS") router.HandleFunc("/api/setup-keys/{keyId}", handler.GetSetupKey).Methods("GET", "OPTIONS") router.HandleFunc("/api/setup-keys/{keyId}", handler.UpdateSetupKey).Methods("PUT", "OPTIONS") + router.HandleFunc("/api/setup-keys/{keyId}", handler.DeleteSetupKey).Methods("DELETE", "OPTIONS") router.ServeHTTP(recorder, req) res := recorder.Result() diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index 05163f51493..d7139bb2a5f 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -109,11 +109,14 @@ type MockAccountManager struct { GetAccountByIDFunc func(ctx context.Context, accountID string, userID string) (*server.Account, error) GetUserByIDFunc func(ctx context.Context, id string) (*server.User, error) GetAccountSettingsFunc func(ctx context.Context, accountID string, userID string) (*server.Settings, error) + DeleteSetupKeyFunc func(ctx context.Context, accountID, userID, keyID string) error } func (am *MockAccountManager) DeleteSetupKey(ctx context.Context, accountID, userID, keyID string) error { - // TODO implement me - panic("implement me") + if am.DeleteSetupKeyFunc != nil { + return am.DeleteSetupKeyFunc(ctx, accountID, userID, keyID) + } + return status.Errorf(codes.Unimplemented, "method DeleteSetupKey is not implemented") } func (am *MockAccountManager) SyncAndMarkPeer(ctx context.Context, accountID string, peerPubKey string, meta nbpeer.PeerSystemMeta, realIP net.IP) (*nbpeer.Peer, *server.NetworkMap, []*posture.Checks, error) { From f392043e83e525e76c119ecd891a260d1b45cdd8 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 24 Oct 2024 18:39:19 +0200 Subject: [PATCH 12/14] update api validation --- management/server/http/setupkeys_handler.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/management/server/http/setupkeys_handler.go b/management/server/http/setupkeys_handler.go index f3e8bdbbcc5..31859f59bf0 100644 --- a/management/server/http/setupkeys_handler.go +++ b/management/server/http/setupkeys_handler.go @@ -61,9 +61,8 @@ func (h *SetupKeysHandler) CreateSetupKey(w http.ResponseWriter, r *http.Request expiresIn := time.Duration(req.ExpiresIn) * time.Second - day := time.Hour * 24 - if expiresIn < day || expiresIn == 0 { - util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "expiresIn should be at least 1 day"), w) + if expiresIn < 0 { + util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "expiresIn can not be in the past"), w) return } From d460d47a1293eacba95559add7c1327a6dbd0875 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Thu, 24 Oct 2024 18:42:11 +0200 Subject: [PATCH 13/14] update api validation --- management/server/setupkey.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/management/server/setupkey.go b/management/server/setupkey.go index 9de143383f1..6103ae31b4e 100644 --- a/management/server/setupkey.go +++ b/management/server/setupkey.go @@ -229,11 +229,6 @@ func (am *DefaultAccountManager) CreateSetupKey(ctx context.Context, accountID s unlock := am.Store.AcquireWriteLockByUID(ctx, accountID) defer unlock() - keyDuration := DefaultSetupKeyDuration - if expiresIn != 0 { - keyDuration = expiresIn - } - account, err := am.Store.GetAccount(ctx, accountID) if err != nil { return nil, err @@ -243,7 +238,7 @@ func (am *DefaultAccountManager) CreateSetupKey(ctx context.Context, accountID s return nil, err } - setupKey, plainKey := GenerateSetupKey(keyName, keyType, keyDuration, autoGroups, usageLimit, ephemeral) + setupKey, plainKey := GenerateSetupKey(keyName, keyType, expiresIn, autoGroups, usageLimit, ephemeral) account.SetupKeys[setupKey.Key] = setupKey err = am.Store.SaveAccount(ctx, account) if err != nil { From 9bc24dac881c364599a6db24cd1943a642058b48 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Fri, 25 Oct 2024 16:12:27 +0200 Subject: [PATCH 14/14] send event on key deletion --- management/server/activity/codes.go | 3 +++ management/server/setupkey.go | 14 +++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/management/server/activity/codes.go b/management/server/activity/codes.go index 188494241c6..603260dbcb2 100644 --- a/management/server/activity/codes.go +++ b/management/server/activity/codes.go @@ -146,6 +146,8 @@ const ( AccountPeerInactivityExpirationEnabled Activity = 65 AccountPeerInactivityExpirationDisabled Activity = 66 AccountPeerInactivityExpirationDurationUpdated Activity = 67 + + SetupKeyDeleted Activity = 68 ) var activityMap = map[Activity]Code{ @@ -219,6 +221,7 @@ var activityMap = map[Activity]Code{ AccountPeerInactivityExpirationEnabled: {"Account peer inactivity expiration enabled", "account.peer.inactivity.expiration.enable"}, AccountPeerInactivityExpirationDisabled: {"Account peer inactivity expiration disabled", "account.peer.inactivity.expiration.disable"}, AccountPeerInactivityExpirationDurationUpdated: {"Account peer inactivity expiration duration updated", "account.peer.inactivity.expiration.update"}, + SetupKeyDeleted: {"Setup key deleted", "setupkey.delete"}, } // StringCode returns a string code of the activity diff --git a/management/server/setupkey.go b/management/server/setupkey.go index 6103ae31b4e..43b6e02c936 100644 --- a/management/server/setupkey.go +++ b/management/server/setupkey.go @@ -394,7 +394,19 @@ func (am *DefaultAccountManager) DeleteSetupKey(ctx context.Context, accountID, return status.NewUnauthorizedToViewSetupKeysError() } - return am.Store.DeleteSetupKey(ctx, accountID, keyID) + deletedSetupKey, err := am.Store.GetSetupKeyByID(ctx, LockingStrengthShare, keyID, accountID) + if err != nil { + return fmt.Errorf("failed to get setup key: %w", err) + } + + err = am.Store.DeleteSetupKey(ctx, accountID, keyID) + if err != nil { + return fmt.Errorf("failed to delete setup key: %w", err) + } + + am.StoreEvent(ctx, userID, keyID, accountID, activity.SetupKeyDeleted, deletedSetupKey.EventMeta()) + + return nil } func validateSetupKeyAutoGroups(account *Account, autoGroups []string) error {