Skip to content

Commit 72d9876

Browse files
mafredrimafredri
authored
fix(coderd/workspaceapps): prevent race in workspace app audit session updates (coder#17020)
Fixes coder/internal#520
1 parent 6862409 commit 72d9876

13 files changed

+68
-32
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4625,9 +4625,9 @@ func (q *querier) UpsertWorkspaceAgentPortShare(ctx context.Context, arg databas
46254625
return q.db.UpsertWorkspaceAgentPortShare(ctx, arg)
46264626
}
46274627

4628-
func (q *querier) UpsertWorkspaceAppAuditSession(ctx context.Context, arg database.UpsertWorkspaceAppAuditSessionParams) (time.Time, error) {
4628+
func (q *querier) UpsertWorkspaceAppAuditSession(ctx context.Context, arg database.UpsertWorkspaceAppAuditSessionParams) (bool, error) {
46294629
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceSystem); err != nil {
4630-
return time.Time{}, err
4630+
return false, err
46314631
}
46324632
return q.db.UpsertWorkspaceAppAuditSession(ctx, arg)
46334633
}

‎coderd/database/dbmem/dbmem.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12298,10 +12298,10 @@ func (q *FakeQuerier) UpsertWorkspaceAgentPortShare(_ context.Context, arg datab
1229812298
return psl, nil
1229912299
}
1230012300

12301-
func (q *FakeQuerier) UpsertWorkspaceAppAuditSession(_ context.Context, arg database.UpsertWorkspaceAppAuditSessionParams) (time.Time, error) {
12301+
func (q *FakeQuerier) UpsertWorkspaceAppAuditSession(_ context.Context, arg database.UpsertWorkspaceAppAuditSessionParams) (bool, error) {
1230212302
err := validateDatabaseType(arg)
1230312303
if err != nil {
12304-
return time.Time{}, err
12304+
return false, err
1230512305
}
1230612306

1230712307
q.mutex.Lock()
@@ -12335,10 +12335,11 @@ func (q *FakeQuerier) UpsertWorkspaceAppAuditSession(_ context.Context, arg data
1233512335

1233612336
q.workspaceAppAuditSessions[i].UpdatedAt = arg.UpdatedAt
1233712337
if !fresh {
12338+
q.workspaceAppAuditSessions[i].ID = arg.ID
1233812339
q.workspaceAppAuditSessions[i].StartedAt = arg.StartedAt
12339-
return arg.StartedAt, nil
12340+
return true, nil
1234012341
}
12341-
return s.StartedAt, nil
12342+
return false, nil
1234212343
}
1234312344

1234412345
q.workspaceAppAuditSessions = append(q.workspaceAppAuditSessions, database.WorkspaceAppAuditSession{
@@ -12352,7 +12353,7 @@ func (q *FakeQuerier) UpsertWorkspaceAppAuditSession(_ context.Context, arg data
1235212353
StartedAt: arg.StartedAt,
1235312354
UpdatedAt: arg.UpdatedAt,
1235412355
})
12355-
return arg.StartedAt, nil
12356+
return true, nil
1235612357
}
1235712358

1235812359
func (q *FakeQuerier) GetAuthorizedTemplates(ctx context.Context, arg database.GetTemplatesWithFilterParams, prepared rbac.PreparedAuthorized) ([]database.Template, error) {

‎coderd/database/dbmetrics/querymetrics.go

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dump.sql

Lines changed: 5 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000302_fix_app_audit_session_race.down.sql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
ALTER TABLE workspace_app_audit_sessions
2+
DROP COLUMN id;

‎coderd/database/migrations/000302_fix_app_audit_session_race.up.sql

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
-- Add column with default to fix existing rows.
2+
ALTER TABLE workspace_app_audit_sessions
3+
ADD COLUMN id UUID PRIMARY KEY DEFAULT gen_random_uuid();
4+
ALTER TABLE workspace_app_audit_sessions
5+
ALTER COLUMN id DROP DEFAULT;

‎coderd/database/models.go

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/querier.go

Lines changed: 4 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

Lines changed: 20 additions & 9 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/workspaceappaudit.sql

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
-- name: UpsertWorkspaceAppAuditSession :one
22
--
3-
-- Insert a new workspace app audit session or update an existing one, if
4-
-- started_at is updated, it means the session has been restarted.
3+
-- The returned boolean, new_or_stale, can be used to deduce if a new session
4+
-- was started. This means that a new row was inserted (no previous session) or
5+
-- the updated_at is older than stale interval.
56
INSERT INTO
67
workspace_app_audit_sessions (
8+
id,
79
agent_id,
810
app_id,
911
user_id,
@@ -24,18 +26,25 @@ VALUES
2426
$6,
2527
$7,
2628
$8,
27-
$9
29+
$9,
30+
$10
2831
)
2932
ON CONFLICT
3033
(agent_id, app_id, user_id, ip, user_agent, slug_or_port, status_code)
3134
DO
3235
UPDATE
3336
SET
37+
-- ID is used to know if session was reset on upsert.
38+
id = CASE
39+
WHEN workspace_app_audit_sessions.updated_at > NOW() - (@stale_interval_ms::bigint || ' ms')::interval
40+
THEN workspace_app_audit_sessions.id
41+
ELSE EXCLUDED.id
42+
END,
3443
started_at = CASE
3544
WHEN workspace_app_audit_sessions.updated_at > NOW() - (@stale_interval_ms::bigint || ' ms')::interval
3645
THEN workspace_app_audit_sessions.started_at
3746
ELSE EXCLUDED.started_at
3847
END,
3948
updated_at = EXCLUDED.updated_at
4049
RETURNING
41-
started_at;
50+
id = $1 AS new_or_stale;

‎coderd/database/unique_constraint.go

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/workspaceapps/db.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -447,16 +447,17 @@ func (p *DBTokenProvider) auditInitRequest(ctx context.Context, w http.ResponseW
447447
slog.F("status_code", statusCode),
448448
)
449449

450-
var startedAt time.Time
450+
var newOrStale bool
451451
err := p.Database.InTx(func(tx database.Store) (err error) {
452452
// nolint:gocritic // System context is needed to write audit sessions.
453453
dangerousSystemCtx := dbauthz.AsSystemRestricted(ctx)
454454

455-
startedAt, err = tx.UpsertWorkspaceAppAuditSession(dangerousSystemCtx, database.UpsertWorkspaceAppAuditSessionParams{
455+
newOrStale, err = tx.UpsertWorkspaceAppAuditSession(dangerousSystemCtx, database.UpsertWorkspaceAppAuditSessionParams{
456456
// Config.
457457
StaleIntervalMS: p.WorkspaceAppAuditSessionTimeout.Milliseconds(),
458458

459459
// Data.
460+
ID: uuid.New(),
460461
AgentID: aReq.dbReq.Agent.ID,
461462
AppID: aReq.dbReq.App.ID, // Can be unset, in which case uuid.Nil is fine.
462463
UserID: userID, // Can be unset, in which case uuid.Nil is fine.
@@ -481,9 +482,9 @@ func (p *DBTokenProvider) auditInitRequest(ctx context.Context, w http.ResponseW
481482
return
482483
}
483484

484-
if !startedAt.Equal(aReq.time) {
485-
// If the unique session wasn't renewed, we don't want to log a new
486-
// audit event for it.
485+
if !newOrStale {
486+
// We either didn't insert a new session, or the session
487+
// didn't timeout due to inactivity.
487488
return
488489
}
489490

0 commit comments

Comments
 (0)