mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-01 20:41:28 +02:00
[management] Deny reverse proxy access to pending and blocked users (#7105)
A user in the Pending Approval state could complete SSO and reach any SSO-protected reverse proxy service distributed to a group they belong to, including the All Users group. The reverse proxy authorization path checked the session token signature, that the user exists, that the user's account matches the service's account, and group membership — never the user's account status. The REST API (`permissions/manager.go`) and peer registration both gate on that state, but the proxy gRPC service does not go through the permissions manager, so neither gate applied. A pending user is persisted as blocked and pending approval, so blocked users reached those services the same way. `ValidateSession` now denies on account status, reporting `pending_approval` or `user_blocked` so the proxy access log and the denied page carry the cause rather than a generic refusal. `GenerateSessionToken` refuses to mint a token for such a user at all, so the browser never receives a session cookie and the OIDC callback can tell the user why instead of showing "Service configuration error". `ValidateUserGroupAccess` and `ValidateTunnelPeer` close the same gap; for the tunnel path this covers a user blocked after their peer was registered, since peer group membership alone kept mesh-origin access open. A single helper produces both the denied reason for the RPC responses and the sentinel error for the error-returning callers, so the four entry points cannot drift apart. A user the store cannot resolve is denied rather than passed through. One thing deliberately left out: session cookies are validated locally by the proxy against the service public key with no management round-trip, so a cookie issued before a user is blocked stays valid until it expires (24h by default). That is a revocation-propagation problem rather than this authorization gap, and every option for it (per-request validation with a cache, short-lived tokens with refresh, push-based revocation) changes the proxy hot path or the proxy/management protocol. Worth its own ticket.
This commit is contained in:
@@ -2,6 +2,7 @@ package proxy
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"net"
|
||||
"net/http"
|
||||
"net/netip"
|
||||
@@ -108,7 +109,7 @@ func (h *AuthCallbackHandler) handleCallback(w http.ResponseWriter, r *http.Requ
|
||||
redirectURL.Scheme = "https"
|
||||
query := redirectURL.Query()
|
||||
query.Set("error", "access_denied")
|
||||
query.Set("error_description", "Service configuration error")
|
||||
query.Set("error_description", sessionTokenErrorDescription(err))
|
||||
redirectURL.RawQuery = query.Encode()
|
||||
http.Redirect(w, r, redirectURL.String(), http.StatusFound)
|
||||
return
|
||||
@@ -124,6 +125,20 @@ func (h *AuthCallbackHandler) handleCallback(w http.ResponseWriter, r *http.Requ
|
||||
http.Redirect(w, r, redirectURL.String(), http.StatusFound)
|
||||
}
|
||||
|
||||
// sessionTokenErrorDescription maps a session token failure to the text the
|
||||
// proxy renders on its access denied page. Account status denials get a message
|
||||
// the user can act on, while everything else stays generic so a lookup or
|
||||
// signing failure does not describe management internals to the browser.
|
||||
func sessionTokenErrorDescription(err error) string {
|
||||
if errors.Is(err, nbgrpc.ErrUserPendingApproval) {
|
||||
return "Your account is pending approval by an administrator"
|
||||
}
|
||||
if errors.Is(err, nbgrpc.ErrUserBlocked) {
|
||||
return "Your account is blocked"
|
||||
}
|
||||
return "Service configuration error"
|
||||
}
|
||||
|
||||
func extractUserIDFromToken(ctx context.Context, provider *oidc.Provider, config nbgrpc.ProxyOIDCConfig, token *oauth2.Token) string {
|
||||
rawIDToken, ok := token.Extra("id_token").(string)
|
||||
if !ok {
|
||||
|
||||
@@ -360,6 +360,51 @@ func createTestAccountsAndUsers(t *testing.T, ctx context.Context, testStore sto
|
||||
Issued: "api",
|
||||
}
|
||||
require.NoError(t, testStore.SaveUser(ctx, allowedUser))
|
||||
|
||||
// A second tenant, whose users must never be issued a token signed with
|
||||
// the first tenant's service session key.
|
||||
otherAccount := &types.Account{
|
||||
Id: "otherAccountId",
|
||||
Domain: "other.com",
|
||||
DomainCategory: "private",
|
||||
IsDomainPrimaryAccount: true,
|
||||
CreatedAt: time.Now(),
|
||||
}
|
||||
require.NoError(t, testStore.SaveAccount(ctx, otherAccount))
|
||||
|
||||
otherAccountUser := &types.User{
|
||||
Id: "otherAccountUserId",
|
||||
AccountID: "otherAccountId",
|
||||
Role: types.UserRoleUser,
|
||||
CreatedAt: time.Now(),
|
||||
Issued: "api",
|
||||
}
|
||||
require.NoError(t, testStore.SaveUser(ctx, otherAccountUser))
|
||||
|
||||
// A user awaiting approval is stored as blocked and pending approval, and
|
||||
// carries the same group membership as the approved one.
|
||||
pendingUser := &types.User{
|
||||
Id: "pendingUserId",
|
||||
AccountID: "testAccountId",
|
||||
Role: types.UserRoleUser,
|
||||
AutoGroups: []string{"allowedGroupId"},
|
||||
Blocked: true,
|
||||
PendingApproval: true,
|
||||
CreatedAt: time.Now(),
|
||||
Issued: "api",
|
||||
}
|
||||
require.NoError(t, testStore.SaveUser(ctx, pendingUser))
|
||||
|
||||
blockedUser := &types.User{
|
||||
Id: "blockedUserId",
|
||||
AccountID: "testAccountId",
|
||||
Role: types.UserRoleUser,
|
||||
AutoGroups: []string{"allowedGroupId"},
|
||||
Blocked: true,
|
||||
CreatedAt: time.Now(),
|
||||
Issued: "api",
|
||||
}
|
||||
require.NoError(t, testStore.SaveUser(ctx, blockedUser))
|
||||
}
|
||||
|
||||
// testServiceManager is a minimal implementation for testing.
|
||||
@@ -490,6 +535,64 @@ func TestAuthCallback_UserAllowedToLogin(t *testing.T) {
|
||||
require.Empty(t, parsedLocation.Query().Get("error"), "Should not have error parameter")
|
||||
}
|
||||
|
||||
// TestAuthCallback_UserDeniedByAccountStatus asserts that a user whose account
|
||||
// is pending approval or blocked never receives a session token from the OIDC
|
||||
// callback, and that the redirect carries a description the proxy can render.
|
||||
func TestAuthCallback_UserDeniedByAccountStatus(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
subject string
|
||||
expectErrorDesc string
|
||||
}{
|
||||
{
|
||||
name: "pending approval",
|
||||
subject: "pendingUserId",
|
||||
expectErrorDesc: "Your account is pending approval by an administrator",
|
||||
},
|
||||
{
|
||||
name: "blocked",
|
||||
subject: "blockedUserId",
|
||||
expectErrorDesc: "Your account is blocked",
|
||||
},
|
||||
{
|
||||
name: "unknown to management",
|
||||
subject: "userMissingFromStoreId",
|
||||
expectErrorDesc: "Service configuration error",
|
||||
},
|
||||
{
|
||||
// The account topology stays out of the browser-visible message.
|
||||
name: "belongs to another account",
|
||||
subject: "otherAccountUserId",
|
||||
expectErrorDesc: "Service configuration error",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
setup := setupAuthCallbackTest(t)
|
||||
defer setup.cleanup()
|
||||
|
||||
setup.oidcServer.tokenSubject = tt.subject
|
||||
|
||||
state := createTestState(t, setup.proxyService, "https://test-proxy.example.com/dashboard")
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, "/reverse-proxy/callback?code=test-auth-code&state="+url.QueryEscape(state), nil)
|
||||
rec := httptest.NewRecorder()
|
||||
|
||||
setup.router.ServeHTTP(rec, req)
|
||||
|
||||
require.Equal(t, http.StatusFound, rec.Code)
|
||||
|
||||
parsedLocation, err := url.Parse(rec.Header().Get("Location"))
|
||||
require.NoError(t, err)
|
||||
|
||||
require.Empty(t, parsedLocation.Query().Get("session_token"), "Denied user must not receive a session token")
|
||||
require.Equal(t, "access_denied", parsedLocation.Query().Get("error"))
|
||||
require.Equal(t, tt.expectErrorDesc, parsedLocation.Query().Get("error_description"))
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestAuthCallback_ProxyNotFound(t *testing.T) {
|
||||
setup := setupAuthCallbackTest(t)
|
||||
defer setup.cleanup()
|
||||
|
||||
Reference in New Issue
Block a user