diff --git a/management/internals/modules/reverseproxy/proxytoken/handler.go b/management/internals/modules/reverseproxy/proxytoken/handler.go index ed098a6dd..d8578db4c 100644 --- a/management/internals/modules/reverseproxy/proxytoken/handler.go +++ b/management/internals/modules/reverseproxy/proxytoken/handler.go @@ -1,6 +1,7 @@ package proxytoken import ( + "context" "encoding/json" "net/http" "time" @@ -18,13 +19,29 @@ import ( "github.com/netbirdio/netbird/shared/management/status" ) +// RevocationGuard vetoes the tenant-facing revocation of a proxy access +// token. Implementations are supplied by integrations; none is installed by +// default, so every token the caller's account owns may be revoked. It is +// consulted after the ownership check and before the token is revoked. A +// returned status error is written with util.WriteError: its type selects the +// HTTP status and its message is shown to the caller, so it must not carry +// internal detail. Any other error is reported as a generic internal error. +type RevocationGuard interface { + CheckProxyAccessTokenRevocation(ctx context.Context, token *types.ProxyAccessToken) error +} + type handler struct { store store.Store permissionsManager permissions.Manager + // revocationGuard vetoes revocations. Optional — when nil every owned + // token may be revoked. + revocationGuard RevocationGuard } -func RegisterEndpoints(s store.Store, permissionsManager permissions.Manager, router *mux.Router) { - h := &handler{store: s, permissionsManager: permissionsManager} +// RegisterEndpoints registers the proxy token endpoints. revocationGuard is +// optional; pass nil for no revocation policy. +func RegisterEndpoints(s store.Store, permissionsManager permissions.Manager, revocationGuard RevocationGuard, router *mux.Router) { + h := &handler{store: s, permissionsManager: permissionsManager, revocationGuard: revocationGuard} router.HandleFunc("/reverse-proxies/proxy-tokens", h.listTokens).Methods("GET", "OPTIONS") router.HandleFunc("/reverse-proxies/proxy-tokens", h.createToken).Methods("POST", "OPTIONS") router.HandleFunc("/reverse-proxies/proxy-tokens/{tokenId}", h.revokeToken).Methods("DELETE", "OPTIONS") @@ -154,6 +171,13 @@ func (h *handler) revokeToken(w http.ResponseWriter, r *http.Request) { return } + if h.revocationGuard != nil { + if err := h.revocationGuard.CheckProxyAccessTokenRevocation(ctx, token); err != nil { + util.WriteError(ctx, err, w) + return + } + } + if err := h.store.RevokeProxyAccessToken(ctx, tokenID); err != nil { util.WriteErrorResponse("failed to revoke token", http.StatusInternalServerError, w) return diff --git a/management/internals/modules/reverseproxy/proxytoken/handler_test.go b/management/internals/modules/reverseproxy/proxytoken/handler_test.go index c71fe59f6..da37261c4 100644 --- a/management/internals/modules/reverseproxy/proxytoken/handler_test.go +++ b/management/internals/modules/reverseproxy/proxytoken/handler_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "net/http" "net/http/httptest" "testing" @@ -22,6 +23,7 @@ import ( "github.com/netbirdio/netbird/management/server/types" "github.com/netbirdio/netbird/shared/auth" "github.com/netbirdio/netbird/shared/management/http/api" + "github.com/netbirdio/netbird/shared/management/status" ) func authContext(accountID, userID string) context.Context { @@ -273,3 +275,152 @@ func TestRevokeToken_ManagementWideToken(t *testing.T) { h.revokeToken(w, req) assert.Equal(t, http.StatusNotFound, w.Code) } + +type revocationGuardFunc func(ctx context.Context, token *types.ProxyAccessToken) error + +func (f revocationGuardFunc) CheckProxyAccessTokenRevocation(ctx context.Context, token *types.ProxyAccessToken) error { + return f(ctx, token) +} + +func TestRevokeToken_GuardRefuses(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + accountID := "acc-123" + + // No RevokeProxyAccessToken expectation: a refused revocation must not + // reach the store. + mockStore := store.NewMockStore(ctrl) + mockStore.EXPECT().GetProxyAccessTokenByID(gomock.Any(), store.LockingStrengthNone, "tok-1").Return(&types.ProxyAccessToken{ + ID: "tok-1", + AccountID: &accountID, + }, nil) + + permsMgr := permissions.NewMockManager(ctrl) + permsMgr.EXPECT().ValidateUserPermissions(gomock.Any(), accountID, "user-1", modules.Services, operations.Delete).Return(true, context.Background(), nil) + + var checked *types.ProxyAccessToken + h := &handler{ + store: mockStore, + permissionsManager: permsMgr, + revocationGuard: revocationGuardFunc(func(_ context.Context, token *types.ProxyAccessToken) error { + checked = token + return status.Errorf(status.PreconditionFailed, "token is in use") + }), + } + + req := httptest.NewRequest("DELETE", "/reverse-proxies/proxy-tokens/tok-1", nil) + req = req.WithContext(authContext(accountID, "user-1")) + req = mux.SetURLVars(req, map[string]string{"tokenId": "tok-1"}) + w := httptest.NewRecorder() + + h.revokeToken(w, req) + assert.Equal(t, http.StatusPreconditionFailed, w.Code) + assert.Contains(t, w.Body.String(), "token is in use") + require.NotNil(t, checked) + assert.Equal(t, "tok-1", checked.ID) +} + +func TestRevokeToken_GuardAllows(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + accountID := "acc-123" + + mockStore := store.NewMockStore(ctrl) + mockStore.EXPECT().GetProxyAccessTokenByID(gomock.Any(), store.LockingStrengthNone, "tok-1").Return(&types.ProxyAccessToken{ + ID: "tok-1", + AccountID: &accountID, + }, nil) + mockStore.EXPECT().RevokeProxyAccessToken(gomock.Any(), "tok-1").Return(nil) + + permsMgr := permissions.NewMockManager(ctrl) + permsMgr.EXPECT().ValidateUserPermissions(gomock.Any(), accountID, "user-1", modules.Services, operations.Delete).Return(true, context.Background(), nil) + + h := &handler{ + store: mockStore, + permissionsManager: permsMgr, + revocationGuard: revocationGuardFunc(func(context.Context, *types.ProxyAccessToken) error { + return nil + }), + } + + req := httptest.NewRequest("DELETE", "/reverse-proxies/proxy-tokens/tok-1", nil) + req = req.WithContext(authContext(accountID, "user-1")) + req = mux.SetURLVars(req, map[string]string{"tokenId": "tok-1"}) + w := httptest.NewRecorder() + + h.revokeToken(w, req) + assert.Equal(t, http.StatusOK, w.Code) +} + +func TestRevokeToken_GuardFailure(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + accountID := "acc-123" + + // No RevokeProxyAccessToken expectation: a guard that cannot decide must + // not let the revocation through. + mockStore := store.NewMockStore(ctrl) + mockStore.EXPECT().GetProxyAccessTokenByID(gomock.Any(), store.LockingStrengthNone, "tok-1").Return(&types.ProxyAccessToken{ + ID: "tok-1", + AccountID: &accountID, + }, nil) + + permsMgr := permissions.NewMockManager(ctrl) + permsMgr.EXPECT().ValidateUserPermissions(gomock.Any(), accountID, "user-1", modules.Services, operations.Delete).Return(true, context.Background(), nil) + + h := &handler{ + store: mockStore, + permissionsManager: permsMgr, + revocationGuard: revocationGuardFunc(func(context.Context, *types.ProxyAccessToken) error { + return errors.New("connection refused") + }), + } + + req := httptest.NewRequest("DELETE", "/reverse-proxies/proxy-tokens/tok-1", nil) + req = req.WithContext(authContext(accountID, "user-1")) + req = mux.SetURLVars(req, map[string]string{"tokenId": "tok-1"}) + w := httptest.NewRecorder() + + h.revokeToken(w, req) + assert.Equal(t, http.StatusInternalServerError, w.Code) + assert.Contains(t, w.Body.String(), "internal server error") + assert.NotContains(t, w.Body.String(), "connection refused") +} + +func TestRevokeToken_GuardNotConsultedForForeignToken(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + otherAccount := "acc-other" + + mockStore := store.NewMockStore(ctrl) + mockStore.EXPECT().GetProxyAccessTokenByID(gomock.Any(), store.LockingStrengthNone, "tok-1").Return(&types.ProxyAccessToken{ + ID: "tok-1", + AccountID: &otherAccount, + }, nil) + + permsMgr := permissions.NewMockManager(ctrl) + permsMgr.EXPECT().ValidateUserPermissions(gomock.Any(), "acc-123", "user-1", modules.Services, operations.Delete).Return(true, context.Background(), nil) + + // A foreign token must read as not found, not reveal through the guard's + // answer that it belongs to some account's managed proxy. + h := &handler{ + store: mockStore, + permissionsManager: permsMgr, + revocationGuard: revocationGuardFunc(func(context.Context, *types.ProxyAccessToken) error { + t.Fatal("guard consulted for a token the caller does not own") + return nil + }), + } + + req := httptest.NewRequest("DELETE", "/reverse-proxies/proxy-tokens/tok-1", nil) + req = req.WithContext(authContext("acc-123", "user-1")) + req = mux.SetURLVars(req, map[string]string{"tokenId": "tok-1"}) + w := httptest.NewRecorder() + + h.revokeToken(w, req) + assert.Equal(t, http.StatusNotFound, w.Code) +} diff --git a/management/internals/server/boot.go b/management/internals/server/boot.go index 9eb8b7748..0f136ce82 100644 --- a/management/internals/server/boot.go +++ b/management/internals/server/boot.go @@ -159,7 +159,7 @@ func (s *BaseServer) EventStore() activity.Store { func (s *BaseServer) APIHandler() http.Handler { return Create(s, func() http.Handler { - httpAPIHandler, err := nbhttp.NewAPIHandler(context.Background(), s.Router(), s.AccountManager(), s.NetworksManager(), s.ResourcesManager(), s.RoutesManager(), s.GroupsManager(), s.GeoLocationManager(), s.AuthManager(), s.Metrics(), s.PermissionsManager(), s.SettingsManager(), s.ZonesManager(), s.RecordsManager(), s.NetworkMapController(), s.IdpManager(), s.ServiceManager(), s.ReverseProxyDomainManager(), s.AccessLogsManager(), s.ReverseProxyGRPCServer(), s.Config.ReverseProxy.TrustedHTTPProxies, s.RateLimiter(), s.IsValidChildAccount, s.AgentNetworkManager()) + httpAPIHandler, err := nbhttp.NewAPIHandler(context.Background(), s.Router(), s.AccountManager(), s.NetworksManager(), s.ResourcesManager(), s.RoutesManager(), s.GroupsManager(), s.GeoLocationManager(), s.AuthManager(), s.Metrics(), s.PermissionsManager(), s.SettingsManager(), s.ZonesManager(), s.RecordsManager(), s.NetworkMapController(), s.IdpManager(), s.ServiceManager(), s.ReverseProxyDomainManager(), s.AccessLogsManager(), s.ReverseProxyGRPCServer(), s.Config.ReverseProxy.TrustedHTTPProxies, s.RateLimiter(), s.IsValidChildAccount, s.AgentNetworkManager(), nil) if err != nil { log.Fatalf("failed to create API handler: %v", err) } diff --git a/management/server/http/handler.go b/management/server/http/handler.go index cd365ec13..8ebfd6c64 100644 --- a/management/server/http/handler.go +++ b/management/server/http/handler.go @@ -62,7 +62,7 @@ import ( ) // NewAPIHandler creates the Management service HTTP API handler registering all the available endpoints. -func NewAPIHandler(ctx context.Context, router *mux.Router, accountManager account.Manager, networksManager nbnetworks.Manager, resourceManager resources.Manager, routerManager routers.Manager, groupsManager nbgroups.Manager, LocationManager geolocation.Geolocation, authManager auth.Manager, appMetrics telemetry.AppMetrics, permissionsManager permissions.Manager, settingsManager settings.Manager, zManager zones.Manager, rManager records.Manager, networkMapController network_map.Controller, idpManager idpmanager.Manager, serviceManager service.Manager, reverseProxyDomainManager *manager.Manager, reverseProxyAccessLogsManager accesslogs.Manager, proxyGRPCServer *nbgrpc.ProxyServiceServer, trustedHTTPProxies []netip.Prefix, rateLimiter *ratelimit.APIRateLimiter, isValidChildAccount middleware.IsValidChildAccountFunc, agentNetworkManager agentnetwork.Manager) (http.Handler, error) { +func NewAPIHandler(ctx context.Context, router *mux.Router, accountManager account.Manager, networksManager nbnetworks.Manager, resourceManager resources.Manager, routerManager routers.Manager, groupsManager nbgroups.Manager, LocationManager geolocation.Geolocation, authManager auth.Manager, appMetrics telemetry.AppMetrics, permissionsManager permissions.Manager, settingsManager settings.Manager, zManager zones.Manager, rManager records.Manager, networkMapController network_map.Controller, idpManager idpmanager.Manager, serviceManager service.Manager, reverseProxyDomainManager *manager.Manager, reverseProxyAccessLogsManager accesslogs.Manager, proxyGRPCServer *nbgrpc.ProxyServiceServer, trustedHTTPProxies []netip.Prefix, rateLimiter *ratelimit.APIRateLimiter, isValidChildAccount middleware.IsValidChildAccountFunc, agentNetworkManager agentnetwork.Manager, proxyTokenRevocationGuard proxytoken.RevocationGuard) (http.Handler, error) { // Register bypass paths for unauthenticated endpoints if err := bypass.AddBypassPath("/api/instance"); err != nil { @@ -136,7 +136,7 @@ func NewAPIHandler(ctx context.Context, router *mux.Router, accountManager accou reverseproxymanager.RegisterEndpoints(serviceManager, *reverseProxyDomainManager, reverseProxyAccessLogsManager, permissionsManager, router) } - proxytoken.RegisterEndpoints(accountManager.GetStore(), permissionsManager, router) + proxytoken.RegisterEndpoints(accountManager.GetStore(), permissionsManager, proxyTokenRevocationGuard, router) // Register OAuth callback handler for proxy authentication if proxyGRPCServer != nil { diff --git a/management/server/http/testing/testing_tools/channel/channel.go b/management/server/http/testing/testing_tools/channel/channel.go index 6ea26e634..598adafcc 100644 --- a/management/server/http/testing/testing_tools/channel/channel.go +++ b/management/server/http/testing/testing_tools/channel/channel.go @@ -147,7 +147,7 @@ func BuildApiBlackBoxWithDBState(t testing_tools.TB, sqlFile string, expectedPee zoneRecordsManager := recordsManager.NewManager(store, am, permissionsManager) apiRouter := mux.NewRouter().PathPrefix("/api").Subrouter() - apiHandler, err := http2.NewAPIHandler(ctx, apiRouter, am, networksManager, resourcesManager, routersManager, groupsManager, geoMock, authManagerMock, metrics, permissionsManager, settingsManager, customZonesManager, zoneRecordsManager, networkMapController, nil, serviceManager, nil, nil, nil, nil, nil, nil, nil) + apiHandler, err := http2.NewAPIHandler(ctx, apiRouter, am, networksManager, resourcesManager, routersManager, groupsManager, geoMock, authManagerMock, metrics, permissionsManager, settingsManager, customZonesManager, zoneRecordsManager, networkMapController, nil, serviceManager, nil, nil, nil, nil, nil, nil, nil, nil) if err != nil { t.Fatalf("Failed to create API handler: %v", err) } @@ -287,7 +287,7 @@ func BuildApiBlackBoxWithDBStateAndPeerChannel(t testing_tools.TB, sqlFile strin zoneRecordsManager := recordsManager.NewManager(store, am, permissionsManager) apiRouter := mux.NewRouter().PathPrefix("/api").Subrouter() - apiHandler, err := http2.NewAPIHandler(ctx, apiRouter, am, networksManager, resourcesManager, routersManager, groupsManager, geoMock, authManagerMock, metrics, permissionsManager, settingsManager, customZonesManager, zoneRecordsManager, networkMapController, nil, serviceManager, nil, nil, nil, nil, nil, nil, nil) + apiHandler, err := http2.NewAPIHandler(ctx, apiRouter, am, networksManager, resourcesManager, routersManager, groupsManager, geoMock, authManagerMock, metrics, permissionsManager, settingsManager, customZonesManager, zoneRecordsManager, networkMapController, nil, serviceManager, nil, nil, nil, nil, nil, nil, nil, nil) if err != nil { t.Fatalf("Failed to create API handler: %v", err) }