diff --git a/client/internal/engine_vnc.go b/client/internal/engine_vnc.go index 12d803360..8270b2e88 100644 --- a/client/internal/engine_vnc.go +++ b/client/internal/engine_vnc.go @@ -321,11 +321,29 @@ func (a *vncApprover) Request(ctx context.Context, info vncserver.ApprovalInfo) Metadata: meta, }) if err != nil { - return vncserver.ApprovalDecision{}, err + return vncserver.ApprovalDecision{}, approvalCause(err) } return vncserver.ApprovalDecision{ViewOnly: d.ViewOnly}, nil } +// approvalCause restates a broker failure as the cause the VNC server +// classifies, so the peer that dialled learns whether the user refused or +// never answered. The broker's denial and timeout carry nothing beyond the +// sentinel, so those are returned as the server's own; the two unavailable +// cases differ in a way worth keeping, so they are wrapped. +func approvalCause(err error) error { + switch { + case errors.Is(err, approval.ErrDenied): + return vncserver.ErrApprovalDenied + case errors.Is(err, approval.ErrTimeout): + return vncserver.ErrApprovalTimeout + case errors.Is(err, approval.ErrNoSubscriber), errors.Is(err, approval.ErrPromptNotShown): + return fmt.Errorf("%w: %w", vncserver.ErrApprovalUnavailable, err) + default: + return err + } +} + func displayPeer(info vncserver.ApprovalInfo) string { if info.Initiator != "" { return info.Initiator diff --git a/client/internal/engine_vnc_test.go b/client/internal/engine_vnc_test.go new file mode 100644 index 000000000..70f666928 --- /dev/null +++ b/client/internal/engine_vnc_test.go @@ -0,0 +1,60 @@ +//go:build !js && !ios && !android + +package internal + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/netbirdio/netbird/client/internal/approval" + vncserver "github.com/netbirdio/netbird/client/vnc/server" +) + +// TestApprovalCause checks that each way the broker can refuse arrives at +// the VNC server as the matching cause. Without this the server sees one +// opaque error and tells every caller it was denied, which reads as "the +// user said no" even when no prompt was ever answered or shown. +func TestApprovalCause(t *testing.T) { + cases := []struct { + name string + in error + want error + }{ + {"denied", approval.ErrDenied, vncserver.ErrApprovalDenied}, + {"timeout", approval.ErrTimeout, vncserver.ErrApprovalTimeout}, + {"no_subscriber", approval.ErrNoSubscriber, vncserver.ErrApprovalUnavailable}, + {"prompt_not_shown", approval.ErrPromptNotShown, vncserver.ErrApprovalUnavailable}, + // Wrapped on the way in: the broker may add context, and the + // classification must survive it. + {"wrapped_timeout", fmt.Errorf("request: %w", approval.ErrTimeout), vncserver.ErrApprovalTimeout}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := approvalCause(tc.in) + assert.ErrorIs(t, got, tc.want, "broker error %v must classify as %v", tc.in, tc.want) + }) + } +} + +// TestApprovalCausePreservesUnavailableDetail: the two unavailable causes +// mean different things to whoever reads the daemon log ("nobody was +// listening" versus "somebody was, and never got it"), so collapsing them +// onto one sentinel must not discard which one occurred. +func TestApprovalCausePreservesUnavailableDetail(t *testing.T) { + got := approvalCause(approval.ErrPromptNotShown) + assert.ErrorIs(t, got, vncserver.ErrApprovalUnavailable) + assert.ErrorIs(t, got, approval.ErrPromptNotShown, "the specific cause must stay readable in the log") +} + +// TestApprovalCausePassesThroughUnknown keeps an error the mapping does not +// recognise intact, so the daemon log still shows what actually happened. +// The server rejects it as a denial either way. +func TestApprovalCausePassesThroughUnknown(t *testing.T) { + for _, err := range []error{context.Canceled, errors.New("something else")} { + assert.ErrorIs(t, approvalCause(err), err, "unknown cause must pass through unchanged") + } +} diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index dcee59a00..ed013249e 100644 --- a/client/vnc/server/server.go +++ b/client/vnc/server/server.go @@ -33,14 +33,16 @@ const ( // stable so clients can branch on them without parsing free text. // Format: "CODE: human message". const ( - RejectCodeAuthForbidden = "AUTH_FORBIDDEN" - RejectCodeSessionError = "SESSION_ERROR" - RejectCodeCapturerError = "CAPTURER_ERROR" - RejectCodeUnsupportedOS = "UNSUPPORTED" - RejectCodeBadRequest = "BAD_REQUEST" - RejectCodeNoConsoleUser = "NO_CONSOLE_USER" - RejectCodeApprovalDenied = "APPROVAL_DENIED" - RejectCodeNoApprover = "NO_APPROVER" + RejectCodeAuthForbidden = "AUTH_FORBIDDEN" + RejectCodeSessionError = "SESSION_ERROR" + RejectCodeCapturerError = "CAPTURER_ERROR" + RejectCodeUnsupportedOS = "UNSUPPORTED" + RejectCodeBadRequest = "BAD_REQUEST" + RejectCodeNoConsoleUser = "NO_CONSOLE_USER" + RejectCodeApprovalDenied = "APPROVAL_DENIED" + RejectCodeApprovalTimeout = "APPROVAL_TIMEOUT" + RejectCodeApprovalUnavailable = "APPROVAL_UNAVAILABLE" + RejectCodeNoApprover = "NO_APPROVER" ) // EnvVNCDisableDownscale disables any platform-specific framebuffer @@ -355,6 +357,23 @@ type Approver interface { Request(ctx context.Context, info ApprovalInfo) (ApprovalDecision, error) } +// Causes an Approver can report so the rejection names why the connection +// was refused rather than blaming the user for every outcome. An error +// matching none of these still rejects, reported as a denial. +var ( + // ErrApprovalDenied means the user answered and said no. + ErrApprovalDenied = errors.New("approval denied") + // ErrApprovalTimeout means the prompt was raised and nobody answered + // it in time. Distinct from a denial because the two call for + // different things from whoever is dialling: try again later, versus + // stop asking. + ErrApprovalTimeout = errors.New("approval timed out") + // ErrApprovalUnavailable means the prompt never reached a user, so + // there was no answer to wait for. A device with no way to display + // one lands here, as does a daemon with no UI attached. + ErrApprovalUnavailable = errors.New("no approval prompt reached the user") +) + // ApprovalDecision carries the parts of the user's response the VNC // server acts on. Accept is implicit (errors signal deny). ViewOnly puts // the session into read-only mode: the server drops input events. @@ -618,7 +637,8 @@ func (s *Server) gateApproval(conn net.Conn, header *connectionHeader) (Approval } decision, err := s.approver.Request(s.ctx, info) if err != nil { - rejectConnection(conn, codeMessage(RejectCodeApprovalDenied, "approval denied")) + code, message := approvalRejection(err) + rejectConnection(conn, codeMessage(code, message)) return ApprovalDecision{}, fmt.Errorf("approval: %w", err) } return decision, nil @@ -1259,6 +1279,21 @@ func modeString(m byte) string { } } +// approvalRejection maps an approver's error onto the code and message the +// client is told. An error matching no known cause is reported as a denial: +// the connection is refused either way, and describing a cause the server +// could not identify would put a wrong explanation in front of the caller. +func approvalRejection(err error) (string, string) { + switch { + case errors.Is(err, ErrApprovalTimeout): + return RejectCodeApprovalTimeout, "approval timed out" + case errors.Is(err, ErrApprovalUnavailable): + return RejectCodeApprovalUnavailable, "no approval prompt reached the user" + default: + return RejectCodeApprovalDenied, "approval denied" + } +} + // acceptRetryable reports whether an Accept error can plausibly clear on its // own. Running out of file descriptors can, once open connections close, and an // aborted handshake concerns one client rather than the listener. Anything else diff --git a/client/vnc/server/server_test.go b/client/vnc/server/server_test.go index 4e4bd93a3..a703730c0 100644 --- a/client/vnc/server/server_test.go +++ b/client/vnc/server/server_test.go @@ -8,10 +8,12 @@ import ( "encoding/binary" "encoding/hex" "errors" + "fmt" "image" "io" "net" "net/netip" + "strings" "sync/atomic" "testing" "time" @@ -443,6 +445,37 @@ func newGateConn(t *testing.T) net.Conn { return srv } +// newGateConnWithReason is newGateConn plus the reject reason the server +// wrote, so a test can assert on the code the client would read. +func newGateConnWithReason(t *testing.T) (net.Conn, <-chan string) { + t.Helper() + srv, cli := net.Pipe() + t.Cleanup(func() { _ = srv.Close() }) + + reasons := make(chan string, 1) + go func() { + defer cli.Close() + var srvVer [12]byte + if _, err := io.ReadFull(cli, srvVer[:]); err != nil { + return + } + if _, err := cli.Write([]byte("RFB 003.008\n")); err != nil { + return + } + // Server sends numTypes=0, then a 4-byte reason length, then the reason. + var head [5]byte + if _, err := io.ReadFull(cli, head[:]); err != nil { + return + } + reason := make([]byte, binary.BigEndian.Uint32(head[1:5])) + if _, err := io.ReadFull(cli, reason); err != nil { + return + } + reasons <- string(reason) + }() + return srv, reasons +} + func gateTestServer(requireApproval bool, approver Approver) *Server { return &Server{ log: log.WithField("test", "gate"), @@ -540,6 +573,48 @@ func TestGateApproval_ApproverDenies(t *testing.T) { } } +// TestGateApproval_RejectCodeNamesCause pins the reject code the client +// reads for each cause an approver can report. A device that cannot show a +// prompt at all, or a user who walked away, must not be reported as a user +// who said no: the caller acts on that difference, and a peer told "denied" +// when nobody was ever asked has no reason to retry. Causes are wrapped in +// the cases that carry one so the mapping cannot regress to == comparison. +func TestGateApproval_RejectCodeNamesCause(t *testing.T) { + cases := []struct { + name string + err error + code string + }{ + {"denied", ErrApprovalDenied, RejectCodeApprovalDenied}, + {"denied_wrapped", fmt.Errorf("broker: %w", ErrApprovalDenied), RejectCodeApprovalDenied}, + {"timeout", ErrApprovalTimeout, RejectCodeApprovalTimeout}, + {"timeout_wrapped", fmt.Errorf("broker: %w", ErrApprovalTimeout), RejectCodeApprovalTimeout}, + {"unavailable", ErrApprovalUnavailable, RejectCodeApprovalUnavailable}, + {"unavailable_wrapped", fmt.Errorf("no UI: %w", ErrApprovalUnavailable), RejectCodeApprovalUnavailable}, + // An unclassified cause still rejects, reported as a denial. + {"ctx_canceled", context.Canceled, RejectCodeApprovalDenied}, + {"unknown", errors.New("anything else"), RejectCodeApprovalDenied}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + app := &recordingApprover{respond: tc.err} + srv := gateTestServer(true, app) + conn, reasons := newGateConnWithReason(t) + + _, err := srv.gateApproval(conn, &connectionHeader{mode: ModeAttach}) + require.Error(t, err, "approver error %v must deny", tc.err) + + select { + case reason := <-reasons: + assert.True(t, strings.HasPrefix(reason, tc.code+":"), + "reject reason %q must name the cause with code %s", reason, tc.code) + case <-time.After(2 * time.Second): + t.Fatal("did not observe rejection reason") + } + }) + } +} + // TestGateApproval_ApproverAccepts confirms the happy path actually // returns true so we know the deny path is not the only outcome the // gate can produce. diff --git a/util/capture/text.go b/util/capture/text.go index 4ceafedd5..b2c341bd9 100644 --- a/util/capture/text.go +++ b/util/capture/text.go @@ -557,6 +557,8 @@ var vncRejectCodes = [...]string{ "BAD_REQUEST", "NO_CONSOLE_USER", "APPROVAL_DENIED", + "APPROVAL_TIMEOUT", + "APPROVAL_UNAVAILABLE", "NO_APPROVER", }