From e830903a94da4662da2e9d4b9291651677bc4042 Mon Sep 17 00:00:00 2001 From: Brad Ison Date: Tue, 29 Sep 2026 09:28:30 +0200 Subject: [PATCH] [management] Add a revocation guard hook to the proxy token API (#7732) Let an embedding binary refuse DELETE /api/reverse-proxies/proxy-tokens/{id} through an optional proxytoken.RevocationGuard passed to NewAPIHandler. It is checked after the ownership check, so another account's token still returns 404 without reaching the guard, and before the token is revoked. A status error from the guard is written with util.WriteError; any other error becomes a generic 500. Nothing installs a guard here, so OSS behavior is unchanged. --- .../reverseproxy/proxytoken/handler.go | 28 +++- .../reverseproxy/proxytoken/handler_test.go | 151 ++++++++++++++++++ management/internals/server/boot.go | 2 +- management/server/http/handler.go | 4 +- .../testing/testing_tools/channel/channel.go | 4 +- 5 files changed, 182 insertions(+), 7 deletions(-) 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) }