Skip to content

Commit 090e37f

Browse files
Kira-PilotKira-Pilot
authored
feat(audit): auditing token addition and removal (coder#6649)
* auditing tokens * adding diffs for token auditing * added test * generating docs * auditing owner field
1 parent 5b07f1e commit 090e37f

File tree

7 files changed

+75
-32
lines changed

7 files changed

+75
-32
lines changed

‎coderd/apikey.go

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"github.com/tabbed/pqtype"
1919
"golang.org/x/xerrors"
2020

21+
"github.com/coder/coder/coderd/audit"
2122
"github.com/coder/coder/coderd/database"
2223
"github.com/coder/coder/coderd/httpapi"
2324
"github.com/coder/coder/coderd/httpmw"
@@ -40,8 +41,19 @@ import (
4041
// @Success 201 {object} codersdk.GenerateAPIKeyResponse
4142
// @Router /users/{user}/keys/tokens [post]
4243
func (api *API) postToken(rw http.ResponseWriter, r *http.Request) {
43-
ctx := r.Context()
44-
user := httpmw.UserParam(r)
44+
var (
45+
ctx = r.Context()
46+
user = httpmw.UserParam(r)
47+
auditor = api.Auditor.Load()
48+
aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{
49+
Audit: *auditor,
50+
Log: api.Logger,
51+
Request: r,
52+
Action: database.AuditActionCreate,
53+
})
54+
)
55+
aReq.Old = database.APIKey{}
56+
defer commitAudit()
4557

4658
if !api.Authorize(r, rbac.ActionCreate, rbac.ResourceAPIKey.WithOwner(user.ID.String())) {
4759
httpapi.ResourceNotFound(rw)
@@ -79,7 +91,7 @@ func (api *API) postToken(rw http.ResponseWriter, r *http.Request) {
7991
return
8092
}
8193

82-
cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{
94+
cookie, key, err := api.createAPIKey(ctx, createAPIKeyParams{
8395
UserID: user.ID,
8496
LoginType: database.LoginTypeToken,
8597
ExpiresAt: database.Now().Add(lifeTime),
@@ -104,7 +116,7 @@ func (api *API) postToken(rw http.ResponseWriter, r *http.Request) {
104116
})
105117
return
106118
}
107-
119+
aReq.New = *key
108120
httpapi.Write(ctx, rw, http.StatusCreated, codersdk.GenerateAPIKeyResponse{Key: cookie.Value})
109121
}
110122

@@ -314,17 +326,30 @@ func (api *API) tokens(rw http.ResponseWriter, r *http.Request) {
314326
// @Router /users/{user}/keys/{keyid} [delete]
315327
func (api *API) deleteAPIKey(rw http.ResponseWriter, r *http.Request) {
316328
var (
317-
ctx = r.Context()
318-
user = httpmw.UserParam(r)
319-
keyID = chi.URLParam(r, "keyid")
329+
ctx = r.Context()
330+
user = httpmw.UserParam(r)
331+
keyID = chi.URLParam(r, "keyid")
332+
auditor = api.Auditor.Load()
333+
aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{
334+
Audit: *auditor,
335+
Log: api.Logger,
336+
Request: r,
337+
Action: database.AuditActionDelete,
338+
})
339+
key, err = api.Database.GetAPIKeyByID(ctx, keyID)
320340
)
341+
if err != nil {
342+
api.Logger.Warn(ctx, "get API Key for audit log")
343+
}
344+
aReq.Old = key
345+
defer commitAudit()
321346

322347
if !api.Authorize(r, rbac.ActionDelete, rbac.ResourceAPIKey.WithIDString(keyID).WithOwner(user.ID.String())) {
323348
httpapi.ResourceNotFound(rw)
324349
return
325350
}
326351

327-
err := api.Database.DeleteAPIKeyByID(ctx, keyID)
352+
err = api.Database.DeleteAPIKeyByID(ctx, keyID)
328353
if errors.Is(err, sql.ErrNoRows) {
329354
httpapi.ResourceNotFound(rw)
330355
return

‎coderd/apikey_test.go

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"github.com/stretchr/testify/require"
1212

1313
"github.com/coder/coder/cli/clibase"
14+
"github.com/coder/coder/coderd/audit"
1415
"github.com/coder/coder/coderd/coderdtest"
1516
"github.com/coder/coder/coderd/database"
1617
"github.com/coder/coder/coderd/database/dbtestutil"
@@ -23,15 +24,20 @@ func TestTokenCRUD(t *testing.T) {
2324

2425
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
2526
defer cancel()
26-
client := coderdtest.New(t, nil)
27+
auditor := audit.NewMock()
28+
numLogs := len(auditor.AuditLogs)
29+
client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor})
2730
_ = coderdtest.CreateFirstUser(t, client)
31+
numLogs++ // add an audit log for user creation
32+
2833
keys, err := client.Tokens(ctx, codersdk.Me, codersdk.TokensFilter{})
2934
require.NoError(t, err)
3035
require.Empty(t, keys)
3136

3237
res, err := client.CreateToken(ctx, codersdk.Me, codersdk.CreateTokenRequest{})
3338
require.NoError(t, err)
3439
require.Greater(t, len(res.Key), 2)
40+
numLogs++ // add an audit log for token creation
3541

3642
keys, err = client.Tokens(ctx, codersdk.Me, codersdk.TokensFilter{})
3743
require.NoError(t, err)
@@ -46,9 +52,15 @@ func TestTokenCRUD(t *testing.T) {
4652

4753
err = client.DeleteAPIKey(ctx, codersdk.Me, keys[0].ID)
4854
require.NoError(t, err)
55+
numLogs++ // add an audit log for token deletion
4956
keys, err = client.Tokens(ctx, codersdk.Me, codersdk.TokensFilter{})
5057
require.NoError(t, err)
5158
require.Empty(t, keys)
59+
60+
// ensure audit log count is correct
61+
require.Len(t, auditor.AuditLogs, numLogs)
62+
require.Equal(t, database.AuditActionCreate, auditor.AuditLogs[numLogs-2].Action)
63+
require.Equal(t, database.AuditActionDelete, auditor.AuditLogs[numLogs-1].Action)
5264
}
5365

5466
func TestTokenScoped(t *testing.T) {

‎coderd/audit.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -254,9 +254,10 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow) string {
254254
codersdk.AuditAction(alog.Action).Friendly(),
255255
)
256256

257-
// API Key resources do not have targets and follow the below format:
257+
// API Key resources (used for authentication) do not have targets and follow the below format:
258258
// "User {logged in | logged out}"
259-
if alog.ResourceType == database.ResourceTypeApiKey {
259+
if alog.ResourceType == database.ResourceTypeApiKey &&
260+
(alog.Action == database.AuditActionLogin || alog.Action == database.AuditActionLogout) {
260261
return str
261262
}
262263

‎coderd/audit/request.go

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,11 @@ func ResourceTarget[T Auditable](tgt T) string {
7070
case database.AuditableGroup:
7171
return typed.Group.Name
7272
case database.APIKey:
73-
// this isn't used
73+
if typed.TokenName != "nil" {
74+
return typed.TokenName
75+
}
76+
// API Keys without names are used for auth
77+
// and don't have a target
7478
return ""
7579
case database.License:
7680
return strconv.Itoa(int(typed.ID))
@@ -159,8 +163,10 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request
159163
}
160164

161165
diffRaw := []byte("{}")
162-
// Only generate diffs if the request succeeded.
163-
if sw.Status < 400 {
166+
// Only generate diffs if the request succeeded
167+
// and only if we aren't auditing authentication actions
168+
if sw.Status < 400 &&
169+
req.params.Action != database.AuditActionLogin && req.params.Action != database.AuditActionLogout {
164170
diff := Diff(p.Audit, req.Old, req.New)
165171

166172
var err error

‎codersdk/audit.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ func (r ResourceType) FriendlyString() string {
4242
case ResourceTypeGitSSHKey:
4343
return "git ssh key"
4444
case ResourceTypeAPIKey:
45-
return "api key"
45+
return "token"
4646
case ResourceTypeGroup:
4747
return "group"
4848
case ResourceTypeLicense:

0 commit comments

Comments
 (0)