fix: create agent firewall sessions without requiring agent read access (#26990)

## Overview Part of the **boundary correlation** feature. Fixes lazy creation of `boundary_sessions` rows so it works within the agent's RBAC constraints, and consumes the new `ConfinedProcessName` field reported by boundary. Pairs with coder/boundary#206, which adds `ConfinedProcessName` to `ReportBoundaryLogsRequest`. This branch bumps the `github.com/coder/boundary` module to pick up that work. ## Problem `ensureSession` did a pre-insert existence check via `GetBoundarySessionByID`. Agents are **not permitted to read boundary sessions**, so that read path is not viable when the session is created from an agent-reported log batch. ## Changes - **Remove the pre-insert read.** `ensureSession` now inserts directly and treats a primary-key unique violation as success, covering sessions already created by a prior batch, a reconnection, or another coderd replica — without requiring read access. - **Per-connection guard.** Add a mutex-protected `ensuredSessions` set so repeated log batches on the same connection skip the existence check and insert entirely, touching the database only for the logs. On a transient insert failure the session is left unmarked so the next batch retries. - **Consume `ConfinedProcessName`.** Pass `req.GetConfinedProcessName()` through to the session insert. - **Bump boundary module** from `v0.9.0` to `v0.9.1-0.20260706095856-35ba90f9e8b2`. - **Tests.** - Add `TestReportBoundaryLogsAgentRBAC` (`coderd/boundary_logs_test.go`), an integration test that connects as a real workspace agent, verifies the session and log are persisted under agent RBAC, and asserts the agent subject cannot read boundary sessions — guarding against reintroducing a pre-insert read. - Add `TestReportBoundaryLogsSessionGuard` (session inserted once across two batches, logs inserted per batch) and `TestReportBoundaryLogsSessionRetriedOnError` (insert retried after a transient error). - Regenerate `agent-firewall` CLI docs/golden files and adjust the clidocgen template to render the YAML path when a flag has no long name. > 🤖 This PR was opened by Coder Agents on behalf of @SasSwart.
2026-09-21 20:51:01 +08:00 · 2026-07-07 10:42:01 +00:00
parent 0ae4554de9
commit fc188fdaee
9 changed files with 283 additions and 36 deletions
@@ -2,9 +2,8 @@ package agentapi

 import (
 	"context"
-	"database/sql"
-	"errors"
 	"fmt"
+	"sync"
 	"time"

 	"github.com/google/uuid"
@@ -47,6 +46,13 @@ type BoundaryLogsAPI struct {
 	TemplateID           uuid.UUID
 	TemplateVersionID    uuid.UUID
 	BoundaryUsageTracker *boundaryusage.Tracker
+
+	// mu guards ensuredSessions, which records session IDs already persisted
+	// by this connection so repeated batches skip the existence check and
+	// insert. The API is one instance per agent connection, so this lives for
+	// the session's lifetime.
+	mu              sync.Mutex
+	ensuredSessions map[uuid.UUID]struct{}
 }

 func (a *BoundaryLogsAPI) ReportBoundaryLogs(ctx context.Context, req *agentproto.ReportBoundaryLogsRequest) (*agentproto.ReportBoundaryLogsResponse, error) {
@@ -80,16 +86,19 @@ func (a *BoundaryLogsAPI) ReportBoundaryLogs(ctx context.Context, req *agentprot
 		}
 	}

-	if persistEnabled {
+	if persistEnabled && !a.sessionEnsured(sessionID) {
 		// Lazy-create the boundary session on first log arrival.
 		// If this fails (transient DB error), we continue so that
 		// logs are still persisted. The session will be created on
 		// a subsequent batch since every request carries the session
-		// details.
+		// details. On success we record the session so later batches
+		// skip the existence check and insert entirely.
 		if sessionErr := a.ensureSession(ctx, sessionID, req.GetConfinedProcessName(), now); sessionErr != nil {
 			a.Log.Error(ctx, "failed to ensure boundary session",
 				slog.F("session_id", sessionID.String()),
 				slog.Error(sessionErr))
+		} else {
+			a.markSessionEnsured(sessionID)
 		}
 	}

@@ -179,6 +188,25 @@ func (a *BoundaryLogsAPI) ReportBoundaryLogs(ctx context.Context, req *agentprot
 	return &agentproto.ReportBoundaryLogsResponse{}, nil
 }

+// sessionEnsured reports whether this connection has already persisted the
+// session, letting repeated batches skip the database round-trip.
+func (a *BoundaryLogsAPI) sessionEnsured(sessionID uuid.UUID) bool {
+	a.mu.Lock()
+	defer a.mu.Unlock()
+	_, ok := a.ensuredSessions[sessionID]
+	return ok
+}
+
+// markSessionEnsured records that the session has been persisted.
+func (a *BoundaryLogsAPI) markSessionEnsured(sessionID uuid.UUID) {
+	a.mu.Lock()
+	defer a.mu.Unlock()
+	if a.ensuredSessions == nil {
+		a.ensuredSessions = make(map[uuid.UUID]struct{})
+	}
+	a.ensuredSessions[sessionID] = struct{}{}
+}
+
 // ensureSession creates the boundary_sessions row if it does not
 // already exist.
 func (a *BoundaryLogsAPI) ensureSession(ctx context.Context, sessionID uuid.UUID, confinedProcess string, now time.Time) error {
@@ -186,19 +214,7 @@ func (a *BoundaryLogsAPI) ensureSession(ctx context.Context, sessionID uuid.UUID
 		return nil
 	}

-	// Check the database in case another replica or reconnection
-	// already created this session.
-	_, err := a.Database.GetBoundarySessionByID(ctx, sessionID)
-	if err == nil {
-		return nil
-	}
-	if !errors.Is(err, sql.ErrNoRows) {
-		return xerrors.Errorf("check boundary session existence: %w", err)
-	}
-
-	// Session does not exist; create it. started_at is the time
-	// the first log is received by coderd, per the RFC.
-	_, err = a.Database.InsertBoundarySession(ctx, database.InsertBoundarySessionParams{
+	_, err := a.Database.InsertBoundarySession(ctx, database.InsertBoundarySessionParams{
 		ID:                  sessionID,
 		WorkspaceAgentID:    a.AgentID,
 		OwnerID:             uuid.NullUUID{UUID: a.OwnerID, Valid: true},
@@ -207,13 +223,8 @@ func (a *BoundaryLogsAPI) ensureSession(ctx context.Context, sessionID uuid.UUID
 		UpdatedAt:           now,
 	})
 	if err != nil {
-		// A second coderd replica may receive a batch for this session
-		// before the first replica has finished inserting it. Both
-		// attempt the INSERT; the second fails with a primary-key
-		// unique violation. Treat it as success because the session
-		// now exists.
 		if database.IsUniqueViolation(err, database.UniqueBoundarySessionsPkey) {
-			a.Log.Debug(ctx, "boundary session already created by another replica",
+			a.Log.Debug(ctx, "boundary session already created",
 				slog.F("session_id", sessionID.String()))
 			return nil
 		}
@@ -2,16 +2,20 @@ package agentapi_test

 import (
 	"context"
+	"database/sql"
 	"testing"

 	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 	"google.golang.org/protobuf/types/known/timestamppb"

+	"cdr.dev/slog/v3/sloggers/slogtest"
 	agentproto "github.com/coder/coder/v2/agent/proto"
 	"github.com/coder/coder/v2/coderd/agentapi"
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/dbgen"
+	"github.com/coder/coder/v2/coderd/database/dbmock"
 	"github.com/coder/coder/v2/coderd/database/dbtestutil"
 	"github.com/coder/coder/v2/coderd/database/dbtime"
 	"github.com/coder/coder/v2/testutil"
@@ -454,3 +458,98 @@ func TestReportBoundaryLogs(t *testing.T) {
 		require.Len(t, logs, 2, "logs from both agents must be persisted")
 	})
 }
+
+// httpLogRequest builds a ReportBoundaryLogsRequest carrying a single allowed
+// HTTP log for the given session.
+func httpLogRequest(sessionID uuid.UUID, seq int32) *agentproto.ReportBoundaryLogsRequest {
+	return &agentproto.ReportBoundaryLogsRequest{
+		SessionId:           sessionID.String(),
+		ConfinedProcessName: "claude-code",
+		Logs: []*agentproto.BoundaryLog{
+			{
+				Allowed:        true,
+				Time:           timestamppb.New(dbtime.Now()),
+				SequenceNumber: seq,
+				Resource: &agentproto.BoundaryLog_HttpRequest_{
+					HttpRequest: &agentproto.BoundaryLog_HttpRequest{
+						Method:      "GET",
+						Url:         "https://example.com",
+						MatchedRule: "domain=example.com",
+					},
+				},
+			},
+		},
+	}
+}
+
+// TestReportBoundaryLogsSessionGuard verifies that once a session has been
+// ensured, later batches from the same connection skip the existence check and
+// insert entirely, touching the database only for the logs themselves.
+func TestReportBoundaryLogsSessionGuard(t *testing.T) {
+	t.Parallel()
+
+	ctrl := gomock.NewController(t)
+	db := dbmock.NewMockStore(ctrl)
+
+	sessionID := uuid.New()
+	api := &agentapi.BoundaryLogsAPI{
+		Log:               testutil.Logger(t),
+		Database:          db,
+		AgentID:           uuid.New(),
+		WorkspaceID:       uuid.New(),
+		OwnerID:           uuid.New(),
+		TemplateID:        uuid.New(),
+		TemplateVersionID: uuid.New(),
+	}
+
+	// The session is inserted once, even though two batches arrive. Times(1)
+	// fails the test if the guard does not suppress the second ensure attempt.
+	// Logs insert on every batch.
+	db.EXPECT().InsertBoundarySession(gomock.Any(), gomock.Any()).
+		Return(database.BoundarySession{}, nil).Times(1)
+	db.EXPECT().InsertBoundaryLogs(gomock.Any(), gomock.Any()).
+		Return([]database.BoundaryLog{}, nil).Times(2)
+
+	_, err := api.ReportBoundaryLogs(context.Background(), httpLogRequest(sessionID, 0))
+	require.NoError(t, err)
+	_, err = api.ReportBoundaryLogs(context.Background(), httpLogRequest(sessionID, 1))
+	require.NoError(t, err)
+}
+
+// TestReportBoundaryLogsSessionRetriedOnError verifies that when ensureSession
+// fails, the guard is not set, so the next batch retries the existence check.
+func TestReportBoundaryLogsSessionRetriedOnError(t *testing.T) {
+	t.Parallel()
+
+	ctrl := gomock.NewController(t)
+	db := dbmock.NewMockStore(ctrl)
+
+	sessionID := uuid.New()
+	api := &agentapi.BoundaryLogsAPI{
+		// The first batch deliberately triggers a transient error, which
+		// logs at ERROR level. Ignore errors so slogtest does not fail.
+		Log:               slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}),
+		Database:          db,
+		AgentID:           uuid.New(),
+		WorkspaceID:       uuid.New(),
+		OwnerID:           uuid.New(),
+		TemplateID:        uuid.New(),
+		TemplateVersionID: uuid.New(),
+	}
+
+	// First batch: insert fails transiently, so the session is not marked
+	// ensured. Second batch: insert is retried and succeeds. Logs insert on both.
+	gomock.InOrder(
+		db.EXPECT().InsertBoundarySession(gomock.Any(), gomock.Any()).
+			Return(database.BoundarySession{}, sql.ErrConnDone),
+		db.EXPECT().InsertBoundarySession(gomock.Any(), gomock.Any()).
+			Return(database.BoundarySession{}, nil),
+	)
+	db.EXPECT().InsertBoundaryLogs(gomock.Any(), gomock.Any()).
+		Return([]database.BoundaryLog{}, nil).Times(2)
+
+	_, err := api.ReportBoundaryLogs(context.Background(), httpLogRequest(sessionID, 0))
+	require.NoError(t, err)
+	_, err = api.ReportBoundaryLogs(context.Background(), httpLogRequest(sessionID, 1))
+	require.NoError(t, err)
+}