From e290769df10ceb7fd0176c9c8c2cca2d2d545c86 Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:28:34 +0900 Subject: [PATCH] [client] Take the graphical session answer from the caller instead of the daemon environment (#7187) --- client/cmd/login.go | 16 +++------- client/cmd/up.go | 4 +-- client/proto/daemon.pb.go | 44 ++++++++++++++++++++------ client/proto/daemon.proto | 8 +++++ client/server/server.go | 15 +++------ client/ssh/common.go | 5 +-- client/ui/authsession/service.go | 3 +- client/ui/services/connection.go | 9 +++--- util/common.go | 53 +++++++++++++++++++++++++++++++- util/session_test.go | 50 ++++++++++++++++++++++++++++++ 10 files changed, 164 insertions(+), 43 deletions(-) create mode 100644 util/session_test.go diff --git a/client/cmd/login.go b/client/cmd/login.go index a53cb6d5f..6aa019896 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,7 +408,7 @@ 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 { return nil, err } @@ -458,14 +458,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/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..f33e19075 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -1723,8 +1723,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 +1827,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 +2000,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..c08be3617 100644 --- a/util/common.go +++ b/util/common.go @@ -3,18 +3,69 @@ package util import ( "os" "os/exec" + "runtime" + "slices" "github.com/skratchdot/open-golang/open" ) +const ( + // envBrowser overrides the browser OpenBrowser launches + envBrowser = "BROWSER" + // envDesktopSession and envXDGCurrentDesktop are what xdg-open uses to pick a handler + envDesktopSession = "DESKTOP_SESSION" + envXDGCurrentDesktop = "XDG_CURRENT_DESKTOP" + // envDisplay and envWaylandDisplay are what a graphical browser needs to reach a display + envDisplay = "DISPLAY" + envWaylandDisplay = "WAYLAND_DISPLAY" + // envXDGSessionType names the session kind, e.g. tty, x11 or wayland + envXDGSessionType = "XDG_SESSION_TYPE" +) + // OpenBrowser opens the URL in a browser, respecting the BROWSER environment variable. func OpenBrowser(url string) error { - if browser := os.Getenv("BROWSER"); browser != "" { + if browser := os.Getenv(envBrowser); browser != "" { return exec.Command(browser, url).Start() } return open.Run(url) } +// browserSessionEnvVars returns the variables that decide whether OpenBrowser can open a URL. +// DISPLAY and WAYLAND_DISPLAY are exactly what xdg-open's own has_display() checks, and without +// them it degrades to terminal browsers. BROWSER is the explicit override both xdg-open and +// OpenBrowser honor first. DESKTOP_SESSION and XDG_CURRENT_DESKTOP only tell xdg-open which +// desktop-specific opener to prefer, so they are weaker evidence, kept because the previous +// detection relied on them alone and dropping them would demote sessions that work today. +func browserSessionEnvVars() []string { + return []string{envDisplay, envWaylandDisplay, envBrowser, envDesktopSession, envXDGCurrentDesktop} +} + +// graphicalXDGSessionTypes are the systemd-logind session types that come with a display. The +// other documented values are "tty" and "unspecified"; anything unrecognized is treated as no +// display, so an unknown value picks the device code flow, which works without a browser. +func graphicalXDGSessionTypes() []string { + return []string{"x11", "wayland", "mir"} +} + +// 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 + } + } + + return slices.Contains(graphicalXDGSessionTypes(), os.Getenv(envXDGSessionType)) +} + // 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..f301ff8f7 --- /dev/null +++ b/util/session_test.go @@ -0,0 +1,50 @@ +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(), envXDGSessionType) { + 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: envDisplay, value: ":0", expected: true}, + {env: envWaylandDisplay, value: "wayland-0", expected: true}, + {env: envDesktopSession, value: "gnome", expected: true}, + {env: envXDGCurrentDesktop, value: "KDE", expected: true}, + {env: envBrowser, value: "firefox", expected: true}, + {env: envXDGSessionType, value: "wayland", expected: true}, + {env: envXDGSessionType, value: "x11", expected: true}, + {env: envXDGSessionType, value: "mir", expected: true}, + {env: envXDGSessionType, value: "tty", expected: false}, + {env: envXDGSessionType, value: "unspecified", expected: false}, + // an unrecognized type must not be read as a display: the device code flow works anyway + {env: envXDGSessionType, value: "something-new", 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) + }) + } +}