From df3a6ce3b2299aa527a348269cc61939171213de Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Fri, 18 Nov 2022 16:02:01 +0000 Subject: [PATCH 1/9] auditing failed builds --- coderd/audit.go | 10 +- coderd/audit/request.go | 51 +++++++++- coderd/database/dump.sql | 4 +- ...077_add_audit_log_nullable_fields.down.sql | 2 + ...00077_add_audit_log_nullable_fields.up.sql | 2 + coderd/database/models.go | 2 +- coderd/database/queries.sql.go | 4 +- coderd/provisionerdaemons.go | 1 + .../provisionerdserver/provisionerdserver.go | 92 +++++++++++++++++++ .../components/AuditLogRow/AuditLogRow.tsx | 5 +- 10 files changed, 164 insertions(+), 9 deletions(-) create mode 100644 coderd/database/migrations/000077_add_audit_log_nullable_fields.down.sql create mode 100644 coderd/database/migrations/000077_add_audit_log_nullable_fields.up.sql diff --git a/coderd/audit.go b/coderd/audit.go index d43f804e6a52d..e2ac3b0f659e1 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -1,6 +1,7 @@ package coderd import ( + "database/sql" "encoding/json" "fmt" "net" @@ -155,7 +156,7 @@ func (api *API) generateFakeAuditLog(rw http.ResponseWriter, r *http.Request) { Time: params.Time, UserID: user.ID, Ip: ipNet, - UserAgent: r.UserAgent(), + UserAgent: sql.NullString{String: r.UserAgent(), Valid: true}, ResourceType: database.ResourceType(params.ResourceType), ResourceID: params.ResourceID, ResourceTarget: user.Username, @@ -189,6 +190,11 @@ func convertAuditLog(dblog database.GetAuditLogsOffsetRow) codersdk.AuditLog { _ = json.Unmarshal(dblog.Diff, &diff) var user *codersdk.User + var agent string + + if dblog.UserAgent.Valid { + agent = dblog.UserAgent.String + } if dblog.UserUsername.Valid { user = &codersdk.User{ ID: dblog.UserID, @@ -212,7 +218,7 @@ func convertAuditLog(dblog database.GetAuditLogsOffsetRow) codersdk.AuditLog { Time: dblog.Time, OrganizationID: dblog.OrganizationID, IP: ip, - UserAgent: dblog.UserAgent, + UserAgent: agent, ResourceType: codersdk.ResourceType(dblog.ResourceType), ResourceID: dblog.ResourceID, ResourceTarget: dblog.ResourceTarget, diff --git a/coderd/audit/request.go b/coderd/audit/request.go index ef15611fa8de2..55c4599cdc61a 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -2,6 +2,7 @@ package audit import ( "context" + "database/sql" "encoding/json" "fmt" "net" @@ -32,6 +33,20 @@ type Request[T Auditable] struct { New T } +type BuildAuditParams[T Auditable] struct { + Audit Auditor + Log slog.Logger + + UserID uuid.UUID + JobID uuid.UUID + Status int + Action database.AuditAction + AdditionalFields json.RawMessage + + New T + Old T +} + func ResourceTarget[T Auditable](tgt T) string { switch typed := any(tgt).(type) { case database.Organization: @@ -147,7 +162,7 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request Time: database.Now(), UserID: httpmw.APIKey(p.Request).UserID, Ip: ip, - UserAgent: p.Request.UserAgent(), + 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]), @@ -164,6 +179,40 @@ func InitRequest[T Auditable](w http.ResponseWriter, p *RequestParams) (*Request } } +// BuildAudit creates an audit log for a workspace build. +// The audit log is committed upon invocation. +func BuildAudit[T Auditable](ctx context.Context, p *BuildAuditParams[T]) { + // As the audit request has not been initiated directly by a user, we omit + // certain user details. + ip := parseIP("") + // We do not show diffs for build audit logs + var diffRaw = []byte("{}") + + if p.AdditionalFields == nil { + p.AdditionalFields = json.RawMessage("{}") + } + + err := p.Audit.Export(ctx, database.AuditLog{ + ID: uuid.New(), + Time: database.Now(), + 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]), + Action: p.Action, + Diff: diffRaw, + StatusCode: int32(p.Status), + RequestID: p.JobID, + AdditionalFields: p.AdditionalFields, + }) + if err != nil { + p.Log.Error(ctx, "export audit log", slog.Error(err)) + return + } +} + func either[T Auditable, R any](old, new T, fn func(T) R) R { if ResourceID(new) != uuid.Nil { return fn(new) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 0eca61e961c8a..feeec4bae18f5 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -143,8 +143,8 @@ CREATE TABLE audit_logs ( "time" timestamp with time zone NOT NULL, user_id uuid NOT NULL, organization_id uuid NOT NULL, - ip inet NOT NULL, - user_agent character varying(256) NOT NULL, + ip inet, + user_agent character varying(256), resource_type resource_type NOT NULL, resource_id uuid NOT NULL, resource_target text NOT NULL, diff --git a/coderd/database/migrations/000077_add_audit_log_nullable_fields.down.sql b/coderd/database/migrations/000077_add_audit_log_nullable_fields.down.sql new file mode 100644 index 0000000000000..ed62e38ab8bad --- /dev/null +++ b/coderd/database/migrations/000077_add_audit_log_nullable_fields.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE audit_logs ALTER COLUMN ip SET NOT NULL; +ALTER TABLE audit_logs ALTER COLUMN user_agent SET NOT NULL; diff --git a/coderd/database/migrations/000077_add_audit_log_nullable_fields.up.sql b/coderd/database/migrations/000077_add_audit_log_nullable_fields.up.sql new file mode 100644 index 0000000000000..21dee2a224ee4 --- /dev/null +++ b/coderd/database/migrations/000077_add_audit_log_nullable_fields.up.sql @@ -0,0 +1,2 @@ +ALTER TABLE audit_logs ALTER COLUMN ip DROP NOT NULL; +ALTER TABLE audit_logs ALTER COLUMN user_agent DROP NOT NULL; diff --git a/coderd/database/models.go b/coderd/database/models.go index 08dfdfd6d6c72..85b9538e10bdc 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -410,7 +410,7 @@ type AuditLog struct { UserID uuid.UUID `db:"user_id" json:"user_id"` OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` Ip pqtype.Inet `db:"ip" json:"ip"` - UserAgent string `db:"user_agent" json:"user_agent"` + UserAgent sql.NullString `db:"user_agent" json:"user_agent"` ResourceType ResourceType `db:"resource_type" json:"resource_type"` ResourceID uuid.UUID `db:"resource_id" json:"resource_id"` ResourceTarget string `db:"resource_target" json:"resource_target"` diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 9a298dc0053cd..f5d3d14b39554 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -535,7 +535,7 @@ type GetAuditLogsOffsetRow struct { UserID uuid.UUID `db:"user_id" json:"user_id"` OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` Ip pqtype.Inet `db:"ip" json:"ip"` - UserAgent string `db:"user_agent" json:"user_agent"` + UserAgent sql.NullString `db:"user_agent" json:"user_agent"` ResourceType ResourceType `db:"resource_type" json:"resource_type"` ResourceID uuid.UUID `db:"resource_id" json:"resource_id"` ResourceTarget string `db:"resource_target" json:"resource_target"` @@ -640,7 +640,7 @@ type InsertAuditLogParams struct { UserID uuid.UUID `db:"user_id" json:"user_id"` OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` Ip pqtype.Inet `db:"ip" json:"ip"` - UserAgent string `db:"user_agent" json:"user_agent"` + UserAgent sql.NullString `db:"user_agent" json:"user_agent"` ResourceType ResourceType `db:"resource_type" json:"resource_type"` ResourceID uuid.UUID `db:"resource_id" json:"resource_id"` ResourceTarget string `db:"resource_target" json:"resource_target"` diff --git a/coderd/provisionerdaemons.go b/coderd/provisionerdaemons.go index fb21c75956aaf..8c0f7fe6250dc 100644 --- a/coderd/provisionerdaemons.go +++ b/coderd/provisionerdaemons.go @@ -87,6 +87,7 @@ func (api *API) ListenProvisionerDaemon(ctx context.Context, acquireJobDebounce Logger: api.Logger.Named(fmt.Sprintf("provisionerd-%s", daemon.Name)), AcquireJobDebounce: acquireJobDebounce, QuotaCommitter: &api.QuotaCommitter, + Auditor: &api.Auditor, }) if err != nil { return nil, err diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 0b267de5b17fc..8dbc20f0cfd3c 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -19,6 +19,7 @@ import ( "cdr.dev/slog" + "github.com/coder/coder/coderd/audit" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/parameter" "github.com/coder/coder/coderd/telemetry" @@ -43,6 +44,7 @@ type Server struct { Pubsub database.Pubsub Telemetry telemetry.Reporter QuotaCommitter *atomic.Pointer[proto.QuotaCommitter] + Auditor *atomic.Pointer[audit.Auditor] AcquireJobDebounce time.Duration } @@ -492,6 +494,51 @@ func (server *Server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*p ProvisionerJobs: []telemetry.ProvisionerJob{telemetry.ConvertProvisionerJob(job)}, }) + // if job.Type == database.ProvisionerJobTypeWorkspaceBuild { + // auditor := server.Auditor.Load() + // build, err := server.Database.GetWorkspaceBuildByJobID(ctx, job.ID) + // var auditAction database.AuditAction + // if build.Transition == database.WorkspaceTransitionStart { + // auditAction = database.AuditActionStart + // } else if build.Transition == database.WorkspaceTransitionStop { + // auditAction = database.AuditActionStop + // } else if build.Transition == database.WorkspaceTransitionDelete { + // auditAction = database.AuditActionDelete + // } else { + // auditAction = database.AuditActionWrite + // } + // if err != nil { + // server.Logger.Error(ctx, "failed to create audit log") + // } else { + // workspace, err := server.Database.GetWorkspaceByID(ctx, build.WorkspaceID) + // if err != nil { + // server.Logger.Error(ctx, "failed to create audit log") + // } else { + // // We pass the workspace name to the Auditor so that it + // // can form a friendly string for the user. + // workspaceResourceInfo := map[string]string{ + // "workspaceName": workspace.Name, + // } + // + // wriBytes, err := json.Marshal(workspaceResourceInfo) + // if err != nil { + // server.Logger.Error(ctx, "could not marshal workspace name", slog.Error(err)) + // } + // audit.BuildAudit(ctx, &audit.BuildAuditParams[database.WorkspaceBuild]{ + // Audit: *auditor, + // Log: server.Logger, + // UserID: job.InitiatorID, + // JobID: job.ID, + // Action: auditAction, + // New: build, + // Status: 500, + // AdditionalFields: wriBytes, + // }) + // } + // + // } + // } + switch jobType := failJob.Type.(type) { case *proto.FailedJob_WorkspaceBuild_: if jobType.WorkspaceBuild.State == nil { @@ -518,6 +565,51 @@ func (server *Server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*p case *proto.FailedJob_TemplateImport_: } + if job.Type == database.ProvisionerJobTypeWorkspaceBuild { + auditor := server.Auditor.Load() + build, getBuildErr := server.Database.GetWorkspaceBuildByJobID(ctx, job.ID) + if getBuildErr != nil { + server.Logger.Error(ctx, "failed to create audit log - get build err", slog.Error(err)) + } else { + var auditAction database.AuditAction + if build.Transition == database.WorkspaceTransitionStart { + auditAction = database.AuditActionStart + } else if build.Transition == database.WorkspaceTransitionStop { + auditAction = database.AuditActionStop + } else if build.Transition == database.WorkspaceTransitionDelete { + auditAction = database.AuditActionDelete + } else { + auditAction = database.AuditActionWrite + } + workspace, getWorkspaceErr := server.Database.GetWorkspaceByID(ctx, build.WorkspaceID) + if getWorkspaceErr != nil { + server.Logger.Error(ctx, "failed to create audit log - get workspace err", slog.Error(err)) + } else { + // We pass the workspace name to the Auditor so that it + // can form a friendly string for the user. + workspaceResourceInfo := map[string]string{ + "workspaceName": workspace.Name, + } + + wriBytes, err := json.Marshal(workspaceResourceInfo) + if err != nil { + server.Logger.Error(ctx, "could not marshal workspace name", slog.Error(err)) + } + + audit.BuildAudit(ctx, &audit.BuildAuditParams[database.WorkspaceBuild]{ + Audit: *auditor, + Log: server.Logger, + UserID: job.InitiatorID, + JobID: job.ID, + Action: auditAction, + New: build, + Status: 500, + AdditionalFields: wriBytes, + }) + } + } + } + data, err := json.Marshal(ProvisionerJobLogsNotifyMessage{EndOfLogs: true}) if err != nil { return nil, xerrors.Errorf("marshal job log: %w", err) diff --git a/site/src/components/AuditLogRow/AuditLogRow.tsx b/site/src/components/AuditLogRow/AuditLogRow.tsx index c00d1564d5c95..aa51fd6f01734 100644 --- a/site/src/components/AuditLogRow/AuditLogRow.tsx +++ b/site/src/components/AuditLogRow/AuditLogRow.tsx @@ -20,7 +20,10 @@ export const readableActionMessage = (auditLog: AuditLog): string => { let target = auditLog.resource_target.trim() // audit logs with a resource_type of workspace build use workspace name as a target - if (auditLog.resource_type === "workspace_build") { + if ( + auditLog.resource_type === "workspace_build" && + auditLog.additional_fields.workspaceName + ) { target = auditLog.additional_fields.workspaceName.trim() } From ab1fec00f7e5c0af1041b076708420949d7ed5c6 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Fri, 18 Nov 2022 21:19:11 +0000 Subject: [PATCH 2/9] logging workspace build successes --- .../provisionerdserver/provisionerdserver.go | 102 ++++++++---------- 1 file changed, 45 insertions(+), 57 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 8dbc20f0cfd3c..d3d2b8fb94302 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -494,51 +494,6 @@ func (server *Server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*p ProvisionerJobs: []telemetry.ProvisionerJob{telemetry.ConvertProvisionerJob(job)}, }) - // if job.Type == database.ProvisionerJobTypeWorkspaceBuild { - // auditor := server.Auditor.Load() - // build, err := server.Database.GetWorkspaceBuildByJobID(ctx, job.ID) - // var auditAction database.AuditAction - // if build.Transition == database.WorkspaceTransitionStart { - // auditAction = database.AuditActionStart - // } else if build.Transition == database.WorkspaceTransitionStop { - // auditAction = database.AuditActionStop - // } else if build.Transition == database.WorkspaceTransitionDelete { - // auditAction = database.AuditActionDelete - // } else { - // auditAction = database.AuditActionWrite - // } - // if err != nil { - // server.Logger.Error(ctx, "failed to create audit log") - // } else { - // workspace, err := server.Database.GetWorkspaceByID(ctx, build.WorkspaceID) - // if err != nil { - // server.Logger.Error(ctx, "failed to create audit log") - // } else { - // // We pass the workspace name to the Auditor so that it - // // can form a friendly string for the user. - // workspaceResourceInfo := map[string]string{ - // "workspaceName": workspace.Name, - // } - // - // wriBytes, err := json.Marshal(workspaceResourceInfo) - // if err != nil { - // server.Logger.Error(ctx, "could not marshal workspace name", slog.Error(err)) - // } - // audit.BuildAudit(ctx, &audit.BuildAuditParams[database.WorkspaceBuild]{ - // Audit: *auditor, - // Log: server.Logger, - // UserID: job.InitiatorID, - // JobID: job.ID, - // Action: auditAction, - // New: build, - // Status: 500, - // AdditionalFields: wriBytes, - // }) - // } - // - // } - // } - switch jobType := failJob.Type.(type) { case *proto.FailedJob_WorkspaceBuild_: if jobType.WorkspaceBuild.State == nil { @@ -571,16 +526,7 @@ func (server *Server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*p if getBuildErr != nil { server.Logger.Error(ctx, "failed to create audit log - get build err", slog.Error(err)) } else { - var auditAction database.AuditAction - if build.Transition == database.WorkspaceTransitionStart { - auditAction = database.AuditActionStart - } else if build.Transition == database.WorkspaceTransitionStop { - auditAction = database.AuditActionStop - } else if build.Transition == database.WorkspaceTransitionDelete { - auditAction = database.AuditActionDelete - } else { - auditAction = database.AuditActionWrite - } + auditAction := determineAuditAction(build.Transition) workspace, getWorkspaceErr := server.Database.GetWorkspaceByID(ctx, build.WorkspaceID) if getWorkspaceErr != nil { server.Logger.Error(ctx, "failed to create audit log - get workspace err", slog.Error(err)) @@ -688,11 +634,12 @@ func (server *Server) CompleteJob(ctx context.Context, completed *proto.Complete return nil, xerrors.Errorf("get workspace build: %w", err) } + workspace, getWorkspaceError := server.Database.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID) + err = server.Database.InTx(func(db database.Store) error { now := database.Now() var workspaceDeadline time.Time - workspace, err := db.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID) - if err == nil { + if getWorkspaceError == nil { if workspace.Ttl.Valid { workspaceDeadline = now.Add(time.Duration(workspace.Ttl.Int64)) } @@ -748,6 +695,34 @@ func (server *Server) CompleteJob(ctx context.Context, completed *proto.Complete return nil, xerrors.Errorf("complete job: %w", err) } + if getWorkspaceError == nil { + auditor := server.Auditor.Load() + + auditAction := determineAuditAction(workspaceBuild.Transition) + + // We pass the workspace name to the Auditor so that it + // can form a friendly string for the user. + workspaceResourceInfo := map[string]string{ + "workspaceName": workspace.Name, + } + + wriBytes, err := json.Marshal(workspaceResourceInfo) + if err != nil { + server.Logger.Error(ctx, "could not marshal workspace name", slog.Error(err)) + } + + audit.BuildAudit(ctx, &audit.BuildAuditParams[database.WorkspaceBuild]{ + Audit: *auditor, + Log: server.Logger, + UserID: job.InitiatorID, + JobID: job.ID, + Action: auditAction, + New: workspaceBuild, + Status: 200, + AdditionalFields: wriBytes, + }) + } + err = server.Pubsub.Publish(codersdk.WorkspaceNotifyChannel(workspaceBuild.WorkspaceID), []byte{}) if err != nil { return nil, xerrors.Errorf("update workspace: %w", err) @@ -1059,6 +1034,19 @@ func convertWorkspaceTransition(transition database.WorkspaceTransition) (sdkpro } } +func determineAuditAction(transition database.WorkspaceTransition) database.AuditAction { + switch transition { + case database.WorkspaceTransitionStart: + return database.AuditActionStart + case database.WorkspaceTransitionStop: + return database.AuditActionStop + case database.WorkspaceTransitionDelete: + return database.AuditActionDelete + default: + return database.AuditActionWrite + } +} + // WorkspaceProvisionJob is the payload for the "workspace_provision" job type. type WorkspaceProvisionJob struct { WorkspaceBuildID uuid.UUID `json:"workspace_build_id"` From 9895b8849d2cc6bfce8d3f5fe125708e8e3e48f8 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Mon, 21 Nov 2022 15:07:33 +0000 Subject: [PATCH 3/9] remove duplicate workspace build entry --- .../provisionerdserver/provisionerdserver.go | 3 +- coderd/workspacebuilds.go | 54 +------------------ 2 files changed, 3 insertions(+), 54 deletions(-) diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index d3d2b8fb94302..ecdc1ab3f902c 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -520,6 +520,7 @@ func (server *Server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*p case *proto.FailedJob_TemplateImport_: } + // if failed job is a workspace build, audit the outcome if job.Type == database.ProvisionerJobTypeWorkspaceBuild { auditor := server.Auditor.Load() build, getBuildErr := server.Database.GetWorkspaceBuildByJobID(ctx, job.ID) @@ -695,9 +696,9 @@ func (server *Server) CompleteJob(ctx context.Context, completed *proto.Complete return nil, xerrors.Errorf("complete job: %w", err) } + // audit the outcome of the workspace build if getWorkspaceError == nil { auditor := server.Auditor.Load() - auditAction := determineAuditAction(workspaceBuild.Transition) // We pass the workspace name to the Auditor so that it diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 89dd05dec1a0e..7bce22cc84a6f 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -15,8 +15,6 @@ import ( "golang.org/x/exp/slices" "golang.org/x/xerrors" - "cdr.dev/slog" - "github.com/coder/coder/coderd/audit" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" @@ -280,58 +278,8 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { return } - auditor := api.Auditor.Load() - - // if user deletes a workspace, audit the workspace - if action == rbac.ActionDelete { - aReq, commitAudit := audit.InitRequest[database.Workspace](rw, &audit.RequestParams{ - Audit: *auditor, - Log: api.Logger, - Request: r, - Action: database.AuditActionDelete, - }) - - defer commitAudit() - aReq.Old = workspace - } - - latestBuild, latestBuildErr := api.Database.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspace.ID) - - // if a user starts/stops a workspace, audit the workspace build - if action == rbac.ActionUpdate { - var auditAction database.AuditAction - if createBuild.Transition == codersdk.WorkspaceTransitionStart { - auditAction = database.AuditActionStart - } else if createBuild.Transition == codersdk.WorkspaceTransitionStop { - auditAction = database.AuditActionStop - } else { - auditAction = database.AuditActionWrite - } - - // We pass the workspace name to the Auditor so that it - // can form a friendly string for the user. - workspaceResourceInfo := map[string]string{ - "workspaceName": workspace.Name, - } - - wriBytes, err := json.Marshal(workspaceResourceInfo) - if err != nil { - api.Logger.Error(ctx, "could not marshal workspace name", slog.Error(err)) - } - - aReq, commitAudit := audit.InitRequest[database.WorkspaceBuild](rw, &audit.RequestParams{ - Audit: *auditor, - Log: api.Logger, - Request: r, - Action: auditAction, - AdditionalFields: wriBytes, - }) - - defer commitAudit() - aReq.Old = latestBuild - } - if createBuild.TemplateVersionID == uuid.Nil { + latestBuild, latestBuildErr := api.Database.GetLatestWorkspaceBuildByWorkspaceID(ctx, workspace.ID) if latestBuildErr != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching the latest workspace build.", From d5aed0e5d749522e7cb9e8d0a131cabcdd752130 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Mon, 21 Nov 2022 17:06:13 +0000 Subject: [PATCH 4/9] fixed workspacebuilds_test --- coderd/workspacebuilds_test.go | 8 +++++--- coderd/workspaces_test.go | 4 ++-- enterprise/audit/table.go | 5 ++--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 6d3721dccc1fd..f82f24b2fb5f4 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -560,6 +560,8 @@ func TestWorkspaceBuildStatus(t *testing.T) { require.NoError(t, err) require.EqualValues(t, codersdk.WorkspaceStatusRunning, workspace.LatestBuild.Status) + numLogs++ // add an audit log for workspace_build starting + // after successful stop is "stopped" build := coderdtest.CreateWorkspaceBuild(t, client, workspace, database.WorkspaceTransitionStop) _ = coderdtest.AwaitWorkspaceBuildJob(t, client, build.ID) @@ -578,14 +580,14 @@ func TestWorkspaceBuildStatus(t *testing.T) { err = client.CancelWorkspaceBuild(ctx, build.ID) require.NoError(t, err) - numLogs++ // add an audit log for workspace build start // assert an audit log has been created workspace starting + require.Equal(t, database.AuditActionStart, auditor.AuditLogs[numLogs-2].Action) require.Len(t, auditor.AuditLogs, numLogs) - require.Equal(t, database.AuditActionStart, auditor.AuditLogs[numLogs-1].Action) workspace, err = client.Workspace(ctx, workspace.ID) require.NoError(t, err) require.EqualValues(t, codersdk.WorkspaceStatusCanceled, workspace.LatestBuild.Status) + numLogs++ // add an audit log for workspace_build cancel _ = coderdtest.NewProvisionerDaemon(t, api) // after successful delete is "deleted" @@ -594,7 +596,7 @@ func TestWorkspaceBuildStatus(t *testing.T) { workspace, err = client.DeletedWorkspace(ctx, workspace.ID) require.NoError(t, err) require.EqualValues(t, codersdk.WorkspaceStatusDeleted, workspace.LatestBuild.Status) - numLogs++ // add an audit log for workspace build deletion + numLogs++ // add an audit log for workspace_build deletion // assert an audit log has been created for deletion require.Len(t, auditor.AuditLogs, numLogs) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 73a7ce19dd013..82ddce16bc8e5 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -1205,8 +1205,8 @@ func TestWorkspaceUpdateTTL(t *testing.T) { require.Equal(t, testCase.ttlMillis, updated.TTLMillis, "expected autostop ttl to equal requested") - require.Len(t, auditor.AuditLogs, 5) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[4].Action) + require.Len(t, auditor.AuditLogs, 6) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[5].Action) }) } diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 68f51813627c1..598ce3596f030 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -111,14 +111,13 @@ var AuditableResources = auditMap(map[any]map[string]Action{ "avatar_url": ActionTrack, "quota_allowance": ActionTrack, }, - // We don't show any diff for the WorkspaceBuild resource, - // save for the template_version_id + // We don't show any diff for the WorkspaceBuild resource &database.WorkspaceBuild{}: { "id": ActionIgnore, "created_at": ActionIgnore, "updated_at": ActionIgnore, "workspace_id": ActionIgnore, - "template_version_id": ActionTrack, + "template_version_id": ActionIgnore, "build_number": ActionIgnore, "transition": ActionIgnore, "initiator_id": ActionIgnore, From 3b635acbbf781c275a8bb0ab3087a7e43f770c1a Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Mon, 21 Nov 2022 21:13:57 +0000 Subject: [PATCH 5/9] PR feedback --- coderd/audit.go | 6 +----- coderd/provisionerdserver/provisionerdserver.go | 17 ++++++++++------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/coderd/audit.go b/coderd/audit.go index e2ac3b0f659e1..e25f5e78edc59 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -190,11 +190,7 @@ func convertAuditLog(dblog database.GetAuditLogsOffsetRow) codersdk.AuditLog { _ = json.Unmarshal(dblog.Diff, &diff) var user *codersdk.User - var agent string - if dblog.UserAgent.Valid { - agent = dblog.UserAgent.String - } if dblog.UserUsername.Valid { user = &codersdk.User{ ID: dblog.UserID, @@ -218,7 +214,7 @@ func convertAuditLog(dblog database.GetAuditLogsOffsetRow) codersdk.AuditLog { Time: dblog.Time, OrganizationID: dblog.OrganizationID, IP: ip, - UserAgent: agent, + UserAgent: dblog.UserAgent.String, ResourceType: codersdk.ResourceType(dblog.ResourceType), ResourceID: dblog.ResourceID, ResourceTarget: dblog.ResourceTarget, diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index ecdc1ab3f902c..af93b2b2e4a23 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "net/http" "net/url" "reflect" "sync" @@ -527,7 +528,7 @@ func (server *Server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*p if getBuildErr != nil { server.Logger.Error(ctx, "failed to create audit log - get build err", slog.Error(err)) } else { - auditAction := determineAuditAction(build.Transition) + auditAction := auditActionFromTransition(build.Transition) workspace, getWorkspaceErr := server.Database.GetWorkspaceByID(ctx, build.WorkspaceID) if getWorkspaceErr != nil { server.Logger.Error(ctx, "failed to create audit log - get workspace err", slog.Error(err)) @@ -550,7 +551,7 @@ func (server *Server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*p JobID: job.ID, Action: auditAction, New: build, - Status: 500, + Status: http.StatusInternalServerError, AdditionalFields: wriBytes, }) } @@ -635,11 +636,13 @@ func (server *Server) CompleteJob(ctx context.Context, completed *proto.Complete return nil, xerrors.Errorf("get workspace build: %w", err) } - workspace, getWorkspaceError := server.Database.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID) + var workspace database.Workspace + var getWorkspaceError error err = server.Database.InTx(func(db database.Store) error { now := database.Now() var workspaceDeadline time.Time + workspace, getWorkspaceError = db.GetWorkspaceByID(ctx, workspaceBuild.WorkspaceID) if getWorkspaceError == nil { if workspace.Ttl.Valid { workspaceDeadline = now.Add(time.Duration(workspace.Ttl.Int64)) @@ -699,7 +702,7 @@ func (server *Server) CompleteJob(ctx context.Context, completed *proto.Complete // audit the outcome of the workspace build if getWorkspaceError == nil { auditor := server.Auditor.Load() - auditAction := determineAuditAction(workspaceBuild.Transition) + auditAction := auditActionFromTransition(workspaceBuild.Transition) // We pass the workspace name to the Auditor so that it // can form a friendly string for the user. @@ -709,7 +712,7 @@ func (server *Server) CompleteJob(ctx context.Context, completed *proto.Complete wriBytes, err := json.Marshal(workspaceResourceInfo) if err != nil { - server.Logger.Error(ctx, "could not marshal workspace name", slog.Error(err)) + server.Logger.Error(ctx, "marshal resource info", slog.Error(err)) } audit.BuildAudit(ctx, &audit.BuildAuditParams[database.WorkspaceBuild]{ @@ -719,7 +722,7 @@ func (server *Server) CompleteJob(ctx context.Context, completed *proto.Complete JobID: job.ID, Action: auditAction, New: workspaceBuild, - Status: 200, + Status: http.StatusOK, AdditionalFields: wriBytes, }) } @@ -1035,7 +1038,7 @@ func convertWorkspaceTransition(transition database.WorkspaceTransition) (sdkpro } } -func determineAuditAction(transition database.WorkspaceTransition) database.AuditAction { +func auditActionFromTransition(transition database.WorkspaceTransition) database.AuditAction { switch transition { case database.WorkspaceTransitionStart: return database.AuditActionStart From b8ab4791d185bd1bd7433b837892610b9ae58f9e Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Mon, 21 Nov 2022 21:45:56 +0000 Subject: [PATCH 6/9] lint and migrations --- ....down.sql => 000082_add_audit_log_nullable_fields.down.sql} | 0 ...elds.up.sql => 000082_add_audit_log_nullable_fields.up.sql} | 0 enterprise/audit/audittest/rand.go | 3 ++- enterprise/trialer/trialer.go | 3 ++- 4 files changed, 4 insertions(+), 2 deletions(-) rename coderd/database/migrations/{000077_add_audit_log_nullable_fields.down.sql => 000082_add_audit_log_nullable_fields.down.sql} (100%) rename coderd/database/migrations/{000077_add_audit_log_nullable_fields.up.sql => 000082_add_audit_log_nullable_fields.up.sql} (100%) diff --git a/coderd/database/migrations/000077_add_audit_log_nullable_fields.down.sql b/coderd/database/migrations/000082_add_audit_log_nullable_fields.down.sql similarity index 100% rename from coderd/database/migrations/000077_add_audit_log_nullable_fields.down.sql rename to coderd/database/migrations/000082_add_audit_log_nullable_fields.down.sql diff --git a/coderd/database/migrations/000077_add_audit_log_nullable_fields.up.sql b/coderd/database/migrations/000082_add_audit_log_nullable_fields.up.sql similarity index 100% rename from coderd/database/migrations/000077_add_audit_log_nullable_fields.up.sql rename to coderd/database/migrations/000082_add_audit_log_nullable_fields.up.sql diff --git a/enterprise/audit/audittest/rand.go b/enterprise/audit/audittest/rand.go index ea44c8f8e64dd..5bf443142ae19 100644 --- a/enterprise/audit/audittest/rand.go +++ b/enterprise/audit/audittest/rand.go @@ -1,6 +1,7 @@ package audittest import ( + "database/sql" "net" "net/http" "time" @@ -22,7 +23,7 @@ func RandomLog() database.AuditLog { IPNet: *inet, Valid: true, }, - UserAgent: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/100.0.4896.127 Safari/537.36", + UserAgent: sql.NullString{String: "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/100.0.4896.127 Safari/537.36", Valid: true}, ResourceType: database.ResourceTypeOrganization, ResourceID: uuid.New(), ResourceTarget: "colin's organization", diff --git a/enterprise/trialer/trialer.go b/enterprise/trialer/trialer.go index 1ee49343d823b..48b67a9b9576c 100644 --- a/enterprise/trialer/trialer.go +++ b/enterprise/trialer/trialer.go @@ -11,9 +11,10 @@ import ( "golang.org/x/xerrors" + "github.com/google/uuid" + "github.com/coder/coder/coderd/database" "github.com/coder/coder/enterprise/coderd/license" - "github.com/google/uuid" ) type request struct { From 8a8a3eb24b7254fcc9c6a98c5f66bd6805f22bf8 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Tue, 22 Nov 2022 10:54:36 -0600 Subject: [PATCH 7/9] fix nil auditors --- coderd/coderd.go | 1 + coderd/provisionerdserver/provisionerdserver_test.go | 11 +++++++++++ enterprise/coderd/provisionerdaemons.go | 1 + 3 files changed, 13 insertions(+) diff --git a/coderd/coderd.go b/coderd/coderd.go index 6cc154757364f..c9954a9f3412c 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -681,6 +681,7 @@ func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context, debounce ti Telemetry: api.Telemetry, Tags: tags, QuotaCommitter: &api.QuotaCommitter, + Auditor: &api.Auditor, AcquireJobDebounce: debounce, Logger: api.Logger.Named(fmt.Sprintf("provisionerd-%s", daemon.Name)), }) diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index c63775a4e1839..cd1c65de16ace 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -5,6 +5,7 @@ import ( "database/sql" "encoding/json" "net/url" + "sync/atomic" "testing" "time" @@ -12,6 +13,7 @@ import ( "github.com/stretchr/testify/require" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/coderd/audit" "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/database/databasefake" "github.com/coder/coder/coderd/provisionerdserver" @@ -21,6 +23,13 @@ import ( sdkproto "github.com/coder/coder/provisionersdk/proto" ) +func mockAuditor() *atomic.Pointer[audit.Auditor] { + ptr := &atomic.Pointer[audit.Auditor]{} + mock := audit.Auditor(audit.NewMock()) + ptr.Store(&mock) + return ptr +} + func TestAcquireJob(t *testing.T) { t.Parallel() t.Run("Debounce", func(t *testing.T) { @@ -36,6 +45,7 @@ func TestAcquireJob(t *testing.T) { Pubsub: pubsub, Telemetry: telemetry.NewNoop(), AcquireJobDebounce: time.Hour, + Auditor: mockAuditor(), } job, err := srv.AcquireJob(context.Background(), nil) require.NoError(t, err) @@ -799,5 +809,6 @@ func setup(t *testing.T) *provisionerdserver.Server { Database: db, Pubsub: pubsub, Telemetry: telemetry.NewNoop(), + Auditor: mockAuditor(), } } diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index e9f357f970e30..95aab294d5524 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -206,6 +206,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) Pubsub: api.Pubsub, Provisioners: daemon.Provisioners, Telemetry: api.Telemetry, + Auditor: &api.AGPL.Auditor, Logger: api.Logger.Named(fmt.Sprintf("provisionerd-%s", daemon.Name)), Tags: rawTags, }) From d5c8045e67b7a2c0308b0cfa8f84fcc4da761720 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Tue, 22 Nov 2022 17:46:50 +0000 Subject: [PATCH 8/9] workspace_build test --- coderd/workspacebuilds_test.go | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 15592fef4758e..77183886ef2c2 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -576,9 +576,9 @@ func TestWorkspaceBuildStatus(t *testing.T) { numLogs := len(auditor.AuditLogs) client, closeDaemon, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true, Auditor: auditor}) user := coderdtest.CreateFirstUser(t, client) - numLogs++ // add an audit log for user version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - numLogs++ // add an audit log for template version + numLogs++ // add an audit log for template version creation + numLogs++ // add an audit log for template version update coderdtest.AwaitTemplateVersionJob(t, client, version.ID) closeDaemon.Close() @@ -618,14 +618,9 @@ func TestWorkspaceBuildStatus(t *testing.T) { err = client.CancelWorkspaceBuild(ctx, build.ID) require.NoError(t, err) - // assert an audit log has been created workspace starting - require.Equal(t, database.AuditActionStart, auditor.AuditLogs[numLogs-2].Action) - require.Len(t, auditor.AuditLogs, numLogs) - workspace, err = client.Workspace(ctx, workspace.ID) require.NoError(t, err) require.EqualValues(t, codersdk.WorkspaceStatusCanceled, workspace.LatestBuild.Status) - numLogs++ // add an audit log for workspace_build cancel _ = coderdtest.NewProvisionerDaemon(t, api) // after successful delete is "deleted" @@ -634,9 +629,4 @@ func TestWorkspaceBuildStatus(t *testing.T) { workspace, err = client.DeletedWorkspace(ctx, workspace.ID) require.NoError(t, err) require.EqualValues(t, codersdk.WorkspaceStatusDeleted, workspace.LatestBuild.Status) - numLogs++ // add an audit log for workspace_build deletion - - // assert an audit log has been created for deletion - require.Len(t, auditor.AuditLogs, numLogs) - require.Equal(t, database.AuditActionDelete, auditor.AuditLogs[numLogs-1].Action) } From 1b276e4282071f3c0be7ca8d2122eed42eadf0b0 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Tue, 22 Nov 2022 18:10:24 +0000 Subject: [PATCH 9/9] fixed workspaces_teest --- coderd/workspaces_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 8a898954bccff..89a126d0233ec 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -1101,6 +1101,9 @@ func TestWorkspaceUpdateAutostart(t *testing.T) { }) ) + // await job to ensure audit logs for workspace_build start are created + _ = coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) + // ensure test invariant: new workspaces have no autostart schedule. require.Empty(t, workspace.AutostartSchedule, "expected newly-minted workspace to have no autostart schedule") @@ -1136,8 +1139,8 @@ func TestWorkspaceUpdateAutostart(t *testing.T) { interval := next.Sub(testCase.at) require.Equal(t, testCase.expectedInterval, interval, "unexpected interval") - require.Len(t, auditor.AuditLogs, 5) - assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[4].Action) + require.Len(t, auditor.AuditLogs, 6) + assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[5].Action) }) }