diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 6ec51ff20f..65f526f656 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -19766,7 +19766,8 @@ const docTemplate = `{ "workspace_app", "task", "ai_seat", - "chat" + "chat", + "user_secret" ], "x-enum-varnames": [ "ResourceTypeTemplate", @@ -19796,7 +19797,8 @@ const docTemplate = `{ "ResourceTypeWorkspaceApp", "ResourceTypeTask", "ResourceTypeAISeat", - "ResourceTypeChat" + "ResourceTypeChat", + "ResourceTypeUserSecret" ] }, "codersdk.Response": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 348b8bf2cd..17d5c02c18 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -18098,7 +18098,8 @@ "workspace_app", "task", "ai_seat", - "chat" + "chat", + "user_secret" ], "x-enum-varnames": [ "ResourceTypeTemplate", @@ -18128,7 +18129,8 @@ "ResourceTypeWorkspaceApp", "ResourceTypeTask", "ResourceTypeAISeat", - "ResourceTypeChat" + "ResourceTypeChat", + "ResourceTypeUserSecret" ] }, "codersdk.Response": { diff --git a/coderd/audit.go b/coderd/audit.go index b63c0c0eff..d54874669f 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -445,6 +445,18 @@ func (api *API) auditLogIsResourceDeleted(ctx context.Context, alog database.Get api.Logger.Error(ctx, "unable to fetch chat", slog.Error(err)) } return false + case database.ResourceTypeUserSecret: + _, err := api.Database.GetUserSecretByID(ctx, alog.AuditLog.ResourceID) + if xerrors.Is(err, sql.ErrNoRows) { + return true + } + // Only users have user_secret:read on their own secrets. If dbauthz returns + // ErrUnauthorized, it's not an error worth logging because we have enough + // information to know it's not deleted. + if err != nil && !dbauthz.IsNotAuthorizedError(err) { + api.Logger.Error(ctx, "unable to fetch user secret", slog.Error(err)) + } + return false default: return false } @@ -536,6 +548,10 @@ func (api *API) auditLogResourceLink(ctx context.Context, alog database.GetAudit // Chats are surfaced at /agents/{id}. They are owner-scoped but // not username-scoped in the URL like workspaces or tasks. return fmt.Sprintf("/agents/%s", alog.AuditLog.ResourceID) + case database.ResourceTypeUserSecret: + // TODO(PLAT-102): point at the user secrets management page once + // it ships. Until then, the audit row links nowhere. + return "" default: return "" } diff --git a/coderd/audit/diff.go b/coderd/audit/diff.go index a2c609d91d..a95301ad78 100644 --- a/coderd/audit/diff.go +++ b/coderd/audit/diff.go @@ -34,7 +34,8 @@ type Auditable interface { idpsync.RoleSyncSettings | database.TaskTable | database.AiSeatState | - database.Chat + database.Chat | + database.UserSecret } // Map is a map of changed fields in an audited resource. It maps field names to diff --git a/coderd/audit/request.go b/coderd/audit/request.go index b9bf307ab4..178153660f 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -142,6 +142,8 @@ func ResourceTarget[T Auditable](tgt T) string { // for display; collisions affect the display label and search // filter but not the primary resource identifier. return typed.ID.String()[:8] + case database.UserSecret: + return typed.Name default: panic(fmt.Sprintf("unknown resource %T for ResourceTarget", tgt)) } @@ -210,6 +212,8 @@ func ResourceID[T Auditable](tgt T) uuid.UUID { return typed.UserID case database.Chat: return typed.ID + case database.UserSecret: + return typed.ID default: panic(fmt.Sprintf("unknown resource %T for ResourceID", tgt)) } @@ -269,6 +273,8 @@ func ResourceType[T Auditable](tgt T) database.ResourceType { return database.ResourceTypeAiSeat case database.Chat: return database.ResourceTypeChat + case database.UserSecret: + return database.ResourceTypeUserSecret default: panic(fmt.Sprintf("unknown resource %T for ResourceType", typed)) } @@ -333,6 +339,9 @@ func ResourceRequiresOrgID[T Auditable]() bool { // Chats always have a non-null organization_id (since // migration 000467). return true + case database.UserSecret: + // User secrets are global to the user across organizations. + return false default: panic(fmt.Sprintf("unknown resource %T for ResourceRequiresOrgID", tgt)) } diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index c57e60322e..1412054c5f 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -2223,10 +2223,10 @@ func (q *querier) DeleteUserChatProviderKey(ctx context.Context, arg database.De return q.db.DeleteUserChatProviderKey(ctx, arg) } -func (q *querier) DeleteUserSecretByUserIDAndName(ctx context.Context, arg database.DeleteUserSecretByUserIDAndNameParams) (int64, error) { +func (q *querier) DeleteUserSecretByUserIDAndName(ctx context.Context, arg database.DeleteUserSecretByUserIDAndNameParams) (database.UserSecret, error) { obj := rbac.ResourceUserSecret.WithOwner(arg.UserID.String()) if err := q.authorizeContext(ctx, policy.ActionDelete, obj); err != nil { - return 0, err + return database.UserSecret{}, err } return q.db.DeleteUserSecretByUserIDAndName(ctx, arg) } @@ -4378,6 +4378,10 @@ func (q *querier) GetUserNotificationPreferences(ctx context.Context, userID uui return q.db.GetUserNotificationPreferences(ctx, userID) } +func (q *querier) GetUserSecretByID(ctx context.Context, id uuid.UUID) (database.UserSecret, error) { + return fetch(q.log, q.auth, q.db.GetUserSecretByID)(ctx, id) +} + func (q *querier) GetUserSecretByUserIDAndName(ctx context.Context, arg database.GetUserSecretByUserIDAndNameParams) (database.UserSecret, error) { obj := rbac.ResourceUserSecret.WithOwner(arg.UserID.String()) if err := q.authorizeContext(ctx, policy.ActionRead, obj); err != nil { diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index bf4bf7614c..1ccb0bd4df 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -5690,11 +5690,20 @@ func (s *MethodTestSuite) TestUserSecrets() { })) s.Run("DeleteUserSecretByUserIDAndName", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { user := testutil.Fake(s.T(), faker, database.User{}) + deleted := testutil.Fake(s.T(), faker, database.UserSecret{UserID: user.ID, Name: "test"}) arg := database.DeleteUserSecretByUserIDAndNameParams{UserID: user.ID, Name: "test"} - dbm.EXPECT().DeleteUserSecretByUserIDAndName(gomock.Any(), arg).Return(int64(1), nil).AnyTimes() + dbm.EXPECT().DeleteUserSecretByUserIDAndName(gomock.Any(), arg).Return(deleted, nil).AnyTimes() check.Args(arg). Asserts(rbac.ResourceUserSecret.WithOwner(user.ID.String()), policy.ActionDelete). - Returns(int64(1)) + Returns(deleted) + })) + s.Run("GetUserSecretByID", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { + user := testutil.Fake(s.T(), faker, database.User{}) + secret := testutil.Fake(s.T(), faker, database.UserSecret{UserID: user.ID}) + dbm.EXPECT().GetUserSecretByID(gomock.Any(), secret.ID).Return(secret, nil).AnyTimes() + check.Args(secret.ID). + Asserts(secret, policy.ActionRead). + Returns(secret) })) } diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index 9739359aac..5ca4e1d6cb 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -768,7 +768,7 @@ func (m queryMetricsStore) DeleteUserChatProviderKey(ctx context.Context, arg da return r0 } -func (m queryMetricsStore) DeleteUserSecretByUserIDAndName(ctx context.Context, arg database.DeleteUserSecretByUserIDAndNameParams) (int64, error) { +func (m queryMetricsStore) DeleteUserSecretByUserIDAndName(ctx context.Context, arg database.DeleteUserSecretByUserIDAndNameParams) (database.UserSecret, error) { start := time.Now() r0, r1 := m.s.DeleteUserSecretByUserIDAndName(ctx, arg) m.queryLatencies.WithLabelValues("DeleteUserSecretByUserIDAndName").Observe(time.Since(start).Seconds()) @@ -2848,6 +2848,14 @@ func (m queryMetricsStore) GetUserNotificationPreferences(ctx context.Context, u return r0, r1 } +func (m queryMetricsStore) GetUserSecretByID(ctx context.Context, id uuid.UUID) (database.UserSecret, error) { + start := time.Now() + r0, r1 := m.s.GetUserSecretByID(ctx, id) + m.queryLatencies.WithLabelValues("GetUserSecretByID").Observe(time.Since(start).Seconds()) + m.queryCounts.WithLabelValues(httpmw.ExtractHTTPRoute(ctx), httpmw.ExtractHTTPMethod(ctx), "GetUserSecretByID").Inc() + return r0, r1 +} + func (m queryMetricsStore) GetUserSecretByUserIDAndName(ctx context.Context, arg database.GetUserSecretByUserIDAndNameParams) (database.UserSecret, error) { start := time.Now() r0, r1 := m.s.GetUserSecretByUserIDAndName(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 34369f4288..a9dacf2a93 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1303,10 +1303,10 @@ func (mr *MockStoreMockRecorder) DeleteUserChatProviderKey(ctx, arg any) *gomock } // DeleteUserSecretByUserIDAndName mocks base method. -func (m *MockStore) DeleteUserSecretByUserIDAndName(ctx context.Context, arg database.DeleteUserSecretByUserIDAndNameParams) (int64, error) { +func (m *MockStore) DeleteUserSecretByUserIDAndName(ctx context.Context, arg database.DeleteUserSecretByUserIDAndNameParams) (database.UserSecret, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "DeleteUserSecretByUserIDAndName", ctx, arg) - ret0, _ := ret[0].(int64) + ret0, _ := ret[0].(database.UserSecret) ret1, _ := ret[1].(error) return ret0, ret1 } @@ -5326,6 +5326,21 @@ func (mr *MockStoreMockRecorder) GetUserNotificationPreferences(ctx, userID any) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserNotificationPreferences", reflect.TypeOf((*MockStore)(nil).GetUserNotificationPreferences), ctx, userID) } +// GetUserSecretByID mocks base method. +func (m *MockStore) GetUserSecretByID(ctx context.Context, id uuid.UUID) (database.UserSecret, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetUserSecretByID", ctx, id) + ret0, _ := ret[0].(database.UserSecret) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetUserSecretByID indicates an expected call of GetUserSecretByID. +func (mr *MockStoreMockRecorder) GetUserSecretByID(ctx, id any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserSecretByID", reflect.TypeOf((*MockStore)(nil).GetUserSecretByID), ctx, id) +} + // GetUserSecretByUserIDAndName mocks base method. func (m *MockStore) GetUserSecretByUserIDAndName(ctx context.Context, arg database.GetUserSecretByUserIDAndNameParams) (database.UserSecret, error) { m.ctrl.T.Helper() diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 7d0f8db486..e168cee8fc 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -526,7 +526,8 @@ CREATE TYPE resource_type AS ENUM ( 'prebuilds_settings', 'task', 'ai_seat', - 'chat' + 'chat', + 'user_secret' ); CREATE TYPE shareable_workspace_owners AS ENUM ( diff --git a/coderd/database/migrations/000481_user_secret_audit.down.sql b/coderd/database/migrations/000481_user_secret_audit.down.sql new file mode 100644 index 0000000000..5bfcd5e0f1 --- /dev/null +++ b/coderd/database/migrations/000481_user_secret_audit.down.sql @@ -0,0 +1 @@ +-- no-op because resource_type enum values cannot be removed safely. diff --git a/coderd/database/migrations/000481_user_secret_audit.up.sql b/coderd/database/migrations/000481_user_secret_audit.up.sql new file mode 100644 index 0000000000..2b94841460 --- /dev/null +++ b/coderd/database/migrations/000481_user_secret_audit.up.sql @@ -0,0 +1 @@ +ALTER TYPE resource_type ADD VALUE IF NOT EXISTS 'user_secret'; diff --git a/coderd/database/models.go b/coderd/database/models.go index e406ee9e1c..750f52bec8 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -3206,6 +3206,7 @@ const ( ResourceTypeTask ResourceType = "task" ResourceTypeAiSeat ResourceType = "ai_seat" ResourceTypeChat ResourceType = "chat" + ResourceTypeUserSecret ResourceType = "user_secret" ) func (e *ResourceType) Scan(src interface{}) error { @@ -3272,7 +3273,8 @@ func (e ResourceType) Valid() bool { ResourceTypePrebuildsSettings, ResourceTypeTask, ResourceTypeAiSeat, - ResourceTypeChat: + ResourceTypeChat, + ResourceTypeUserSecret: return true } return false @@ -3308,6 +3310,7 @@ func AllResourceTypeValues() []ResourceType { ResourceTypeTask, ResourceTypeAiSeat, ResourceTypeChat, + ResourceTypeUserSecret, } } diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 49ac2243be..5fb8b8baa7 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -186,7 +186,7 @@ type sqlcQuerier interface { DeleteTask(ctx context.Context, arg DeleteTaskParams) (uuid.UUID, error) DeleteUserChatCompactionThreshold(ctx context.Context, arg DeleteUserChatCompactionThresholdParams) error DeleteUserChatProviderKey(ctx context.Context, arg DeleteUserChatProviderKeyParams) error - DeleteUserSecretByUserIDAndName(ctx context.Context, arg DeleteUserSecretByUserIDAndNameParams) (int64, error) + DeleteUserSecretByUserIDAndName(ctx context.Context, arg DeleteUserSecretByUserIDAndNameParams) (UserSecret, error) DeleteWebpushSubscriptionByUserIDAndEndpoint(ctx context.Context, arg DeleteWebpushSubscriptionByUserIDAndEndpointParams) error DeleteWebpushSubscriptions(ctx context.Context, ids []uuid.UUID) error DeleteWorkspaceACLByID(ctx context.Context, id uuid.UUID) error @@ -704,6 +704,7 @@ type sqlcQuerier interface { GetUserLinkByUserIDLoginType(ctx context.Context, arg GetUserLinkByUserIDLoginTypeParams) (UserLink, error) GetUserLinksByUserID(ctx context.Context, userID uuid.UUID) ([]UserLink, error) GetUserNotificationPreferences(ctx context.Context, userID uuid.UUID) ([]NotificationPreference, error) + GetUserSecretByID(ctx context.Context, id uuid.UUID) (UserSecret, error) GetUserSecretByUserIDAndName(ctx context.Context, arg GetUserSecretByUserIDAndNameParams) (UserSecret, error) // GetUserStatusCounts returns the count of users in each status over time. // The time range is inclusively defined by the start_time and end_time parameters. diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 3770e140bd..27e5474f03 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -24654,9 +24654,10 @@ func (q *sqlQuerier) CreateUserSecret(ctx context.Context, arg CreateUserSecretP return i, err } -const deleteUserSecretByUserIDAndName = `-- name: DeleteUserSecretByUserIDAndName :execrows +const deleteUserSecretByUserIDAndName = `-- name: DeleteUserSecretByUserIDAndName :one DELETE FROM user_secrets WHERE user_id = $1 AND name = $2 +RETURNING id, user_id, name, description, value, env_name, file_path, created_at, updated_at, value_key_id ` type DeleteUserSecretByUserIDAndNameParams struct { @@ -24664,12 +24665,46 @@ type DeleteUserSecretByUserIDAndNameParams struct { Name string `db:"name" json:"name"` } -func (q *sqlQuerier) DeleteUserSecretByUserIDAndName(ctx context.Context, arg DeleteUserSecretByUserIDAndNameParams) (int64, error) { - result, err := q.db.ExecContext(ctx, deleteUserSecretByUserIDAndName, arg.UserID, arg.Name) - if err != nil { - return 0, err - } - return result.RowsAffected() +func (q *sqlQuerier) DeleteUserSecretByUserIDAndName(ctx context.Context, arg DeleteUserSecretByUserIDAndNameParams) (UserSecret, error) { + row := q.db.QueryRowContext(ctx, deleteUserSecretByUserIDAndName, arg.UserID, arg.Name) + var i UserSecret + err := row.Scan( + &i.ID, + &i.UserID, + &i.Name, + &i.Description, + &i.Value, + &i.EnvName, + &i.FilePath, + &i.CreatedAt, + &i.UpdatedAt, + &i.ValueKeyID, + ) + return i, err +} + +const getUserSecretByID = `-- name: GetUserSecretByID :one +SELECT id, user_id, name, description, value, env_name, file_path, created_at, updated_at, value_key_id +FROM user_secrets +WHERE id = $1 +` + +func (q *sqlQuerier) GetUserSecretByID(ctx context.Context, id uuid.UUID) (UserSecret, error) { + row := q.db.QueryRowContext(ctx, getUserSecretByID, id) + var i UserSecret + err := row.Scan( + &i.ID, + &i.UserID, + &i.Name, + &i.Description, + &i.Value, + &i.EnvName, + &i.FilePath, + &i.CreatedAt, + &i.UpdatedAt, + &i.ValueKeyID, + ) + return i, err } const getUserSecretByUserIDAndName = `-- name: GetUserSecretByUserIDAndName :one diff --git a/coderd/database/queries/user_secrets.sql b/coderd/database/queries/user_secrets.sql index cf55c72e62..ee37837c14 100644 --- a/coderd/database/queries/user_secrets.sql +++ b/coderd/database/queries/user_secrets.sql @@ -3,6 +3,11 @@ SELECT * FROM user_secrets WHERE user_id = @user_id AND name = @name; +-- name: GetUserSecretByID :one +SELECT * +FROM user_secrets +WHERE id = @id; + -- name: ListUserSecrets :many -- Returns metadata only (no value or value_key_id) for the -- REST API list and get endpoints. @@ -56,6 +61,7 @@ SET WHERE user_id = @user_id AND name = @name RETURNING *; --- name: DeleteUserSecretByUserIDAndName :execrows +-- name: DeleteUserSecretByUserIDAndName :one DELETE FROM user_secrets -WHERE user_id = @user_id AND name = @name; +WHERE user_id = @user_id AND name = @name +RETURNING *; diff --git a/coderd/usersecrets.go b/coderd/usersecrets.go index e613d03044..57c0fbcf24 100644 --- a/coderd/usersecrets.go +++ b/coderd/usersecrets.go @@ -7,7 +7,9 @@ import ( "github.com/go-chi/chi/v5" "github.com/google/uuid" + "golang.org/x/xerrors" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/httpapi" @@ -26,8 +28,18 @@ import ( // @Success 201 {object} codersdk.UserSecret // @Router /users/{user}/secrets [post] func (api *API) postUserSecret(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.UserSecret](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionCreate, + }) + ) + defer commitAudit() var req codersdk.CreateUserSecretRequest if !httpapi.Read(ctx, rw, r, &req) { @@ -92,6 +104,7 @@ func (api *API) postUserSecret(rw http.ResponseWriter, r *http.Request) { }) return } + aReq.New = secret httpapi.Write(ctx, rw, http.StatusCreated, db2sdk.UserSecretFromFull(secret)) } @@ -165,9 +178,19 @@ func (api *API) getUserSecret(rw http.ResponseWriter, r *http.Request) { //nolin // @Success 200 {object} codersdk.UserSecret // @Router /users/{user}/secrets/{name} [patch] func (api *API) patchUserSecret(rw http.ResponseWriter, r *http.Request) { - ctx := r.Context() - user := httpmw.UserParam(r) - name := chi.URLParam(r, "name") + var ( + ctx = r.Context() + user = httpmw.UserParam(r) + name = chi.URLParam(r, "name") + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.UserSecret](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionWrite, + }) + ) + defer commitAudit() var req codersdk.UpdateUserSecretRequest if !httpapi.Read(ctx, rw, r, &req) { @@ -198,6 +221,15 @@ func (api *API) patchUserSecret(rw http.ResponseWriter, r *http.Request) { return } } + if req.Value != nil { + if err := codersdk.UserSecretValueValid(*req.Value); err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid secret value.", + Detail: err.Error(), + }) + return + } + } params := database.UpdateUserSecretByUserIDAndNameParams{ UserID: user.ID, @@ -213,13 +245,6 @@ func (api *API) patchUserSecret(rw http.ResponseWriter, r *http.Request) { FilePath: "", } if req.Value != nil { - if err := codersdk.UserSecretValueValid(*req.Value); err != nil { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Invalid secret value.", - Detail: err.Error(), - }) - return - } params.Value = *req.Value } if req.Description != nil { @@ -232,7 +257,33 @@ func (api *API) patchUserSecret(rw http.ResponseWriter, r *http.Request) { params.FilePath = *req.FilePath } - secret, err := api.Database.UpdateUserSecretByUserIDAndName(ctx, params) + // Pre-read the secret inside a transaction so the audit diff has both an + // "old" and "new" snapshot. + // + // Under read committed isolation, a concurrent writer between our SELECT + // and our UPDATE can cause the audit diff to attribute changes to us that + // we did not make. We accept this race to match other audit log diffs + // (templates, workspaces, chats, etc). In practice this should be unlikely + // to hit since a user can only modify their own secrets. + var secret database.UserSecret + err := api.Database.InTx(func(tx database.Store) error { + old, err := tx.GetUserSecretByUserIDAndName(ctx, database.GetUserSecretByUserIDAndNameParams{ + UserID: user.ID, + Name: name, + }) + if err != nil { + return xerrors.Errorf("fetch user secret: %w", err) + } + aReq.Old = old + + updated, err := tx.UpdateUserSecretByUserIDAndName(ctx, params) + if err != nil { + return xerrors.Errorf("update user secret: %w", err) + } + secret = updated + aReq.New = updated + return nil + }, nil) if err != nil { if errors.Is(err, sql.ErrNoRows) { httpapi.ResourceNotFound(rw) @@ -264,25 +315,36 @@ func (api *API) patchUserSecret(rw http.ResponseWriter, r *http.Request) { // @Success 204 // @Router /users/{user}/secrets/{name} [delete] func (api *API) deleteUserSecret(rw http.ResponseWriter, r *http.Request) { - ctx := r.Context() - user := httpmw.UserParam(r) - name := chi.URLParam(r, "name") + var ( + ctx = r.Context() + user = httpmw.UserParam(r) + name = chi.URLParam(r, "name") + auditor = api.Auditor.Load() + aReq, commitAudit = audit.InitRequest[database.UserSecret](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionDelete, + }) + ) + defer commitAudit() - rowsAffected, err := api.Database.DeleteUserSecretByUserIDAndName(ctx, database.DeleteUserSecretByUserIDAndNameParams{ + deleted, err := api.Database.DeleteUserSecretByUserIDAndName(ctx, database.DeleteUserSecretByUserIDAndNameParams{ UserID: user.ID, Name: name, }) if err != nil { + if errors.Is(err, sql.ErrNoRows) { + httpapi.ResourceNotFound(rw) + return + } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error deleting secret.", Detail: err.Error(), }) return } - if rowsAffected == 0 { - httpapi.ResourceNotFound(rw) - return - } + aReq.Old = deleted rw.WriteHeader(http.StatusNoContent) } diff --git a/coderd/usersecrets_audit_test.go b/coderd/usersecrets_audit_test.go new file mode 100644 index 0000000000..ba1fdd96f3 --- /dev/null +++ b/coderd/usersecrets_audit_test.go @@ -0,0 +1,178 @@ +package coderd_test + +import ( + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/audit" + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" +) + +//nolint:paralleltest,tparallel // Subtests share one coderdtest.New server and run sequentially. +func TestUserSecretAudit(t *testing.T) { + t.Parallel() + + auditor := audit.NewMock() + client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + _ = coderdtest.CreateFirstUser(t, client) + ctx := testutil.Context(t, testutil.WaitMedium) + + genSecretName := func(t *testing.T) string { + // Use test name derived secret names so subtests cannot + // collide in the shared user's secret namespace. + return strings.ReplaceAll(t.Name(), "/", "-") + } + + t.Run("CreateEmitsLog", func(t *testing.T) { + auditor.ResetLogs() + + secret, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{ + Name: genSecretName(t), + Value: "ghp_xxxxxxxxxxxx", + }) + require.NoError(t, err) + + logs := auditor.AuditLogs() + require.Len(t, logs, 1) + assert.Equal(t, database.AuditActionCreate, logs[0].Action) + assert.Equal(t, secret.ID, logs[0].ResourceID) + assert.Equal(t, secret.Name, logs[0].ResourceTarget) + assert.EqualValues(t, http.StatusCreated, logs[0].StatusCode) + }) + + t.Run("UpdateEmitsLog", func(t *testing.T) { + auditor.ResetLogs() + + secret, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{ + Name: genSecretName(t), + Value: "old", + }) + require.NoError(t, err) + + newDescription := "rotated" + newValue := "new-value" + _, err = client.UpdateUserSecret(ctx, codersdk.Me, secret.Name, codersdk.UpdateUserSecretRequest{ + Description: &newDescription, + Value: &newValue, + }) + require.NoError(t, err) + + logs := auditor.AuditLogs() + require.Len(t, logs, 2) + assert.Equal(t, database.AuditActionCreate, logs[0].Action) + assert.Equal(t, database.AuditActionWrite, logs[1].Action) + assert.Equal(t, secret.ID, logs[1].ResourceID) + assert.Equal(t, secret.Name, logs[1].ResourceTarget) + assert.EqualValues(t, http.StatusOK, logs[1].StatusCode) + }) + + t.Run("DeleteEmitsLog", func(t *testing.T) { + auditor.ResetLogs() + + secret, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{ + Name: genSecretName(t), + Value: "value", + }) + require.NoError(t, err) + + require.NoError(t, client.DeleteUserSecret(ctx, codersdk.Me, secret.Name)) + + logs := auditor.AuditLogs() + require.Len(t, logs, 2) + assert.Equal(t, database.AuditActionCreate, logs[0].Action) + assert.Equal(t, database.AuditActionDelete, logs[1].Action) + assert.Equal(t, secret.ID, logs[1].ResourceID) + assert.Equal(t, secret.Name, logs[1].ResourceTarget) + assert.EqualValues(t, http.StatusNoContent, logs[1].StatusCode) + }) + + t.Run("DeleteOfMissingWritesNoLog", func(t *testing.T) { + auditor.ResetLogs() + + err := client.DeleteUserSecret(ctx, codersdk.Me, "does-not-exist") + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + assert.Equal(t, http.StatusNotFound, sdkErr.StatusCode()) + + require.Empty(t, auditor.AuditLogs()) + }) + + t.Run("UpdateOfMissingWritesNoLog", func(t *testing.T) { + auditor.ResetLogs() + + desc := "anything" + _, err := client.UpdateUserSecret(ctx, codersdk.Me, "does-not-exist", codersdk.UpdateUserSecretRequest{ + Description: &desc, + }) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + assert.Equal(t, http.StatusNotFound, sdkErr.StatusCode()) + + require.Empty(t, auditor.AuditLogs()) + }) + + t.Run("ValidationFailureWritesNoLog", func(t *testing.T) { + auditor.ResetLogs() + + _, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{ + Name: genSecretName(t), + Value: "value", + EnvName: "1invalid", + }) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + assert.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) + + require.Empty(t, auditor.AuditLogs()) + }) + + t.Run("EmptyUpdateWritesNoLog", func(t *testing.T) { + auditor.ResetLogs() + name := genSecretName(t) + + _, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{ + Name: name, + Value: "value", + }) + require.NoError(t, err) + // Reset to ignore the created log. We are only testing that the + // no-op update does not add a new log. + auditor.ResetLogs() + + _, err = client.UpdateUserSecret(ctx, codersdk.Me, name, codersdk.UpdateUserSecretRequest{}) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + assert.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) + + require.Empty(t, auditor.AuditLogs()) + }) + + t.Run("ReadsDoNotAudit", func(t *testing.T) { + auditor.ResetLogs() + secretName := genSecretName(t) + + _, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{ + Name: secretName, + Value: "value", + }) + require.NoError(t, err) + // Discard the create log so the assertion below only sees audit entries + // produced by later reads. + auditor.ResetLogs() + + _, err = client.UserSecrets(ctx, codersdk.Me) + require.NoError(t, err) + + _, err = client.UserSecretByName(ctx, codersdk.Me, secretName) + require.NoError(t, err) + + require.Empty(t, auditor.AuditLogs()) + }) +} diff --git a/codersdk/audit.go b/codersdk/audit.go index d8dc880589..1a06aecd31 100644 --- a/codersdk/audit.go +++ b/codersdk/audit.go @@ -47,6 +47,7 @@ const ( ResourceTypeTask ResourceType = "task" ResourceTypeAISeat ResourceType = "ai_seat" ResourceTypeChat ResourceType = "chat" + ResourceTypeUserSecret ResourceType = "user_secret" ) func (r ResourceType) FriendlyString() string { @@ -109,6 +110,8 @@ func (r ResourceType) FriendlyString() string { return "ai seat" case ResourceTypeChat: return "chat" + case ResourceTypeUserSecret: + return "user secret" default: return "unknown" } diff --git a/docs/admin/security/audit-logs.md b/docs/admin/security/audit-logs.md index 4876e0fde4..60aa73ba70 100644 --- a/docs/admin/security/audit-logs.md +++ b/docs/admin/security/audit-logs.md @@ -38,6 +38,7 @@ We track the following resources: | Template
write, delete | |
FieldTracked
active_version_idtrue
activity_bumptrue
allow_user_autostarttrue
allow_user_autostoptrue
allow_user_cancel_workspace_jobstrue
autostart_block_days_of_weektrue
autostop_requirement_days_of_weektrue
autostop_requirement_weekstrue
cors_behaviortrue
created_atfalse
created_bytrue
created_by_avatar_urlfalse
created_by_namefalse
created_by_usernamefalse
default_ttltrue
deletedfalse
deprecatedtrue
descriptiontrue
disable_module_cachetrue
display_nametrue
failure_ttltrue
group_acltrue
icontrue
idtrue
max_port_sharing_leveltrue
nametrue
organization_display_namefalse
organization_iconfalse
organization_idfalse
organization_namefalse
provisionertrue
require_active_versiontrue
time_til_dormanttrue
time_til_dormant_autodeletetrue
updated_atfalse
use_classic_parameter_flowtrue
user_acltrue
| | TemplateVersion
create, write | |
FieldTracked
archivedtrue
created_atfalse
created_bytrue
created_by_avatar_urlfalse
created_by_namefalse
created_by_usernamefalse
external_auth_providersfalse
has_ai_taskfalse
has_external_agentfalse
idtrue
job_idfalse
messagefalse
nametrue
organization_idfalse
readmetrue
source_example_idfalse
template_idtrue
updated_atfalse
| | User
create, write, delete | |
FieldTracked
avatar_urlfalse
chat_spend_limit_microstrue
created_atfalse
deletedtrue
emailtrue
github_com_user_idfalse
hashed_one_time_passcodefalse
hashed_passwordtrue
idtrue
is_service_accounttrue
is_systemtrue
last_seen_atfalse
login_typetrue
nametrue
one_time_passcode_expires_attrue
quiet_hours_scheduletrue
rbac_rolestrue
statustrue
updated_atfalse
usernametrue
| +| UserSecret
create, write, delete | |
FieldTracked
created_atfalse
descriptiontrue
env_nametrue
file_pathtrue
idtrue
nametrue
updated_atfalse
user_idtrue
valuetrue
value_key_idfalse
| | WorkspaceBuild
start, stop | |
FieldTracked
build_numberfalse
created_atfalse
daily_costfalse
deadlinefalse
has_ai_taskfalse
has_external_agentfalse
idfalse
initiator_by_avatar_urlfalse
initiator_by_namefalse
initiator_by_usernamefalse
initiator_idfalse
job_idfalse
max_deadlinefalse
reasonfalse
template_version_idtrue
template_version_preset_idfalse
transitionfalse
updated_atfalse
workspace_idfalse
| | WorkspaceProxy
| |
FieldTracked
created_attrue
deletedfalse
derp_enabledtrue
derp_onlytrue
display_nametrue
icontrue
idtrue
nametrue
region_idtrue
token_hashed_secrettrue
updated_atfalse
urltrue
versiontrue
wildcard_hostnametrue
| | WorkspaceTable
| |
FieldTracked
automatic_updatestrue
autostart_scheduletrue
created_atfalse
deletedfalse
deleting_attrue
dormant_attrue
favoritetrue
group_acltrue
idtrue
last_used_atfalse
nametrue
next_start_attrue
organization_idfalse
owner_idtrue
template_idtrue
ttltrue
updated_atfalse
user_acltrue
| diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index e5788ba9d6..e84f954e7c 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -8490,9 +8490,9 @@ Only certain features set these fields: - FeatureManagedAgentLimit| #### Enumerated Values -| Value(s) | -|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `ai_seat`, `api_key`, `chat`, `convert_login`, `custom_role`, `git_ssh_key`, `group`, `health_settings`, `idp_sync_settings_group`, `idp_sync_settings_organization`, `idp_sync_settings_role`, `license`, `notification_template`, `notifications_settings`, `oauth2_provider_app`, `oauth2_provider_app_secret`, `organization`, `organization_member`, `prebuilds_settings`, `task`, `template`, `template_version`, `user`, `workspace`, `workspace_agent`, `workspace_app`, `workspace_build`, `workspace_proxy` | +| Value(s) | +|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `ai_seat`, `api_key`, `chat`, `convert_login`, `custom_role`, `git_ssh_key`, `group`, `health_settings`, `idp_sync_settings_group`, `idp_sync_settings_organization`, `idp_sync_settings_role`, `license`, `notification_template`, `notifications_settings`, `oauth2_provider_app`, `oauth2_provider_app_secret`, `organization`, `organization_member`, `prebuilds_settings`, `task`, `template`, `template_version`, `user`, `user_secret`, `workspace`, `workspace_agent`, `workspace_app`, `workspace_build`, `workspace_proxy` | ## codersdk.Response diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 7b3354d6c5..98f26b9100 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -30,6 +30,7 @@ var AuditActionMap = map[string][]codersdk.AuditAction{ "Task": {codersdk.AuditActionCreate, codersdk.AuditActionWrite, codersdk.AuditActionDelete}, "AiSeatState": {codersdk.AuditActionCreate}, "Chat": {codersdk.AuditActionCreate, codersdk.AuditActionWrite}, // chats get 'archived' by users, not deleted. + "UserSecret": {codersdk.AuditActionCreate, codersdk.AuditActionWrite, codersdk.AuditActionDelete}, } type Action string @@ -408,6 +409,20 @@ var auditableResourcesTypes = map[any]map[string]Action{ "plan_mode": ActionIgnore, // Can flip back and forth during a session. "client_type": ActionIgnore, // Set at creation. }, + &database.UserSecret{}: { + "id": ActionTrack, + "user_id": ActionTrack, + "name": ActionTrack, + "description": ActionTrack, + "env_name": ActionTrack, + "file_path": ActionTrack, + + "value": ActionSecret, + + "value_key_id": ActionIgnore, + "created_at": ActionIgnore, + "updated_at": ActionIgnore, + }, } // auditMap converts a map of struct pointers to a map of struct names as diff --git a/enterprise/coderd/usersecrets_audit_test.go b/enterprise/coderd/usersecrets_audit_test.go new file mode 100644 index 0000000000..46deac17f7 --- /dev/null +++ b/enterprise/coderd/usersecrets_audit_test.go @@ -0,0 +1,132 @@ +package coderd_test + +import ( + "context" + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/audit" + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/codersdk" + entaudit "github.com/coder/coder/v2/enterprise/audit" + "github.com/coder/coder/v2/enterprise/audit/backends" + "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" + "github.com/coder/coder/v2/enterprise/coderd/license" + "github.com/coder/coder/v2/testutil" +) + +func TestUserSecretAuditDiffRedaction(t *testing.T) { + // Ensure secret values never appear in plaintext in audit diffs. The + // enterprise auditor needs to be used because it writes actual diffs. + // We read straight from the audit_logs table to exercise the full + // insert, filter, dbauthz read path. + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + auditor := entaudit.NewAuditor( + db, + entaudit.DefaultFilter, + backends.NewPostgres(db, true), + ) + + ownerClient, owner := coderdenttest.New(t, &coderdenttest.Options{ + AuditLogging: true, + Options: &coderdtest.Options{ + Database: db, + Pubsub: ps, + Auditor: auditor, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureAuditLog: 1, + }, + }, + }) + memberClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) + defer cancel() + + initialDescription := "initial" + initialValue := "initial-secret-value" + secret, err := memberClient.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{ + Name: "createDiff-target", + Description: initialDescription, + Value: initialValue, + }) + require.NoError(t, err) + + newDescription := "after" + newValue := "new-secret-value" + _, err = memberClient.UpdateUserSecret(ctx, codersdk.Me, secret.Name, codersdk.UpdateUserSecretRequest{ + Description: &newDescription, + Value: &newValue, + }) + require.NoError(t, err) + + // Read straight from the database. AsSystemRestricted is necessary because + // the test does not authenticate as an admin when querying the store directly. + rows, err := db.GetAuditLogsOffset( + dbauthz.AsSystemRestricted(ctx), + database.GetAuditLogsOffsetParams{ + ResourceType: string(database.ResourceTypeUserSecret), + LimitOpt: 10, + }, + ) + require.NoError(t, err) + require.Equal(t, len(rows), 2, "expected exactly two rows") + // GetAuditLogsOffset returns entries sorted by time in descending order. + createLog := rows[1].AuditLog + updateLog := rows[0].AuditLog + + var createDiff audit.Map + require.NoError(t, json.Unmarshal(createLog.Diff, &createDiff)) + + // Creation must show both old and new non-secret values verbatim. + if assert.Contains(t, createDiff, "description", "tracked field missing from createDiff") { + assert.Equal(t, "", createDiff["description"].Old) + assert.Equal(t, initialDescription, createDiff["description"].New) + assert.False(t, createDiff["description"].Secret) + } + + // Creation must record that it changed but with zero-valued old/new and + // indicate the value is secret. + if assert.Contains(t, createDiff, "value", "value field missing from createDiff") { + assert.True(t, createDiff["value"].Secret, "value field must be marked secret") + assert.Equal(t, "", createDiff["value"].Old) + assert.Equal(t, "", createDiff["value"].New) + } + + // Ensure ignored fields are excluded from the create diff. + assert.NotContains(t, createDiff, "value_key_id") + assert.NotContains(t, createDiff, "created_at") + assert.NotContains(t, createDiff, "updated_at") + + var updateDiff audit.Map + require.NoError(t, json.Unmarshal(updateLog.Diff, &updateDiff)) + + // Update must show both old and new non-secret values verbatim. + if assert.Contains(t, updateDiff, "description", "tracked field missing from updateDiff") { + assert.Equal(t, initialDescription, updateDiff["description"].Old) + assert.Equal(t, newDescription, updateDiff["description"].New) + assert.False(t, updateDiff["description"].Secret) + } + + // Update must record that it changed but with zero-valued old/new and + // indicate the value is secret. + if assert.Contains(t, updateDiff, "value", "value field missing from updateDiff") { + assert.True(t, updateDiff["value"].Secret, "value field must be marked secret") + assert.Equal(t, "", updateDiff["value"].Old) + assert.Equal(t, "", updateDiff["value"].New) + } + + // Ensure ignored fields are excluded from update diff. + assert.NotContains(t, updateDiff, "value_key_id") + assert.NotContains(t, updateDiff, "created_at") + assert.NotContains(t, updateDiff, "updated_at") +} diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 19e9b9d58b..304ead89d0 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -6351,6 +6351,7 @@ export type ResourceType = | "template" | "template_version" | "user" + | "user_secret" | "workspace" | "workspace_agent" | "workspace_app" @@ -6381,6 +6382,7 @@ export const ResourceTypes: ResourceType[] = [ "template", "template_version", "user", + "user_secret", "workspace", "workspace_agent", "workspace_app",