diff --git a/client/vnc/server/agent_ipc.go b/client/vnc/server/agent_ipc.go index dc2e96a47..abe903d9e 100644 --- a/client/vnc/server/agent_ipc.go +++ b/client/vnc/server/agent_ipc.go @@ -72,10 +72,16 @@ func (s *Server) handleServiceConnection(conn net.Conn, sa sessionAgent) { } s.registerConnAuth(conn, header) - allow, decision := s.gateApproval(conn, header, authedLog) - if !allow { + decision, err := s.gateApproval(conn, header) + if err != nil { + authedLog.Infof("VNC connection rejected: %v", err) return } + if decision.ViewOnly { + authedLog.Info("VNC connection approved by user (view-only)") + } else if s.requireApproval { + authedLog.Info("VNC connection approved by user") + } socketPath, token, err := sa.Resolve(s.ctx) if err != nil { diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index daa274f66..0f137d3c5 100644 --- a/client/vnc/server/server.go +++ b/client/vnc/server/server.go @@ -422,20 +422,22 @@ func (s *Server) untrackConn(c net.Conn) { // gateApproval prompts the local user to accept or deny conn before any // session resources are allocated. On rejection the conn already received // an RFB reject reason; the gate does not close it. -func (s *Server) gateApproval(conn net.Conn, header *connectionHeader, connLog *log.Entry) (bool, ApprovalDecision) { +// gateApproval returns the user's decision when approval is enabled, or a +// zero decision when it isn't. On rejection it writes the RFB rejection +// message to conn and returns an error; the caller is responsible for +// logging it (this function does not log on its own). +func (s *Server) gateApproval(conn net.Conn, header *connectionHeader) (ApprovalDecision, error) { if !s.requireApproval { - return true, ApprovalDecision{} + return ApprovalDecision{}, nil } if s.approver == nil { rejectConnection(conn, codeMessage(RejectCodeNoApprover, "approval required but no approver configured")) - connLog.Warn("VNC connection rejected: approval required but no approver") - return false, ApprovalDecision{} + return ApprovalDecision{}, errors.New("approval required but no approver configured") } if s.serviceMode { if err := interactiveUserError(); err != nil { rejectConnection(conn, codeMessage(RejectCodeNoConsoleUser, "no interactive user session")) - connLog.Infof("VNC connection rejected: no interactive user session to approve: %v", err) - return false, ApprovalDecision{} + return ApprovalDecision{}, fmt.Errorf("no interactive user session: %w", err) } } info := ApprovalInfo{ @@ -452,15 +454,9 @@ func (s *Server) gateApproval(conn net.Conn, header *connectionHeader, connLog * decision, err := s.approver.Request(s.ctx, info) if err != nil { rejectConnection(conn, codeMessage(RejectCodeApprovalDenied, "approval denied")) - connLog.Infof("VNC connection rejected: approval %v", err) - return false, ApprovalDecision{} + return ApprovalDecision{}, fmt.Errorf("approval: %w", err) } - if decision.ViewOnly { - connLog.Info("VNC connection approved by user (view-only)") - } else { - connLog.Info("VNC connection approved by user") - } - return true, decision + return decision, nil } // sourceIPString returns the IP portion of a remote address, or the full @@ -804,10 +800,16 @@ func (s *Server) handleConnection(conn net.Conn) { } s.registerConnAuth(conn, header) - allow, decision := s.gateApproval(conn, header, connLog) - if !allow { + decision, err := s.gateApproval(conn, header) + if err != nil { + connLog.Infof("VNC connection rejected: %v", err) return } + if decision.ViewOnly { + connLog.Info("VNC connection approved by user (view-only)") + } else if s.requireApproval { + connLog.Info("VNC connection approved by user") + } capturer, injector, sessionCleanup, ok := s.acquireSessionResources(conn, header, &connLog) if !ok { diff --git a/client/vnc/server/server_test.go b/client/vnc/server/server_test.go index a8a6dffb8..a820469e6 100644 --- a/client/vnc/server/server_test.go +++ b/client/vnc/server/server_test.go @@ -405,7 +405,8 @@ func TestGateApproval_Disabled_NoApproverCall(t *testing.T) { defer conn.Close() header := &connectionHeader{mode: ModeAttach} - allowed, _ := srv.gateApproval(conn, header, srv.log) + _, err := srv.gateApproval(conn, header) + allowed := err == nil assert.True(t, allowed, "gate must pass through when requireApproval is false") assert.Equal(t, int32(0), app.calls.Load(), "approver must not be called when disabled") } @@ -439,7 +440,8 @@ func TestGateApproval_Enabled_NilApproverDenies(t *testing.T) { }() header := &connectionHeader{mode: ModeAttach} - allowed, _ := srv.gateApproval(srvConn, header, srv.log) + _, err := srv.gateApproval(srvConn, header) + allowed := err == nil assert.False(t, allowed, "missing approver MUST deny; never silently pass") select { @@ -472,7 +474,8 @@ func TestGateApproval_ApproverDenies(t *testing.T) { defer conn.Close() header := &connectionHeader{mode: ModeAttach} - allowed, _ := srv.gateApproval(conn, header, srv.log) + _, err := srv.gateApproval(conn, header) + allowed := err == nil assert.False(t, allowed, "approver error %v must deny", tc.err) assert.Equal(t, int32(1), app.calls.Load()) }) @@ -489,7 +492,8 @@ func TestGateApproval_ApproverAccepts(t *testing.T) { defer conn.Close() header := &connectionHeader{mode: ModeAttach, username: "alice"} - allowed, _ := srv.gateApproval(conn, header, srv.log) + _, err := srv.gateApproval(conn, header) + allowed := err == nil assert.True(t, allowed, "approver returning nil must let the gate pass") assert.Equal(t, int32(1), app.calls.Load()) assert.Equal(t, "alice", app.lastIn.Username, "header username must reach the approver") @@ -510,7 +514,8 @@ func TestGateApproval_PassesPubKeyHex(t *testing.T) { pub[i] = byte(i) } header := &connectionHeader{mode: ModeAttach, clientStatic: pub} - allowed, _ := srv.gateApproval(conn, header, srv.log) + _, err := srv.gateApproval(conn, header) + allowed := err == nil assert.True(t, allowed) assert.Equal(t, hex.EncodeToString(pub), app.lastIn.PeerPubKey) }