diff --git a/client/server/logout_gate_test.go b/client/server/logout_gate_test.go new file mode 100644 index 000000000..2d84d1b6a --- /dev/null +++ b/client/server/logout_gate_test.go @@ -0,0 +1,200 @@ +package server + +import ( + "context" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + gstatus "google.golang.org/grpc/status" + + "github.com/netbirdio/netbird/client/internal" + "github.com/netbirdio/netbird/client/internal/profilemanager" + "github.com/netbirdio/netbird/client/proto" +) + +// unreachableManagementURL keeps a test that is expected to stop at a gate from +// reaching the network if the gate ever regresses: the profiles a logout must +// not touch point here, so a leak fails fast instead of contacting a real +// management server. +const unreachableManagementURL = "https://127.0.0.1:9" + +// enableSSHOnProfile rewrites the profile config at cfgPath with the SSH server +// enabled. Deregistering an SSH-enabled profile is a privileged change, so an +// unprivileged caller is refused by requirePrivilegeForDeregistration before any +// management connection is attempted, which is what keeps these tests offline. +func enableSSHOnProfile(t *testing.T, cfgPath string) { + t.Helper() + _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ + ConfigPath: cfgPath, + ManagementURL: "https://api.netbird.io:443", + ServerSSHAllowed: boolPtr(true), + }) + require.NoError(t, err) +} + +// Logging out of the profile the daemon is already running is a deregistration, +// not profile management, so the profiles-disabled kill switch must not block +// it. The desktop UI always addresses logout by profile (both the profile menu +// and the session-expiration dialog), so gating it left users with +// disableProfiles enforced unable to log out at all. +func TestLogout_ActiveProfileAllowedWhenProfilesDisabled(t *testing.T) { + s, _, activeProfile, username, cfgPath := setupServerWithProfile(t) + s.rootCtx = internal.CtxInitState(context.Background()) + enableSSHOnProfile(t, cfgPath) + + s.profilesDisabled = true + + _, err := s.Logout(userCtx(), &proto.LogoutRequest{ + ProfileName: &activeProfile, + Username: &username, + }) + + require.Error(t, err, "the SSH privilege gate is expected to refuse this unprivileged caller") + require.Equal(t, codes.PermissionDenied, gstatus.Code(err), + "logout of the active profile must reach the deregistration path, not be refused as profile management: %v", err) + require.NotContains(t, gstatus.Convert(err).Message(), errProfilesDisabled) +} + +// A profile-addressed logout that targets some *other* profile does manage +// profiles, so it stays gated: with profiles disabled the daemon must not +// deregister a peer the user is not currently running. +func TestLogout_OtherProfileStaysGatedWhenProfilesDisabled(t *testing.T) { + s, _, _, username, _ := setupServerWithProfile(t) + s.rootCtx = internal.CtxInitState(context.Background()) + + other := "other-profile" + _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ + ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, other+".json"), + ManagementURL: unreachableManagementURL, + }) + require.NoError(t, err) + + s.profilesDisabled = true + + _, err = s.Logout(userCtx(), &proto.LogoutRequest{ + ProfileName: &other, + Username: &username, + }) + + require.Error(t, err) + require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the profiles-disabled refusal, got %v", err) + require.Contains(t, gstatus.Convert(err).Message(), errProfilesDisabled) +} + +// A legacy profile ID is a display name, so two users can hold the same ID in +// their own profile directories. Matching on the ID alone would let one user's +// logout pass the gate against the other user's active profile, so the username +// is part of the comparison. +func TestLogout_ForeignUserProfileStaysGatedWhenProfilesDisabled(t *testing.T) { + s, _, _, username, _ := setupServerWithProfile(t) + s.rootCtx = internal.CtxInitState(context.Background()) + + // A legacy-style profile whose ID is its filename stem, and an active state + // claiming that same ID for a different user. + shared := "shared-legacy-name" + _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ + ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, shared+".json"), + ManagementURL: unreachableManagementURL, + }) + require.NoError(t, err) + require.NoError(t, s.profileManager.SetActiveProfileState(&profilemanager.ActiveProfileState{ + ID: profilemanager.ID(shared), + Username: "someone-else", + })) + + s.profilesDisabled = true + + _, err = s.Logout(userCtx(), &proto.LogoutRequest{ + ProfileName: &shared, + Username: &username, + }) + + require.Error(t, err) + require.Equal(t, codes.Unavailable, gstatus.Code(err), + "another user's profile must not pass the gate on an ID match alone: %v", err) +} + +// Deregistering a namesake profile must not go out with the running config. +// logoutFromProfile reuses the connected client's config when the target is the +// active profile, and on an ID-only match a shared legacy ID made it reuse it +// for another user's profile, deregistering the active peer instead. +func TestLogout_ForeignUserProfileDoesNotUseTheRunningConfig(t *testing.T) { + s, _, _, username, cfgPath := setupServerWithProfile(t) + s.rootCtx = internal.CtxInitState(context.Background()) + + // The running config has the SSH server enabled, so reusing it would be + // refused with PermissionDenied. The namesake profile does not, so the + // correct path gets as far as dialing its own unreachable management URL. + enableSSHOnProfile(t, cfgPath) + running, err := profilemanager.GetConfig(cfgPath) + require.NoError(t, err) + s.config = running + s.connectClient = newDummyConnectClient(context.Background()) + + shared := "shared-legacy-name" + _, err = profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ + ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, shared+".json"), + ManagementURL: unreachableManagementURL, + }) + require.NoError(t, err) + require.NoError(t, s.profileManager.SetActiveProfileState(&profilemanager.ActiveProfileState{ + ID: profilemanager.ID(shared), + Username: "someone-else", + })) + + // Bounded so the deregistration the fixed path attempts fails on the dial + // rather than sitting in gRPC backoff for the whole test timeout. + ctx, cancel := context.WithTimeout(userCtx(), 2*time.Second) + t.Cleanup(cancel) + + _, err = s.Logout(ctx, &proto.LogoutRequest{ + ProfileName: &shared, + Username: &username, + }) + + require.Error(t, err) + require.NotEqual(t, codes.PermissionDenied, gstatus.Code(err), + "the namesake profile was deregistered with the running config: %v", err) +} + +// The connection teardown follows the profile that is active when the logout +// completes, not the one seen before it started: Login switches profiles under +// guardedConfigMu, which the logout path does not hold, so a login that landed +// meanwhile must keep its connection. +func TestCleanupAfterProfileLogout_FollowsTheCurrentActiveProfile(t *testing.T) { + s, _, activeProfile, username, _ := setupServerWithProfile(t) + s.rootCtx = internal.CtxInitState(context.Background()) + + state := internal.CtxGetState(s.rootCtx) + + s.cleanupAfterProfileLogout("some-other-profile", username) + status, err := state.Status() + require.NoError(t, err) + require.NotEqual(t, internal.StatusNeedsLogin, status, + "logging out of a profile that is not active must not ask for a new login") + + s.cleanupAfterProfileLogout(profilemanager.ID(activeProfile), username) + status, err = state.Status() + require.NoError(t, err) + require.Equal(t, internal.StatusNeedsLogin, status, + "logging out of the active profile must ask for a new login") +} + +// With profiles enabled the gate is out of the way on both surfaces; the active +// profile still reaches the deregistration path. +func TestLogout_ActiveProfileAllowedWhenProfilesEnabled(t *testing.T) { + s, _, activeProfile, username, cfgPath := setupServerWithProfile(t) + s.rootCtx = internal.CtxInitState(context.Background()) + enableSSHOnProfile(t, cfgPath) + + _, err := s.Logout(userCtx(), &proto.LogoutRequest{ + ProfileName: &activeProfile, + Username: &username, + }) + + require.Error(t, err, "the SSH privilege gate is expected to refuse this unprivileged caller") + require.Equal(t, codes.PermissionDenied, gstatus.Code(err), "want the privilege refusal, got %v", err) +} diff --git a/client/server/server.go b/client/server/server.go index 23dccc9b1..b066e9719 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -710,54 +710,7 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro } if msg.SetupKey == "" { - hint := "" - if msg.Hint != nil { - hint = *msg.Hint - } - oAuthFlow, err := auth.NewOAuthFlow(ctx, config, msg.IsUnixDesktopClient, false, hint) - if err != nil { - state.Set(internal.StatusLoginFailed) - return nil, err - } - - if s.oauthAuthFlow.flow != nil && s.oauthAuthFlow.flow.GetClientID(ctx) == oAuthFlow.GetClientID(ctx) { - if s.oauthAuthFlow.expiresAt.After(time.Now().Add(90 * time.Second)) { - log.Debugf("using previous oauth flow info") - state.Set(internal.StatusNeedsLogin) - return &proto.LoginResponse{ - NeedsSSOLogin: true, - VerificationURI: s.oauthAuthFlow.info.VerificationURI, - VerificationURIComplete: s.oauthAuthFlow.info.VerificationURIComplete, - UserCode: s.oauthAuthFlow.info.UserCode, - }, nil - } else { - log.Warnf("canceling previous waiting execution") - if s.oauthAuthFlow.waitCancel != nil { - s.oauthAuthFlow.waitCancel() - } - } - } - - authInfo, err := oAuthFlow.RequestAuthInfo(ctx) - if err != nil { - log.Errorf("getting a request OAuth flow failed: %v", err) - return nil, err - } - - s.mutex.Lock() - s.oauthAuthFlow.flow = oAuthFlow - s.oauthAuthFlow.info = authInfo - s.oauthAuthFlow.expiresAt = time.Now().Add(time.Duration(authInfo.ExpiresIn) * time.Second) - s.mutex.Unlock() - - state.Set(internal.StatusNeedsLogin) - - return &proto.LoginResponse{ - NeedsSSOLogin: true, - VerificationURI: authInfo.VerificationURI, - VerificationURIComplete: authInfo.VerificationURIComplete, - UserCode: authInfo.UserCode, - }, nil + return s.beginSSOLogin(ctx, config, msg) } // Setup-key path: we are about to dial Management with the key, so the @@ -773,6 +726,76 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro return &proto.LoginResponse{}, nil } +// beginSSOLogin starts the browser leg of a login that carries no setup key and +// returns the response that parks the caller on it. +func (s *Server) beginSSOLogin(ctx context.Context, config *profilemanager.Config, msg *proto.LoginRequest) (*proto.LoginResponse, error) { + state := internal.CtxGetState(s.rootCtx) + + hint := "" + if msg.Hint != nil { + hint = *msg.Hint + } + oAuthFlow, err := auth.NewOAuthFlow(ctx, config, msg.IsUnixDesktopClient, false, hint) + if err != nil { + state.Set(internal.StatusLoginFailed) + return nil, err + } + + if resp := s.pendingOAuthFlowResponse(ctx, oAuthFlow); resp != nil { + state.Set(internal.StatusNeedsLogin) + return resp, nil + } + + authInfo, err := oAuthFlow.RequestAuthInfo(ctx) + if err != nil { + log.Errorf("getting a request OAuth flow failed: %v", err) + return nil, err + } + + s.mutex.Lock() + s.oauthAuthFlow.flow = oAuthFlow + s.oauthAuthFlow.info = authInfo + s.oauthAuthFlow.expiresAt = time.Now().Add(time.Duration(authInfo.ExpiresIn) * time.Second) + s.mutex.Unlock() + + state.Set(internal.StatusNeedsLogin) + + return &proto.LoginResponse{ + NeedsSSOLogin: true, + VerificationURI: authInfo.VerificationURI, + VerificationURIComplete: authInfo.VerificationURIComplete, + UserCode: authInfo.UserCode, + }, nil +} + +// pendingOAuthFlowResponse returns the in-flight flow's response when it +// targets the same IdP client and has enough time left for the user to finish +// the browser leg, so a second login joins the pending flow instead of opening +// a competing one. A flow too close to expiry has its waiter cancelled and nil +// returned, leaving the caller to start a fresh flow. +func (s *Server) pendingOAuthFlowResponse(ctx context.Context, oAuthFlow auth.OAuthFlow) *proto.LoginResponse { + if s.oauthAuthFlow.flow == nil || s.oauthAuthFlow.flow.GetClientID(ctx) != oAuthFlow.GetClientID(ctx) { + return nil + } + + if s.oauthAuthFlow.expiresAt.After(time.Now().Add(90 * time.Second)) { + log.Debugf("using previous oauth flow info") + return &proto.LoginResponse{ + NeedsSSOLogin: true, + VerificationURI: s.oauthAuthFlow.info.VerificationURI, + VerificationURIComplete: s.oauthAuthFlow.info.VerificationURIComplete, + UserCode: s.oauthAuthFlow.info.UserCode, + } + } + + log.Warnf("canceling previous waiting execution") + if s.oauthAuthFlow.waitCancel != nil { + s.oauthAuthFlow.waitCancel() + } + + return nil +} + // WaitSSOLogin validates the supplied userCode against the in-flight OAuth // device/PKCE flow and blocks until the user finishes the browser leg. // @@ -1347,11 +1370,16 @@ func (s *Server) handleProfileLogout(ctx context.Context, msg *proto.LogoutReque return nil, err } - if err := s.validateProfileOperation(resolved.ID, true); err != nil { + activeProf, err := s.profileManager.GetActiveProfileState() + if err != nil { + return nil, gstatus.Errorf(codes.FailedPrecondition, "failed to get active profile state: %v", err) + } + + if err := s.validateProfileLogout(resolved.ID, isActiveProfile(activeProf, resolved.ID, username)); err != nil { return nil, err } - if err := s.logoutFromProfile(ctx, resolved); err != nil { + if err := s.logoutFromProfile(ctx, resolved, username); err != nil { log.Errorf("failed to logout from profile %s: %v", resolved.ID, err) // A refused deregistration is already a status error carrying the reason // and the command to run; rewrapping it as Internal would flatten both @@ -1362,18 +1390,35 @@ func (s *Server) handleProfileLogout(ctx context.Context, msg *proto.LogoutReque return nil, gstatus.Errorf(codes.Internal, "logout: %v", err) } - activeProf, _ := s.profileManager.GetActiveProfileState() - if activeProf != nil && activeProf.ID == resolved.ID { - if err := s.cleanupConnection(); err != nil && !errors.Is(err, ErrServiceNotUp) { - log.Errorf("failed to cleanup connection: %v", err) - } - state := internal.CtxGetState(s.rootCtx) - state.Set(internal.StatusNeedsLogin) - } + s.cleanupAfterProfileLogout(resolved.ID, username) return &proto.LogoutResponse{}, nil } +// cleanupAfterProfileLogout tears the connection down and asks for a new login +// when the profile that was just deregistered is the one the daemon is running. +// The active profile is read again here rather than reused from the pre-flight +// check: Login switches profiles under guardedConfigMu, which this path does not +// hold, so a login that landed meanwhile must not have its fresh connection +// dropped by a logout that targeted the profile it replaced. +func (s *Server) cleanupAfterProfileLogout(id profilemanager.ID, username string) { + activeProf, err := s.profileManager.GetActiveProfileState() + if err != nil { + log.Errorf("failed to get active profile state after logout from profile %s: %v", id, err) + return + } + + if !isActiveProfile(activeProf, id, username) { + return + } + + if err := s.cleanupConnection(); err != nil && !errors.Is(err, ErrServiceNotUp) { + log.Errorf("failed to cleanup connection: %v", err) + } + state := internal.CtxGetState(s.rootCtx) + state.Set(internal.StatusNeedsLogin) +} + func (s *Server) handleActiveProfileLogout(ctx context.Context) (*proto.LogoutResponse, error) { if s.config == nil { activeProf, err := s.profileManager.GetActiveProfileState() @@ -1425,40 +1470,47 @@ func (s *Server) getConfig(activeProf *profilemanager.ActiveProfileState) (*prof return config, configExisted, nil } -func (s *Server) canRemoveProfile(id profilemanager.ID) error { - if id == profilemanager.DefaultProfileName { - return fmt.Errorf("remove profile with reserved name: %s", profilemanager.DefaultProfileName) - } - - activeProf, err := s.profileManager.GetActiveProfileState() - if err == nil && activeProf.ID == id { - return fmt.Errorf("remove active profile: %s", id) - } - - return nil -} - -func (s *Server) validateProfileOperation(id profilemanager.ID, allowActiveProfile bool) error { - if s.checkProfilesDisabled() { - return gstatus.Errorf(codes.Unavailable, errProfilesDisabled) - } - +// validateProfileLogout gates a profile-addressed logout. Deregistering the +// profile the daemon already runs is what a plain `netbird logout` does, so the +// profiles-disabled kill switch must not block it. Logging out of any other +// profile is profile management and stays gated. +func (s *Server) validateProfileLogout(id profilemanager.ID, isActive bool) error { if id == "" { return gstatus.Errorf(codes.InvalidArgument, "profile name must be provided") } - if !allowActiveProfile { - if err := s.canRemoveProfile(id); err != nil { - return gstatus.Errorf(codes.InvalidArgument, "%v", err) - } + if isActive { + return nil + } + + if s.checkProfilesDisabled() { + return gstatus.Errorf(codes.Unavailable, errProfilesDisabled) } return nil } -func (s *Server) logoutFromProfile(ctx context.Context, profile *profilemanager.Profile) error { +// isActiveProfile reports whether id is the profile the daemon runs for +// username. The username is part of the comparison because legacy profile IDs +// are display names, which two users can both hold; the default profile is +// shared by every user and carries no username. +func isActiveProfile(activeProf *profilemanager.ActiveProfileState, id profilemanager.ID, username string) bool { + if activeProf == nil || activeProf.ID != id { + return false + } + + return id == profilemanager.DefaultProfileName || activeProf.Username == username +} + +// logoutFromProfile deregisters profile, reusing the running config when +// profile is the one the daemon is connected with. The username takes part in +// that decision for the same reason it does in the logout gate: a legacy +// profile ID is a display name two users can share, and sending the running +// config for a namesake would deregister the active peer instead of the +// requested one. +func (s *Server) logoutFromProfile(ctx context.Context, profile *profilemanager.Profile, username string) error { activeProf, err := s.profileManager.GetActiveProfileState() - if err == nil && activeProf.ID == profile.ID && s.connectClient != nil { + if err == nil && isActiveProfile(activeProf, profile.ID, username) && s.connectClient != nil { return s.sendLogoutRequest(ctx) } @@ -2227,7 +2279,7 @@ func (s *Server) RemoveProfile(ctx context.Context, msg *proto.RemoveProfileRequ return nil, err } - if err := s.logoutFromProfile(ctx, resolved); err != nil { + if err := s.logoutFromProfile(ctx, resolved, msg.Username); err != nil { // Deregistration is best-effort here: the local profile is removed // either way, so an unprivileged caller leaves the peer registered on // the management server rather than being blocked from removing it.