From ae7ad5e0c8a31638752c2838257353d31be874f2 Mon Sep 17 00:00:00 2001 From: dmitri-netbird Date: Wed, 7 Oct 2026 10:21:24 +0200 Subject: [PATCH] [management] Return permission-denied error when setup key is expired or invalid (#8086) * return permission-denied error when setup key is expired or invalid Signed-off-by: Dmitri Dolguikh * fix tests, don't return permission-denied on internal errors Signed-off-by: Dmitri Dolguikh * return not found error when key fetch failed Signed-off-by: Dmitri Dolguikh * normalise errors returned from each call-site of GetSetupKeyBySecret Signed-off-by: Dmitri Dolguikh * return PermissionDenied after isValid() check failure Signed-off-by: Dmitri Dolguikh --------- Signed-off-by: Dmitri Dolguikh --- management/server/peer.go | 8 ++++---- management/server/peer_test.go | 6 +++++- management/server/store/sql_store_setup_key.go | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/management/server/peer.go b/management/server/peer.go index 8d99bebb0..f3a5ebf46 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -710,11 +710,11 @@ func (am *DefaultAccountManager) handleUserAddedPeer(ctx context.Context, accoun func (am *DefaultAccountManager) handleSetupKeyAddedPeer(ctx context.Context, encodedHashedKey string, peer *nbpeer.Peer, opEvent *activity.Event, config *peerAddAuthConfig) error { sk, err := am.Store.GetSetupKeyBySecret(ctx, store.LockingStrengthNone, encodedHashedKey) if err != nil { - return status.Errorf(status.NotFound, "couldn't add peer: setup key is invalid") + return err } if !sk.IsValid() { - return status.Errorf(status.NotFound, "couldn't add peer: setup key is invalid") + return status.Errorf(status.PermissionDenied, "couldn't add peer: setup key is invalid") } if !sk.AllowExtraDNSLabels && len(peer.ExtraDNSLabels) > 0 { @@ -915,12 +915,12 @@ func (am *DefaultAccountManager) AddPeer(ctx context.Context, accountID, setupKe case addedBySetupKey: sk, err := transaction.GetSetupKeyBySecret(ctx, store.LockingStrengthUpdate, encodedHashedKey) if err != nil { - return fmt.Errorf("failed to get setup key: %w", err) + return err } // we validate at the end to not block the setup key for too long if !sk.IsValid() { - return status.Errorf(status.PreconditionFailed, "couldn't add peer: setup key is invalid") + return status.Errorf(status.PermissionDenied, "couldn't add peer: setup key is invalid") } err = transaction.IncrementSetupKeyUsage(ctx, peerAddConfig.SetupKeyID) diff --git a/management/server/peer_test.go b/management/server/peer_test.go index ec4f0ef01..fa3be9a58 100644 --- a/management/server/peer_test.go +++ b/management/server/peer_test.go @@ -1495,7 +1495,7 @@ func Test_RegisterPeerBySetupKey(t *testing.T) { name: "Absent setup key", existingSetupKeyID: "AAAAAAAA-38F5-4553-B31E-DD66C696CEBB", expectAddPeerError: true, - errorType: status.NotFound, + errorType: status.PermissionDenied, expectedErrorMsgSubstring: "couldn't add peer: setup key is invalid", }, } @@ -2642,6 +2642,8 @@ func TestHandleSetupKeyAddedPeer(t *testing.T) { err = manager.handleSetupKeyAddedPeer(context.Background(), encodedHashedKey, peer, opEvent, config) require.Error(t, err) + require.IsType(t, &status.Error{}, err) + require.Equal(t, err.(*status.Error).ErrorType, status.PermissionDenied) assert.Contains(t, err.Error(), "setup key is invalid") }) @@ -2662,6 +2664,8 @@ func TestHandleSetupKeyAddedPeer(t *testing.T) { err = manager.handleSetupKeyAddedPeer(context.Background(), encodedHashedKey, peer, opEvent, config) require.Error(t, err) + require.IsType(t, &status.Error{}, err) + require.Equal(t, err.(*status.Error).ErrorType, status.PermissionDenied) assert.Contains(t, err.Error(), "setup key is invalid") }) diff --git a/management/server/store/sql_store_setup_key.go b/management/server/store/sql_store_setup_key.go index 3857f6e8c..ac42ef725 100644 --- a/management/server/store/sql_store_setup_key.go +++ b/management/server/store/sql_store_setup_key.go @@ -127,7 +127,7 @@ func (s *SqlStore) GetSetupKeyBySecret(ctx context.Context, lockStrength Locking if result.Error != nil { if errors.Is(result.Error, gorm.ErrRecordNotFound) { - return nil, status.Errorf(status.PreconditionFailed, "setup key not found") + return nil, status.Errorf(status.PermissionDenied, "couldn't add peer: setup key is invalid") } log.WithContext(ctx).Errorf("failed to get setup key by secret from store: %v", result.Error) return nil, status.Errorf(status.Internal, "failed to get setup key by secret from store")