From 0f593450d707b909aa7c0f45b73c3da367752d06 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Wed, 12 Aug 2026 17:27:52 +0200 Subject: [PATCH] Prefer an available OAuth flow instead of hard-coding device code --- client/cmd/login.go | 21 +- client/cmd/up.go | 4 +- client/internal/auth/auth.go | 29 ++- client/internal/auth/device_flow.go | 47 ++++- client/internal/auth/oauth.go | 303 +++++++++++++++++++++++----- client/internal/auth/oauth_test.go | 221 ++++++++++++++++++++ client/internal/auth/pkce_flow.go | 11 +- client/proto/daemon.pb.go | 44 +++- client/proto/daemon.proto | 8 + client/server/server.go | 20 +- client/ssh/common.go | 5 +- client/ui/authsession/service.go | 3 +- client/ui/services/connection.go | 9 +- util/common.go | 34 ++++ util/session_test.go | 47 +++++ 15 files changed, 695 insertions(+), 111 deletions(-) create mode 100644 client/internal/auth/oauth_test.go create mode 100644 util/session_test.go diff --git a/client/cmd/login.go b/client/cmd/login.go index a53cb6d5f..4ae83e1a9 100644 --- a/client/cmd/login.go +++ b/client/cmd/login.go @@ -5,7 +5,6 @@ import ( "fmt" "os" "os/user" - "runtime" "strings" log "github.com/sirupsen/logrus" @@ -121,7 +120,7 @@ func doDaemonLogin(ctx context.Context, cmd *cobra.Command, providedSetupKey str loginRequest := proto.LoginRequest{ SetupKey: providedSetupKey, ManagementUrl: managementURL, - IsUnixDesktopClient: isUnixRunningDesktop(), + IsUnixDesktopClient: util.HasGraphicalSession(), Hostname: hostName, DnsLabels: dnsLabelsReq, ProfileName: &handle, @@ -189,7 +188,8 @@ func doExtendSession(ctx context.Context, cmd *cobra.Command) error { client := proto.NewDaemonServiceClient(conn) - req := &proto.RequestExtendAuthSessionRequest{} + // the CLI runs in the user's session, the daemon does not: tell it what we can see + req := &proto.RequestExtendAuthSessionRequest{HasGraphicalSession: util.HasGraphicalSession()} // Pre-fill the IdP login hint from the active profile so the user // doesn't have to retype their email. Best-effort: we still proceed // without a hint if the lookup fails. @@ -408,8 +408,13 @@ func foregroundGetTokenInfo(ctx context.Context, cmd *cobra.Command, config *pro hint = profileState.Email } - oAuthFlow, err := auth.NewOAuthFlow(ctx, config, isUnixRunningDesktop(), false, hint) + oAuthFlow, err := auth.NewOAuthFlow(ctx, config, util.HasGraphicalSession(), false, hint) if err != nil { + // enrolling a device is the one flow a setup key can replace + if auth.IsSSOUnavailable(err) { + return nil, fmt.Errorf("%w. Set this device up with a setup key instead: "+ + "https://docs.netbird.io/how-to/register-machines-using-setup-keys", err) + } return nil, err } @@ -458,14 +463,6 @@ func openURL(cmd *cobra.Command, verificationURIComplete, userCode string, noBro } } -// isUnixRunningDesktop checks if a Linux OS is running desktop environment -func isUnixRunningDesktop() bool { - if runtime.GOOS != "linux" && runtime.GOOS != "freebsd" { - return false - } - return os.Getenv("DESKTOP_SESSION") != "" || os.Getenv("XDG_CURRENT_DESKTOP") != "" -} - func setEnvAndFlags(cmd *cobra.Command) error { SetFlagsFromEnvVars(rootCmd) diff --git a/client/cmd/up.go b/client/cmd/up.go index 142bcf6bd..9f4fa8c33 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -21,8 +21,8 @@ import ( "github.com/netbirdio/netbird/client/internal" "github.com/netbirdio/netbird/client/internal/peer" "github.com/netbirdio/netbird/client/internal/profilemanager" - "github.com/netbirdio/netbird/client/proto" nbnet "github.com/netbirdio/netbird/client/net" + "github.com/netbirdio/netbird/client/proto" "github.com/netbirdio/netbird/client/server" "github.com/netbirdio/netbird/client/system" "github.com/netbirdio/netbird/shared/management/domain" @@ -626,7 +626,7 @@ func setupLoginRequest(providedSetupKey string, customDNSAddressConverted []byte NatExternalIPs: natExternalIPs, CleanNATExternalIPs: natExternalIPs != nil && len(natExternalIPs) == 0, CustomDNSAddress: customDNSAddressConverted, - IsUnixDesktopClient: isUnixRunningDesktop(), + IsUnixDesktopClient: util.HasGraphicalSession(), Hostname: hostName, ExtraIFaceBlacklist: extraIFaceBlackList, DnsLabels: dnsLabels, diff --git a/client/internal/auth/auth.go b/client/internal/auth/auth.go index 153727a6c..0362dbc39 100644 --- a/client/internal/auth/auth.go +++ b/client/internal/auth/auth.go @@ -2,6 +2,7 @@ package auth import ( "context" + "errors" "net/url" "strings" "sync" @@ -140,25 +141,21 @@ func (a *Auth) IsSSOSupported(ctx context.Context) (bool, error) { // This avoids creating a new connection to the management server func (a *Auth) GetOAuthFlow(ctx context.Context, forceDeviceAuth bool) (OAuthFlow, error) { var flow OAuthFlow - var err error - err = a.withRetry(ctx, func(client *mgm.GrpcClient) error { - if forceDeviceAuth { - flow, err = a.getDeviceFlow(client) - return err - } + // the connection is owned by a and outlives this call, so a later fallback reuses it + newAuth := func(context.Context) (*Auth, func(), error) { + return a, func() {}, nil + } - // Try PKCE flow first - flow, err = a.getPKCEFlow(client) - if err != nil { - // If PKCE not supported, try Device flow - if s, ok := status.FromError(err); ok && (s.Code() == codes.NotFound || s.Code() == codes.Unimplemented) { - flow, err = a.getDeviceFlow(client) - return err - } - return err + err := a.withRetry(ctx, func(client *mgm.GrpcClient) error { + var err error + flow, err = oauthFlowWithFallback(a, client, flowOrder(forceDeviceAuth), "", newAuth) + + var ssoUnavailable *ssoUnavailableError + if errors.As(err, &ssoUnavailable) { + return backoff.Permanent(err) } - return nil + return err }) return flow, err diff --git a/client/internal/auth/device_flow.go b/client/internal/auth/device_flow.go index 9dec7cf53..e6afb6eda 100644 --- a/client/internal/auth/device_flow.go +++ b/client/internal/auth/device_flow.go @@ -48,8 +48,17 @@ type DeviceAuthProviderConfig struct { LoginHint string } -// validateDeviceAuthConfig validates device authorization provider configuration +// validateDeviceAuthConfig validates device authorization provider configuration. A missing +// value means management does not have this flow configured, so the error wraps +// errFlowNotConfigured and the caller can fall back to the other flow. func validateDeviceAuthConfig(config *DeviceAuthProviderConfig) error { + if err := checkDeviceAuthConfig(config); err != nil { + return fmt.Errorf("%w: %w", errFlowNotConfigured, err) + } + return nil +} + +func checkDeviceAuthConfig(config *DeviceAuthProviderConfig) error { errorMsgFormat := "invalid provider configuration received from management: %s value is empty. Contact your NetBird administrator" if config.Audience == "" { @@ -161,8 +170,12 @@ func (d *DeviceAuthorizationFlow) RequestAuthInfo(ctx context.Context) (AuthFlow return AuthFlowInfo{}, fmt.Errorf("reading body failed with error: %v", err) } - if res.StatusCode != 200 { - return AuthFlowInfo{}, fmt.Errorf("request device code returned status %d error: %s", res.StatusCode, string(body)) + if res.StatusCode != http.StatusOK { + reqErr := fmt.Errorf("request device code returned status %d error: %s", res.StatusCode, string(body)) + if deviceGrantUnsupported(res.StatusCode, body) { + return AuthFlowInfo{}, fmt.Errorf("%w: %w", errFlowNotConfigured, reqErr) + } + return AuthFlowInfo{}, reqErr } deviceCode := AuthFlowInfo{} @@ -186,6 +199,34 @@ func (d *DeviceAuthorizationFlow) RequestAuthInfo(ctx context.Context) (AuthFlow return deviceCode, err } +// deviceGrantUnsupported reports whether the IdP's answer to a device code request means it does +// not serve the device authorization grant at all, rather than a transient or request-specific +// failure. An IdP that does not route the endpoint answers 404/405/501; one that knows the +// endpoint but has the grant disabled for this client answers with an OAuth 2.0 error code. +func deviceGrantUnsupported(statusCode int, body []byte) bool { + switch statusCode { + case http.StatusNotFound, http.StatusMethodNotAllowed, http.StatusNotImplemented: + return true + case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden: + default: + return false + } + + var oauthErr struct { + Error string `json:"error"` + } + if err := json.Unmarshal(body, &oauthErr); err != nil { + return false + } + + switch oauthErr.Error { + case "unsupported_grant_type", "unauthorized_client", "invalid_client": + return true + default: + return false + } +} + func appendLoginHint(uri, loginHint string) string { if uri == "" || loginHint == "" { return uri diff --git a/client/internal/auth/oauth.go b/client/internal/auth/oauth.go index a50a2ce6f..58780d7c2 100644 --- a/client/internal/auth/oauth.go +++ b/client/internal/auth/oauth.go @@ -2,15 +2,19 @@ package auth import ( "context" + "errors" "fmt" "net/http" + "net/url" "runtime" + "sync" log "github.com/sirupsen/logrus" "google.golang.org/grpc/codes" gstatus "google.golang.org/grpc/status" "github.com/netbirdio/netbird/client/internal/profilemanager" + mgm "github.com/netbirdio/netbird/shared/management/client" ) // OAuthFlow represents an interface for authorization using different OAuth 2.0 flows @@ -59,77 +63,278 @@ func (t TokenInfo) GetTokenToUse() string { return t.AccessToken } -func shouldUseDeviceFlow(force bool, isUnixDesktopClient bool) bool { - return force || (runtime.GOOS == "linux" || runtime.GOOS == "freebsd") && !isUnixDesktopClient +// errFlowNotConfigured marks a flow this deployment does not offer: management returned no +// configuration for it, the configuration it returned is incomplete, or the IdP refuses to serve +// the grant. It is the only condition that makes the client try the other flow, so that a +// transient failure keeps failing on the flow the user actually wants. +var errFlowNotConfigured = errors.New("authorization flow is not configured") + +// ssoUnavailableError reports that the management server offers no usable SSO flow at all. +// Retrying cannot help, so callers should surface it to the user instead of backing off. +type ssoUnavailableError struct { + msg string } -// NewOAuthFlow initializes and returns the appropriate OAuth flow based on the management configuration -// -// It starts by initializing the PKCE.If this process fails, it resorts to the Device Code Flow, -// and if that also fails, the authentication process is deemed unsuccessful -// -// On Linux distros without desktop environment support, it only tries to initialize the Device Code Flow -// forceDeviceCodeFlow can be used to skip PKCE and go directly to Device Code Flow (e.g., for Android TV) -func NewOAuthFlow(ctx context.Context, config *profilemanager.Config, isUnixDesktopClient bool, forceDeviceCodeFlow bool, hint string) (OAuthFlow, error) { - if shouldUseDeviceFlow(forceDeviceCodeFlow, isUnixDesktopClient) { - return authenticateWithDeviceCodeFlow(ctx, config, hint) - } - - pkceFlow, err := authenticateWithPKCEFlow(ctx, config, hint) - if err != nil { - log.Debugf("failed to initialize pkce authentication with error: %v\n", err) - log.Debug("falling back to device code flow") - return authenticateWithDeviceCodeFlow(ctx, config, hint) - } - return pkceFlow, nil +func (e *ssoUnavailableError) Error() string { + return e.msg } -// authenticateWithPKCEFlow initializes the Proof Key for Code Exchange flow auth flow -func authenticateWithPKCEFlow(ctx context.Context, config *profilemanager.Config, hint string) (OAuthFlow, error) { - authClient, err := NewAuth(ctx, config.PrivateKey, config.ManagementURL, config) - if err != nil { - return nil, fmt.Errorf("failed to create auth client: %v", err) - } - defer authClient.Close() +// oauthFlowInit names one of the OAuth flows and builds it from the management configuration. +type oauthFlowInit struct { + name string + init func(a *Auth, client *mgm.GrpcClient, hint string) (OAuthFlow, error) +} - pkceFlowInfo, err := authClient.getPKCEFlow(authClient.client) +// authFactory hands out a management connection to build a flow with, plus the cleanup that +// releases it. Callers that own a long-lived connection return it with a no-op cleanup. +type authFactory func(ctx context.Context) (*Auth, func(), error) + +// fallbackFlow wraps the flow that was picked at initialization time with the flows that were +// not tried. Whether the IdP actually serves a flow only shows up when the flow is run: an IdP +// with the device grant disabled answers the device code request with 404 even though +// management handed out a device flow configuration. When that happens the wrapper swaps in the +// next flow instead of failing the login. +type fallbackFlow struct { + mu sync.Mutex + active OAuthFlow + remaining []oauthFlowInit + hint string + newAuth authFactory +} + +func (f *fallbackFlow) RequestAuthInfo(ctx context.Context) (AuthFlowInfo, error) { + info, err := f.current().RequestAuthInfo(ctx) + if err == nil || !isFlowUnavailable(err) { + return info, err + } + + next, nextErr := f.initNext(ctx) + if nextErr != nil { + log.Debugf("failed to fall back to another authorization flow: %v", nextErr) + return AuthFlowInfo{}, err + } + + return next.RequestAuthInfo(ctx) +} + +func (f *fallbackFlow) WaitToken(ctx context.Context, info AuthFlowInfo) (TokenInfo, error) { + return f.current().WaitToken(ctx, info) +} + +func (f *fallbackFlow) GetClientID(ctx context.Context) string { + return f.current().GetClientID(ctx) +} + +func (f *fallbackFlow) current() OAuthFlow { + f.mu.Lock() + defer f.mu.Unlock() + + return f.active +} + +// initNext initializes the next flow this deployment offers and makes it the active one. +func (f *fallbackFlow) initNext(ctx context.Context) (OAuthFlow, error) { + f.mu.Lock() + defer f.mu.Unlock() + + if len(f.remaining) == 0 { + return nil, errors.New("no authorization flow left to try") + } + + a, cleanup, err := f.newAuth(ctx) if err != nil { - return nil, fmt.Errorf("getting pkce authorization flow info failed with error: %v", err) + return nil, err + } + defer cleanup() + + flow, remaining, err := initFirstAvailableFlow(a, a.client, f.remaining, f.hint) + if err != nil { + return nil, err + } + + log.Infof("the identity provider does not serve the selected authorization flow, continuing with the next one") + f.active = flow + f.remaining = remaining + + return flow, nil +} + +// preferDeviceFlow reports whether the device code flow should be tried before PKCE. PKCE needs +// a browser on this host and a loopback listener to receive the redirect, neither of which +// exists on a Unix host without a graphical session. The GOOS guard keeps a caller that reports +// no graphical session on a platform that always has one from changing the preference. +func preferDeviceFlow(force bool, hasGraphicalSession bool) bool { + return force || (runtime.GOOS == "linux" || runtime.GOOS == "freebsd") && !hasGraphicalSession +} + +// flowOrder returns both flows in the order they should be attempted. +func flowOrder(preferDevice bool) []oauthFlowInit { + pkce := oauthFlowInit{name: "pkce authorization flow", init: initPKCEFlow} + device := oauthFlowInit{name: "device code flow", init: initDeviceFlow} + + if preferDevice { + return []oauthFlowInit{device, pkce} + } + return []oauthFlowInit{pkce, device} +} + +func initPKCEFlow(a *Auth, client *mgm.GrpcClient, hint string) (OAuthFlow, error) { + flow, err := a.getPKCEFlow(client) + if err != nil { + return nil, err } if hint != "" { - pkceFlowInfo.SetLoginHint(hint) + flow.SetLoginHint(hint) } - return pkceFlowInfo, nil + return flow, nil } -// authenticateWithDeviceCodeFlow initializes the Device Code auth Flow -func authenticateWithDeviceCodeFlow(ctx context.Context, config *profilemanager.Config, hint string) (OAuthFlow, error) { +func initDeviceFlow(a *Auth, client *mgm.GrpcClient, hint string) (OAuthFlow, error) { + flow, err := a.getDeviceFlow(client) + if err != nil { + return nil, err + } + + if hint != "" { + flow.SetLoginHint(hint) + } + + return flow, nil +} + +// NewOAuthFlow initializes and returns an OAuth flow based on the management configuration. +// +// Both flows are optional server side: management answers NotFound for a flow it has no +// configuration for. The preferred flow is tried first and the other one is used as a fallback, +// so a server that only offers one of them still works. forceDeviceCodeFlow prefers the device +// code flow regardless of platform (e.g. for Android TV). +func NewOAuthFlow(ctx context.Context, config *profilemanager.Config, hasGraphicalSession bool, forceDeviceCodeFlow bool, hint string) (OAuthFlow, error) { authClient, err := NewAuth(ctx, config.PrivateKey, config.ManagementURL, config) if err != nil { - return nil, fmt.Errorf("failed to create auth client: %v", err) + return nil, fmt.Errorf("create auth client: %w", err) } defer authClient.Close() - deviceFlowInfo, err := authClient.getDeviceFlow(authClient.client) + // the connection above is closed on return, so a later fallback opens its own + newAuth := func(ctx context.Context) (*Auth, func(), error) { + a, err := NewAuth(ctx, config.PrivateKey, config.ManagementURL, config) + if err != nil { + return nil, nil, fmt.Errorf("create auth client: %w", err) + } + return a, func() { + if err := a.Close(); err != nil { + log.Debugf("failed to close auth client: %v", err) + } + }, nil + } + + flows := flowOrder(preferDeviceFlow(forceDeviceCodeFlow, hasGraphicalSession)) + return oauthFlowWithFallback(authClient, authClient.client, flows, hint, newAuth) +} + +// oauthFlowWithFallback initializes the first flow this deployment offers, moving on to the next +// one when a flow is not configured here. It only fails once every flow has been tried, and any +// flow left untried is handed to the returned flow so it can still fall back if the IdP rejects +// the flow that was picked. +func oauthFlowWithFallback(a *Auth, client *mgm.GrpcClient, flows []oauthFlowInit, hint string, newAuth authFactory) (OAuthFlow, error) { + flow, remaining, err := initFirstAvailableFlow(a, client, flows, hint) if err != nil { - switch s, ok := gstatus.FromError(err); { - case ok && s.Code() == codes.NotFound: - return nil, fmt.Errorf("no SSO provider returned from management. " + - "Please proceed with setting up this device using setup keys " + - "https://docs.netbird.io/how-to/register-machines-using-setup-keys") - case ok && s.Code() == codes.Unimplemented: - return nil, fmt.Errorf("the management server, %s, does not support SSO providers, "+ - "please update your server or use Setup Keys to login", config.ManagementURL) - default: - return nil, fmt.Errorf("getting device authorization flow info failed with error: %v", err) + return nil, err + } + + if len(remaining) == 0 { + return flow, nil + } + + return &fallbackFlow{ + active: flow, + remaining: remaining, + hint: hint, + newAuth: newAuth, + }, nil +} + +// initFirstAvailableFlow returns the first flow that could be initialized along with the flows +// after it, which are still untried. +func initFirstAvailableFlow(a *Auth, client *mgm.GrpcClient, flows []oauthFlowInit, hint string) (OAuthFlow, []oauthFlowInit, error) { + var errs []error + for i, f := range flows { + flow, err := f.init(a, client, hint) + if err == nil { + return flow, flows[i+1:], nil + } + + errs = append(errs, fmt.Errorf("%s: %w", f.name, err)) + + // only a flow this deployment does not offer is worth replacing with another one + if !isFlowUnavailable(err) { + break + } + if i < len(flows)-1 { + log.Infof("%s is not configured (%v), falling back to %s", f.name, err, flows[i+1].name) } } - if hint != "" { - deviceFlowInfo.SetLoginHint(hint) + return nil, nil, flowInitError(a.mgmURL, errs) +} + +// flowInitError turns the per-flow initialization errors into a single actionable error. The +// message stays neutral about what to do instead: SSO is also how a peer extends its session and +// authenticates SSH, where a setup key is no alternative. Callers that are enrolling a device add +// that advice themselves, see IsSSOUnavailable. +func flowInitError(mgmURL *url.URL, errs []error) error { + if allMatch(errs, isFlowUnimplemented) { + return &ssoUnavailableError{msg: fmt.Sprintf("the management server, %s, does not support SSO providers, "+ + "please update your server", mgmURL)} } - return deviceFlowInfo, nil + if allMatch(errs, isFlowUnavailable) { + return &ssoUnavailableError{msg: "the management server has no SSO provider configured: " + + "neither the pkce authorization flow nor the device code flow is available"} + } + + return fmt.Errorf("initialize authorization flow: %w", errors.Join(errs...)) +} + +// IsSSOUnavailable reports whether err means the management server offers no usable SSO flow, so +// no retry and no other flow can help. Enrollment paths use it to point the user at setup keys. +func IsSSOUnavailable(err error) bool { + var ssoUnavailable *ssoUnavailableError + return errors.As(err, &ssoUnavailable) +} + +func allMatch(errs []error, match func(error) bool) bool { + if len(errs) == 0 { + return false + } + + for _, err := range errs { + if !match(err) { + return false + } + } + return true +} + +// isFlowUnavailable reports whether the flow is not on offer here: management has no +// configuration for it (NotFound), predates the RPC entirely (Unimplemented), returned an +// incomplete configuration, or the IdP does not serve the grant. +func isFlowUnavailable(err error) bool { + return errors.Is(err, errFlowNotConfigured) || + hasStatusCode(err, codes.NotFound) || + hasStatusCode(err, codes.Unimplemented) +} + +func isFlowUnimplemented(err error) bool { + return hasStatusCode(err, codes.Unimplemented) +} + +func hasStatusCode(err error, code codes.Code) bool { + s, ok := gstatus.FromError(err) + if !ok { + return false + } + return s.Code() == code } diff --git a/client/internal/auth/oauth_test.go b/client/internal/auth/oauth_test.go new file mode 100644 index 000000000..89df61afd --- /dev/null +++ b/client/internal/auth/oauth_test.go @@ -0,0 +1,221 @@ +package auth + +import ( + "context" + "errors" + "fmt" + "net/url" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + mgm "github.com/netbirdio/netbird/shared/management/client" +) + +// stubFlow is a minimal OAuthFlow returned by the fake initializers below. requestErr, when set, +// is what its RequestAuthInfo returns, standing in for an IdP that rejects the flow. +type stubFlow struct { + name string + hint string + requestErr error +} + +func (s *stubFlow) RequestAuthInfo(context.Context) (AuthFlowInfo, error) { + if s.requestErr != nil { + return AuthFlowInfo{}, s.requestErr + } + return AuthFlowInfo{UserCode: s.name}, nil +} + +func (s *stubFlow) WaitToken(context.Context, AuthFlowInfo) (TokenInfo, error) { + return TokenInfo{}, nil +} + +func (s *stubFlow) GetClientID(context.Context) string { + return "" +} + +// stubInit returns a flow initializer that yields a named stub flow, or err when err is non-nil. +func stubInit(name string, err error) oauthFlowInit { + return stubInitFlow(name, err, nil) +} + +// stubInitFlow is stubInit with control over what the resulting flow's RequestAuthInfo returns. +func stubInitFlow(name string, initErr, requestErr error) oauthFlowInit { + return oauthFlowInit{ + name: name, + init: func(_ *Auth, _ *mgm.GrpcClient, hint string) (OAuthFlow, error) { + if initErr != nil { + return nil, initErr + } + return &stubFlow{name: name, hint: hint, requestErr: requestErr}, nil + }, + } +} + +// stubAuthFactory hands out an Auth without a management connection, which the stub +// initializers above never touch. +func stubAuthFactory(a *Auth) authFactory { + return func(context.Context) (*Auth, func(), error) { + return a, func() {}, nil + } +} + +func TestOAuthFlowWithFallback(t *testing.T) { + notFound := status.Error(codes.NotFound, "no device authorization flow information available") + unimplemented := status.Error(codes.Unimplemented, "unknown method") + incompleteConfig := fmt.Errorf("%w: Client ID value is empty", errFlowNotConfigured) + unreachable := status.Error(codes.Unavailable, "connection refused") + + tests := []struct { + name string + flows []oauthFlowInit + expectedFlow string + expectedErr string + expectedNoSSO bool + }{ + { + name: "preferred flow is used", + flows: []oauthFlowInit{stubInit("device", nil), stubInit("pkce", nil)}, + expectedFlow: "device", + }, + { + // the RedHat case: device code flow disabled on management, PKCE configured + name: "falls back when preferred flow is not configured", + flows: []oauthFlowInit{stubInit("device", notFound), stubInit("pkce", nil)}, + expectedFlow: "pkce", + }, + { + name: "falls back on an incomplete flow configuration", + flows: []oauthFlowInit{stubInit("pkce", incompleteConfig), stubInit("device", nil)}, + expectedFlow: "device", + }, + { + name: "does not fall back when the preferred flow fails for another reason", + flows: []oauthFlowInit{stubInit("pkce", unreachable), stubInit("device", nil)}, + expectedErr: "connection refused", + }, + { + // stays neutral about the remedy: --extend and SSH auth cannot use a setup key + name: "neither flow configured reports no SSO provider", + flows: []oauthFlowInit{stubInit("device", notFound), stubInit("pkce", notFound)}, + expectedErr: "no SSO provider configured", + expectedNoSSO: true, + }, + { + name: "old server without the flow RPCs asks for an update", + flows: []oauthFlowInit{stubInit("device", unimplemented), stubInit("pkce", unimplemented)}, + expectedErr: "does not support SSO providers", + expectedNoSSO: true, + }, + } + + mgmURL, err := url.Parse("https://api.netbird.io:443") + require.NoError(t, err) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := &Auth{mgmURL: mgmURL} + flow, err := oauthFlowWithFallback(a, nil, tt.flows, "user@example.com", stubAuthFactory(a)) + + if tt.expectedErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.expectedErr) + + var ssoUnavailable *ssoUnavailableError + assert.Equal(t, tt.expectedNoSSO, errors.As(err, &ssoUnavailable), + "terminal SSO-unavailable classification mismatch for %v", err) + return + } + + require.NoError(t, err) + stub := activeStub(t, flow) + assert.Equal(t, tt.expectedFlow, stub.name) + assert.Equal(t, "user@example.com", stub.hint, "login hint must be passed to the flow") + }) + } +} + +// activeStub unwraps the flow currently in use, which is behind a fallbackFlow whenever an +// untried flow is left. +func activeStub(t *testing.T, flow OAuthFlow) *stubFlow { + t.Helper() + + if fallback, ok := flow.(*fallbackFlow); ok { + flow = fallback.current() + } + + stub, ok := flow.(*stubFlow) + require.True(t, ok, "unexpected flow type %T", flow) + return stub +} + +// TestFallbackFlowRequestAuthInfo covers the failure the RedHat report hit: management hands out +// a device flow configuration, but the IdP does not serve the grant and only says so when the +// device code is requested. +func TestFallbackFlowRequestAuthInfo(t *testing.T) { + mgmURL, err := url.Parse("https://api.netbird.io:443") + require.NoError(t, err) + a := &Auth{mgmURL: mgmURL} + + idpRejects := fmt.Errorf("%w: request device code returned status 404", errFlowNotConfigured) + + t.Run("swaps in the untried flow", func(t *testing.T) { + flows := []oauthFlowInit{stubInitFlow("device", nil, idpRejects), stubInit("pkce", nil)} + + flow, err := oauthFlowWithFallback(a, nil, flows, "", stubAuthFactory(a)) + require.NoError(t, err) + require.Equal(t, "device", activeStub(t, flow).name) + + info, err := flow.RequestAuthInfo(context.Background()) + require.NoError(t, err) + assert.Equal(t, "pkce", info.UserCode, "the request must be served by the fallback flow") + assert.Equal(t, "pkce", activeStub(t, flow).name, "the fallback flow must stay active for WaitToken") + }) + + t.Run("keeps the original error when nothing else is configured", func(t *testing.T) { + flows := []oauthFlowInit{ + stubInitFlow("device", nil, idpRejects), + stubInit("pkce", status.Error(codes.NotFound, "no pkce authorization flow information available")), + } + + flow, err := oauthFlowWithFallback(a, nil, flows, "", stubAuthFactory(a)) + require.NoError(t, err) + + _, err = flow.RequestAuthInfo(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "status 404") + }) + + t.Run("does not swap flows on an unrelated failure", func(t *testing.T) { + flows := []oauthFlowInit{ + stubInitFlow("device", nil, errors.New("timeout talking to the IdP")), + stubInit("pkce", nil), + } + + flow, err := oauthFlowWithFallback(a, nil, flows, "", stubAuthFactory(a)) + require.NoError(t, err) + + _, err = flow.RequestAuthInfo(context.Background()) + require.Error(t, err) + assert.Equal(t, "device", activeStub(t, flow).name, "the preferred flow must stay active") + }) +} + +func TestFlowOrder(t *testing.T) { + assert.Equal(t, "pkce authorization flow", flowOrder(false)[0].name) + assert.Equal(t, "device code flow", flowOrder(true)[0].name) + assert.Len(t, flowOrder(false), 2, "both flows must always be attempted") +} + +func TestPreferDeviceFlow(t *testing.T) { + isUnix := runtime.GOOS == "linux" || runtime.GOOS == "freebsd" + + assert.True(t, preferDeviceFlow(true, true), "forced device flow wins over a desktop session") + assert.Equal(t, isUnix, preferDeviceFlow(false, false), "headless unix hosts prefer the device flow") + assert.False(t, preferDeviceFlow(false, true), "desktop clients prefer PKCE") +} diff --git a/client/internal/auth/pkce_flow.go b/client/internal/auth/pkce_flow.go index be64cc6a8..71e39a8a5 100644 --- a/client/internal/auth/pkce_flow.go +++ b/client/internal/auth/pkce_flow.go @@ -62,8 +62,17 @@ type PKCEAuthProviderConfig struct { LoginHint string } -// validatePKCEConfig validates PKCE provider configuration +// validatePKCEConfig validates PKCE provider configuration. A missing value means management +// does not have this flow configured, so the error wraps errFlowNotConfigured and the caller can +// fall back to the other flow. func validatePKCEConfig(config *PKCEAuthProviderConfig) error { + if err := checkPKCEConfig(config); err != nil { + return fmt.Errorf("%w: %w", errFlowNotConfigured, err) + } + return nil +} + +func checkPKCEConfig(config *PKCEAuthProviderConfig) error { errorMsgFormat := "invalid provider configuration received from management: %s value is empty. Contact your NetBird administrator" if config.ClientID == "" { diff --git a/client/proto/daemon.pb.go b/client/proto/daemon.pb.go index 83243be49..b438a310a 100644 --- a/client/proto/daemon.pb.go +++ b/client/proto/daemon.pb.go @@ -5628,9 +5628,13 @@ func (x *GetPeerSSHHostKeyResponse) GetFound() bool { type RequestJWTAuthRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // hint for OIDC login_hint parameter (typically email address) - Hint *string `protobuf:"bytes,1,opt,name=hint,proto3,oneof" json:"hint,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + Hint *string `protobuf:"bytes,1,opt,name=hint,proto3,oneof" json:"hint,omitempty"` + // hasGraphicalSession tells the daemon that the caller has a graphical session, + // which decides whether PKCE or the device code flow is preferred. The daemon + // cannot detect this itself: it does not inherit the session environment. + HasGraphicalSession bool `protobuf:"varint,2,opt,name=hasGraphicalSession,proto3" json:"hasGraphicalSession,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *RequestJWTAuthRequest) Reset() { @@ -5670,6 +5674,13 @@ func (x *RequestJWTAuthRequest) GetHint() string { return "" } +func (x *RequestJWTAuthRequest) GetHasGraphicalSession() bool { + if x != nil { + return x.HasGraphicalSession + } + return false +} + // RequestJWTAuthResponse contains authentication flow information type RequestJWTAuthResponse struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -5894,9 +5905,13 @@ type RequestExtendAuthSessionRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // Optional OIDC login_hint (typically the user's email) to pre-fill the // IdP login form. - Hint *string `protobuf:"bytes,1,opt,name=hint,proto3,oneof" json:"hint,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + Hint *string `protobuf:"bytes,1,opt,name=hint,proto3,oneof" json:"hint,omitempty"` + // hasGraphicalSession tells the daemon that the caller has a graphical session, + // which decides whether PKCE or the device code flow is preferred. The daemon + // cannot detect this itself: it does not inherit the session environment. + HasGraphicalSession bool `protobuf:"varint,2,opt,name=hasGraphicalSession,proto3" json:"hasGraphicalSession,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *RequestExtendAuthSessionRequest) Reset() { @@ -5936,6 +5951,13 @@ func (x *RequestExtendAuthSessionRequest) GetHint() string { return "" } +func (x *RequestExtendAuthSessionRequest) GetHasGraphicalSession() bool { + if x != nil { + return x.HasGraphicalSession + } + return false +} + // RequestExtendAuthSessionResponse carries the verification URI the UI // should open in a browser. The daemon retains the flow state and resolves // it via WaitExtendAuthSession. @@ -7503,9 +7525,10 @@ const file_daemon_proto_rawDesc = "" + "sshHostKey\x12\x16\n" + "\x06peerIP\x18\x02 \x01(\tR\x06peerIP\x12\x1a\n" + "\bpeerFQDN\x18\x03 \x01(\tR\bpeerFQDN\x12\x14\n" + - "\x05found\x18\x04 \x01(\bR\x05found\"9\n" + + "\x05found\x18\x04 \x01(\bR\x05found\"k\n" + "\x15RequestJWTAuthRequest\x12\x17\n" + - "\x04hint\x18\x01 \x01(\tH\x00R\x04hint\x88\x01\x01B\a\n" + + "\x04hint\x18\x01 \x01(\tH\x00R\x04hint\x88\x01\x01\x120\n" + + "\x13hasGraphicalSession\x18\x02 \x01(\bR\x13hasGraphicalSessionB\a\n" + "\x05_hint\"\x9a\x02\n" + "\x16RequestJWTAuthResponse\x12(\n" + "\x0fverificationURI\x18\x01 \x01(\tR\x0fverificationURI\x128\n" + @@ -7525,9 +7548,10 @@ const file_daemon_proto_rawDesc = "" + "\x14WaitJWTTokenResponse\x12\x14\n" + "\x05token\x18\x01 \x01(\tR\x05token\x12\x1c\n" + "\ttokenType\x18\x02 \x01(\tR\ttokenType\x12\x1c\n" + - "\texpiresIn\x18\x03 \x01(\x03R\texpiresIn\"C\n" + + "\texpiresIn\x18\x03 \x01(\x03R\texpiresIn\"u\n" + "\x1fRequestExtendAuthSessionRequest\x12\x17\n" + - "\x04hint\x18\x01 \x01(\tH\x00R\x04hint\x88\x01\x01B\a\n" + + "\x04hint\x18\x01 \x01(\tH\x00R\x04hint\x88\x01\x01\x120\n" + + "\x13hasGraphicalSession\x18\x02 \x01(\bR\x13hasGraphicalSessionB\a\n" + "\x05_hint\"\xe0\x01\n" + " RequestExtendAuthSessionResponse\x12(\n" + "\x0fverificationURI\x18\x01 \x01(\tR\x0fverificationURI\x128\n" + diff --git a/client/proto/daemon.proto b/client/proto/daemon.proto index 18ce0e79c..a3e3f4500 100644 --- a/client/proto/daemon.proto +++ b/client/proto/daemon.proto @@ -894,6 +894,10 @@ message GetPeerSSHHostKeyResponse { message RequestJWTAuthRequest { // hint for OIDC login_hint parameter (typically email address) optional string hint = 1; + // hasGraphicalSession tells the daemon that the caller has a graphical session, + // which decides whether PKCE or the device code flow is preferred. The daemon + // cannot detect this itself: it does not inherit the session environment. + bool hasGraphicalSession = 2; } // RequestJWTAuthResponse contains authentication flow information @@ -937,6 +941,10 @@ message RequestExtendAuthSessionRequest { // Optional OIDC login_hint (typically the user's email) to pre-fill the // IdP login form. optional string hint = 1; + // hasGraphicalSession tells the daemon that the caller has a graphical session, + // which decides whether PKCE or the device code flow is preferred. The daemon + // cannot detect this itself: it does not inherit the session environment. + bool hasGraphicalSession = 2; } // RequestExtendAuthSessionResponse carries the verification URI the UI diff --git a/client/server/server.go b/client/server/server.go index 01778b8e0..a9e70b183 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -682,6 +682,11 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro oAuthFlow, err := auth.NewOAuthFlow(ctx, config, msg.IsUnixDesktopClient, false, hint) if err != nil { state.Set(internal.StatusLoginFailed) + // enrolling a device is the one flow a setup key can replace + if auth.IsSSOUnavailable(err) { + return nil, fmt.Errorf("%w. Set this device up with a setup key instead: "+ + "https://docs.netbird.io/how-to/register-machines-using-setup-keys", err) + } return nil, err } @@ -1723,8 +1728,8 @@ func (s *Server) RequestJWTAuth( hint = profilemanager.GetLoginHint() } - isDesktop := isUnixRunningDesktop() - oAuthFlow, err := auth.NewOAuthFlow(ctx, config, isDesktop, false, hint) + // the daemon has no graphical session of its own, only the caller can answer this + oAuthFlow, err := auth.NewOAuthFlow(ctx, config, msg.GetHasGraphicalSession(), false, hint) if err != nil { return nil, gstatus.Errorf(codes.Internal, "failed to create OAuth flow: %v", err) } @@ -1827,8 +1832,8 @@ func (s *Server) RequestExtendAuthSession( hint = profilemanager.GetLoginHint() } - isDesktop := isUnixRunningDesktop() - oAuthFlow, err := auth.NewOAuthFlow(ctx, config, isDesktop, false, hint) + // the daemon has no graphical session of its own, only the caller can answer this + oAuthFlow, err := auth.NewOAuthFlow(ctx, config, msg.GetHasGraphicalSession(), false, hint) if err != nil { return nil, gstatus.Errorf(codes.Internal, "failed to create OAuth flow: %v", err) } @@ -2000,13 +2005,6 @@ func (s *Server) ExposeService(req *proto.ExposeServiceRequest, srv proto.Daemon return nil } -func isUnixRunningDesktop() bool { - if runtime.GOOS != "linux" && runtime.GOOS != "freebsd" { - return false - } - return os.Getenv("DESKTOP_SESSION") != "" || os.Getenv("XDG_CURRENT_DESKTOP") != "" -} - func (s *Server) runProbes(ctx context.Context, waitForProbeResult bool) { if s.connectClient == nil { return diff --git a/client/ssh/common.go b/client/ssh/common.go index 92e647b7d..3f4f3e9d1 100644 --- a/client/ssh/common.go +++ b/client/ssh/common.go @@ -13,6 +13,7 @@ import ( "golang.org/x/crypto/ssh" "github.com/netbirdio/netbird/client/proto" + "github.com/netbirdio/netbird/util" ) const ( @@ -92,7 +93,8 @@ func printAuthInstructions(stderr io.Writer, authResponse *proto.RequestJWTAuthR // RequestJWTToken requests or retrieves a JWT token for SSH authentication func RequestJWTToken(ctx context.Context, client proto.DaemonServiceClient, stdout, stderr io.Writer, useCache bool, hint string, openBrowser func(string) error) (string, error) { - req := &proto.RequestJWTAuthRequest{} + // the ssh client runs in the user's session, the daemon does not: tell it what we can see + req := &proto.RequestJWTAuthRequest{HasGraphicalSession: util.HasGraphicalSession()} if hint != "" { req.Hint = &hint } @@ -193,4 +195,3 @@ func buildAddressList(hostname string, remote net.Addr) []string { } return addresses } - diff --git a/client/ui/authsession/service.go b/client/ui/authsession/service.go index 28efe7cfd..d94cef696 100644 --- a/client/ui/authsession/service.go +++ b/client/ui/authsession/service.go @@ -58,7 +58,8 @@ func (s *Session) RequestExtend(ctx context.Context, p ExtendStartParams) (Exten return ExtendStartResult{}, err } - req := &proto.RequestExtendAuthSessionRequest{} + // a request from the UI implies a graphical session, which the daemon cannot detect itself + req := &proto.RequestExtendAuthSessionRequest{HasGraphicalSession: true} if p.Hint != "" { h := p.Hint req.Hint = &h diff --git a/client/ui/services/connection.go b/client/ui/services/connection.go index 1069f8754..aa649bb6d 100644 --- a/client/ui/services/connection.go +++ b/client/ui/services/connection.go @@ -108,10 +108,11 @@ func (s *Connection) Login(ctx context.Context, p LoginParams) (LoginResult, err } req := &proto.LoginRequest{ - ManagementUrl: p.ManagementURL, - SetupKey: p.SetupKey, - Hostname: p.Hostname, - IsUnixDesktopClient: runtime.GOOS == "linux", + ManagementUrl: p.ManagementURL, + SetupKey: p.SetupKey, + Hostname: p.Hostname, + // a login driven by the UI always has a graphical session available + IsUnixDesktopClient: true, } if profileName != "" { req.ProfileName = ptrStr(profileName) diff --git a/util/common.go b/util/common.go index 89903b609..e5f08052c 100644 --- a/util/common.go +++ b/util/common.go @@ -3,6 +3,7 @@ package util import ( "os" "os/exec" + "runtime" "github.com/skratchdot/open-golang/open" ) @@ -15,6 +16,39 @@ func OpenBrowser(url string) error { return open.Run(url) } +// browserSessionEnvVars returns the variables that decide whether OpenBrowser can open a URL: +// BROWSER is the explicit override it honors first, DESKTOP_SESSION and XDG_CURRENT_DESKTOP are +// what xdg-open uses to pick a handler, and DISPLAY / WAYLAND_DISPLAY are what any graphical +// browser it launches needs. +func browserSessionEnvVars() []string { + return []string{"BROWSER", "DESKTOP_SESSION", "XDG_CURRENT_DESKTOP", "DISPLAY", "WAYLAND_DISPLAY"} +} + +// HasGraphicalSession reports whether this process can open a browser and serve a loopback +// redirect back to it. Windows and macOS always can. On Linux and FreeBSD the answer is env +// based, so it only holds for a process started from the graphical session itself: a service +// does not inherit those variables and always reports false, which is why callers running in +// the user's session pass their own answer to the daemon. +func HasGraphicalSession() bool { + if runtime.GOOS != "linux" && runtime.GOOS != "freebsd" { + return true + } + + for _, env := range browserSessionEnvVars() { + if os.Getenv(env) != "" { + return true + } + } + + // tty and unspecified sessions have no display; anything else (x11, wayland, mir) does + switch os.Getenv("XDG_SESSION_TYPE") { + case "", "tty", "unspecified": + return false + default: + return true + } +} + // SliceDiff returns the elements in slice `x` that are not in slice `y` func SliceDiff(x, y []string) []string { mapY := make(map[string]struct{}, len(y)) diff --git a/util/session_test.go b/util/session_test.go new file mode 100644 index 000000000..f37f692ff --- /dev/null +++ b/util/session_test.go @@ -0,0 +1,47 @@ +package util + +import ( + "os" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestHasGraphicalSession(t *testing.T) { + if runtime.GOOS != "linux" && runtime.GOOS != "freebsd" { + assert.True(t, HasGraphicalSession(), "%s always has a graphical session", runtime.GOOS) + return + } + + // clear anything inherited from the session running the test, restored on cleanup + for _, env := range append(browserSessionEnvVars(), "XDG_SESSION_TYPE") { + t.Setenv(env, "") + os.Unsetenv(env) + } + + assert.False(t, HasGraphicalSession(), "no session variables means no graphical session") + + tests := []struct { + env string + value string + expected bool + }{ + {env: "DISPLAY", value: ":0", expected: true}, + {env: "WAYLAND_DISPLAY", value: "wayland-0", expected: true}, + {env: "DESKTOP_SESSION", value: "gnome", expected: true}, + {env: "XDG_CURRENT_DESKTOP", value: "KDE", expected: true}, + {env: "BROWSER", value: "firefox", expected: true}, + {env: "XDG_SESSION_TYPE", value: "wayland", expected: true}, + {env: "XDG_SESSION_TYPE", value: "x11", expected: true}, + {env: "XDG_SESSION_TYPE", value: "tty", expected: false}, + {env: "XDG_SESSION_TYPE", value: "unspecified", expected: false}, + } + + for _, tt := range tests { + t.Run(tt.env+"="+tt.value, func(t *testing.T) { + t.Setenv(tt.env, tt.value) + assert.Equal(t, tt.expected, HasGraphicalSession(), "%s=%s", tt.env, tt.value) + }) + } +}