diff --git a/convox.yml b/convox.yml index 47d96676..fabf758d 100644 --- a/convox.yml +++ b/convox.yml @@ -30,7 +30,7 @@ services: # Pinned to an immutable release tag. scripts/bump-version.sh sets it to the new # version (:vX.Y.Z); the release workflow publishes that tag when the v* git tag is pushed. # :e327add is the v0.1.1 release (identical to :latest when this was pinned). - image: docker.io/docspringcom/rack-gateway:v0.1.23 + image: docker.io/docspringcom/rack-gateway:v0.1.24 command: ./scripts/start-gateway.sh environment: - PORT=8080 @@ -54,7 +54,7 @@ services: admin: # Uses same image as gateway service (kept in sync by scripts/bump-version.sh) - image: docker.io/docspringcom/rack-gateway:v0.1.23 + image: docker.io/docspringcom/rack-gateway:v0.1.24 command: echo "Use this service to run migrations, database admin tasks, etc." environment: - PORT=8080 diff --git a/docs/src/content/docs/concepts/audit-logging.mdx b/docs/src/content/docs/concepts/audit-logging.mdx index df00cf80..defee766 100644 --- a/docs/src/content/docs/concepts/audit-logging.mdx +++ b/docs/src/content/docs/concepts/audit-logging.mdx @@ -325,8 +325,9 @@ the log can't show in full, and the refusal is audited as a denial. The rack-gat starts a process, so it can say why; the Convox SDK only shows `websocket: bad handshake` for a refused exec. A command's justification (`--justification`, or the deploy approval's message for a command run under one) is -recorded with it as `justification` in the details. An app can require one for every command -(`require_command_justification`): a command without one is refused with `400` and audited as a denial. +recorded with it as `justification` in the details. The gateway can require one for every command, in every app +or in one app (`require_command_justification`): a command without one is refused with `400` and audited as a +denial. A command run with a script (`--script`, see [Scripts](/user-guide/cli/commands/#scripts)) is recorded readably: `command` is the command as a person would run it locally (`rails runner check.rb`), `script` the script's text diff --git a/docs/src/content/docs/configuration/environment-variables.mdx b/docs/src/content/docs/configuration/environment-variables.mdx index 49de5172..0805cdae 100644 --- a/docs/src/content/docs/configuration/environment-variables.mdx +++ b/docs/src/content/docs/configuration/environment-variables.mdx @@ -206,6 +206,7 @@ startup log lists them, and the settings page marks them and lets an admin clear | `RGW_SETTING_DEPLOY_APPROVAL_WINDOW_MINUTES` | Approval window | | `RGW_SETTING_DEPLOY_APPROVAL_REQUIRE_DIFFERENT_APPROVER` | Require a different approver | | `RGW_SETTING_ALLOW_DESTRUCTIVE_ACTIONS` | Allow destructive actions | +| `RGW_SETTING_REQUIRE_COMMAND_JUSTIFICATION` | Require a justification for every command in every app | | `RGW_SETTING_DEFAULT_CI_PROVIDER` | Default CI provider | | `RGW_SETTING_DEFAULT_VCS_PROVIDER` | Default VCS provider | @@ -229,7 +230,8 @@ RGW_APP_MY_APP_SETTING_SERVICE_IMAGE_PATTERNS={"*":"registry\\.example\\.com/my- See [Deploy Approvals](/integrations/deploy-approvals/) for common keys. `require_command_justification` makes every command run in the app (`run`, `exec`) carry a `--justification` (see -[Justifications](/user-guide/cli/commands/#justifications)). +[Justifications](/user-guide/cli/commands/#justifications)). `RGW_SETTING_REQUIRE_COMMAND_JUSTIFICATION` does the +same for every app, and an app can't opt out of it. ## Audit Logging diff --git a/docs/src/content/docs/development/api-reference.mdx b/docs/src/content/docs/development/api-reference.mdx index 87a94d67..1db9fc58 100644 --- a/docs/src/content/docs/development/api-reference.mdx +++ b/docs/src/content/docs/development/api-reference.mdx @@ -111,6 +111,7 @@ All under `/auth/mfa`: - `GET /settings` - `PUT /settings/mfa-configuration`, `DELETE /settings/mfa-configuration` - `PUT /settings/allow-destructive-actions`, `DELETE /settings/allow-destructive-actions` +- `PUT /settings/require-command-justification`, `DELETE /settings/require-command-justification` - `PUT /settings/vcs-and-ci-defaults`, `DELETE /settings/vcs-and-ci-defaults` - `PUT /settings/deploy-approvals`, `DELETE /settings/deploy-approvals` - `PUT /settings/session-configuration`, `DELETE /settings/session-configuration` diff --git a/docs/src/content/docs/operations/upgrades.mdx b/docs/src/content/docs/operations/upgrades.mdx index f5cf803d..e261a36e 100644 --- a/docs/src/content/docs/operations/upgrades.mdx +++ b/docs/src/content/docs/operations/upgrades.mdx @@ -207,6 +207,17 @@ A restore to before the upgrade loses the audit events written since; see ## Release Notes That Need Action +### Gateway-wide command justifications (v0.1.24) + +`require_command_justification` is now a gateway-wide setting as well as a per-app one (the Settings page, or +`RGW_SETTING_REQUIRE_COMMAND_JUSTIFICATION=true`). With it on, every command in every app needs a justification, +whatever the app's own setting says. + +1. **Turn it on once your users' CLIs are on v0.1.24.** An older CLI reads only the app's setting, so it doesn't + stop an `exec` without a justification before it starts: the gateway refuses it, and the CLI only shows + `websocket: bad handshake`. A `run` still shows the gateway's reason. +2. **`scripts/deploy.sh` gives its migration run a justification**, so deploys keep working with the setting on. + ### Fresh MFA for every command (v0.1.19 to v0.1.22) v0.1.19 asks for MFA on every `run` and `exec`, adds justifications and `--script`. Upgrade straight to v0.1.22: @@ -224,8 +235,9 @@ without a terminal needs no separate PIN dialog. command by its runner, which v0.1.21 changed: when only one side has that change, the audit log records the raw `sh -c` command instead of the script's name, text and SHA-256. 3. **`require_command_justification` is off by default.** Turn it on per app (app settings page, the API, or - `RGW_APP__SETTING_REQUIRE_COMMAND_JUSTIFICATION=true`) once your users' CLIs are on v0.1.19+. Commands run - under a deploy approval use the approval's message as their justification. + `RGW_APP__SETTING_REQUIRE_COMMAND_JUSTIFICATION=true`) once your users' CLIs are on v0.1.19+, or for every + app from v0.1.24 once their CLIs are on v0.1.24 (see above). Commands run under a deploy approval use the + approval's message as their justification. 4. **To hear about commands in Slack, map `process.exec.start` and `process.start`** to a channel. The message never includes the command or the script, and links to the audit entry. diff --git a/docs/src/content/docs/security/rbac/permissions.mdx b/docs/src/content/docs/security/rbac/permissions.mdx index 9eb4aa51..d694efba 100644 --- a/docs/src/content/docs/security/rbac/permissions.mdx +++ b/docs/src/content/docs/security/rbac/permissions.mdx @@ -342,9 +342,10 @@ Reads and lists need none. Any other permission needs **always** unless it's lis So these always need MFA on the request itself: running a command in an app (`convox:process:exec`, and `convox:process:start` except `convox run`'s placeholder start, whose command comes as an exec), deleting apps, importing builds, creating, changing, locking and deleting users, revoking sessions, creating, changing and deleting API tokens, approving, rejecting and extending deploy approvals, removing an MFA method, refreshing the rack TLS certificate, -changing protected env vars, privileged process options, and the settings that protect secrets or loosen deploy -checks (`mfa_configuration`, `allow_destructive_actions`, `deploy_approvals`, `vcs_ci_deploy`, -`protected_env_vars`, `secret_env_vars`, `github_verification`, `require_pr_for_branch`). +changing protected env vars, privileged process options, and the settings that protect secrets, loosen deploy +checks or require justifications (`mfa_configuration`, `allow_destructive_actions`, `deploy_approvals`, +`vcs_ci_deploy`, `protected_env_vars`, `secret_env_vars`, `github_verification`, `require_pr_for_branch`, and +`require_command_justification`, gateway-wide and per app). API tokens can't verify MFA, so MFA levels don't apply to them; what a token may do is limited by its permissions and its owner's role. The levels are defined in `internal/gateway/rbac/mfa_requirements.go`. diff --git a/docs/src/content/docs/user-guide/cli/commands.mdx b/docs/src/content/docs/user-guide/cli/commands.mdx index 3f2ff798..a439ba22 100644 --- a/docs/src/content/docs/user-guide/cli/commands.mdx +++ b/docs/src/content/docs/user-guide/cli/commands.mdx @@ -350,8 +350,10 @@ is running, just before the command runs (see [Running commands](/user-guide/cli #### Justifications `--justification` says why a command is run and what it does, in one line of 16 to 1000 characters. The gateway -records it with the command in the audit log, and the PIN dialog shows it. An app can require one for every -command (the `require_command_justification` setting): `run` and `exec` without one are then refused before +records it with the command in the audit log, and the PIN dialog shows it. The `require_command_justification` +setting requires one for every command, in every app (on the Settings page, or +`RGW_SETTING_REQUIRE_COMMAND_JUSTIFICATION=true`) or in one app (its own setting). Either one is enough, and an app +can't opt out of the gateway-wide setting. `run` and `exec` without a justification are then refused before anything starts. A command run under a deploy approval is justified by the approval's message. #### Scripts diff --git a/docs/src/content/docs/user-guide/web-ui/settings.mdx b/docs/src/content/docs/user-guide/web-ui/settings.mdx index 3893c5d6..8b187a99 100644 --- a/docs/src/content/docs/user-guide/web-ui/settings.mdx +++ b/docs/src/content/docs/user-guide/web-ui/settings.mdx @@ -55,6 +55,12 @@ Set the defaults for app-level deploy approvals: These defaults are used when an app does not specify its own provider settings. +### Command Justifications + +Require a justification for every command run in every app (`run`, `exec`): each one must say why it's run and +what it does (see [Justifications](/user-guide/cli/commands/#justifications)). Apps can also require one on their +own; with this on, an app requires one whatever its own setting says. Changing it needs MFA on the request. + ## App Settings (Per-App) App-specific settings are **not** on this page. Go to **Apps → `{app}` → Settings** to configure: @@ -66,7 +72,8 @@ App-specific settings are **not** on this page. Go to **Apps → `{app}` → Set - Approved deploy commands - Service image patterns - Command justifications: whether every command run in the app (`run`, `exec`) must say why it's run (see - [Justifications](/user-guide/cli/commands/#justifications)) + [Justifications](/user-guide/cli/commands/#justifications)). The gateway-wide setting above requires one in + every app, and the app's card says so when it's on ## Related diff --git a/internal/cli/AGENTS.md b/internal/cli/AGENTS.md index ea38f672..c9a6355e 100644 --- a/internal/cli/AGENTS.md +++ b/internal/cli/AGENTS.md @@ -237,9 +237,9 @@ placeholder start and the waits go without a proof, and the proof is collected j detached run's start), with the terminal taken out of the raw mode Convox put it in, so a TOTP code or a WebAuthn challenge doesn't expire while a slow process starts. The PIN dialog summary names the app, rack, command and justification (`commandPrompt`). `--justification` travels in `audit.JustificationHeader` (percent-encoded), which -the gateway records and doesn't forward. When the app's `require_command_justification` is on, the CLI refuses a -command without one before anything starts (it reads the app's settings; API tokens can't, and rely on the -gateway's refusal). +the gateway records and doesn't forward. When `require_command_justification` is on (gateway-wide or the app's), +the CLI refuses a command without one before anything starts (it reads the global and app settings; API tokens +can't, and rely on the gateway's refusal). `--script FILE` (`convox_process_command.go`) sends a local script with the command words: `scriptrun.Build` wraps them in a fixed `sh` runner that decodes the base64 script into a temp directory and runs the words with the diff --git a/internal/cli/convox_env_protected_test.go b/internal/cli/convox_env_protected_test.go index ff97372b..7b1859d8 100644 --- a/internal/cli/convox_env_protected_test.go +++ b/internal/cli/convox_env_protected_test.go @@ -22,25 +22,30 @@ import ( ) // envChangeGateway is a fake gateway whose MFA status reports mfaMethods (a TOTP method by default) and a step-up -// window that is still open, that starts WebAuthn assertions, and whose apps have appSettings (none by default). It -// records the headers of every rack-proxy request. +// window that is still open, that starts WebAuthn assertions, and that has globalSettings and appSettings (none by +// default). It records the headers of every rack-proxy request. type envChangeGateway struct { - mu sync.Mutex - proxied []http.Header - appSettings string - mfaMethods []MFAMethodResponse + mu sync.Mutex + proxied []http.Header + globalSettings string + appSettings string + mfaMethods []MFAMethodResponse } func (g *envChangeGateway) handler(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - if strings.HasPrefix(r.URL.Path, "/api/v1/apps/") && strings.HasSuffix(r.URL.Path, "/settings") { + if r.URL.Path == "/api/v1/settings" || + strings.HasPrefix(r.URL.Path, "/api/v1/apps/") && strings.HasSuffix(r.URL.Path, "/settings") { g.mu.Lock() - appSettings := g.appSettings + values := g.appSettings + if r.URL.Path == "/api/v1/settings" { + values = g.globalSettings + } g.mu.Unlock() - if appSettings == "" { - appSettings = "{}" + if values == "" { + values = "{}" } - _, _ = w.Write([]byte(appSettings)) + _, _ = w.Write([]byte(values)) return } if r.URL.Path == "/api/v1/auth/mfa/webauthn/assertion/start" { diff --git a/internal/cli/convox_process_approval.go b/internal/cli/convox_process_approval.go index 5aee099a..b0a1c7e4 100644 --- a/internal/cli/convox_process_approval.go +++ b/internal/cli/convox_process_approval.go @@ -50,8 +50,9 @@ func commandJustification(cmd *cobra.Command, app string) (string, error) { return "", nil } -// appRequiresJustification reads the app's require_command_justification setting. API tokens can't read settings; -// the gateway still refuses their commands without one (deploy approvals justify theirs with the approval). +// appRequiresJustification reads require_command_justification: the gateway-wide setting, then the app's. Either +// one requires a justification. API tokens can't read settings; the gateway still refuses their commands without +// one (deploy approvals justify theirs with the approval). func appRequiresJustification(cmd *cobra.Command, app string) (bool, error) { if apiTokenOverride() != "" { return false, nil @@ -60,15 +61,19 @@ func appRequiresJustification(cmd *cobra.Command, app string) (bool, error) { if err != nil { return false, err } - var appSettings map[string]struct { - Value interface{} `json:"value"` - } - path := "/apps/" + url.PathEscape(app) + "/settings" - if err := gatewayRequest(cmd, rack, http.MethodGet, path, nil, &appSettings); err != nil { - return false, fmt.Errorf("failed to read whether app %s requires a justification: %w", displayText(app), err) + for _, path := range []string{"/settings", "/apps/" + url.PathEscape(app) + "/settings"} { + var values map[string]struct { + Value interface{} `json:"value"` + } + if err := gatewayRequest(cmd, rack, http.MethodGet, path, nil, &values); err != nil { + return false, fmt.Errorf("failed to read whether app %s requires a justification: %w", + displayText(app), err) + } + if required, _ := values[settings.KeyRequireCommandJustification].Value.(bool); required { + return true, nil + } } - required, _ := appSettings[settings.KeyRequireCommandJustification].Value.(bool) - return required, nil + return false, nil } // commandPrompt describes a command for the person approving it with MFA: where it runs, the command (a script's diff --git a/internal/cli/convox_process_command_test.go b/internal/cli/convox_process_command_test.go index 73d0d382..ffe78070 100644 --- a/internal/cli/convox_process_command_test.go +++ b/internal/cli/convox_process_command_test.go @@ -46,17 +46,26 @@ func processCobraCommand(t *testing.T, cmd *cobra.Command, flags map[string]stri return cmd } -// Without --justification, a command in an app that requires one is refused before anything is sent to the rack -// (the gateway's refusal of an exec would only show as "websocket: bad handshake"). +// Without --justification, a command in an app that requires one (its own setting, or the gateway-wide one) is +// refused before anything is sent to the rack (the gateway's refusal of an exec would only show as +// "websocket: bad handshake"). func TestProcessCommandRefusedWithoutAJustificationTheAppRequires(t *testing.T) { - gateway := newEnvChangeGateway(t) - gateway.appSettings = `{"require_command_justification":{"value":true}}` - for _, cmd := range []*cobra.Command{ExecCommand(), RunCommand()} { - cmd = processCobraCommand(t, cmd, nil) - _, _, err := setupProcessCommand(cmd, []string{"web", "rails", "runner", "puts 1"}, cmd.Name()) - require.ErrorContains(t, err, "app myapp requires a justification for every command") + const required = `{"require_command_justification":{"value":true}}` + for name, set := range map[string]func(*envChangeGateway){ + "app setting": func(g *envChangeGateway) { g.appSettings = required }, + "gateway-wide": func(g *envChangeGateway) { g.globalSettings = required }, + } { + t.Run(name, func(t *testing.T) { + gateway := newEnvChangeGateway(t) + set(gateway) + for _, cmd := range []*cobra.Command{ExecCommand(), RunCommand()} { + cmd = processCobraCommand(t, cmd, nil) + _, _, err := setupProcessCommand(cmd, []string{"web", "rails", "runner", "puts 1"}, cmd.Name()) + require.ErrorContains(t, err, "app myapp requires a justification for every command") + } + require.Empty(t, gateway.requests()) + }) } - require.Empty(t, gateway.requests()) } // A justification that can't say why the command is run is refused. A good one goes with each of the command's diff --git a/internal/gateway/AGENTS.md b/internal/gateway/AGENTS.md index 0dd45ac0..66283b10 100644 --- a/internal/gateway/AGENTS.md +++ b/internal/gateway/AGENTS.md @@ -108,8 +108,9 @@ Request forwarding with WebSocket support: no proof; the exec that follows carries the command's proof - Command justifications (`proxy/command_justification.go`): an exec's or process start's `X-Command-Justification` (percent-encoded, checked by `audit.CheckJustification`, never forwarded), or the - deploy approval's message, goes on the request context and into its audit entries' details. An app with - `require_command_justification` refuses a command without one, the placeholder start included + deploy approval's message, goes on the request context and into its audit entries' details. With + `require_command_justification` on (gateway-wide, or the app's own; `GetRequireCommandJustification` reads both), + a command without one is refused, the placeholder start included - MFA settings are read when needed (`settings.Service.GetMFASettings`, one query; the middleware reads them once per request), never kept from startup, so a change applies at once on every instance - Client IPs: use `c.ClientIP()` in gin handlers and `netutil.ClientIP(r)` elsewhere. Both trust diff --git a/internal/gateway/handlers/settings.go b/internal/gateway/handlers/settings.go index ad21d1ce..ed20c49b 100644 --- a/internal/gateway/handlers/settings.go +++ b/internal/gateway/handlers/settings.go @@ -126,6 +126,12 @@ func (h *SettingsHandler) UpdateGlobalAllowDestructiveActions(c *gin.Context) { h.updateGlobalSettingsWithKeys(c, allowedKeys) } +// UpdateGlobalRequireCommandJustification updates whether every command in every app needs a justification. +func (h *SettingsHandler) UpdateGlobalRequireCommandJustification(c *gin.Context) { + allowedKeys := settings.GlobalSettingGroupKeyStrings(settings.GlobalSettingGroupCommandJustification) + h.updateGlobalSettingsWithKeys(c, allowedKeys) +} + // UpdateGlobalVCSAndCIDefaults updates VCS and CI default settings. func (h *SettingsHandler) UpdateGlobalVCSAndCIDefaults(c *gin.Context) { allowedKeys := settings.GlobalSettingGroupKeyStrings(settings.GlobalSettingGroupVCSAndCIDefaults) @@ -176,6 +182,12 @@ func (h *SettingsHandler) DeleteGlobalAllowDestructiveActions(c *gin.Context) { h.deleteGlobalSettingsWithKeys(c, allowedKeys) } +// DeleteGlobalRequireCommandJustification deletes the gateway-wide require_command_justification. +func (h *SettingsHandler) DeleteGlobalRequireCommandJustification(c *gin.Context) { + allowedKeys := settings.GlobalSettingGroupKeyStrings(settings.GlobalSettingGroupCommandJustification) + h.deleteGlobalSettingsWithKeys(c, allowedKeys) +} + // DeleteGlobalVCSAndCIDefaults deletes VCS and CI default settings. func (h *SettingsHandler) DeleteGlobalVCSAndCIDefaults(c *gin.Context) { allowedKeys := settings.GlobalSettingGroupKeyStrings(settings.GlobalSettingGroupVCSAndCIDefaults) diff --git a/internal/gateway/proxy/command_justification.go b/internal/gateway/proxy/command_justification.go index 2f90f405..e8355b32 100644 --- a/internal/gateway/proxy/command_justification.go +++ b/internal/gateway/proxy/command_justification.go @@ -15,9 +15,10 @@ var errJustificationRequired = errors.New("this app requires a justification for // checkCommandJustification finds the justification of the command r runs (an exec's, or a process start's) and // puts it on r, so the request's audit entries record it. A command under a deploy approval is justified by the -// approval's message. In an app that requires one (require_command_justification), a command without one is -// refused: `convox run`'s placeholder start too, so an attached run without one is refused before its process -// starts, with a reason the CLI can show (it can't show why an exec's WebSocket handshake was refused). +// approval's message. In an app that requires one (require_command_justification, the app's own or the +// gateway-wide one), a command without one is refused: `convox run`'s placeholder start too, so an attached run +// without one is refused before its process starts, with a reason the CLI can show (it can't show why an exec's +// WebSocket handshake was refused). func (h *Handler) checkCommandJustification(r *http.Request, tracker *deployApprovalTracker) error { if _, ok := audit.RequestCommand(r); !ok { return nil diff --git a/internal/gateway/proxy/command_justification_test.go b/internal/gateway/proxy/command_justification_test.go index 3c944ead..f2b078f6 100644 --- a/internal/gateway/proxy/command_justification_test.go +++ b/internal/gateway/proxy/command_justification_test.go @@ -80,21 +80,28 @@ func TestInvalidCommandJustificationIsRefused(t *testing.T) { require.Contains(t, f.processStartDetails(t, "denied")["reason"], "the justification") } -// An app that requires a justification refuses a command without one, `convox run`'s placeholder start included -// (so an attached run stops before its process starts), and takes one with it. +// An app that requires a justification (its own setting, or the gateway-wide one) refuses a command without one, +// `convox run`'s placeholder start included (so an attached run stops before its process starts), and takes one +// with it. func TestRequiredCommandJustification(t *testing.T) { - f := newProtectedEnvFixture(t) - app := "app" - require.NoError(t, settings.NewService(f.database).SetAppSettings( - app, map[string]interface{}{settings.KeyRequireCommandJustification: true}, nil)) + required := map[string]interface{}{settings.KeyRequireCommandJustification: true} + for name, set := range map[string]func(*settings.Service) error{ + "app setting": func(svc *settings.Service) error { return svc.SetAppSettings("app", required, nil) }, + "gateway-wide": func(svc *settings.Service) error { return svc.SetGlobalSettings(required, nil) }, + } { + t.Run(name, func(t *testing.T) { + f := newProtectedEnvFixture(t) + require.NoError(t, set(settings.NewService(f.database))) - rr := f.processRequest(t, http.MethodPost, processStartPath, "", placeholderStart("")) - require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String()) - require.Contains(t, rr.Body.String(), "this app requires a justification for every command") - require.Empty(t, f.processStarts()) + rr := f.processRequest(t, http.MethodPost, processStartPath, "", placeholderStart("")) + require.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String()) + require.Contains(t, rr.Body.String(), "this app requires a justification for every command") + require.Empty(t, f.processStarts()) - justified := placeholderStart(audit.EncodeJustification(testJustification)) - rr = f.processRequest(t, http.MethodPost, processStartPath, "", justified) - require.Equal(t, http.StatusOK, rr.Code, rr.Body.String()) - require.Len(t, f.processStarts(), 1) + justified := placeholderStart(audit.EncodeJustification(testJustification)) + rr = f.processRequest(t, http.MethodPost, processStartPath, "", justified) + require.Equal(t, http.StatusOK, rr.Code, rr.Body.String()) + require.Len(t, f.processStarts(), 1) + }) + } } diff --git a/internal/gateway/rbac/gateway_routes.go b/internal/gateway/rbac/gateway_routes.go index 66dd1053..58e71b23 100644 --- a/internal/gateway/rbac/gateway_routes.go +++ b/internal/gateway/rbac/gateway_routes.go @@ -117,6 +117,16 @@ var httpRouteSpecs = []RouteSpec{ globalSettingsGroupPath(settings.GlobalSettingGroupAllowDestructive), GatewayGlobalSettingGroup(settings.GlobalSettingGroupAllowDestructive), ), + newHTTPRoute( + "PUT", + globalSettingsGroupPath(settings.GlobalSettingGroupCommandJustification), + GatewayGlobalSettingGroup(settings.GlobalSettingGroupCommandJustification), + ), + newHTTPRoute( + "DELETE", + globalSettingsGroupPath(settings.GlobalSettingGroupCommandJustification), + GatewayGlobalSettingGroup(settings.GlobalSettingGroupCommandJustification), + ), newHTTPRoute( "PUT", globalSettingsGroupPath(settings.GlobalSettingGroupVCSAndCIDefaults), diff --git a/internal/gateway/rbac/http_routes_test.go b/internal/gateway/rbac/http_routes_test.go index 64c6f272..5823377d 100644 --- a/internal/gateway/rbac/http_routes_test.go +++ b/internal/gateway/rbac/http_routes_test.go @@ -15,6 +15,7 @@ func TestAllGlobalSettingGroupsHaveHTTPRoutes(t *testing.T) { allGroups := []settings.GlobalSettingGroup{ settings.GlobalSettingGroupMFAConfiguration, settings.GlobalSettingGroupAllowDestructive, + settings.GlobalSettingGroupCommandJustification, settings.GlobalSettingGroupVCSAndCIDefaults, settings.GlobalSettingGroupDeployApprovals, settings.GlobalSettingGroupSessionConfiguration, @@ -55,6 +56,7 @@ func TestAllGlobalSettingGroupsHaveMFARequirements(t *testing.T) { allGroups := []settings.GlobalSettingGroup{ settings.GlobalSettingGroupMFAConfiguration, settings.GlobalSettingGroupAllowDestructive, + settings.GlobalSettingGroupCommandJustification, settings.GlobalSettingGroupVCSAndCIDefaults, settings.GlobalSettingGroupDeployApprovals, settings.GlobalSettingGroupSessionConfiguration, diff --git a/internal/gateway/rbac/mfa_requirements.go b/internal/gateway/rbac/mfa_requirements.go index fbe785c8..3a569a3d 100644 --- a/internal/gateway/rbac/mfa_requirements.go +++ b/internal/gateway/rbac/mfa_requirements.go @@ -133,6 +133,7 @@ var MFARequirements = map[string]MFALevel{ // Global Settings - Security-critical configurations GatewayGlobalSettingGroup(settings.GlobalSettingGroupAllowDestructive): MFAAlways, // Destructive ops + GatewayGlobalSettingGroup(settings.GlobalSettingGroupCommandJustification): MFAAlways, // Requiring justifications GatewayGlobalSettingGroup(settings.GlobalSettingGroupDeployApprovals): MFAAlways, // Approval workflow GatewayGlobalSettingGroup(settings.GlobalSettingGroupMFAConfiguration): MFAAlways, // MFA config GatewayGlobalSettingGroup(settings.GlobalSettingGroupSessionConfiguration): MFAStepUp, // Session timeouts diff --git a/internal/gateway/routes/route_registration.go b/internal/gateway/routes/route_registration.go index c5274228..4830ca5e 100644 --- a/internal/gateway/routes/route_registration.go +++ b/internal/gateway/routes/route_registration.go @@ -135,6 +135,8 @@ func registerGlobalSettingsRoutes(authenticated *gin.RouterGroup, cfg *Config, h settingsMutations.DELETE("/mfa-configuration", h.settings.DeleteGlobalMFAConfiguration) settingsMutations.PUT("/allow-destructive-actions", h.settings.UpdateGlobalAllowDestructiveActions) settingsMutations.DELETE("/allow-destructive-actions", h.settings.DeleteGlobalAllowDestructiveActions) + settingsMutations.PUT("/require-command-justification", h.settings.UpdateGlobalRequireCommandJustification) + settingsMutations.DELETE("/require-command-justification", h.settings.DeleteGlobalRequireCommandJustification) settingsMutations.PUT("/vcs-and-ci-defaults", h.settings.UpdateGlobalVCSAndCIDefaults) settingsMutations.DELETE("/vcs-and-ci-defaults", h.settings.DeleteGlobalVCSAndCIDefaults) settingsMutations.PUT("/deploy-approvals", h.settings.UpdateGlobalDeployApprovals) diff --git a/internal/gateway/settings/defaults.go b/internal/gateway/settings/defaults.go index 69fcc87f..50c6073f 100644 --- a/internal/gateway/settings/defaults.go +++ b/internal/gateway/settings/defaults.go @@ -19,6 +19,7 @@ const ( GlobalSettingDeployApprovalsEnabled GlobalSettingDeployApprovalWindowMinutes GlobalSettingMFARequireAllUsers + GlobalSettingRequireCommandJustification GlobalSettingSessionTimeoutMinutes GlobalSettingStepUpWindowMinutes GlobalSettingTrustedDeviceTTLDays @@ -46,6 +47,7 @@ var globalSettingKeyToString = [...]string{ KeyDeployApprovalsEnabled, KeyDeployApprovalWindowMinutes, KeyMFARequireAllUsers, + KeyRequireCommandJustification, KeySessionTimeoutMinutes, "mfa_step_up_window_minutes", KeyTrustedDeviceTTLDays, @@ -110,7 +112,7 @@ const ( KeyGitHubPostPRComment = "github_post_pr_comment" KeyGitHubVerification = "github_verification" KeyProtectedEnvVars = "protected_env_vars" - KeyRequireCommandJustification = "require_command_justification" + KeyRequireCommandJustification = "require_command_justification" // also a global setting KeyRequirePRForBranch = "require_pr_for_branch" KeyServiceImagePatterns = "service_image_patterns" KeyVCSProvider = "vcs_provider" @@ -186,6 +188,7 @@ var DefaultGlobalSettings = map[string]interface{}{ KeyDeployApprovalsEnabled: true, KeyDeployApprovalWindowMinutes: DefaultDeployApprovalWindowMinutes, KeyMFARequireAllUsers: true, + KeyRequireCommandJustification: false, KeySessionTimeoutMinutes: 5, GlobalSettingStepUpWindowMinutes.String(): defaultStepUpWindowMinutes, KeyTrustedDeviceTTLDays: defaultTrustedDeviceTTLDays, diff --git a/internal/gateway/settings/groups.go b/internal/gateway/settings/groups.go index 4e63ed16..932e7b0e 100644 --- a/internal/gateway/settings/groups.go +++ b/internal/gateway/settings/groups.go @@ -9,6 +9,7 @@ type GlobalSettingGroup string const ( GlobalSettingGroupMFAConfiguration GlobalSettingGroup = "mfa_configuration" GlobalSettingGroupAllowDestructive GlobalSettingGroup = "allow_destructive_actions" + GlobalSettingGroupCommandJustification GlobalSettingGroup = "require_command_justification" GlobalSettingGroupVCSAndCIDefaults GlobalSettingGroup = "vcs_and_ci_defaults" GlobalSettingGroupDeployApprovals GlobalSettingGroup = "deploy_approvals" GlobalSettingGroupSessionConfiguration GlobalSettingGroup = "session_configuration" @@ -23,6 +24,9 @@ var globalSettingGroupKeys = map[GlobalSettingGroup][]GlobalSettingKey{ GlobalSettingGroupAllowDestructive: { GlobalSettingAllowDestructiveActions, }, + GlobalSettingGroupCommandJustification: { + GlobalSettingRequireCommandJustification, + }, GlobalSettingGroupVCSAndCIDefaults: { GlobalSettingDefaultCIProvider, GlobalSettingDefaultVCSProvider, diff --git a/internal/gateway/settings/service_commands.go b/internal/gateway/settings/service_commands.go index d551e7a5..e777d5b1 100644 --- a/internal/gateway/settings/service_commands.go +++ b/internal/gateway/settings/service_commands.go @@ -2,14 +2,19 @@ package settings import "fmt" -// GetRequireCommandJustification returns whether every command run in the app (exec, run) needs a justification. -// A stored value that isn't true or false is an error, which requires one. +// GetRequireCommandJustification returns whether every command run in the app (exec, run) needs a justification: +// when the gateway-wide require_command_justification is on, or the app's own is. An app can't opt out of the +// gateway-wide setting. A stored value that isn't true or false is an error, which requires one. func (s *Service) GetRequireCommandJustification(appName string) (bool, error) { + required, err := readGlobal(s, KeyRequireCommandJustification, nil, boolValue) + if err != nil || required { + return true, err + } setting, err := s.GetAppSetting(appName, KeyRequireCommandJustification, false) if err != nil { return true, err } - required, err := boolValue(KeyRequireCommandJustification, setting.Value) + required, err = boolValue(KeyRequireCommandJustification, setting.Value) if err != nil { return true, fmt.Errorf("app %s: %w", appName, err) } diff --git a/internal/gateway/settings/service_commands_test.go b/internal/gateway/settings/service_commands_test.go new file mode 100644 index 00000000..53c847eb --- /dev/null +++ b/internal/gateway/settings/service_commands_test.go @@ -0,0 +1,57 @@ +package settings + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/DocSpring/rack-gateway/internal/gateway/testutil/dbtest" +) + +func requiresJustification(t *testing.T, svc *Service, app string) bool { + t.Helper() + required, err := svc.GetRequireCommandJustification(app) + require.NoError(t, err) + return required +} + +// TestRequireCommandJustificationIsGatewayWideOrPerApp: a command needs a justification when the gateway-wide +// setting is on or its app's own is, and an app can't opt out of the gateway-wide one. +func TestRequireCommandJustificationIsGatewayWideOrPerApp(t *testing.T) { + database := dbtest.NewDatabase(t) + t.Cleanup(func() { _ = database.Close() }) + svc := NewService(database) + + require.False(t, requiresJustification(t, svc, "web")) + + require.NoError(t, svc.SetAppSettings("web", map[string]interface{}{KeyRequireCommandJustification: true}, nil)) + require.True(t, requiresJustification(t, svc, "web")) + require.False(t, requiresJustification(t, svc, "worker"), "the app setting covers only its app") + + require.NoError(t, svc.SetAppSettings("web", map[string]interface{}{KeyRequireCommandJustification: false}, nil)) + require.NoError(t, svc.SetGlobalSettings(map[string]interface{}{KeyRequireCommandJustification: true}, nil)) + require.True(t, requiresJustification(t, svc, "web"), "an app turned off can't opt out") + require.True(t, requiresJustification(t, svc, "worker")) +} + +func TestRequireCommandJustificationFromTheEnvironment(t *testing.T) { + database := dbtest.NewDatabase(t) + t.Cleanup(func() { _ = database.Close() }) + svc := NewService(database) + + t.Setenv("RGW_SETTING_REQUIRE_COMMAND_JUSTIFICATION", "true") + require.True(t, requiresJustification(t, svc, "web")) +} + +// TestRequireCommandJustificationFailsClosed: a stored gateway-wide value that isn't true or false is an error that +// requires a justification. +func TestRequireCommandJustificationFailsClosed(t *testing.T) { + database := dbtest.NewDatabase(t) + t.Cleanup(func() { _ = database.Close() }) + svc := NewService(database) + + require.NoError(t, database.UpsertSetting(nil, KeyRequireCommandJustification, "sometimes", nil)) + required, err := svc.GetRequireCommandJustification("web") + require.Error(t, err) + require.True(t, required) +} diff --git a/scripts/deploy.sh b/scripts/deploy.sh index 475f7663..d565936d 100755 --- a/scripts/deploy.sh +++ b/scripts/deploy.sh @@ -74,7 +74,8 @@ set +e rack-gateway run admin ./rack-gateway-api migrate \ $RACK_FLAG \ --app "$APP_NAME" \ - --release "$RELEASE_ID" + --release "$RELEASE_ID" \ + --justification "Run database migrations for $APP_NAME release $RELEASE_ID before promoting it" MIGRATE_EXIT_CODE=$? set -e diff --git a/web/package.json b/web/package.json index 01882145..5803d87d 100644 --- a/web/package.json +++ b/web/package.json @@ -1,6 +1,6 @@ { "name": "web", - "version": "0.1.23", + "version": "0.1.24", "private": true, "type": "module", "scripts": { diff --git a/web/src/lib/generated/mfa-requirements.ts b/web/src/lib/generated/mfa-requirements.ts index 8388f8d6..86f9bc9f 100644 --- a/web/src/lib/generated/mfa-requirements.ts +++ b/web/src/lib/generated/mfa-requirements.ts @@ -108,6 +108,12 @@ export const HTTP_ROUTE_MFA_REQUIREMENTS: HttpRouteMfaRequirement[] = [ permissions: ["gateway:setting_group:mfa_configuration"], mfaLevel: "always", }, + { + method: "DELETE", + pattern: "/api/v1/settings/require-command-justification", + permissions: ["gateway:setting_group:require_command_justification"], + mfaLevel: "always", + }, { method: "DELETE", pattern: "/api/v1/settings/session-configuration", @@ -576,6 +582,12 @@ export const HTTP_ROUTE_MFA_REQUIREMENTS: HttpRouteMfaRequirement[] = [ permissions: ["gateway:setting_group:mfa_configuration"], mfaLevel: "always", }, + { + method: "PUT", + pattern: "/api/v1/settings/require-command-justification", + permissions: ["gateway:setting_group:require_command_justification"], + mfaLevel: "always", + }, { method: "PUT", pattern: "/api/v1/settings/session-configuration", diff --git a/web/src/pages/app-settings-page.tsx b/web/src/pages/app-settings-page.tsx index a62180c3..faaa1300 100644 --- a/web/src/pages/app-settings-page.tsx +++ b/web/src/pages/app-settings-page.tsx @@ -10,6 +10,7 @@ import { ServiceImagePatternsCard } from '@/pages/app-settings/service-image-pat import { StringArrayCard } from '@/pages/app-settings/string-array-card' import type { AppSettingsResponse } from '@/pages/app-settings/types' import { VCSCIProvidersCard } from '@/pages/app-settings/vcs-ci-providers-card' +import type { GlobalSettingsResponse } from '@/pages/settings/types' export function AppSettingsPage() { const { app } = useParams({ from: '/_app/apps/$app/settings' }) as { app: string } @@ -28,6 +29,13 @@ export function AppSettingsPage() { staleTime: 0, }) + // The gateway-wide require_command_justification applies to every app, whatever the app's own setting says. + const { data: globalSettings } = useQuery({ + queryKey: ['globalSettings'], + queryFn: () => api.get('/api/v1/settings'), + staleTime: 0, + }) + if (isLoading) { return (
@@ -102,7 +110,12 @@ export function AppSettingsPage() {
- +
diff --git a/web/src/pages/app-settings/command-justification-card.test.tsx b/web/src/pages/app-settings/command-justification-card.test.tsx index 5019a3e0..320208fb 100644 --- a/web/src/pages/app-settings/command-justification-card.test.tsx +++ b/web/src/pages/app-settings/command-justification-card.test.tsx @@ -1,6 +1,7 @@ import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import { fireEvent, render, screen, waitFor } from '@testing-library/react' import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { SettingsSetting } from '@/api/schemas' import { CommandJustificationCard } from './command-justification-card' import type { AppSettingsResponse } from './types' @@ -10,14 +11,21 @@ vi.mock('@/lib/api', async (importOriginal) => { return { ...actual, api: { ...actual.api, put: mockPut } } }) -function renderCard(settings: AppSettingsResponse) { +function renderCard(settings: AppSettingsResponse, gatewayWide?: SettingsSetting) { return render( - + ) } +const gatewayWideNote = /The gateway-wide setting requires a justification/ + const checkbox = () => screen.getByRole('checkbox', { name: /Require a justification for every command/ }) @@ -54,4 +62,18 @@ describe('CommandJustificationCard', () => { expect(checkbox()).toBeChecked() expect(checkbox()).toBeDisabled() }) + + // With the gateway-wide setting on, the app requires one whatever its own setting says, and the card says so. + it('says when the gateway-wide setting requires a justification', () => { + const appOff = { + require_command_justification: { value: false, source: 'default' }, + } as AppSettingsResponse + const { unmount } = renderCard(appOff, { value: false, source: 'default' } as SettingsSetting) + expect(screen.queryByText(gatewayWideNote)).not.toBeInTheDocument() + unmount() + + renderCard(appOff, { value: true, source: 'env' } as SettingsSetting) + expect(screen.getByText(gatewayWideNote)).toBeInTheDocument() + expect(checkbox()).not.toBeChecked() + }) }) diff --git a/web/src/pages/app-settings/command-justification-card.tsx b/web/src/pages/app-settings/command-justification-card.tsx index 0f1a2308..05fe1c7e 100644 --- a/web/src/pages/app-settings/command-justification-card.tsx +++ b/web/src/pages/app-settings/command-justification-card.tsx @@ -1,4 +1,5 @@ import { useQueryClient } from '@tanstack/react-query' +import type { SettingsSetting } from '@/api/schemas' import { getSettingValue, hasStoredValue, @@ -20,18 +21,22 @@ const settingKey = 'require_command_justification' interface CommandJustificationCardProps { app: string settings: AppSettingsResponse | undefined + // gatewayWide is the global require_command_justification, which requires one in every app. + gatewayWide: SettingsSetting | undefined disabled: boolean } -// CommandJustificationCard turns require_command_justification on or off. A change saves at once. +// CommandJustificationCard turns the app's require_command_justification on or off. A change saves at once. export function CommandJustificationCard({ app, settings, + gatewayWide, disabled, }: CommandJustificationCardProps) { const qc = useQueryClient() const setting = settings?.[settingKey] const required = getSettingValue(setting, false) + const requiredGatewayWide = getSettingValue(gatewayWide, false) const onSaved = async (message: string) => { await qc.invalidateQueries({ queryKey: ['appSettings', app] }) @@ -71,6 +76,12 @@ export function CommandJustificationCard({ records it with the command. A command run under a deploy approval is justified by the approval's message.

+ {requiredGatewayWide && ( +

+ The gateway-wide setting requires a justification for every command in every app, so + this app requires one whatever its own setting says. Change it on the Settings page. +

+ )}