From e20d7e65116001a5bcadd4bd4bf763e78b927257 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Thu, 16 Mar 2023 20:50:39 +0000 Subject: [PATCH 1/5] auditing tokens --- coderd/apikey.go | 41 +++++++++++++++++++++++++++++++-------- coderd/audit.go | 5 +++-- coderd/audit/request.go | 6 +++++- codersdk/audit.go | 2 +- enterprise/audit/table.go | 2 +- 5 files changed, 43 insertions(+), 13 deletions(-) diff --git a/coderd/apikey.go b/coderd/apikey.go index 19867afb77a95..8330488191e53 100644 --- a/coderd/apikey.go +++ b/coderd/apikey.go @@ -18,6 +18,7 @@ import ( "github.com/tabbed/pqtype" "golang.org/x/xerrors" + "github.com/coder/coder/coderd/audit" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" @@ -40,8 +41,19 @@ import ( // @Success 201 {object} codersdk.GenerateAPIKeyResponse // @Router /users/{user}/keys/tokens [post] func (api *API) postToken(rw http.ResponseWriter, r *http.Request) { - ctx := r.Context() - user := httpmw.UserParam(r) + var ( + ctx = r.Context() + user = httpmw.UserParam(r) + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionCreate, + }) + ) + aReq.Old = database.APIKey{} + defer commitAudit() if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(user.ID.String())) { httpapi.ResourceNotFound(rw) @@ -79,7 +91,7 @@ func (api *API) postToken(rw http.ResponseWriter, r *http.Request) { return } - cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{ + cookie, key, err := api.createAPIKey(ctx, createAPIKeyParams{ UserID: user.ID, LoginType: database.LoginTypeToken, ExpiresAt: database.Now().Add(lifeTime), @@ -104,7 +116,7 @@ func (api *API) postToken(rw http.ResponseWriter, r *http.Request) { }) return } - + aReq.New = *key httpapi.Write(ctx, rw, http.StatusCreated, codersdk.GenerateAPIKeyResponse{Key: cookie.Value}) } @@ -314,17 +326,30 @@ func (api *API) tokens(rw http.ResponseWriter, r *http.Request) { // @Router /users/{user}/keys/{keyid} [delete] func (api *API) deleteAPIKey(rw http.ResponseWriter, r *http.Request) { var ( - ctx = r.Context() - user = httpmw.UserParam(r) - keyID = chi.URLParam(r, "keyid") + ctx = r.Context() + user = httpmw.UserParam(r) + keyID = chi.URLParam(r, "keyid") + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionDelete, + }) + key, err = api.Database.GetAPIKeyByID(ctx, keyID) ) + if err != nil { + api.Logger.Warn(ctx, "get API Key for audit log") + } + aReq.Old = key + defer commitAudit() if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceAPIKey.WithIDString(keyID).WithOwner(user.ID.String())) { httpapi.ResourceNotFound(rw) return } - err := api.Database.DeleteAPIKeyByID(ctx, keyID) + err = api.Database.DeleteAPIKeyByID(ctx, keyID) if errors.Is(err, sql.ErrNoRows) { httpapi.ResourceNotFound(rw) return diff --git a/coderd/audit.go b/coderd/audit.go index 493b5bec795f4..ebad019abb15a 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -254,9 +254,10 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow) string { codersdk.AuditAction(alog.Action).Friendly(), ) - // API Key resources do not have targets and follow the below format: + // API Key (used for authentication) do not have targets and follow the below format: // "User {logged in | logged out}" - if alog.ResourceType == database.ResourceTypeApiKey { + if alog.ResourceType == database.ResourceTypeApiKey && + (alog.Action == database.AuditActionLogin || alog.Action == database.AuditActionLogout) { return str } diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 8aba0ffc30adf..221775b83c40a 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -70,7 +70,11 @@ func ResourceTarget[T Auditable](tgt T) string { case database.AuditableGroup: return typed.Group.Name case database.APIKey: - // this isn't used + if typed.TokenName != "nil" { + return typed.TokenName + } + // API Keys without names are used for Auth + // and don't have a target return "" case database.License: return strconv.Itoa(int(typed.ID)) diff --git a/codersdk/audit.go b/codersdk/audit.go index 464d350fc5532..e32bd7e8da708 100644 --- a/codersdk/audit.go +++ b/codersdk/audit.go @@ -42,7 +42,7 @@ func (r ResourceType) FriendlyString() string { case ResourceTypeGitSSHKey: return "git ssh key" case ResourceTypeAPIKey: - return "api key" + return "token" case ResourceTypeGroup: return "group" case ResourceTypeLicense: diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index d6e6acbdbf4bd..b86a0ef84d633 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -21,7 +21,7 @@ var AuditActionMap = map[string][]codersdk.AuditAction{ "Workspace": {codersdk.AuditActionCreate, codersdk.AuditActionWrite, codersdk.AuditActionDelete}, "WorkspaceBuild": {codersdk.AuditActionStart, codersdk.AuditActionStop}, "Group": {codersdk.AuditActionCreate, codersdk.AuditActionWrite, codersdk.AuditActionDelete}, - "APIKey": {codersdk.AuditActionWrite}, + "APIKey": {codersdk.AuditActionLogin, codersdk.AuditActionLogout, codersdk.AuditActionCreate, codersdk.AuditActionDelete}, "License": {codersdk.AuditActionCreate, codersdk.AuditActionDelete}, } From c339a72ff567f06a2d9f56f78285061aba3f6d55 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Thu, 16 Mar 2023 21:07:49 +0000 Subject: [PATCH 2/5] adding diffs for token auditing --- coderd/audit/request.go | 6 ++++-- enterprise/audit/table.go | 7 +++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 221775b83c40a..e2c6a3b3dda67 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -163,8 +163,10 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request } diffRaw := []byte("{}") - // Only generate diffs if the request succeeded. - if sw.Status < 400 { + // Only generate diffs if the request succeeded + // and only if we aren't auditing authentication actions + if sw.Status < 400 && + req.params.Action != database.AuditActionLogin && req.params.Action != database.AuditActionLogout { diff := Diff(p.Audit, req.Old, req.New) var err error diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index b86a0ef84d633..674aee0dc9dd4 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -137,14 +137,13 @@ var auditableResourcesTypes = map[any]map[string]Action{ "quota_allowance": ActionTrack, "members": ActionTrack, }, - // We don't show any diff for the APIKey resource &database.APIKey{}: { "id": ActionIgnore, "hashed_secret": ActionIgnore, "user_id": ActionIgnore, - "last_used": ActionIgnore, - "expires_at": ActionIgnore, - "created_at": ActionIgnore, + "last_used": ActionTrack, + "expires_at": ActionTrack, + "created_at": ActionTrack, "updated_at": ActionIgnore, "login_type": ActionIgnore, "lifetime_seconds": ActionIgnore, From 9d456464749b053c1e89f287bdd59f2c7f3d0c39 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Fri, 17 Mar 2023 14:52:03 +0000 Subject: [PATCH 3/5] added test --- coderd/apikey_test.go | 14 +++++++++++++- coderd/audit.go | 2 +- coderd/audit/request.go | 2 +- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/coderd/apikey_test.go b/coderd/apikey_test.go index 913d5f2551ba3..d1bdd714ab485 100644 --- a/coderd/apikey_test.go +++ b/coderd/apikey_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/cli/clibase" + "github.com/coder/coder/coderd/audit" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/database/dbtestutil" @@ -23,8 +24,12 @@ func TestTokenCRUD(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - client := coderdtest.New(t, nil) + auditor := audit.NewMock() + numLogs := len(auditor.AuditLogs) + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) _ = coderdtest.CreateFirstUser(t, client) + numLogs++ // add an audit log for user creation + keys, err := client.Tokens(ctx, codersdk.Me, codersdk.TokensFilter{}) require.NoError(t, err) require.Empty(t, keys) @@ -32,6 +37,7 @@ func TestTokenCRUD(t *testing.T) { res, err := client.CreateToken(ctx, codersdk.Me, codersdk.CreateTokenRequest{}) require.NoError(t, err) require.Greater(t, len(res.Key), 2) + numLogs++ // add an audit log for token creation keys, err = client.Tokens(ctx, codersdk.Me, codersdk.TokensFilter{}) require.NoError(t, err) @@ -46,9 +52,15 @@ func TestTokenCRUD(t *testing.T) { err = client.DeleteAPIKey(ctx, codersdk.Me, keys[0].ID) require.NoError(t, err) + numLogs++ // add an audit log for token deletion keys, err = client.Tokens(ctx, codersdk.Me, codersdk.TokensFilter{}) require.NoError(t, err) require.Empty(t, keys) + + // ensure audit log count is correct + require.Len(t, auditor.AuditLogs, numLogs) + require.Equal(t, database.AuditActionCreate, auditor.AuditLogs[numLogs-2].Action) + require.Equal(t, database.AuditActionDelete, auditor.AuditLogs[numLogs-1].Action) } func TestTokenScoped(t *testing.T) { diff --git a/coderd/audit.go b/coderd/audit.go index ebad019abb15a..f74df92b099c3 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -254,7 +254,7 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow) string { codersdk.AuditAction(alog.Action).Friendly(), ) - // API Key (used for authentication) do not have targets and follow the below format: + // API Key resources (used for authentication) do not have targets and follow the below format: // "User {logged in | logged out}" if alog.ResourceType == database.ResourceTypeApiKey && (alog.Action == database.AuditActionLogin || alog.Action == database.AuditActionLogout) { diff --git a/coderd/audit/request.go b/coderd/audit/request.go index e2c6a3b3dda67..5627e5c3826dd 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -73,7 +73,7 @@ func ResourceTarget[T Auditable](tgt T) string { if typed.TokenName != "nil" { return typed.TokenName } - // API Keys without names are used for Auth + // API Keys without names are used for auth // and don't have a target return "" case database.License: From e8b952bea77beab848943aeb17df84a18d075f7f Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Fri, 17 Mar 2023 15:04:15 +0000 Subject: [PATCH 4/5] generating docs --- docs/admin/audit-logs.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index a532ed520084a..565272b725e50 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -9,17 +9,17 @@ We track the following resources: -| Resource | | -| ----------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| APIKey
write |
FieldTracked
created_atfalse
expires_atfalse
hashed_secretfalse
idfalse
ip_addressfalse
last_usedfalse
lifetime_secondsfalse
login_typefalse
scopefalse
token_namefalse
updated_atfalse
user_idfalse
| -| Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| -| GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| -| License
create, delete |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| -| Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
group_acltrue
icontrue
idtrue
max_ttltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| -| TemplateVersion
create, write |
FieldTracked
created_atfalse
created_bytrue
git_auth_providersfalse
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| -| User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| -| Workspace
create, write, delete |
FieldTracked
autostart_scheduletrue
created_atfalse
deletedfalse
idtrue
last_used_atfalse
nametrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
| -| WorkspaceBuild
start, stop |
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_idfalse
job_idfalse
max_deadlinefalse
provisioner_statefalse
reasonfalse
template_version_idtrue
transitionfalse
updated_atfalse
workspace_idfalse
| +| Resource | | +| ---------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| APIKey
login, logout, create, delete |
FieldTracked
created_attrue
expires_attrue
hashed_secretfalse
idfalse
ip_addressfalse
last_usedtrue
lifetime_secondsfalse
login_typefalse
scopefalse
token_namefalse
updated_atfalse
user_idfalse
| +| Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| +| GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| +| License
create, delete |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| +| Template
write, delete |
FieldTracked
active_version_idtrue
allow_user_cancel_workspace_jobstrue
created_atfalse
created_bytrue
default_ttltrue
deletedfalse
descriptiontrue
display_nametrue
group_acltrue
icontrue
idtrue
max_ttltrue
nametrue
organization_idfalse
provisionertrue
updated_atfalse
user_acltrue
| +| TemplateVersion
create, write |
FieldTracked
created_atfalse
created_bytrue
git_auth_providersfalse
idtrue
job_idfalse
nametrue
organization_idfalse
readmetrue
template_idtrue
updated_atfalse
| +| User
create, write, delete |
FieldTracked
avatar_urlfalse
created_atfalse
deletedtrue
emailtrue
hashed_passwordtrue
idtrue
last_seen_atfalse
login_typefalse
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| +| Workspace
create, write, delete |
FieldTracked
autostart_scheduletrue
created_atfalse
deletedfalse
idtrue
last_used_atfalse
nametrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
| +| WorkspaceBuild
start, stop |
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
idfalse
initiator_idfalse
job_idfalse
max_deadlinefalse
provisioner_statefalse
reasonfalse
template_version_idtrue
transitionfalse
updated_atfalse
workspace_idfalse
| From 7ba67f4b49c977b10d0e34a70f2da7ede129a747 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Fri, 17 Mar 2023 15:16:19 +0000 Subject: [PATCH 5/5] auditing owner field --- docs/admin/audit-logs.md | 2 +- enterprise/audit/table.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/admin/audit-logs.md b/docs/admin/audit-logs.md index 565272b725e50..601146009efb0 100644 --- a/docs/admin/audit-logs.md +++ b/docs/admin/audit-logs.md @@ -11,7 +11,7 @@ We track the following resources: | Resource | | | ---------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| APIKey
login, logout, create, delete |
FieldTracked
created_attrue
expires_attrue
hashed_secretfalse
idfalse
ip_addressfalse
last_usedtrue
lifetime_secondsfalse
login_typefalse
scopefalse
token_namefalse
updated_atfalse
user_idfalse
| +| APIKey
login, logout, create, delete |
FieldTracked
created_attrue
expires_attrue
hashed_secretfalse
idfalse
ip_addressfalse
last_usedtrue
lifetime_secondsfalse
login_typefalse
scopefalse
token_namefalse
updated_atfalse
user_idtrue
| | Group
create, write, delete |
FieldTracked
avatar_urltrue
idtrue
memberstrue
nametrue
organization_idfalse
quota_allowancetrue
| | GitSSHKey
create |
FieldTracked
created_atfalse
private_keytrue
public_keytrue
updated_atfalse
user_idtrue
| | License
create, delete |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 674aee0dc9dd4..d020324748482 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -140,7 +140,7 @@ var auditableResourcesTypes = map[any]map[string]Action{ &database.APIKey{}: { "id": ActionIgnore, "hashed_secret": ActionIgnore, - "user_id": ActionIgnore, + "user_id": ActionTrack, "last_used": ActionTrack, "expires_at": ActionTrack, "created_at": ActionTrack,