sendnrw/netbird
2
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2026-09-06 15:01:28 +02:00
Code Issues Packages Projects Releases Wiki Activity

update account isolation

Browse Source
This commit is contained in:
pascal
2026-04-20 16:33:24 +02:00
parent 0b04c0d03b
commit 470307079b
13 changed files with 190 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
management/server/account.go
View File

@@ -957,6 +957,10 @@ func (am *DefaultAccountManager) lookupUserInCache(ctx context.Context, userID s
return nil, err
}
if user.AccountID != accountID {
return nil, nil
}
key := user.IntegrationReference.CacheKey(accountID, userID)
ud, err := am.externalCacheManager.Get(am.ctx, key)
if err != nil {

16
management/server/http/handlers/accounts/accounts_handler.go
View File

@@ -229,10 +229,9 @@ func (h *handler) updateAccountRequestSettings(req api.PutApiAccountsAccountIdJS
// updateAccount is HTTP PUT handler that updates the provided account. Updates only account settings (server.Settings)
func (h *handler) updateAccount(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) {
vars := mux.Vars(r)
accountID := vars["accountId"]
if len(accountID) == 0 {
util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid accountID ID"), w)
accountID := mux.Vars(r)["accountId"]
if accountID != userAuth.AccountId {
util.WriteError(r.Context(), status.Errorf(status.PermissionDenied, "account ID mismatch"), w)
return
}
@@ -294,14 +293,13 @@ func (h *handler) updateAccount(w http.ResponseWriter, r *http.Request, userAuth
// deleteAccount is a HTTP DELETE handler to delete an account
func (h *handler) deleteAccount(w http.ResponseWriter, r *http.Request, userAuth *auth.UserAuth) {
vars := mux.Vars(r)
targetAccountID := vars["accountId"]
if len(targetAccountID) == 0 {
util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "invalid account ID"), w)
accountID := mux.Vars(r)["accountId"]
if accountID != userAuth.AccountId {
util.WriteError(r.Context(), status.Errorf(status.PermissionDenied, "account ID mismatch"), w)
return
}
err := h.accountManager.DeleteAccount(r.Context(), targetAccountID, userAuth.UserId)
err := h.accountManager.DeleteAccount(r.Context(), userAuth.AccountId, userAuth.UserId)
if err != nil {
util.WriteError(r.Context(), err, w)
return

5
management/server/http/handlers/users/users_handler.go
View File

@@ -66,6 +66,11 @@ func (h *handler) updateUser(w http.ResponseWriter, r *http.Request, userAuth *a
return
}
if existingUser.AccountID != userAuth.AccountId {
util.WriteError(r.Context(), status.Errorf(status.PermissionDenied, "user not found"), w)
return
}
req := &api.PutApiUsersUserIdJSONRequestBody{}
err = json.NewDecoder(r.Body).Decode(&req)
if err != nil {

65
management/server/http/testing/integration/accounts_handler_integration_test.go
View File

@@ -233,6 +233,71 @@ func Test_Accounts_Update(t *testing.T) {
}
}
func Test_Accounts_Update_CrossAccountAttack(t *testing.T) {
t.Run("Other user attempts to update testAccount via URL", func(t *testing.T) {
apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/accounts.sql", nil, false)
body, err := json.Marshal(&api.AccountRequest{
Settings: api.AccountSettings{
PeerLoginExpirationEnabled: false,
PeerLoginExpiration: 86400,
},
})
if err != nil {
t.Fatalf("Failed to marshal request body: %v", err)
}
// OtherUserId belongs to otherAccountId, but we target testAccountId in URL
req := testing_tools.BuildRequest(t, body, http.MethodPut, "/api/accounts/"+testing_tools.TestAccountId, testing_tools.OtherUserId)
recorder := httptest.NewRecorder()
apiHandler.ServeHTTP(recorder, req)
assert.NotEqual(t, http.StatusOK, recorder.Code, "cross-account update must be rejected")
})
}
func Test_Accounts_Delete(t *testing.T) {
users := []struct {
name string
userId string
expectResponse bool
}{
{"Regular user", testing_tools.TestUserId, false},
{"Admin user", testing_tools.TestAdminId, false},
{"Owner user", testing_tools.TestOwnerId, true},
{"Regular service user", testing_tools.TestServiceUserId, false},
{"Admin service user", testing_tools.TestServiceAdminId, false},
{"Blocked user", testing_tools.BlockedUserId, false},
{"Other user", testing_tools.OtherUserId, false},
{"Invalid token", testing_tools.InvalidToken, false},
}
for _, user := range users {
t.Run(user.name+" - Delete account", func(t *testing.T) {
apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/accounts.sql", nil, false)
req := testing_tools.BuildRequest(t, []byte{}, http.MethodDelete, "/api/accounts/"+testing_tools.TestAccountId, user.userId)
recorder := httptest.NewRecorder()
apiHandler.ServeHTTP(recorder, req)
testing_tools.ReadResponse(t, recorder, http.StatusOK, user.expectResponse)
})
}
}
func Test_Accounts_Delete_CrossAccountAttack(t *testing.T) {
t.Run("Other user attempts to delete testAccount via URL", func(t *testing.T) {
apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/accounts.sql", nil, false)
// OtherUserId belongs to otherAccountId, but we target testAccountId in URL
req := testing_tools.BuildRequest(t, []byte{}, http.MethodDelete, "/api/accounts/"+testing_tools.TestAccountId, testing_tools.OtherUserId)
recorder := httptest.NewRecorder()
apiHandler.ServeHTTP(recorder, req)
assert.NotEqual(t, http.StatusOK, recorder.Code, "cross-account delete must be rejected")
})
}
func stringPointer(s string) *string {
return &s
}

32
management/server/http/testing/integration/users_handler_integration_test.go
View File

@@ -637,6 +637,38 @@ func Test_PATs_Create(t *testing.T) {
}
}
func Test_Users_Update_CrossAccountAttack(t *testing.T) {
t.Run("Admin attempts to update user from other account", func(t *testing.T) {
apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/users_integration.sql", nil, false)
body, _ := json.Marshal(&api.UserRequest{
Role: "user",
AutoGroups: []string{},
IsBlocked: true,
})
// TestAdminId belongs to testAccountId, but targets otherUserId which belongs to otherAccountId
req := testing_tools.BuildRequest(t, body, http.MethodPut, "/api/users/otherUserId", testing_tools.TestAdminId)
recorder := httptest.NewRecorder()
apiHandler.ServeHTTP(recorder, req)
assert.NotEqual(t, http.StatusOK, recorder.Code, "cross-account user update must be rejected")
})
}
func Test_Users_Delete_CrossAccountAttack(t *testing.T) {
t.Run("Admin attempts to delete service user from other account", func(t *testing.T) {
apiHandler, _, _ := channel.BuildApiBlackBoxWithDBState(t, "../testdata/users_integration.sql", nil, false)
// TestAdminId belongs to testAccountId, but targets otherServiceUserId which belongs to otherAccountId
req := testing_tools.BuildRequest(t, []byte{}, http.MethodDelete, "/api/users/otherServiceUserId", testing_tools.TestAdminId)
recorder := httptest.NewRecorder()
apiHandler.ServeHTTP(recorder, req)
assert.NotEqual(t, http.StatusOK, recorder.Code, "cross-account user delete must be rejected")
})
}
func Test_PATs_Delete(t *testing.T) {
users := []struct {
name string

1
management/server/http/testing/testdata/users_integration.sql vendored
View File

@@ -16,6 +16,7 @@ INSERT INTO users VALUES('testServiceAdminId','testAccountId','admin',1,0,'testS
INSERT INTO users VALUES('blockedUserId','testAccountId','admin',0,0,'','[]',1,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,'');
INSERT INTO users VALUES('otherUserId','otherAccountId','admin',0,0,'','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,'');
INSERT INTO users VALUES('deletableServiceUserId','testAccountId','user',1,0,'deletableServiceUser','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,'');
INSERT INTO users VALUES('otherServiceUserId','otherAccountId','user',1,0,'otherServiceUser','[]',0,NULL,'2024-10-02 16:01:38.000000000+00:00','api',0,'');
INSERT INTO "groups" VALUES('testGroupId','testAccountId','testGroupName','api','["testPeerId"]',0,'');
INSERT INTO "groups" VALUES('newGroupId','testAccountId','newGroupName','api','[]',0,'');
INSERT INTO setup_keys VALUES('testKeyId','testAccountId','testKey','testK****','existingKey','one-off','2021-08-19 20:46:20.000000000+00:00','2321-09-18 20:46:20.000000000+00:00','2021-08-19 20:46:20.000000000+00:00',0,0,NULL,'["testGroupId"]',1,0);

4
management/server/http/testing/testing_tools/tools.go
View File

@@ -106,6 +106,10 @@ func ReadResponse(t *testing.T, recorder *httptest.ResponseRecorder, expectedSta
}
if !expectResponse {
if recorder.Code == http.StatusOK || recorder.Code == http.StatusCreated {
t.Fatalf("expected unauthorized/error status code but got %d, content: %s",
recorder.Code, string(content))
}
return nil, false
}

4
management/server/store/sql_store.go
View File

@@ -943,8 +943,8 @@ func (s *SqlStore) GetAccountUserInvites(ctx context.Context, lockStrength Locki
}
// DeleteUserInvite deletes a user invite by its ID
func (s *SqlStore) DeleteUserInvite(ctx context.Context, inviteID string) error {
result := s.db.Delete(&types.UserInviteRecord{}, idQueryCondition, inviteID)
func (s *SqlStore) DeleteUserInvite(ctx context.Context, accountID, inviteID string) error {
result := s.db.Delete(&types.UserInviteRecord{}, accountAndIDQueryCondition, accountID, inviteID)
if result.Error != nil {
log.WithContext(ctx).Errorf("failed to delete user invite from store: %s", result.Error)
return status.Errorf(status.Internal, "failed to delete user invite from store")

4
management/server/store/sql_store_user_invite_test.go
View File

@@ -298,7 +298,7 @@ func TestSqlStore_DeleteUserInvite(t *testing.T) {
require.NoError(t, err)
// Delete the invite
err = store.DeleteUserInvite(ctx, invite.ID)
err = store.DeleteUserInvite(ctx, invite.AccountID, invite.ID)
require.NoError(t, err)
// Verify invite is deleted
@@ -346,7 +346,7 @@ func TestSqlStore_DeleteUserInvite_NonExistent(t *testing.T) {
ctx := context.Background()
// Deleting a non-existent invite should not return an error
err := store.DeleteUserInvite(ctx, "non-existent-invite-id")
err := store.DeleteUserInvite(ctx, "non-existent-account", "non-existent-invite-id")
require.NoError(t, err)
})
}

2
management/server/store/store.go
View File

@@ -103,7 +103,7 @@ type Store interface {
GetUserInviteByHashedToken(ctx context.Context, lockStrength LockingStrength, hashedToken string) (*types.UserInviteRecord, error)
GetUserInviteByEmail(ctx context.Context, lockStrength LockingStrength, accountID, email string) (*types.UserInviteRecord, error)
GetAccountUserInvites(ctx context.Context, lockStrength LockingStrength, accountID string) ([]*types.UserInviteRecord, error)
DeleteUserInvite(ctx context.Context, inviteID string) error
DeleteUserInvite(ctx context.Context, accountID, inviteID string) error
GetPATByID(ctx context.Context, lockStrength LockingStrength, userID, patID string) (*types.PersonalAccessToken, error)
GetUserPATs(ctx context.Context, lockStrength LockingStrength, userID string) ([]*types.PersonalAccessToken, error)

9
management/server/store/store_mock.go
View File

@@ -178,6 +178,7 @@ func (mr *MockStoreMockRecorder) GetClusterSupportsCrowdSec(ctx, clusterAddr int
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetClusterSupportsCrowdSec", reflect.TypeOf((*MockStore)(nil).GetClusterSupportsCrowdSec), ctx, clusterAddr)
}
// Close mocks base method.
func (m *MockStore) Close(ctx context.Context) error {
m.ctrl.T.Helper()
@@ -673,17 +674,17 @@ func (mr *MockStoreMockRecorder) DeleteUser(ctx, accountID, userID interface{})
}
// DeleteUserInvite mocks base method.
func (m *MockStore) DeleteUserInvite(ctx context.Context, inviteID string) error {
func (m *MockStore) DeleteUserInvite(ctx context.Context, accountID, inviteID string) error {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "DeleteUserInvite", ctx, inviteID)
ret := m.ctrl.Call(m, "DeleteUserInvite", ctx, accountID, inviteID)
ret0, _ := ret[0].(error)
return ret0
}
// DeleteUserInvite indicates an expected call of DeleteUserInvite.
func (mr *MockStoreMockRecorder) DeleteUserInvite(ctx, inviteID interface{}) *gomock.Call {
func (mr *MockStoreMockRecorder) DeleteUserInvite(ctx, accountID, inviteID interface{}) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteUserInvite", reflect.TypeOf((*MockStore)(nil).DeleteUserInvite), ctx, inviteID)
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteUserInvite", reflect.TypeOf((*MockStore)(nil).DeleteUserInvite), ctx, accountID, inviteID)
}
// DeleteZone mocks base method.

19
management/server/user.go
View File

@@ -252,12 +252,21 @@ func (am *DefaultAccountManager) UpdateUserPassword(ctx context.Context, account
return status.Errorf(status.InvalidArgument, "new password is required")
}
targetUser, err := am.Store.GetUserByUserID(ctx, store.LockingStrengthNone, targetUserID)
if err != nil {
return err
}
if targetUser.AccountID != accountID {
return status.NewUserNotFoundError(targetUserID)
}
embeddedIdp, ok := am.idpManager.(*idp.EmbeddedIdPManager)
if !ok {
return status.Errorf(status.Internal, "failed to get embedded IdP manager")
}
err := embeddedIdp.UpdateUserPassword(ctx, currentUserID, targetUserID, oldPassword, newPassword)
err = embeddedIdp.UpdateUserPassword(ctx, currentUserID, targetUserID, oldPassword, newPassword)
if err != nil {
return status.Errorf(status.InvalidArgument, "failed to update password: %v", err)
}
@@ -292,6 +301,10 @@ func (am *DefaultAccountManager) DeleteUser(ctx context.Context, accountID, init
return err
}
if targetUser.AccountID != accountID {
return status.NewUserNotFoundError(targetUserID)
}
if targetUser.Role == types.UserRoleOwner {
return status.NewOwnerDeletePermissionError()
}
@@ -1604,7 +1617,7 @@ func (am *DefaultAccountManager) AcceptUserInvite(ctx context.Context, token, pa
if err := transaction.SaveUser(ctx, newUser); err != nil {
return fmt.Errorf("failed to save user: %w", err)
}
if err := transaction.DeleteUserInvite(ctx, invite.ID); err != nil {
if err := transaction.DeleteUserInvite(ctx, invite.AccountID, invite.ID); err != nil {
return fmt.Errorf("failed to delete invite: %w", err)
}
return nil
@@ -1687,7 +1700,7 @@ func (am *DefaultAccountManager) DeleteUserInvite(ctx context.Context, accountID
return err
}
if err := am.Store.DeleteUserInvite(ctx, inviteID); err != nil {
if err := am.Store.DeleteUserInvite(ctx, accountID, inviteID); err != nil {
return err
}

46
management/server/user_test.go
View File

@@ -775,6 +775,52 @@ func TestUser_DeleteUser_ServiceUser(t *testing.T) {
}
}
func TestUser_DeleteUser_CrossAccountRejected(t *testing.T) {
s, cleanup, err := store.NewTestStoreFromSQL(context.Background(), "", t.TempDir())
if err != nil {
t.Fatalf("Error when creating store: %s", err)
}
t.Cleanup(cleanup)
// Create two accounts with users
account1 := newAccountWithId(context.Background(), mockAccountID, mockUserID, "", "", "", false)
targetServiceUser := &types.User{
Id: mockServiceUserID,
IsServiceUser: true,
ServiceUserName: mockServiceUserName,
}
account1.Users[mockServiceUserID] = targetServiceUser
otherAccountID := "otherAccountID"
otherUserID := "otherUserID"
account2 := newAccountWithId(context.Background(), otherAccountID, otherUserID, "", "", "", false)
err = s.SaveAccount(context.Background(), account1)
if err != nil {
t.Fatalf("Error when saving account1: %s", err)
}
err = s.SaveAccount(context.Background(), account2)
if err != nil {
t.Fatalf("Error when saving account2: %s", err)
}
permissionsManager := permissions.NewManager(s)
am := DefaultAccountManager{
Store: s,
eventStore: &activity.InMemoryEventStore{},
permissionsManager: permissionsManager,
}
// otherUserID (from account2) tries to delete mockServiceUserID (from account1)
err = am.DeleteUser(context.Background(), otherAccountID, otherUserID, mockServiceUserID)
assert.Error(t, err, "cross-account user deletion should be rejected")
// Verify the target user still exists
account, err := s.GetAccount(context.Background(), mockAccountID)
assert.NoError(t, err)
assert.NotNil(t, account.Users[mockServiceUserID], "target user should not have been deleted")
}
func TestUser_DeleteUser_SelfDelete(t *testing.T) {
store, cleanup, err := store.NewTestStoreFromSQL(context.Background(), "", t.TempDir())
if err != nil {