Skip to content

Commit ff496e8

Browse files
mafredrimafredri
committed
fix(coderd/workspaceapps): prevent race in workspace app audit session updates
Fixes coder/internal#520
1 parent d8d4b9b commit ff496e8

13 files changed

+62
-26
lines changed

‎coderd/database/dbauthz/dbauthz.go

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

4618-
func (q *querier) UpsertWorkspaceAppAuditSession(ctx context.Context, arg database.UpsertWorkspaceAppAuditSessionParams) (time.Time, error) {
4618+
func (q *querier) UpsertWorkspaceAppAuditSession(ctx context.Context, arg database.UpsertWorkspaceAppAuditSessionParams) (uuid.UUID, error) {
46194619
if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceSystem); err != nil {
4620-
return time.Time{}, err
4620+
return uuid.Nil, err
46214621
}
46224622
return q.db.UpsertWorkspaceAppAuditSession(ctx, arg)
46234623
}

‎coderd/database/dbmem/dbmem.go

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

12286-
func (q *FakeQuerier) UpsertWorkspaceAppAuditSession(_ context.Context, arg database.UpsertWorkspaceAppAuditSessionParams) (time.Time, error) {
12286+
func (q *FakeQuerier) UpsertWorkspaceAppAuditSession(_ context.Context, arg database.UpsertWorkspaceAppAuditSessionParams) (uuid.UUID, error) {
1228712287
err := validateDatabaseType(arg)
1228812288
if err != nil {
12289-
return time.Time{}, err
12289+
return uuid.Nil, err
1229012290
}
1229112291

1229212292
q.mutex.Lock()
@@ -12320,10 +12320,11 @@ func (q *FakeQuerier) UpsertWorkspaceAppAuditSession(_ context.Context, arg data
1232012320

1232112321
q.workspaceAppAuditSessions[i].UpdatedAt = arg.UpdatedAt
1232212322
if !fresh {
12323+
q.workspaceAppAuditSessions[i].ID = arg.ID
1232312324
q.workspaceAppAuditSessions[i].StartedAt = arg.StartedAt
12324-
return arg.StartedAt, nil
12325+
return arg.ID, nil
1232512326
}
12326-
return s.StartedAt, nil
12327+
return s.ID, nil
1232712328
}
1232812329

1232912330
q.workspaceAppAuditSessions = append(q.workspaceAppAuditSessions, database.WorkspaceAppAuditSession{
@@ -12337,7 +12338,7 @@ func (q *FakeQuerier) UpsertWorkspaceAppAuditSession(_ context.Context, arg data
1233712338
StartedAt: arg.StartedAt,
1233812339
UpdatedAt: arg.UpdatedAt,
1233912340
})
12340-
return arg.StartedAt, nil
12341+
return arg.ID, nil
1234112342
}
1234212343

1234312344
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: 1 addition & 1 deletion
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: 17 additions & 7 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: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
-- started_at is updated, it means the session has been restarted.
55
INSERT INTO
66
workspace_app_audit_sessions (
7+
id,
78
agent_id,
89
app_id,
910
user_id,
@@ -24,18 +25,25 @@ VALUES
2425
$6,
2526
$7,
2627
$8,
27-
$9
28+
$9,
29+
$10
2830
)
2931
ON CONFLICT
3032
(agent_id, app_id, user_id, ip, user_agent, slug_or_port, status_code)
3133
DO
3234
UPDATE
3335
SET
36+
-- ID is used to know if session was reset on upsert.
37+
id = CASE
38+
WHEN workspace_app_audit_sessions.updated_at > NOW() - (@stale_interval_ms::bigint || ' ms')::interval
39+
THEN workspace_app_audit_sessions.id
40+
ELSE EXCLUDED.id
41+
END,
3442
started_at = CASE
3543
WHEN workspace_app_audit_sessions.updated_at > NOW() - (@stale_interval_ms::bigint || ' ms')::interval
3644
THEN workspace_app_audit_sessions.started_at
3745
ELSE EXCLUDED.started_at
3846
END,
3947
updated_at = EXCLUDED.updated_at
4048
RETURNING
41-
started_at;
49+
id;

‎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: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -447,16 +447,20 @@ 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 (
451+
refreshID = uuid.New()
452+
upsertRefreshID uuid.UUID
453+
)
451454
err := p.Database.InTx(func(tx database.Store) (err error) {
452455
// nolint:gocritic // System context is needed to write audit sessions.
453456
dangerousSystemCtx := dbauthz.AsSystemRestricted(ctx)
454457

455-
startedAt, err = tx.UpsertWorkspaceAppAuditSession(dangerousSystemCtx, database.UpsertWorkspaceAppAuditSessionParams{
458+
upsertRefreshID, err = tx.UpsertWorkspaceAppAuditSession(dangerousSystemCtx, database.UpsertWorkspaceAppAuditSessionParams{
456459
// Config.
457460
StaleIntervalMS: p.WorkspaceAppAuditSessionTimeout.Milliseconds(),
458461

459462
// Data.
463+
ID: refreshID,
460464
AgentID: aReq.dbReq.Agent.ID,
461465
AppID: aReq.dbReq.App.ID, // Can be unset, in which case uuid.Nil is fine.
462466
UserID: userID, // Can be unset, in which case uuid.Nil is fine.
@@ -481,9 +485,9 @@ func (p *DBTokenProvider) auditInitRequest(ctx context.Context, w http.ResponseW
481485
return
482486
}
483487

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.
488+
if sessionExistsOrIsActive := refreshID != upsertRefreshID; sessionExistsOrIsActive {
489+
// We either didn't insert a new session, or the session
490+
// didn't timeout due to inactivity.
487491
return
488492
}
489493

0 commit comments

Comments
 (0)