Skip to content

Commit 46fe59f

Browse files
Kira-PilotEmyrk
Kira-Pilot
and
Emyrk
authored
feat: audit login (coder#5925)
* added migration for api key resource * sort of working * auditing login * passing the correct user id * added and fixed tests * gen documentation * formatting and lint * lint * audit Github oauth and write tests * audit oauth and write tests * added defer fn for login error auditing * fixed test * feat: audit logout (coder#5998) * Update coderd/userauth.go Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com> * fix test * bypassing diff generation if login/logout * lint --------- Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
1 parent 060eeed commit 46fe59f

29 files changed

+679
-259
lines changed

‎coderd/apidoc/docs.go

+6-2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

+4-2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apikey.go

+7-7
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func (api *API) postToken(rw http.ResponseWriter, r *http.Request) {
7070
return
7171
}
7272

73-
cookie, err := api.createAPIKey(ctx, createAPIKeyParams{
73+
cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{
7474
UserID: user.ID,
7575
LoginType: database.LoginTypeToken,
7676
ExpiresAt: database.Now().Add(lifeTime),
@@ -108,7 +108,7 @@ func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) {
108108
}
109109

110110
lifeTime := time.Hour * 24 * 7
111-
cookie, err := api.createAPIKey(ctx, createAPIKeyParams{
111+
cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{
112112
UserID: user.ID,
113113
LoginType: database.LoginTypePassword,
114114
RemoteAddr: r.RemoteAddr,
@@ -281,10 +281,10 @@ func (api *API) validateAPIKeyLifetime(lifetime time.Duration) error {
281281
return nil
282282
}
283283

284-
func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*http.Cookie, error) {
284+
func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*http.Cookie, *database.APIKey, error) {
285285
keyID, keySecret, err := GenerateAPIKeyIDSecret()
286286
if err != nil {
287-
return nil, xerrors.Errorf("generate API key: %w", err)
287+
return nil, nil, xerrors.Errorf("generate API key: %w", err)
288288
}
289289
hashed := sha256.Sum256([]byte(keySecret))
290290

@@ -315,7 +315,7 @@ func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*h
315315
switch scope {
316316
case database.APIKeyScopeAll, database.APIKeyScopeApplicationConnect:
317317
default:
318-
return nil, xerrors.Errorf("invalid API key scope: %q", scope)
318+
return nil, nil, xerrors.Errorf("invalid API key scope: %q", scope)
319319
}
320320

321321
key, err := api.Database.InsertAPIKey(ctx, database.InsertAPIKeyParams{
@@ -338,7 +338,7 @@ func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*h
338338
Scope: scope,
339339
})
340340
if err != nil {
341-
return nil, xerrors.Errorf("insert API key: %w", err)
341+
return nil, nil, xerrors.Errorf("insert API key: %w", err)
342342
}
343343

344344
api.Telemetry.Report(&telemetry.Snapshot{
@@ -354,5 +354,5 @@ func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*h
354354
HttpOnly: true,
355355
SameSite: http.SameSiteLaxMode,
356356
Secure: api.SecureAuthCookie,
357-
}, nil
357+
}, &key, nil
358358
}

‎coderd/audit.go

+10
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,12 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow, additionalFields a
264264
codersdk.AuditAction(alog.Action).Friendly(),
265265
)
266266

267+
// API Key resources do not have targets and follow the below format:
268+
// "User {logged in | logged out}"
269+
if alog.ResourceType == database.ResourceTypeApiKey {
270+
return str
271+
}
272+
267273
// Strings for starting/stopping workspace builds follow the below format:
268274
// "{user | 'Coder automatically'} started build #{build_number} for workspace {target}"
269275
// where target is a workspace (name) instead of a workspace build
@@ -488,6 +494,10 @@ func actionFromString(actionString string) string {
488494
return actionString
489495
case codersdk.AuditActionStop:
490496
return actionString
497+
case codersdk.AuditActionLogin:
498+
return actionString
499+
case codersdk.AuditActionLogout:
500+
return actionString
491501
default:
492502
}
493503
return ""

‎coderd/audit/request.go

+39-9
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@ type Request[T Auditable] struct {
3131

3232
Old T
3333
New T
34+
35+
// This optional field can be passed in when the userID cannot be determined from the API Key
36+
// such as in the case of login, when the audit log is created prior the API Key's existence.
37+
UserID uuid.UUID
3438
}
3539

3640
type BuildAuditParams[T Auditable] struct {
@@ -64,6 +68,9 @@ func ResourceTarget[T Auditable](tgt T) string {
6468
return typed.PublicKey
6569
case database.AuditableGroup:
6670
return typed.Group.Name
71+
case database.APIKey:
72+
// this isn't used
73+
return ""
6774
default:
6875
panic(fmt.Sprintf("unknown resource %T", tgt))
6976
}
@@ -85,6 +92,8 @@ func ResourceID[T Auditable](tgt T) uuid.UUID {
8592
return typed.UserID
8693
case database.AuditableGroup:
8794
return typed.Group.ID
95+
case database.APIKey:
96+
return typed.UserID
8897
default:
8998
panic(fmt.Sprintf("unknown resource %T", tgt))
9099
}
@@ -106,6 +115,8 @@ func ResourceType[T Auditable](tgt T) database.ResourceType {
106115
return database.ResourceTypeGitSshKey
107116
case database.AuditableGroup:
108117
return database.ResourceTypeGroup
118+
case database.APIKey:
119+
return database.ResourceTypeApiKey
109120
default:
110121
panic(fmt.Sprintf("unknown resource %T", tgt))
111122
}
@@ -130,7 +141,14 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request
130141

131142
// If no resources were provided, there's nothing we can audit.
132143
if ResourceID(req.Old) == uuid.Nil && ResourceID(req.New) == uuid.Nil {
133-
return
144+
// If the request action is a login or logout, we always want to audit it even if
145+
// there is no diff. This is so we can capture events where an API Key is never created
146+
// because an unknown user fails to login.
147+
// TODO: introduce the concept of an anonymous user so we always have a userID even
148+
// when dealing with a mystery user. https://github.com/coder/coder/issues/6054
149+
if req.params.Action != database.AuditActionLogin && req.params.Action != database.AuditActionLogout {
150+
return
151+
}
134152
}
135153

136154
var diffRaw = []byte("{}")
@@ -150,16 +168,24 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request
150168
p.AdditionalFields = json.RawMessage("{}")
151169
}
152170

171+
var userID uuid.UUID
172+
key, ok := httpmw.APIKeyOptional(p.Request)
173+
if ok {
174+
userID = key.UserID
175+
} else {
176+
userID = req.UserID
177+
}
178+
153179
ip := parseIP(p.Request.RemoteAddr)
154180
auditLog := database.AuditLog{
155181
ID: uuid.New(),
156182
Time: database.Now(),
157-
UserID: httpmw.APIKey(p.Request).UserID,
183+
UserID: userID,
158184
Ip: ip,
159185
UserAgent: sql.NullString{String: p.Request.UserAgent(), Valid: true},
160-
ResourceType: either(req.Old, req.New, ResourceType[T]),
161-
ResourceID: either(req.Old, req.New, ResourceID[T]),
162-
ResourceTarget: either(req.Old, req.New, ResourceTarget[T]),
186+
ResourceType: either(req.Old, req.New, ResourceType[T], req.params.Action),
187+
ResourceID: either(req.Old, req.New, ResourceID[T], req.params.Action),
188+
ResourceTarget: either(req.Old, req.New, ResourceTarget[T], req.params.Action),
163189
Action: p.Action,
164190
Diff: diffRaw,
165191
StatusCode: int32(sw.Status),
@@ -202,9 +228,9 @@ func BuildAudit[T Auditable](ctx context.Context, p *BuildAuditParams[T]) {
202228
UserID: p.UserID,
203229
Ip: ip,
204230
UserAgent: sql.NullString{},
205-
ResourceType: either(p.Old, p.New, ResourceType[T]),
206-
ResourceID: either(p.Old, p.New, ResourceID[T]),
207-
ResourceTarget: either(p.Old, p.New, ResourceTarget[T]),
231+
ResourceType: either(p.Old, p.New, ResourceType[T], p.Action),
232+
ResourceID: either(p.Old, p.New, ResourceID[T], p.Action),
233+
ResourceTarget: either(p.Old, p.New, ResourceTarget[T], p.Action),
208234
Action: p.Action,
209235
Diff: diffRaw,
210236
StatusCode: int32(p.Status),
@@ -221,11 +247,15 @@ func BuildAudit[T Auditable](ctx context.Context, p *BuildAuditParams[T]) {
221247
}
222248
}
223249

224-
func either[T Auditable, R any](old, new T, fn func(T) R) R {
250+
func either[T Auditable, R any](old, new T, fn func(T) R, auditAction database.AuditAction) R {
225251
if ResourceID(new) != uuid.Nil {
226252
return fn(new)
227253
} else if ResourceID(old) != uuid.Nil {
228254
return fn(old)
255+
} else if auditAction == database.AuditActionLogin || auditAction == database.AuditActionLogout {
256+
// If the request action is a login or logout, we always want to audit it even if
257+
// there is no diff. See the comment in audit.InitRequest for more detail.
258+
return fn(old)
229259
} else {
230260
panic("both old and new are nil")
231261
}

‎coderd/database/dump.sql

+3-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000094_add_resource_type_api_key.down.sql

+2
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
-- It's not possible to drop enum values from enum types, so the UP has "IF NOT
2+
-- EXISTS".

‎coderd/database/migrations/000094_add_resource_type_api_key.up.sql

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
ALTER TYPE audit_action
2+
ADD VALUE IF NOT EXISTS 'login';
3+
4+
ALTER TYPE audit_action
5+
ADD VALUE IF NOT EXISTS 'logout';
6+
7+
ALTER TYPE resource_type
8+
ADD VALUE IF NOT EXISTS 'api_key';
9+

‎coderd/database/models.go

+7-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/gitsshkey_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -95,8 +95,8 @@ func TestGitSSHKey(t *testing.T) {
9595
require.NotEmpty(t, key2.PublicKey)
9696
require.NotEqual(t, key2.PublicKey, key1.PublicKey)
9797

98-
require.Len(t, auditor.AuditLogs, 1)
99-
assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[0].Action)
98+
require.Len(t, auditor.AuditLogs, 2)
99+
assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[1].Action)
100100
})
101101
}
102102

‎coderd/templates_test.go

+9-8
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,11 @@ func TestPostTemplateByOrganization(t *testing.T) {
6161
assert.Equal(t, expected.Name, got.Name)
6262
assert.Equal(t, expected.Description, got.Description)
6363

64-
require.Len(t, auditor.AuditLogs, 3)
65-
assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[0].Action)
66-
assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[1].Action)
67-
assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[2].Action)
64+
require.Len(t, auditor.AuditLogs, 4)
65+
assert.Equal(t, database.AuditActionLogin, auditor.AuditLogs[0].Action)
66+
assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[1].Action)
67+
assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[2].Action)
68+
assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[3].Action)
6869
})
6970

7071
t.Run("AlreadyExists", func(t *testing.T) {
@@ -285,8 +286,8 @@ func TestPatchTemplateMeta(t *testing.T) {
285286
assert.Equal(t, req.DefaultTTLMillis, updated.DefaultTTLMillis)
286287
assert.False(t, req.AllowUserCancelWorkspaceJobs)
287288

288-
require.Len(t, auditor.AuditLogs, 4)
289-
assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[3].Action)
289+
require.Len(t, auditor.AuditLogs, 5)
290+
assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[4].Action)
290291
})
291292

292293
t.Run("NoMaxTTL", func(t *testing.T) {
@@ -448,8 +449,8 @@ func TestDeleteTemplate(t *testing.T) {
448449
err := client.DeleteTemplate(ctx, template.ID)
449450
require.NoError(t, err)
450451

451-
require.Len(t, auditor.AuditLogs, 4)
452-
assert.Equal(t, database.AuditActionDelete, auditor.AuditLogs[3].Action)
452+
require.Len(t, auditor.AuditLogs, 5)
453+
assert.Equal(t, database.AuditActionDelete, auditor.AuditLogs[4].Action)
453454
})
454455

455456
t.Run("Workspaces", func(t *testing.T) {

‎coderd/templateversions_test.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -127,8 +127,8 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) {
127127
require.Equal(t, "bananas", version.Name)
128128
require.Equal(t, provisionerdserver.ScopeOrganization, version.Job.Tags[provisionerdserver.TagScope])
129129

130-
require.Len(t, auditor.AuditLogs, 1)
131-
assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[0].Action)
130+
require.Len(t, auditor.AuditLogs, 2)
131+
assert.Equal(t, database.AuditActionCreate, auditor.AuditLogs[1].Action)
132132
})
133133
t.Run("Example", func(t *testing.T) {
134134
t.Parallel()
@@ -646,8 +646,8 @@ func TestPatchActiveTemplateVersion(t *testing.T) {
646646
})
647647
require.NoError(t, err)
648648

649-
require.Len(t, auditor.AuditLogs, 4)
650-
assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[3].Action)
649+
require.Len(t, auditor.AuditLogs, 5)
650+
assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[4].Action)
651651
})
652652
}
653653

0 commit comments

Comments
 (0)