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>

Author: Kira-Pilot

coderd/apidoc/docs.go

@@ -70,7 +70,7 @@ func (api *API) postToken(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	cookie, err := api.createAPIKey(ctx, createAPIKeyParams{
+	cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{
 		UserID:          user.ID,
 		LoginType:       database.LoginTypeToken,
 		ExpiresAt:       database.Now().Add(lifeTime),
@@ -108,7 +108,7 @@ func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) {
 	}
 
 	lifeTime := time.Hour * 24 * 7
-	cookie, err := api.createAPIKey(ctx, createAPIKeyParams{
+	cookie, _, err := api.createAPIKey(ctx, createAPIKeyParams{
 		UserID:     user.ID,
 		LoginType:  database.LoginTypePassword,
 		RemoteAddr: r.RemoteAddr,
@@ -281,10 +281,10 @@ func (api *API) validateAPIKeyLifetime(lifetime time.Duration) error {
 	return nil
 }
 
-func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*http.Cookie, error) {
+func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*http.Cookie, *database.APIKey, error) {
 	keyID, keySecret, err := GenerateAPIKeyIDSecret()
 	if err != nil {
-		return nil, xerrors.Errorf("generate API key: %w", err)
+		return nil, nil, xerrors.Errorf("generate API key: %w", err)
 	}
 	hashed := sha256.Sum256([]byte(keySecret))
 
@@ -315,7 +315,7 @@ func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*h
 	switch scope {
 	case database.APIKeyScopeAll, database.APIKeyScopeApplicationConnect:
 	default:
-		return nil, xerrors.Errorf("invalid API key scope: %q", scope)
+		return nil, nil, xerrors.Errorf("invalid API key scope: %q", scope)
 	}
 
 	key, err := api.Database.InsertAPIKey(ctx, database.InsertAPIKeyParams{
@@ -338,7 +338,7 @@ func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*h
 		Scope:        scope,
 	})
 	if err != nil {
-		return nil, xerrors.Errorf("insert API key: %w", err)
+		return nil, nil, xerrors.Errorf("insert API key: %w", err)
 	}
 
 	api.Telemetry.Report(&telemetry.Snapshot{
@@ -354,5 +354,5 @@ func (api *API) createAPIKey(ctx context.Context, params createAPIKeyParams) (*h
 		HttpOnly: true,
 		SameSite: http.SameSiteLaxMode,
 		Secure:   api.SecureAuthCookie,
-	}, nil
+	}, &key, nil
 }

coderd/apidoc/swagger.json

@@ -264,6 +264,12 @@ func auditLogDescription(alog database.GetAuditLogsOffsetRow, additionalFields a
 		codersdk.AuditAction(alog.Action).Friendly(),
 	)
 
+	// API Key resources do not have targets and follow the below format:
+	// "User {logged in | logged out}"
+	if alog.ResourceType == database.ResourceTypeApiKey {
+		return str
+	}
+
 	// Strings for starting/stopping workspace builds follow the below format:
 	// "{user | 'Coder automatically'} started build #{build_number} for workspace {target}"
 	// where target is a workspace (name) instead of a workspace build
@@ -488,6 +494,10 @@ func actionFromString(actionString string) string {
 		return actionString
 	case codersdk.AuditActionStop:
 		return actionString
+	case codersdk.AuditActionLogin:
+		return actionString
+	case codersdk.AuditActionLogout:
+		return actionString
 	default:
 	}
 	return ""

coderd/apikey.go

@@ -31,6 +31,10 @@ type Request[T Auditable] struct {
 
 	Old T
 	New T
+
+	// This optional field can be passed in when the userID cannot be determined from the API Key
+	// such as in the case of login, when the audit log is created prior the API Key's existence.
+	UserID uuid.UUID
 }
 
 type BuildAuditParams[T Auditable] struct {
@@ -64,6 +68,9 @@ func ResourceTarget[T Auditable](tgt T) string {
 		return typed.PublicKey
 	case database.AuditableGroup:
 		return typed.Group.Name
+	case database.APIKey:
+		// this isn't used
+		return ""
 	default:
 		panic(fmt.Sprintf("unknown resource %T", tgt))
 	}
@@ -85,6 +92,8 @@ func ResourceID[T Auditable](tgt T) uuid.UUID {
 		return typed.UserID
 	case database.AuditableGroup:
 		return typed.Group.ID
+	case database.APIKey:
+		return typed.UserID
 	default:
 		panic(fmt.Sprintf("unknown resource %T", tgt))
 	}
@@ -106,6 +115,8 @@ func ResourceType[T Auditable](tgt T) database.ResourceType {
 		return database.ResourceTypeGitSshKey
 	case database.AuditableGroup:
 		return database.ResourceTypeGroup
+	case database.APIKey:
+		return database.ResourceTypeApiKey
 	default:
 		panic(fmt.Sprintf("unknown resource %T", tgt))
 	}
@@ -130,7 +141,14 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request
 
 		// If no resources were provided, there's nothing we can audit.
 		if ResourceID(req.Old) == uuid.Nil && ResourceID(req.New) == uuid.Nil {
-			return
+			// If the request action is a login or logout, we always want to audit it even if
+			// there is no diff. This is so we can capture events where an API Key is never created
+			// because an unknown user fails to login.
+			// TODO: introduce the concept of an anonymous user so we always have a userID even
+			// when dealing with a mystery user. https://github.com/coder/coder/issues/6054
+			if req.params.Action != database.AuditActionLogin && req.params.Action != database.AuditActionLogout {
+				return
+			}
 		}
 
 		var diffRaw = []byte("{}")
@@ -150,16 +168,24 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request
 			p.AdditionalFields = json.RawMessage("{}")
 		}
 
+		var userID uuid.UUID
+		key, ok := httpmw.APIKeyOptional(p.Request)
+		if ok {
+			userID = key.UserID
+		} else {
+			userID = req.UserID
+		}
+
 		ip := parseIP(p.Request.RemoteAddr)
 		auditLog := database.AuditLog{
 			ID:               uuid.New(),
 			Time:             database.Now(),
-			UserID:           httpmw.APIKey(p.Request).UserID,
+			UserID:           userID,
 			Ip:               ip,
 			UserAgent:        sql.NullString{String: p.Request.UserAgent(), Valid: true},
-			ResourceType:     either(req.Old, req.New, ResourceType[T]),
-			ResourceID:       either(req.Old, req.New, ResourceID[T]),
-			ResourceTarget:   either(req.Old, req.New, ResourceTarget[T]),
+			ResourceType:     either(req.Old, req.New, ResourceType[T], req.params.Action),
+			ResourceID:       either(req.Old, req.New, ResourceID[T], req.params.Action),
+			ResourceTarget:   either(req.Old, req.New, ResourceTarget[T], req.params.Action),
 			Action:           p.Action,
 			Diff:             diffRaw,
 			StatusCode:       int32(sw.Status),
@@ -202,9 +228,9 @@ func BuildAudit[T Auditable](ctx context.Context, p *BuildAuditParams[T]) {
 		UserID:           p.UserID,
 		Ip:               ip,
 		UserAgent:        sql.NullString{},
-		ResourceType:     either(p.Old, p.New, ResourceType[T]),
-		ResourceID:       either(p.Old, p.New, ResourceID[T]),
-		ResourceTarget:   either(p.Old, p.New, ResourceTarget[T]),
+		ResourceType:     either(p.Old, p.New, ResourceType[T], p.Action),
+		ResourceID:       either(p.Old, p.New, ResourceID[T], p.Action),
+		ResourceTarget:   either(p.Old, p.New, ResourceTarget[T], p.Action),
 		Action:           p.Action,
 		Diff:             diffRaw,
 		StatusCode:       int32(p.Status),
@@ -221,11 +247,15 @@ func BuildAudit[T Auditable](ctx context.Context, p *BuildAuditParams[T]) {
 	}
 }
 
-func either[T Auditable, R any](old, new T, fn func(T) R) R {
+func either[T Auditable, R any](old, new T, fn func(T) R, auditAction database.AuditAction) R {
 	if ResourceID(new) != uuid.Nil {
 		return fn(new)
 	} else if ResourceID(old) != uuid.Nil {
 		return fn(old)
+	} else if auditAction == database.AuditActionLogin || auditAction == database.AuditActionLogout {
+		// If the request action is a login or logout, we always want to audit it even if
+		// there is no diff. See the comment in audit.InitRequest for more detail.
+		return fn(old)
 	} else {
 		panic("both old and new are nil")
 	}

coderd/audit.go

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

coderd/audit/request.go

@@ -0,0 +1,9 @@
+ALTER TYPE audit_action
+  ADD VALUE IF NOT EXISTS 'login';
+
+ALTER TYPE audit_action
+  ADD VALUE IF NOT EXISTS 'logout';
+
+ALTER TYPE resource_type
+  ADD VALUE IF NOT EXISTS 'api_key';
+

coderd/database/dump.sql

@@ -95,8 +95,8 @@ func TestGitSSHKey(t *testing.T) {
 		require.NotEmpty(t, key2.PublicKey)
 		require.NotEqual(t, key2.PublicKey, key1.PublicKey)
 
-		require.Len(t, auditor.AuditLogs, 1)
-		assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[0].Action)
+		require.Len(t, auditor.AuditLogs, 2)
+		assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[1].Action)
 	})
 }