From 04031bd761b82a5e26ff015def8972301e076642 Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Tue, 13 Oct 2026 14:44:33 +1300 Subject: [PATCH 1/2] fix: revoke sessions on CLI logout, audit seeding and MFA auto-locks, enforce forbidigo - `rack-gateway logout` ends the session on the gateway (new POST /api/v1/auth/cli/logout, bearer token only, audited fail-closed) before removing the rack; `--local` skips the gateway. (#127) - Startup seeding only creates missing users and never changes existing roles. Each created user is recorded as user.create by system in the same transaction, and the gateway won't start if it can't be. reset-db no longer seeds. (#128) - The MFA auto-lock writes user.lock by system in the same transaction as the lock (nothing is locked if it can't), and emails every unlocked admin as well as the user. (#129) - forbidigo patterns now match (they had parentheses, so never fired). Removed every time.Sleep and panic: timers with contexts, injectable clocks and explicit timestamps in tests, convoxguard.Run for TestMain. The gateway's PKCE verifier is now RFC 7636's 43 characters. Removed the unused ratelimit AuthEndpointsOnly. (#130) - SECURITY.md, and private vulnerability reporting on GitHub. (#132) Co-Authored-By: Claude Opus 5.5 --- .golangci.yml | 15 +- README.md | 2 + SECURITY.md | 35 ++++ cmd/mock-convox/handlers_logs.go | 3 - .../content/docs/concepts/rbac-principles.mdx | 4 +- .../configuration/environment-variables.mdx | 6 +- .../deployment/terraform/multi-region.mdx | 2 +- .../docs/development/api-reference.mdx | 1 + .../content/docs/development/contributing.mdx | 4 +- docs/src/content/docs/integrations/email.mdx | 6 +- docs/src/content/docs/integrations/index.mdx | 2 +- .../docs/operations/database-maintenance.mdx | 3 + .../docs/operations/troubleshooting.mdx | 4 +- .../security/authentication/api-tokens.mdx | 2 +- .../docs/security/authentication/index.mdx | 2 +- .../docs/security/authentication/sessions.mdx | 6 +- docs/src/content/docs/security/index.mdx | 9 +- .../docs/user-guide/cli/authentication.mdx | 7 +- .../content/docs/user-guide/cli/commands.mdx | 8 +- .../src/content/docs/user-guide/cli/index.mdx | 2 +- .../docs/user-guide/cli/multi-rack.mdx | 5 +- internal/cli/CLAUDE.md | 3 +- internal/cli/cli_logout.go | 87 ++++++++-- internal/cli/cli_logout_test.go | 65 +++++++ internal/cli/deploy_approvals_request.go | 22 ++- internal/cli/deploy_approvals_wait.go | 8 +- internal/cli/pause.go | 24 +++ internal/gateway/app/app.go | 6 +- internal/gateway/app/setup.go | 17 +- .../gateway/audit/deploy_approval_expiry.go | 35 ---- internal/gateway/audit/main_test.go | 10 +- internal/gateway/audit/system_entries.go | 54 ++++++ internal/gateway/auth/main_test.go | 10 +- internal/gateway/auth/mfa/auto_lock.go | 73 ++++++++ internal/gateway/auth/mfa/auto_lock_test.go | 99 +++++++++++ internal/gateway/auth/mfa/service.go | 35 ---- internal/gateway/auth/oauth.go | 75 ++++---- internal/gateway/auth/service_test.go | 8 +- .../db/audit_aggregation_merge_test.go | 6 +- internal/gateway/db/database.go | 11 +- internal/gateway/db/database_test.go | 34 +--- internal/gateway/db/seeds.go | 161 +++++++++--------- internal/gateway/db/seeds_test.go | 78 +++++++-- internal/gateway/db/user_lock.go | 79 +++++++++ internal/gateway/handlers/auth_cli_logout.go | 48 ++++++ internal/gateway/handlers/auth_helpers.go | 11 +- .../gateway/openapi/generated/swagger.json | 46 +++++ internal/gateway/ratelimit/ratelimit.go | 81 +++------ internal/gateway/ratelimit/ratelimit_test.go | 116 +++---------- internal/gateway/rbac/main_test.go | 10 +- internal/gateway/routes/cli_logout_test.go | 92 ++++++++++ internal/gateway/routes/main_test.go | 10 +- internal/gateway/routes/route_registration.go | 1 + .../testutil/convoxguard/convoxguard.go | 52 +++--- internal/gateway/testutil/dbtest/dbtest.go | 33 ++-- internal/gateway/testutil/dbtest/retry.go | 26 +++ internal/gateway/testutil/dbtest/template.go | 34 ++-- internal/integration/main_test.go | 10 +- main_test.go | 12 +- scripts/lib/cli-e2e/cli_helpers.sh | 2 +- web/src/api/generated.ts | 22 +++ web/src/api/openapi.json | 59 +++++++ .../api/schemas/handlersCLILogoutResponse.ts | 11 ++ web/src/api/schemas/index.ts | 1 + web/src/api/types.generated.ts | 71 ++++++++ 65 files changed, 1295 insertions(+), 581 deletions(-) create mode 100644 SECURITY.md create mode 100644 internal/cli/cli_logout_test.go create mode 100644 internal/cli/pause.go delete mode 100644 internal/gateway/audit/deploy_approval_expiry.go create mode 100644 internal/gateway/audit/system_entries.go create mode 100644 internal/gateway/auth/mfa/auto_lock.go create mode 100644 internal/gateway/auth/mfa/auto_lock_test.go create mode 100644 internal/gateway/db/user_lock.go create mode 100644 internal/gateway/handlers/auth_cli_logout.go create mode 100644 internal/gateway/routes/cli_logout_test.go create mode 100644 internal/gateway/testutil/dbtest/retry.go create mode 100644 web/src/api/schemas/handlersCLILogoutResponse.ts diff --git a/.golangci.yml b/.golangci.yml index 04a50799..4fd51ca2 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -41,6 +41,11 @@ linters: - gocognit - funlen - goconst + # Command-line programs print their output. + - path: '^(cmd|internal/cli)/' + linters: + - forbidigo + text: 'fmt\.Print' settings: gosec: # Exclude rules that produce false positives on manually-verified secure code: @@ -96,13 +101,15 @@ linters: line-length: 120 tab-width: 4 forbidigo: + # Patterns match the called identifier (e.g. `time.Sleep`), not source text, so they're anchored and have no + # parentheses. forbid: - - pattern: 'panic\(' + - pattern: '^panic$' msg: "panic is forbidden; return errors instead" - - pattern: 'fmt\.Print(f|ln)?\(' + - pattern: '^fmt\.Print(f|ln)?$' msg: "use structured logging instead of fmt.Print*" - - pattern: 'time\.Sleep\(' - msg: "avoid time.Sleep; use context with timeout" + - pattern: '^time\.Sleep$' + msg: "avoid time.Sleep; wait on a timer or ticker with a context" misspell: locale: US diff --git a/README.md b/README.md index 517568f6..ce6ab5a8 100644 --- a/README.md +++ b/README.md @@ -360,6 +360,8 @@ the Rack Gateway API are exposed to the public internet.** 5. **Audit Logs**: Monitor for anomalies and denied requests 6. **Session Management**: All sessions stored in database and revocable +To report a vulnerability, see [SECURITY.md](./SECURITY.md). Please don't open a public issue. + ## Testing ```bash diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 00000000..f8f73458 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,35 @@ +# Security Policy + +## Supported Versions + +Only the latest release of Rack Gateway gets security fixes. Upgrade to it before reporting, and check whether the +problem is still there. + +## Reporting a Vulnerability + +Please don't report security vulnerabilities in public GitHub issues, pull requests or discussions. + +Report them privately, either way: + +- **GitHub:** [Report a vulnerability](https://github.com/DocSpring/rack-gateway/security/advisories/new) on the + repository's Security tab +- **Email:** [security@docspring.com](mailto:security@docspring.com) + +Include: + +- The version (or commit) you tested +- What an attacker can do, and what they need first (an account, a role, network access) +- Steps or a proof of concept that reproduce it +- Any configuration it depends on + +We'll acknowledge your report, keep you updated while we fix it, and agree a disclosure date with you. We credit +reporters in the release notes and the advisory unless you'd rather we didn't. + +## Scope + +In scope: the gateway server, the `rack-gateway` CLI, the web UI and the release artifacts (Docker image, CLI +archives) published from this repository. + +Out of scope: Convox itself (report those to [Convox](https://github.com/convox/convox)), and problems that need a +misconfiguration the documentation warns against, such as exposing the gateway without TLS or running a test build +(`DEV_MODE`, `E2E_TEST_MODE`) in production. diff --git a/cmd/mock-convox/handlers_logs.go b/cmd/mock-convox/handlers_logs.go index 1f79ec84..897325dd 100644 --- a/cmd/mock-convox/handlers_logs.go +++ b/cmd/mock-convox/handlers_logs.go @@ -19,7 +19,6 @@ func appLogs(w http.ResponseWriter, r *http.Request) { }() _ = c.WriteMessage(websocket.TextMessage, []byte("Promoting release...\n")) - time.Sleep(100 * time.Millisecond) _ = c.WriteMessage(websocket.TextMessage, []byte("Release promoted successfully.\n")) ping := time.NewTicker(30 * time.Second) @@ -57,9 +56,7 @@ func buildLogs(w http.ResponseWriter, r *http.Request) { }() _ = c.WriteMessage(websocket.TextMessage, []byte("Building app...\n")) - time.Sleep(100 * time.Millisecond) _ = c.WriteMessage(websocket.TextMessage, []byte("Step 1/1: mock build step\n")) - time.Sleep(100 * time.Millisecond) _ = c.WriteMessage(websocket.TextMessage, []byte("Build complete\n")) _ = c.WriteMessage(websocket.CloseMessage, websocket.FormatCloseMessage(websocket.CloseNormalClosure, "")) } diff --git a/docs/src/content/docs/concepts/rbac-principles.mdx b/docs/src/content/docs/concepts/rbac-principles.mdx index e3861ec9..9c0275a8 100644 --- a/docs/src/content/docs/concepts/rbac-principles.mdx +++ b/docs/src/content/docs/concepts/rbac-principles.mdx @@ -269,8 +269,8 @@ Wildcards appear only in the admin role; see [Permissions](/security/rbac/permis Roles are assigned through: -1. **Seed lists**: users in `ADMIN_USERS`, `DEPLOYER_USERS`, `OPERATIONS_USERS` and `VIEWER_USERS` are created - with that role at startup +1. **Seed lists**: users in `ADMIN_USERS`, `DEPLOYER_USERS`, `OPERATIONS_USERS` and `VIEWER_USERS` that don't exist + yet are created with that role at startup (audit-logged; existing users are left alone) 2. **Web UI**: Admins add users and assign their roles 3. **API tokens**: Tokens get individual permissions (role shortcuts fill them in), and can only use the ones their owner's current role allows diff --git a/docs/src/content/docs/configuration/environment-variables.mdx b/docs/src/content/docs/configuration/environment-variables.mdx index 3b43c635..af896a3f 100644 --- a/docs/src/content/docs/configuration/environment-variables.mdx +++ b/docs/src/content/docs/configuration/environment-variables.mdx @@ -102,8 +102,10 @@ but the upgrade sweep encrypts any it finds (see **Existing data is upgraded at | `DEPLOYER_USERS` | Comma-separated deployer emails | | `OPERATIONS_USERS` | Comma-separated ops emails | -Every start creates the listed users that don't exist yet, and resets each listed user's roles to that list's role, -so a role you change in the web UI for a seeded user reverts at the next start. Seeding writes no audit entries. +Every start creates the listed users that don't exist yet, with the role of each list that names them. Existing +users are never changed, so a role you change in the web UI stays changed; to take a seeded user's access away, lock +or delete them in the web UI and remove them from the list. Each user it creates is recorded in the audit log as +`user.create` by `system`, in the same transaction, and the gateway doesn't start if that record can't be written. ## Convox Rack diff --git a/docs/src/content/docs/deployment/terraform/multi-region.mdx b/docs/src/content/docs/deployment/terraform/multi-region.mdx index fece9b7e..6a948b14 100644 --- a/docs/src/content/docs/deployment/terraform/multi-region.mdx +++ b/docs/src/content/docs/deployment/terraform/multi-region.mdx @@ -441,7 +441,7 @@ Each gateway has its own users: signing in never creates one. Either: 1. **Add them on each gateway** - an admin adds users and roles on its **Users** page 2. **Seed them from configuration** - set the same `ADMIN_USERS`, `DEPLOYER_USERS`, `OPERATIONS_USERS` and `VIEWER_USERS` on every gateway (for example from one Terraform variable). Each start creates missing users - and resets listed users' roles to the list's role + with the list's role, and leaves existing users alone ## Disaster Recovery Runbook diff --git a/docs/src/content/docs/development/api-reference.mdx b/docs/src/content/docs/development/api-reference.mdx index b130585b..3f7d24c8 100644 --- a/docs/src/content/docs/development/api-reference.mdx +++ b/docs/src/content/docs/development/api-reference.mdx @@ -67,6 +67,7 @@ CLI login uses the RFC 8252 loopback flow (see [OAuth Flow](/security/authentica - `POST /auth/cli/cancel` - cancels the login from the bound browser; returns the loopback URL that tells the CLI - `POST /auth/cli/complete` - redeems the login code with the CLI's code verifier for a session token - `GET /auth/cli/initiator` - the device name and IP that started the login, shown on the approval page +- `POST /auth/cli/logout` - revokes the CLI session sent as the bearer token (cookies are ignored) - `GET /auth/web/login` (also supports `HEAD`) - `GET /auth/web/callback` - `GET /auth/web/logout` diff --git a/docs/src/content/docs/development/contributing.mdx b/docs/src/content/docs/development/contributing.mdx index 109eeb8a..a1637fb6 100644 --- a/docs/src/content/docs/development/contributing.mdx +++ b/docs/src/content/docs/development/contributing.mdx @@ -267,7 +267,9 @@ Why is this change needed? Do NOT open public issues for security vulnerabilities. -Email security concerns to: security@docspring.com +Report them privately: [report a vulnerability](https://github.com/DocSpring/rack-gateway/security/advisories/new) +on GitHub, or email [security@docspring.com](mailto:security@docspring.com). See the +[security policy](https://github.com/DocSpring/rack-gateway/blob/main/SECURITY.md). ### Secure Coding Practices diff --git a/docs/src/content/docs/integrations/email.mdx b/docs/src/content/docs/integrations/email.mdx index fb0f5e71..429b45d0 100644 --- a/docs/src/content/docs/integrations/email.mdx +++ b/docs/src/content/docs/integrations/email.mdx @@ -186,7 +186,8 @@ Sent when a user account is locked. #### MFA Auto-Lock -Sent when an account is locked automatically after 5 failed MFA attempts in 5 minutes. +Sent when an account is locked automatically after 5 failed MFA attempts in 5 minutes. The lock is recorded in +the audit log as `user.lock` by `system`. | Field | Content | |-------|---------| @@ -194,6 +195,9 @@ Sent when an account is locked automatically after 5 failed MFA attempts in 5 mi | To | Locked user email | | Content | Reason, contact instructions | +Every admin who isn't locked also gets **User locked: \{email\}**, asking them to check with the user before +unlocking the account. + ### API Tokens When an API token is created, its owner gets **New API token created** (the token's name and who created it), diff --git a/docs/src/content/docs/integrations/index.mdx b/docs/src/content/docs/integrations/index.mdx index 42056f7f..2c047598 100644 --- a/docs/src/content/docs/integrations/index.mdx +++ b/docs/src/content/docs/integrations/index.mdx @@ -136,7 +136,7 @@ Every audit log entry can be routed to Slack with an action pattern; the default | Category | Examples | Email | |----------|----------|-------| | **Authentication** | New CLI login, failed login | The user | -| **MFA** | Failed verification, automatic lock after 5 failures | The user | +| **MFA** | Failed verification, automatic lock after 5 failures | The user; admins when an account is locked | | **User Management** | User added, locked, unlocked | The user; admins when a user is added | | **API Tokens** | Token created | The owner and the other admins | | **Rack Parameters** | Parameters changed | Admins | diff --git a/docs/src/content/docs/operations/database-maintenance.mdx b/docs/src/content/docs/operations/database-maintenance.mdx index a9ae1b75..8555b49a 100644 --- a/docs/src/content/docs/operations/database-maintenance.mdx +++ b/docs/src/content/docs/operations/database-maintenance.mdx @@ -47,6 +47,9 @@ export DEV_MODE=true # development (testbypass) builds only rack-gateway-api reset-db ``` +It drops every table and re-runs the migrations. The gateway creates the +[seed users](/configuration/environment-variables/#seed-users) again when it next starts. + diff --git a/docs/src/content/docs/operations/troubleshooting.mdx b/docs/src/content/docs/operations/troubleshooting.mdx index 50f69946..fd5f2795 100644 --- a/docs/src/content/docs/operations/troubleshooting.mdx +++ b/docs/src/content/docs/operations/troubleshooting.mdx @@ -141,7 +141,9 @@ to be redirected there with a single-use login code. ### Account Locked After Failed MFA -Five failed MFA attempts in five minutes lock the account. An admin unlocks it on the Users page. +Five failed MFA attempts in five minutes lock the account. The admins are emailed, and the lock is in the audit log +as `user.lock` by `system`. An admin unlocks it on the Users page once they've confirmed the attempts were the +user's. ### Lost MFA Device diff --git a/docs/src/content/docs/security/authentication/api-tokens.mdx b/docs/src/content/docs/security/authentication/api-tokens.mdx index b2735611..a2898a15 100644 --- a/docs/src/content/docs/security/authentication/api-tokens.mdx +++ b/docs/src/content/docs/security/authentication/api-tokens.mdx @@ -16,7 +16,7 @@ API tokens provide authentication for automated systems like CI/CD pipelines, sc | **Lifetime** | Until deleted or the optional `expires_at` | Web: idle timeout; CLI: 90 days from last use | | **MFA** | Not applicable | Supported | | **CSRF** | Not required | Required | -| **Revocation** | Manual delete | Web logout, or revoking it from the user's sessions | +| **Revocation** | Manual delete | Logout (web or `rack-gateway logout`), or revoking it from the user's sessions | ## Token Anatomy diff --git a/docs/src/content/docs/security/authentication/index.mdx b/docs/src/content/docs/security/authentication/index.mdx index eccf75f5..511ae1a4 100644 --- a/docs/src/content/docs/security/authentication/index.mdx +++ b/docs/src/content/docs/security/authentication/index.mdx @@ -164,7 +164,7 @@ See [MFA Overview](/user-guide/mfa/) for setup details. |---------------|-----------------|------------------------| | **Creation** | OAuth flow | Created in the web UI or with `rack-gateway api-token create` (deployers for themselves, admins for anyone) | | **Lifetime** | Web: idle timeout; CLI: 90 days from last use | Until deleted or its optional expiry | -| **Revocation** | Web logout, or revoking it from the user's sessions | Delete token | +| **Revocation** | Logout (web or `rack-gateway logout`), or revoking it from the user's sessions | Delete token | | **MFA** | Required | N/A (limited by permissions instead) | | **CSRF** | Required (web) | Not required | | **Use Case** | Interactive access | CI/CD, scripts | diff --git a/docs/src/content/docs/security/authentication/sessions.mdx b/docs/src/content/docs/security/authentication/sessions.mdx index e9c8edd2..3cd04475 100644 --- a/docs/src/content/docs/security/authentication/sessions.mdx +++ b/docs/src/content/docs/security/authentication/sessions.mdx @@ -154,9 +154,9 @@ Sessions can be revoked through multiple mechanisms: ### User-Initiated Logout -**Logout** in the web UI's sidebar revokes the web session. `rack-gateway logout` only removes the rack and its -token from the CLI's config; it doesn't revoke the CLI session on the gateway, so revoke that from your sessions -(below). +**Logout** in the web UI's sidebar revokes the web session. `rack-gateway logout` revokes the CLI session +(`POST /api/v1/auth/cli/logout` with the session token), then removes the rack and its token from the CLI's config. +Both are audited before the session is revoked, and refused if the audit log can't record them. ### Revoking Sessions diff --git a/docs/src/content/docs/security/index.mdx b/docs/src/content/docs/security/index.mdx index 8de68a37..31bb0834 100644 --- a/docs/src/content/docs/security/index.mdx +++ b/docs/src/content/docs/security/index.mdx @@ -150,8 +150,11 @@ See [Production Checklist](/deployment/production-checklist/) for complete deplo If you discover a security vulnerability in Rack Gateway: 1. **Do not** open a public GitHub issue -2. Email security concerns to the maintainers -3. Include detailed reproduction steps +2. Report it privately: [report a vulnerability](https://github.com/DocSpring/rack-gateway/security/advisories/new) + on GitHub, or email [security@docspring.com](mailto:security@docspring.com) +3. Include the version, what an attacker can do, and steps that reproduce it 4. Allow time for a fix before public disclosure -We appreciate responsible disclosure and will acknowledge security researchers in release notes. +Only the latest release gets security fixes. See the repository's +[security policy](https://github.com/DocSpring/rack-gateway/blob/main/SECURITY.md) for details. We credit reporters +in the release notes. diff --git a/docs/src/content/docs/user-guide/cli/authentication.mdx b/docs/src/content/docs/user-guide/cli/authentication.mdx index cc12092b..90ee8fca 100644 --- a/docs/src/content/docs/user-guide/cli/authentication.mdx +++ b/docs/src/content/docs/user-guide/cli/authentication.mdx @@ -138,9 +138,10 @@ When your session has expired or been revoked, commands fail with an authenticat rack-gateway logout ``` -This removes the rack, with its URL and session token, from your local config. It doesn't revoke the session on -the gateway: to end it there, revoke it under your account's sessions in the web UI. To use the rack again, run -`rack-gateway login `. +This ends the session on the gateway (a copy of the token stops working), then removes the rack, with its URL and +session token, from your local config. If the gateway can't be reached, nothing is removed and the command fails; +`rack-gateway logout --local` removes the rack anyway, and you can revoke the session later under your account's +sessions in the web UI. To use the rack again, run `rack-gateway login `. ## Using API Tokens diff --git a/docs/src/content/docs/user-guide/cli/commands.mdx b/docs/src/content/docs/user-guide/cli/commands.mdx index f679f9ae..1022a1b9 100644 --- a/docs/src/content/docs/user-guide/cli/commands.mdx +++ b/docs/src/content/docs/user-guide/cli/commands.mdx @@ -48,13 +48,17 @@ rack-gateway login # sign in again to the current rack ### logout -Remove the rack, with its URL and session token, from your local config. This doesn't revoke the session on the -gateway: revoke it under your sessions in the web UI. +End the rack's session on the gateway, then remove the rack, with its URL and session token, from your local +config. If the gateway can't be reached or doesn't end the session, the rack stays configured and the command fails. ```bash rack-gateway logout [flags] ``` +**Flags:** +- `--local` - Only remove the rack from this machine. The session stays valid on the gateway until it expires or + you revoke it under your sessions in the web UI. + Use the global `--rack` flag to log out of another rack. **Example:** diff --git a/docs/src/content/docs/user-guide/cli/index.mdx b/docs/src/content/docs/user-guide/cli/index.mdx index ac59e95f..6ab6f407 100644 --- a/docs/src/content/docs/user-guide/cli/index.mdx +++ b/docs/src/content/docs/user-guide/cli/index.mdx @@ -94,7 +94,7 @@ You can override this with the `--config` flag or `GATEWAY_CLI_CONFIG_DIR` envir | Command | Description | |---------|-------------| | `login [rack] [url]` | Login to a gateway via OAuth (no arguments: sign in again to the current rack) | -| `logout` | Logout from the current rack | +| `logout` | End the session on the gateway and remove the rack | | `test-auth` | Test authentication (optionally with MFA) | ### Rack Management diff --git a/docs/src/content/docs/user-guide/cli/multi-rack.mdx b/docs/src/content/docs/user-guide/cli/multi-rack.mdx index 2151c7ed..289ad249 100644 --- a/docs/src/content/docs/user-guide/cli/multi-rack.mdx +++ b/docs/src/content/docs/user-guide/cli/multi-rack.mdx @@ -122,13 +122,14 @@ Your rack configurations are stored in `~/.config/rack-gateway/config.json`: ## Removing a Rack -`logout` removes a rack, with its URL and session token, from your config: +`logout` ends your session on that rack's gateway, then removes the rack, with its URL and session token, from your +config: ```bash rack-gateway logout -r staging ``` -It doesn't revoke the session on that gateway; revoke it under your sessions in the web UI. +If the gateway is gone for good, `rack-gateway logout -r staging --local` removes the rack without contacting it. ## Updating a Gateway URL diff --git a/internal/cli/CLAUDE.md b/internal/cli/CLAUDE.md index e43b8bbd..9e274e27 100644 --- a/internal/cli/CLAUDE.md +++ b/internal/cli/CLAUDE.md @@ -135,7 +135,8 @@ Opens browser for OAuth flow, stores session token in config. rack-gateway logout ``` -Removes current rack from config. +Ends the session on the gateway (`POST /api/v1/auth/cli/logout`), then removes the rack from config. +`--local` skips the gateway call. ### Convox Commands diff --git a/internal/cli/cli_logout.go b/internal/cli/cli_logout.go index 9e363727..331fad58 100644 --- a/internal/cli/cli_logout.go +++ b/internal/cli/cli_logout.go @@ -2,33 +2,92 @@ package cli import ( "fmt" + "net/http" + "strings" "github.com/spf13/cobra" ) -// LogoutCommand creates a cobra command for logging out from the current rack +// LogoutCommand creates a cobra command that ends the session on the gateway, then removes the rack from the +// local config. func LogoutCommand() *cobra.Command { - return &cobra.Command{ + var localOnly bool + cmd := &cobra.Command{ Use: "logout", - Short: "Logout from the current rack", + Short: "Log out of the current rack: end the session on the gateway and remove the rack", + Long: "Ends the rack's session on the gateway, then removes the rack (its URL and session token) from " + + "this machine's config. If the gateway can't end the session, nothing is removed; --local removes " + + "the rack anyway, leaving the session valid until it expires or is revoked in the web UI.", RunE: func(_ *cobra.Command, _ []string) error { rack, err := SelectedRack() if err != nil { return err } + return logout(rack, localOnly) + }, + } + cmd.Flags().BoolVar(&localOnly, "local", false, + "only remove the rack from this machine, without ending the session on the gateway") + return cmd +} - removed, err := RemoveRack(rack) - if err != nil { - return fmt.Errorf("failed to remove rack: %w", err) - } +func logout(rack string, localOnly bool) error { + gateway, found, err := configuredGateway(rack) + if err != nil { + return err + } + if !found { + fmt.Printf("Rack %s not found\n", rack) + return nil + } - if !removed { - fmt.Printf("Rack %s not found\n", rack) - return nil - } + outcome := "the session was not ended on the gateway; revoke it in the web UI" + if !localOnly { + outcome, err = endGatewaySession(gateway) + if err != nil { + return fmt.Errorf("%w\nThe rack is still configured. To remove it from this machine anyway, run "+ + "'rack-gateway logout --local --rack %s', then revoke the session in the web UI", err, rack) + } + } - fmt.Printf("✓ Logged out from %s\n", rack) - return nil - }, + if _, err := RemoveRack(rack); err != nil { + return fmt.Errorf("failed to remove rack: %w", err) + } + fmt.Printf("✓ Logged out from %s (%s)\n", rack, outcome) + return nil +} + +func configuredGateway(rack string) (GatewayConfig, bool, error) { + cfg, exists, err := LoadConfig() + if err != nil || !exists { + return GatewayConfig{}, false, err + } + gateway, ok := cfg.Gateways[rack] + return gateway, ok, nil +} + +// endGatewaySession revokes the rack's session on its gateway and describes what happened. A session the +// gateway no longer accepts (expired, or already revoked) needs nothing more. +func endGatewaySession(gateway GatewayConfig) (string, error) { + token := strings.TrimSpace(gateway.Token) + if token == "" { + return "there was no session", nil + } + gatewayURL, err := NormalizeGatewayURL(gateway.URL) + if err != nil { + return "", err + } + + status, body, err := doGatewayRequest(gatewayURL, token, http.MethodPost, "/auth/cli/logout", nil) + if err != nil { + return "", fmt.Errorf("couldn't reach %s to end the session: %w", gatewayURL, err) + } + switch status { + case http.StatusOK: + return "the session is ended on the gateway", nil + case http.StatusUnauthorized: + return "the session had already ended", nil + default: + return "", fmt.Errorf("%s didn't end the session: %w", gatewayURL, gatewayResponseError(status, body)) } } diff --git a/internal/cli/cli_logout_test.go b/internal/cli/cli_logout_test.go new file mode 100644 index 00000000..94ca4679 --- /dev/null +++ b/internal/cli/cli_logout_test.go @@ -0,0 +1,65 @@ +package cli + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// fakeLogoutGateway answers POST /api/v1/auth/cli/logout with status, records whether it was called, and +// configures the "staging" rack to use it. +func fakeLogoutGateway(t *testing.T, status int) *bool { + t.Helper() + called := false + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost || r.URL.Path != "/api/v1/auth/cli/logout" || + r.Header.Get("Authorization") != "Bearer session-token" { + w.WriteHeader(http.StatusNotFound) + return + } + called = true + w.WriteHeader(status) + _, _ = w.Write([]byte(`{"error":"audit log unavailable; refusing to perform logout"}`)) + })) + t.Cleanup(server.Close) + + withCLIConfig(t, Config{Current: "staging", Gateways: map[string]GatewayConfig{ + "staging": {URL: server.URL, Token: "session-token", ExpiresAt: time.Now().Add(time.Hour)}, + }}) + return &called +} + +func rackConfigured(t *testing.T, rack string) bool { + t.Helper() + _, found, err := configuredGateway(rack) + require.NoError(t, err) + return found +} + +func TestLogoutEndsTheSessionThenRemovesTheRack(t *testing.T) { + for _, status := range []int{http.StatusOK, http.StatusUnauthorized} { + called := fakeLogoutGateway(t, status) + require.NoError(t, logout("staging", false)) + require.True(t, *called) + require.False(t, rackConfigured(t, "staging"), "status %d: the rack must be removed", status) + } +} + +func TestLogoutKeepsTheRackWhenTheGatewayRefuses(t *testing.T) { + called := fakeLogoutGateway(t, http.StatusServiceUnavailable) + err := logout("staging", false) + require.ErrorContains(t, err, "audit log unavailable") + require.ErrorContains(t, err, "rack-gateway logout --local --rack staging") + require.True(t, *called) + require.True(t, rackConfigured(t, "staging"), "a session the gateway didn't end must stay configured") +} + +func TestLogoutLocalSkipsTheGateway(t *testing.T) { + called := fakeLogoutGateway(t, http.StatusOK) + require.NoError(t, logout("staging", true)) + require.False(t, *called) + require.False(t, rackConfigured(t, "staging")) +} diff --git a/internal/cli/deploy_approvals_request.go b/internal/cli/deploy_approvals_request.go index ca6a08db..e820b3bd 100644 --- a/internal/cli/deploy_approvals_request.go +++ b/internal/cli/deploy_approvals_request.go @@ -290,18 +290,26 @@ func waitForDeployApproval( return &result, nil } - if statusLower != lastStatus { - err := writef(cmd.OutOrStdout(), "Waiting for approval (status: %s)\n", displayText(result.Status)) - if err != nil { - return nil, err - } - lastStatus = statusLower + if err := reportApprovalStatus(cmd, &lastStatus, result.Status); err != nil { + return nil, err } if timeout > 0 && time.Since(start) > timeout { return nil, fmt.Errorf("timed out waiting for approval") } - time.Sleep(interval) + if err := pause(cmd, interval); err != nil { + return nil, err + } + } +} + +// reportApprovalStatus prints the request's status when it isn't the last one printed. +func reportApprovalStatus(cmd *cobra.Command, last *string, status string) error { + lower := strings.ToLower(status) + if lower == *last { + return nil } + *last = lower + return writef(cmd.OutOrStdout(), "Waiting for approval (status: %s)\n", displayText(status)) } diff --git a/internal/cli/deploy_approvals_wait.go b/internal/cli/deploy_approvals_wait.go index 08a27ec0..82fbdb99 100644 --- a/internal/cli/deploy_approvals_wait.go +++ b/internal/cli/deploy_approvals_wait.go @@ -134,7 +134,9 @@ func runDeployApprovalWait(cmd *cobra.Command, cfg deployApprovalWaitConfig) err if done { return waiter.printSummary() } - waiter.sleep() + if err := pause(waiter.cmd, waiter.pollInterval); err != nil { + return err + } } } @@ -396,7 +398,3 @@ func (w *deployApprovalWaiter) writeRequestSummary(rack string, request deployAp } return printDeployApprovalDetails(out, &request, rack, true) } - -func (w *deployApprovalWaiter) sleep() { - time.Sleep(w.pollInterval) -} diff --git a/internal/cli/pause.go b/internal/cli/pause.go new file mode 100644 index 00000000..dd44e090 --- /dev/null +++ b/internal/cli/pause.go @@ -0,0 +1,24 @@ +package cli + +import ( + "context" + "time" + + "github.com/spf13/cobra" +) + +// pause waits for d between polls, returning early with the error when the command's context ends. +func pause(cmd *cobra.Command, d time.Duration) error { + ctx := context.Background() + if cmd != nil && cmd.Context() != nil { + ctx = cmd.Context() + } + timer := time.NewTimer(d) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } +} diff --git a/internal/gateway/app/app.go b/internal/gateway/app/app.go index 36f340ff..1658d082 100644 --- a/internal/gateway/app/app.go +++ b/internal/gateway/app/app.go @@ -65,8 +65,10 @@ func New() (*App, error) { return nil, errors.Join(err, database.Close()) } - // Set up router - app.setupRouter() + if err := app.setupRouter(); err != nil { + app.Cleanup() + return nil, err + } return app, nil } diff --git a/internal/gateway/app/setup.go b/internal/gateway/app/setup.go index e6df0069..7520fb5b 100644 --- a/internal/gateway/app/setup.go +++ b/internal/gateway/app/setup.go @@ -105,13 +105,20 @@ func (a *App) ensureAuditSecret() error { return nil } +// seedInitialUsers creates the seed lists' missing users, audit-logged in the same transaction. The gateway doesn't +// start if they can't be recorded. func (a *App) seedInitialUsers() error { - return a.Database.SeedDatabase(&db.SeedConfig{ + events, err := a.Database.SeedDatabase(&db.SeedConfig{ AdminUsers: a.Config.AdminUsers, ViewerUsers: a.Config.ViewerUsers, DeployerUsers: a.Config.DeployerUsers, OperationsUsers: a.Config.OperationsUsers, - }) + }, audit.SeededUserEntry) + audit.RecordedInTransaction(events, err) + if err != nil { + return fmt.Errorf("failed to seed users: %w", err) + } + return nil } // initSettingsService refuses to start on an invalid setting environment variable, and logs stored settings that @@ -398,15 +405,14 @@ func (a *App) initProxyHandler(auditLogger *audit.Logger, rackName, rackAlias st } // setupRouter configures the Gin router with all routes and middleware -func (a *App) setupRouter() { +func (a *App) setupRouter() error { // Always use release mode to suppress route list printing gin.SetMode(gin.ReleaseMode) // Create router without default middleware (we'll add our own) router := gin.New() if err := router.SetTrustedProxies(a.Config.TrustedProxies); err != nil { - log.Printf("failed to configure trusted proxies: %v", err) - panic(fmt.Sprintf("failed to configure trusted proxies: %v", err)) + return fmt.Errorf("failed to configure trusted proxies: %w", err) } // Only X-Forwarded-For, and only from TRUSTED_PROXY_CIDRS (see middleware.ClientIP). router.RemoteIPHeaders = []string{"X-Forwarded-For"} @@ -422,4 +428,5 @@ func (a *App) setupRouter() { }) a.router = router + return nil } diff --git a/internal/gateway/audit/deploy_approval_expiry.go b/internal/gateway/audit/deploy_approval_expiry.go deleted file mode 100644 index c2381ea6..00000000 --- a/internal/gateway/audit/deploy_approval_expiry.go +++ /dev/null @@ -1,35 +0,0 @@ -package audit - -import ( - "encoding/json" - "fmt" - - "github.com/DocSpring/rack-gateway/internal/gateway/db" - "github.com/DocSpring/rack-gateway/internal/gateway/rbac" -) - -// SystemActor is the audit actor for changes the gateway makes by itself, such as expiring deploy approvals. -const SystemActor = "system" - -// DeployApprovalExpiredEntry is the audit event for a deploy approval request the expiry sweep expired. -func DeployApprovalExpiredEntry(expired db.ExpiredDeployApproval) *db.AuditLog { - details, err := json.Marshal(map[string]string{ - "public_id": expired.PublicID, - "app": expired.App, - "git_commit_hash": expired.GitCommitHash, - "previous_status": expired.PreviousStatus, - }) - if err != nil { - details = []byte("{}") - } - return &db.AuditLog{ - UserEmail: SystemActor, - UserName: SystemActor, - ActionType: ActionTypeGateway, - Action: BuildAction(rbac.ResourceDeployApprovalRequest.String(), "expire"), - ResourceType: rbac.ResourceDeployApprovalRequest.String(), - Resource: fmt.Sprintf("%d", expired.ID), - Details: string(details), - Status: StatusSuccess, - } -} diff --git a/internal/gateway/audit/main_test.go b/internal/gateway/audit/main_test.go index 80bcd0b7..528eff54 100644 --- a/internal/gateway/audit/main_test.go +++ b/internal/gateway/audit/main_test.go @@ -8,13 +8,5 @@ import ( ) func TestMain(m *testing.M) { - cleanup, err := convoxguard.Setup() - if err != nil { - panic(err) - } - code := m.Run() - if err := cleanup(); err != nil { - panic("CRITICAL: " + err.Error()) - } - os.Exit(code) + os.Exit(convoxguard.Run(m)) } diff --git a/internal/gateway/audit/system_entries.go b/internal/gateway/audit/system_entries.go new file mode 100644 index 00000000..89223d95 --- /dev/null +++ b/internal/gateway/audit/system_entries.go @@ -0,0 +1,54 @@ +package audit + +import ( + "encoding/json" + "fmt" + + "github.com/DocSpring/rack-gateway/internal/gateway/db" + "github.com/DocSpring/rack-gateway/internal/gateway/rbac" +) + +// SystemActor is the audit actor for changes the gateway makes by itself, such as expiring deploy approvals. +const SystemActor = "system" + +// systemEntry is a successful change the gateway made by itself. +func systemEntry(resourceType rbac.Resource, verb, resource string, details map[string]interface{}) *db.AuditLog { + detailsJSON, err := json.Marshal(details) + if err != nil { + detailsJSON = []byte("{}") + } + return &db.AuditLog{ + UserEmail: SystemActor, + UserName: SystemActor, + ActionType: ActionTypeGateway, + Action: BuildAction(resourceType.String(), verb), + ResourceType: resourceType.String(), + Resource: resource, + Details: string(detailsJSON), + Status: StatusSuccess, + } +} + +// DeployApprovalExpiredEntry is the audit event for a deploy approval request the expiry sweep expired. +func DeployApprovalExpiredEntry(expired db.ExpiredDeployApproval) *db.AuditLog { + return systemEntry(rbac.ResourceDeployApprovalRequest, "expire", fmt.Sprintf("%d", expired.ID), + map[string]interface{}{ + "public_id": expired.PublicID, + "app": expired.App, + "git_commit_hash": expired.GitCommitHash, + "previous_status": expired.PreviousStatus, + }) +} + +// SeededUserEntry is the audit event for a user the startup seed created from the seed lists (ADMIN_USERS etc.). +func SeededUserEntry(user db.SeededUser) *db.AuditLog { + return systemEntry(rbac.ResourceUser, rbac.ActionCreate.String(), user.Email, + map[string]interface{}{"roles": user.Roles, "source": "seed"}) +} + +// UserAutoLockedEntry is the audit event for a user the gateway locked by itself (after too many failed MFA +// attempts). +func UserAutoLockedEntry(user db.LockedUser) *db.AuditLog { + return systemEntry(rbac.ResourceUser, ActionVerbLock, user.Email, + map[string]interface{}{"reason": user.Reason, "target_user": user.Email}) +} diff --git a/internal/gateway/auth/main_test.go b/internal/gateway/auth/main_test.go index 16018c53..cc0ec836 100644 --- a/internal/gateway/auth/main_test.go +++ b/internal/gateway/auth/main_test.go @@ -8,13 +8,5 @@ import ( ) func TestMain(m *testing.M) { - cleanup, err := convoxguard.Setup() - if err != nil { - panic(err) - } - code := m.Run() - if err := cleanup(); err != nil { - panic("CRITICAL: " + err.Error()) - } - os.Exit(code) + os.Exit(convoxguard.Run(m)) } diff --git a/internal/gateway/auth/mfa/auto_lock.go b/internal/gateway/auth/mfa/auto_lock.go new file mode 100644 index 00000000..1904ca31 --- /dev/null +++ b/internal/gateway/auth/mfa/auto_lock.go @@ -0,0 +1,73 @@ +package mfa + +import ( + "errors" + "fmt" + "html" + "log" + + "github.com/DocSpring/rack-gateway/internal/gateway/audit" + "github.com/DocSpring/rack-gateway/internal/gateway/db" +) + +const ( + autoLockAttempts = 5 + autoLockMinutes = 5 +) + +var ( + autoLockReason = fmt.Sprintf("Automatic lock: %d failed MFA attempts in %d minutes", autoLockAttempts, + autoLockMinutes) + errAccountLocked = errors.New("account locked due to multiple failed attempts - contact administrator") +) + +// checkAndLockAccount locks the account after autoLockAttempts failed MFA attempts in autoLockMinutes. The lock is +// audit-logged as a user.lock by the system in the same transaction; then the user and the admins are emailed. +func (s *Service) checkAndLockAccount(userID int64) error { + failedAttempts, err := s.db.CountRecentFailedMFAAttempts(userID, autoLockMinutes) + if err != nil { + return fmt.Errorf("failed to count failed attempts: %w", err) + } + if failedAttempts < autoLockAttempts { + return nil + } + + locked, events, err := s.db.LockUserAudited(userID, autoLockReason, audit.UserAutoLockedEntry) + audit.RecordedInTransaction(events, err) + if err != nil { + return fmt.Errorf("failed to lock account: %w", err) + } + if locked != nil { + s.sendAutoLockEmails(locked) + } + return errAccountLocked +} + +func (s *Service) sendAutoLockEmails(locked *db.LockedUser) { + if s.emailSender == nil { + return + } + reason := fmt.Sprintf("%d failed MFA attempts in %d minutes", autoLockAttempts, autoLockMinutes) + _ = s.emailSender.Send(locked.Email, "Account Locked - Multiple Failed Login Attempts", + "Your account has been automatically locked due to multiple failed authentication attempts.\n\n"+ + "Reason: "+reason+"\n\nIf this was not you, please contact your administrator immediately.", + `

Your account has been automatically locked due to multiple failed authentication `+ + `attempts.

Reason: `+reason+`

`+ + `

If this was not you, please contact your administrator `+ + `immediately.

`) + + admins, err := s.db.UnlockedAdminEmails() + if err != nil { + log.Printf("mfa: couldn't email admins about the automatic lock of %s: %v", locked.Email, err) + return + } + if len(admins) == 0 { + return + } + _ = s.emailSender.SendMany(admins, "User locked: "+locked.Email, + locked.Email+" was locked automatically after "+reason+".\n\n"+ + "Check with them before unlocking the account on the Users page: the attempts may not have been theirs.", + "

"+html.EscapeString(locked.Email)+" was locked automatically after "+reason+".

"+ + "

Check with them before unlocking the account on the Users page: the attempts may not have been "+ + "theirs.

") +} diff --git a/internal/gateway/auth/mfa/auto_lock_test.go b/internal/gateway/auth/mfa/auto_lock_test.go new file mode 100644 index 00000000..158c399f --- /dev/null +++ b/internal/gateway/auth/mfa/auto_lock_test.go @@ -0,0 +1,99 @@ +package mfa + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + + "github.com/DocSpring/rack-gateway/internal/gateway/audit" + "github.com/DocSpring/rack-gateway/internal/gateway/db" + "github.com/DocSpring/rack-gateway/internal/gateway/testutil/dbtest" +) + +type sentEmail struct { + to []string + subject string +} + +type recordingSender struct{ sent []sentEmail } + +func (r *recordingSender) Send(to, subject, textBody, htmlBody string) error { + return r.SendMany([]string{to}, subject, textBody, htmlBody) +} + +func (r *recordingSender) SendMany(to []string, subject, _, _ string) error { + r.sent = append(r.sent, sentEmail{to: to, subject: subject}) + return nil +} + +// autoLockFixture is a viewer about to fail MFA, an admin, and a locked admin. +func autoLockFixture(t *testing.T) (*Service, *db.Database, *db.User, *recordingSender) { + t.Helper() + database := dbtest.NewDatabase(t) + sender := &recordingSender{} + svc, err := NewService(database, "Test", nil, []byte("test-pepper"), "", "", "", "", sender) + require.NoError(t, err) + + viewer, err := database.CreateUser("viewer@example.com", "Viewer", []string{"viewer"}) + require.NoError(t, err) + _, err = database.CreateUser("admin@example.com", "Admin", []string{"admin"}) + require.NoError(t, err) + lockedAdmin, err := database.CreateUser("locked-admin@example.com", "Locked Admin", []string{"admin"}) + require.NoError(t, err) + require.NoError(t, database.LockUser(lockedAdmin.ID, "away", nil)) + return svc, database, viewer, sender +} + +func failTOTP(svc *Service, user *db.User, times int) error { + var err error + for i := 0; i < times; i++ { + _, err = svc.VerifyTOTP(user, "000000", "1.2.3.4", "test-agent", nil) + } + return err +} + +// The automatic lock is recorded as a user.lock by the system, and the user and every unlocked admin are emailed. +func TestAutoLockIsAuditedAndAdminsAreEmailed(t *testing.T) { + t.Parallel() + svc, database, viewer, sender := autoLockFixture(t) + + require.ErrorIs(t, failTOTP(svc, viewer, autoLockAttempts), errAccountLocked) + locked, err := database.IsUserLocked(viewer.ID) + require.NoError(t, err) + require.True(t, locked) + + logs, err := database.GetAuditLogs(audit.SystemActor, time.Time{}, 10) + require.NoError(t, err) + require.Len(t, logs, 1) + require.Equal(t, "user.lock", logs[0].Action) + require.Equal(t, "viewer@example.com", logs[0].Resource) + require.Contains(t, logs[0].Details, autoLockReason) + + require.Equal(t, []sentEmail{ + {to: []string{"viewer@example.com"}, subject: "Account Locked - Multiple Failed Login Attempts"}, + {to: []string{"admin@example.com"}, subject: "User locked: viewer@example.com"}, + }, sender.sent) + + // A locked account is refused before any attempt is checked, so it isn't locked (or announced) again. + require.Error(t, failTOTP(svc, viewer, 1)) + logs, err = database.GetAuditLogs(audit.SystemActor, time.Time{}, 10) + require.NoError(t, err) + require.Len(t, logs, 1) + require.Len(t, sender.sent, 2) +} + +// A user is never locked without the lock's audit event: when it can't be appended, nothing is locked or sent. +func TestAutoLockFailsClosed(t *testing.T) { + t.Parallel() + svc, database, viewer, sender := autoLockFixture(t) + _, err := database.DB().Exec(`ALTER FUNCTION audit.append_audit_event RENAME TO append_audit_event_unavailable`) + require.NoError(t, err) + + err = failTOTP(svc, viewer, autoLockAttempts) + require.ErrorIs(t, err, db.ErrAuditAppend) + locked, lockErr := database.IsUserLocked(viewer.ID) + require.NoError(t, lockErr) + require.False(t, locked) + require.Empty(t, sender.sent) +} diff --git a/internal/gateway/auth/mfa/service.go b/internal/gateway/auth/mfa/service.go index 7daf31e6..04e549ae 100644 --- a/internal/gateway/auth/mfa/service.go +++ b/internal/gateway/auth/mfa/service.go @@ -123,41 +123,6 @@ func NewService( }, nil } -// checkAndLockAccount checks failed MFA attempts and locks account if threshold exceeded. -func (s *Service) checkAndLockAccount(userID int64) error { - failedAttempts, err := s.db.CountRecentFailedMFAAttempts(userID, 5) - if err != nil { - return fmt.Errorf("failed to count failed attempts: %w", err) - } - if failedAttempts < 5 { - return nil - } - - user, err := s.db.GetUserByID(userID) - if err != nil { - return fmt.Errorf("failed to get user: %w", err) - } - - if err := s.db.LockUser(userID, "Automatic lock: 5 failed MFA attempts in 5 minutes", nil); err != nil { - return fmt.Errorf("failed to lock account: %w", err) - } - - if s.emailSender != nil && user != nil { - subject := "Account Locked - Multiple Failed Login Attempts" - textBody := "Your account has been automatically locked due to multiple failed authentication " + - "attempts.\n\nReason: 5 failed MFA attempts in 5 minutes\n\nIf this was not you, please contact " + - "your administrator immediately.\n\nFor assistance, please contact your system administrator." - htmlBody := `

Your account has been automatically locked due to multiple ` + - `failed authentication attempts.

-

Reason: 5 failed MFA attempts in 5 minutes

-

If this was not you, please contact your administrator immediately.

-

For assistance, please contact your system administrator.

` - _ = s.emailSender.Send(user.Email, subject, textBody, htmlBody) - } - - return fmt.Errorf("account locked due to multiple failed attempts - contact administrator") -} - // touchMFAMethod confirms a method if needed or updates its last-used timestamp. func (s *Service) touchMFAMethod(method *db.MFAMethod) error { if method == nil { diff --git a/internal/gateway/auth/oauth.go b/internal/gateway/auth/oauth.go index 9a308ffa..75e41086 100644 --- a/internal/gateway/auth/oauth.go +++ b/internal/gateway/auth/oauth.go @@ -7,7 +7,6 @@ import ( "encoding/base64" "fmt" "log" - "math" "net/url" "strings" "time" @@ -55,37 +54,9 @@ type LoginResponse struct { func NewOAuthHandler( clientID, clientSecret, redirectURLOrBase, allowedDomain, issuerURL string, ) (*OAuthHandler, error) { - ctx := context.Background() - - // Use vetted OIDC provider discovery with exponential backoff retry - var provider *oidc.Provider - var err error - - maxDuration := 20 * time.Second - startTime := time.Now() - backoff := 100 * time.Millisecond - attempt := 1 - - for { - provider, err = oidc.NewProvider(ctx, issuerURL) - if err == nil { - if attempt > 1 { - log.Printf("Successfully connected to OIDC provider after %d attempts", attempt) - } - break - } - - elapsed := time.Since(startTime) - if elapsed >= maxDuration { - return nil, fmt.Errorf("failed to create OIDC provider after %v: %w", elapsed, err) - } - - log.Printf("OIDC provider not ready (attempt %d), retrying in %v: %v", attempt, backoff, err) - time.Sleep(backoff) - - // Exponential backoff with max 2 second delay - backoff = time.Duration(math.Min(float64(backoff*2), float64(2*time.Second))) - attempt++ + provider, err := discoverProvider(issuerURL) + if err != nil { + return nil, err } // Derive redirects from provided value (accept base or full web callback) @@ -136,8 +107,8 @@ func (h *OAuthHandler) StartLogin() (*LoginStartResponse, error) { return nil, fmt.Errorf("oauth handler not configured for CLI login") } - // Generate PKCE parameters using secure methods - codeVerifier := generateSecureRandomString(128) + // PKCE: 32 random bytes make the 43-character verifier RFC 7636 recommends (verifiers are 43 to 128 characters). + codeVerifier := generateSecureRandomString(32) codeChallenge := generatePKCEChallenge(codeVerifier) state := generateSecureRandomString(32) @@ -256,12 +227,40 @@ func (c identityClaims) validate(allowedDomain string) error { return nil } -// generateSecureRandomString generates a cryptographically secure random string +// discoverProvider runs OIDC discovery, retrying with exponential backoff (at most 2 seconds apart) for up to 20 +// seconds while the identity provider starts. +func discoverProvider(issuerURL string) (*oidc.Provider, error) { + deadline, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + backoff := 100 * time.Millisecond + for attempt := 1; ; attempt++ { + provider, err := oidc.NewProvider(context.Background(), issuerURL) + if err == nil { + if attempt > 1 { + log.Printf("Successfully connected to OIDC provider after %d attempts", attempt) + } + return provider, nil + } + if deadline.Err() != nil { + return nil, fmt.Errorf("failed to create OIDC provider after %d attempts: %w", attempt, err) + } + + log.Printf("OIDC provider not ready (attempt %d), retrying in %v: %v", attempt, backoff, err) + timer := time.NewTimer(backoff) + select { + case <-deadline.Done(): + case <-timer.C: + } + timer.Stop() + backoff = min(backoff*2, 2*time.Second) + } +} + +// generateSecureRandomString base64url-encodes length random bytes. crypto/rand.Read never returns an error: it +// crashes the program if the system can't supply randomness. func generateSecureRandomString(length int) string { bytes := make([]byte, length) - if _, err := rand.Read(bytes); err != nil { - panic(fmt.Sprintf("failed to generate secure random string: %v", err)) - } + _, _ = rand.Read(bytes) return base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(bytes) } diff --git a/internal/gateway/auth/service_test.go b/internal/gateway/auth/service_test.go index 245e744d..99053e02 100644 --- a/internal/gateway/auth/service_test.go +++ b/internal/gateway/auth/service_test.go @@ -16,8 +16,8 @@ import ( func TestAuthServiceAllowsCookieSession(t *testing.T) { database := dbtest.NewDatabase(t) - if err := database.InitializeAdmin("user@example.com", "User"); err != nil { - t.Fatalf("initialize admin: %v", err) + if _, err := database.CreateUser("user@example.com", "User", []string{"admin"}); err != nil { + t.Fatalf("create user: %v", err) } sessionManager := NewSessionManager(database, "test-secret", &StaticTTLProvider{TTL: time.Hour}) @@ -66,8 +66,8 @@ func TestAuthServiceAllowsCookieSession(t *testing.T) { func TestValidateSessionRejectsLockedUser(t *testing.T) { database := dbtest.NewDatabase(t) - if err := database.InitializeAdmin("user@example.com", "User"); err != nil { - t.Fatalf("initialize admin: %v", err) + if _, err := database.CreateUser("user@example.com", "User", []string{"admin"}); err != nil { + t.Fatalf("create user: %v", err) } sessionManager := NewSessionManager(database, "test-secret", &StaticTTLProvider{TTL: time.Hour}) diff --git a/internal/gateway/db/audit_aggregation_merge_test.go b/internal/gateway/db/audit_aggregation_merge_test.go index d7a1c7c2..46ff5539 100644 --- a/internal/gateway/db/audit_aggregation_merge_test.go +++ b/internal/gateway/db/audit_aggregation_merge_test.go @@ -18,7 +18,9 @@ func TestAuditAggregationMergeProcessExec(t *testing.T) { defer func() { _ = db.Close() }() // 1. Log process.exec.start (Visibility) + started := time.Now().UTC() startLog := &gwdb.AuditLog{ + Timestamp: started, UserEmail: "dev@example.com", ActionType: "convox", Action: "process.exec.start", @@ -40,12 +42,10 @@ func TestAuditAggregationMergeProcessExec(t *testing.T) { assert.Equal(t, "process.exec.start", aggs[0].Action) assert.Equal(t, 0, aggs[0].AvgResponseTimeMs) - // Simulate time passing - time.Sleep(100 * time.Millisecond) - // 2. Log process.exec (Completion) // Must match key fields (resource, command, ip, user agent) execLog := &gwdb.AuditLog{ + Timestamp: started.Add(100 * time.Millisecond), UserEmail: "dev@example.com", ActionType: "convox", Action: "process.exec", diff --git a/internal/gateway/db/database.go b/internal/gateway/db/database.go index f1362bfa..ae1a9abf 100644 --- a/internal/gateway/db/database.go +++ b/internal/gateway/db/database.go @@ -384,15 +384,8 @@ func (d *Database) ResetDatabase() error { return fmt.Errorf("failed to re-run migrations: %w", err) } - if err := d.SetEnvironment(devMode); err != nil { - return err - } - - if err := d.SeedDatabase(nil); err != nil { - return fmt.Errorf("failed to seed database: %w", err) - } - - return nil + // The gateway seeds its users (audit-logged) when it next starts. + return d.SetEnvironment(devMode) } func (d *Database) validateEnvironmentForReset(devMode, disableEnvCheck bool) error { diff --git a/internal/gateway/db/database_test.go b/internal/gateway/db/database_test.go index 2d35ae76..02158f6f 100644 --- a/internal/gateway/db/database_test.go +++ b/internal/gateway/db/database_test.go @@ -15,30 +15,6 @@ import ( "github.com/DocSpring/rack-gateway/internal/gateway/testutil/dbtest" ) -func TestDatabaseInitializeAdmin(t *testing.T) { - db := dbtest.NewDatabase(t) - - // First initialization should succeed - err := db.InitializeAdmin("admin@example.com", "Admin User") - assert.NoError(t, err) - - // Second initialization should be a no-op (users exist) - err = db.InitializeAdmin("other@example.com", "Other User") - assert.NoError(t, err) - - // Admin user should exist - user, err := db.GetUser("admin@example.com") - require.NoError(t, err) - assert.Equal(t, "admin@example.com", user.Email) - assert.Equal(t, "Admin User", user.Name) - assert.Equal(t, []string{"admin"}, user.Roles) - - // Other user should not exist - user, err = db.GetUser("other@example.com") - assert.NoError(t, err) - assert.Nil(t, user) -} - func TestDatabaseUserCRUD(t *testing.T) { db := dbtest.NewDatabase(t) @@ -81,8 +57,10 @@ func TestDatabaseUserCRUD(t *testing.T) { func TestDatabaseAuditLogs(t *testing.T) { db := dbtest.NewDatabase(t) - // Create audit logs + // Create audit logs, a little apart + first := time.Now().UTC() log1 := &gwdb.AuditLog{ + Timestamp: first, UserEmail: "user1@example.com", UserName: "User One", ActionType: "convox", @@ -96,6 +74,7 @@ func TestDatabaseAuditLogs(t *testing.T) { } log2 := &gwdb.AuditLog{ + Timestamp: first.Add(10 * time.Millisecond), UserEmail: "user2@example.com", UserName: "User Two", ActionType: "users", @@ -106,6 +85,7 @@ func TestDatabaseAuditLogs(t *testing.T) { } log3 := &gwdb.AuditLog{ + Timestamp: first.Add(20 * time.Millisecond), UserEmail: "attacker@evil.com", ActionType: "auth", Action: audit.BuildAction(audit.ActionScopeLogin, audit.ActionVerbOAuthFailed), @@ -122,11 +102,9 @@ func TestDatabaseAuditLogs(t *testing.T) { require.NoError(t, err) t.Logf("After log1: %d audit logs in database", count) - time.Sleep(10 * time.Millisecond) // Ensure different timestamps err = db.CreateAuditLog(log2) require.NoError(t, err, "Failed to create log2: %v", err) - time.Sleep(10 * time.Millisecond) err = db.CreateAuditLog(log3) require.NoError(t, err, "Failed to create log3: %v", err) @@ -505,7 +483,7 @@ func TestDatabaseInitialization(t *testing.T) { // NewFromEnv must see the same data, so point it at this test's database. t.Setenv("DATABASE_URL", os.Getenv("TEST_DATABASE_URL")) - err := db1.InitializeAdmin("admin@example.com", "Admin") + _, err := db1.CreateUser("admin@example.com", "Admin", []string{"admin"}) require.NoError(t, err) _, err = db1.CreateUser("user@example.com", "User", []string{"viewer"}) require.NoError(t, err) diff --git a/internal/gateway/db/seeds.go b/internal/gateway/db/seeds.go index f0c8113e..f9def29b 100644 --- a/internal/gateway/db/seeds.go +++ b/internal/gateway/db/seeds.go @@ -1,7 +1,10 @@ package db import ( + "context" + "database/sql" "encoding/json" + "errors" "fmt" "os" "sort" @@ -17,95 +20,99 @@ type SeedConfig struct { OperationsUsers []string } -// SeedDatabase ensures the gateway database contains the expected bootstrap -// data (admin users, seeded role users, protected env vars). When cfg is nil or -// any slice within is nil, the method falls back to reading the equivalent -// environment variable (e.g., ADMIN_USERS). -func (d *Database) SeedDatabase(cfg *SeedConfig) error { - seed := buildSeedInputs(cfg) +// SeededUser is a user the startup seed created, with the roles of every seed list that names it. +type SeededUser struct { + Email string + Roles []string +} - // Seeded users have no name until they first sign in, when it's filled from the identity provider. - if len(seed.adminUsers) > 0 { - if err := d.InitializeAdmin(seed.adminUsers[0], ""); err != nil { - return fmt.Errorf("failed to initialize admin user: %w", err) - } +// SeedDatabase creates the users in the seed lists that don't exist yet, each with the roles of the lists that name +// it. It never changes an existing user, so a role an admin has changed since stays changed, and seeded users have +// no name until they first sign in, when it's filled from the identity provider. When cfg is nil or any slice +// within is nil, that list is read from its environment variable (e.g., ADMIN_USERS). +// +// Each user's audit event, entry(u), is appended in the same transaction, so a user is never created without its +// event: when an append fails, nothing is created and the error wraps ErrAuditAppend. It returns the stored events. +func (d *Database) SeedDatabase(cfg *SeedConfig, entry func(SeededUser) *AuditLog) ([]*AuditLog, error) { + users := seedUsers(buildSeedInputs(cfg)) + if len(users) == 0 { + return nil, nil + } + ctx, cancel := context.WithTimeout(context.Background(), auditWriteTimeout) + defer cancel() + tx, err := d.beginAuditTx(ctx) + if err != nil { + return nil, err } + defer func() { _ = tx.Rollback() }() - roleSeeds := []struct { - emails []string - roles []string - }{ - {seed.adminUsers, []string{"admin"}}, - {seed.viewerUsers, []string{"viewer"}}, - {seed.deployerUsers, []string{"deployer"}}, - {seed.operationsUsers, []string{"ops"}}, + // The chain lock first, as every audit append takes it before touching the rows its event references. + if _, err := tx.ExecContext(ctx, "SELECT pg_advisory_xact_lock($1)", AdvisoryLockAuditChain); err != nil { + return nil, fmt.Errorf("failed to acquire audit chain lock: %w", err) } - - for _, rs := range roleSeeds { - for _, email := range rs.emails { - if err := d.ensureUserWithRoles(email, rs.roles); err != nil { - return err - } + var events []*AuditLog + for _, user := range users { + created, err := d.insertSeededUser(ctx, tx, user) + if err != nil { + return nil, err + } + if !created { + continue + } + event := entry(user) + if err := appendAuditLogTx(ctx, tx, event); err != nil { + return nil, fmt.Errorf("%w for seeded user %s: %w", ErrAuditAppend, user.Email, err) } + events = append(events, event) } - - // Settings are no longer pre-seeded into the database. - // They are read from environment variables by the settings service on demand. - - return nil + if err := tx.Commit(); err != nil { + return nil, fmt.Errorf("failed to commit seeded users: %w", err) + } + return events, nil } -// InitializeAdmin creates the initial admin user if no users exist -func (d *Database) InitializeAdmin(email, name string) error { - // Check if any users exist - var count int - err := d.db.QueryRow("SELECT COUNT(*) FROM users").Scan(&count) +// insertSeededUser creates the user unless one with that email exists, and reports whether it did. +func (d *Database) insertSeededUser(ctx context.Context, tx *sql.Tx, user SeededUser) (bool, error) { + rolesJSON, err := json.Marshal(user.Roles) if err != nil { - return fmt.Errorf("failed to count users: %w", err) - } - - // If users exist, don't create admin - if count > 0 { - return nil + return false, fmt.Errorf("failed to marshal roles: %w", err) } - - // Create admin user - roles := []string{"admin"} - rolesJSON, _ := json.Marshal(roles) - var id int64 - query := "INSERT INTO users (email, name, roles) VALUES (?, ?, ?) RETURNING id" - if err := d.queryRow(query, email, name, string(rolesJSON)).Scan(&id); err != nil { - return fmt.Errorf("failed to create admin user: %w", err) + err = tx.QueryRowContext(ctx, d.rebind( + "INSERT INTO users (email, name, roles) VALUES (?, '', ?) ON CONFLICT (email) DO NOTHING RETURNING id"), + user.Email, string(rolesJSON)).Scan(&id) + if errors.Is(err, sql.ErrNoRows) { + return false, nil } - return nil -} - -// ensureUserWithRoles creates the user, or sets their roles. It never changes an existing user's name. -func (d *Database) ensureUserWithRoles(email string, roles []string) error { - if email == "" { - return nil - } - - roles = normalizeRoles(roles) - existing, err := d.GetUser(email) if err != nil { - return fmt.Errorf("failed to load user %s: %w", email, err) + return false, fmt.Errorf("failed to create seeded user %s: %w", user.Email, err) } + return true, nil +} - if existing == nil { - if _, err := d.CreateUser(email, "", roles); err != nil { - return fmt.Errorf("failed to create user %s: %w", email, err) +// seedUsers merges the seed lists into one user per email, sorted by email. +func seedUsers(inputs seedInputs) []SeededUser { + lists := []struct { + emails []string + role string + }{ + {inputs.adminUsers, "admin"}, + {inputs.viewerUsers, "viewer"}, + {inputs.deployerUsers, "deployer"}, + {inputs.operationsUsers, "ops"}, + } + roles := map[string][]string{} + for _, list := range lists { + for _, email := range list.emails { + roles[email] = append(roles[email], list.role) } - return nil } - - if !equalStringSlices(normalizeRoles(existing.Roles), roles) { - if err := d.UpdateUserRoles(email, roles); err != nil { - return fmt.Errorf("failed to update roles for %s: %w", email, err) - } + users := make([]SeededUser, 0, len(roles)) + for email, userRoles := range roles { + users = append(users, SeededUser{Email: email, Roles: normalizeRoles(userRoles)}) } - return nil + sort.Slice(users, func(i, j int) bool { return users[i].Email < users[j].Email }) + return users } type seedInputs struct { @@ -188,15 +195,3 @@ func normalizeRoles(roles []string) []string { sort.Strings(out) return out } - -func equalStringSlices(a, b []string) bool { - if len(a) != len(b) { - return false - } - for i := range a { - if a[i] != b[i] { - return false - } - } - return true -} diff --git a/internal/gateway/db/seeds_test.go b/internal/gateway/db/seeds_test.go index e3ffc38c..1c9a7bea 100644 --- a/internal/gateway/db/seeds_test.go +++ b/internal/gateway/db/seeds_test.go @@ -2,34 +2,84 @@ package db_test import ( "testing" + "time" "github.com/stretchr/testify/require" + "github.com/DocSpring/rack-gateway/internal/gateway/audit" gwdb "github.com/DocSpring/rack-gateway/internal/gateway/db" "github.com/DocSpring/rack-gateway/internal/gateway/testutil/dbtest" ) -// Seeded users have no placeholder name, and seeding again keeps the name a user has since been given. -func TestSeedDatabaseLeavesNamesToUsers(t *testing.T) { +func seedLists() *gwdb.SeedConfig { + return &gwdb.SeedConfig{ + AdminUsers: []string{"Admin@example.com"}, + ViewerUsers: []string{"viewer@example.com"}, + DeployerUsers: []string{"both@example.com"}, + OperationsUsers: []string{"both@example.com"}, + } +} + +func requireRoles(t *testing.T, database *gwdb.Database, email string, roles ...string) *gwdb.User { + t.Helper() + user, err := database.GetUser(email) + require.NoError(t, err) + require.NotNil(t, user, email) + require.Equal(t, roles, user.Roles, email) + return user +} + +// Seeding creates the listed users that don't exist, each with the roles of every list that names it and no name, +// and records each creation by the system. +func TestSeedDatabaseCreatesMissingUsersAndAuditsThem(t *testing.T) { database := dbtest.NewDatabase(t) - seed := &gwdb.SeedConfig{ - AdminUsers: []string{"admin@example.com"}, - ViewerUsers: []string{"viewer@example.com"}, + + events, err := database.SeedDatabase(seedLists(), audit.SeededUserEntry) + require.NoError(t, err) + require.Len(t, events, 3) + + require.Empty(t, requireRoles(t, database, "admin@example.com", "admin").Name) + requireRoles(t, database, "viewer@example.com", "viewer") + requireRoles(t, database, "both@example.com", "deployer", "ops") + + logs, err := database.GetAuditLogs(audit.SystemActor, time.Time{}, 10) + require.NoError(t, err) + require.Len(t, logs, 3) + resources := make([]string, 0, len(logs)) + for _, log := range logs { + require.Equal(t, "user.create", log.Action) + require.Equal(t, "success", log.Status) + resources = append(resources, log.Resource) } - require.NoError(t, database.SeedDatabase(seed)) + require.ElementsMatch(t, []string{"admin@example.com", "viewer@example.com", "both@example.com"}, resources) +} - admin, err := database.GetUser("admin@example.com") +// Seeding never changes an existing user: a role or name given since stays, and nothing new is recorded. +func TestSeedDatabaseLeavesExistingUsersAlone(t *testing.T) { + database := dbtest.NewDatabase(t) + _, err := database.SeedDatabase(seedLists(), audit.SeededUserEntry) require.NoError(t, err) - require.Empty(t, admin.Name) - require.Equal(t, []string{"admin"}, admin.Roles) + require.NoError(t, database.UpdateUserRoles("admin@example.com", []string{"viewer"})) require.NoError(t, database.UpdateUserName("admin@example.com", "Real Name")) - require.NoError(t, database.SeedDatabase(seed)) - admin, err = database.GetUser("admin@example.com") + events, err := database.SeedDatabase(seedLists(), audit.SeededUserEntry) require.NoError(t, err) - require.Equal(t, "Real Name", admin.Name) - viewer, err := database.GetUser("viewer@example.com") + require.Empty(t, events) + require.Equal(t, "Real Name", requireRoles(t, database, "admin@example.com", "viewer").Name) +} + +// A user is never created without its audit event: when the event can't be appended, no user is created. +func TestSeedDatabaseCreatesNothingWithoutItsAuditEvent(t *testing.T) { + database := dbtest.NewDatabase(t) + _, err := database.DB().Exec(`ALTER FUNCTION audit.append_audit_event RENAME TO append_audit_event_unavailable`) + require.NoError(t, err) + + events, err := database.SeedDatabase(seedLists(), audit.SeededUserEntry) + require.ErrorIs(t, err, gwdb.ErrAuditAppend) + require.Empty(t, events) + + user, err := database.GetUser("admin@example.com") require.NoError(t, err) - require.Equal(t, []string{"viewer"}, viewer.Roles) + require.Nil(t, user) } diff --git a/internal/gateway/db/user_lock.go b/internal/gateway/db/user_lock.go new file mode 100644 index 00000000..cb656400 --- /dev/null +++ b/internal/gateway/db/user_lock.go @@ -0,0 +1,79 @@ +package db + +import ( + "context" + "database/sql" + "errors" + "fmt" +) + +// LockedUser is a user the gateway locked by itself. +type LockedUser struct { + ID int64 + Email string + Reason string +} + +// LockUserAudited locks the user unless they're already locked. The lock's audit event, entry(user), is appended in +// the same transaction, so the user is never locked without it: when the append fails, nothing is locked and the +// error wraps ErrAuditAppend. It returns the user it locked (nil when they already were) and the stored events. +func (d *Database) LockUserAudited( + userID int64, + reason string, + entry func(LockedUser) *AuditLog, +) (*LockedUser, []*AuditLog, error) { + ctx, cancel := context.WithTimeout(context.Background(), auditWriteTimeout) + defer cancel() + tx, err := d.beginAuditTx(ctx) + if err != nil { + return nil, nil, err + } + defer func() { _ = tx.Rollback() }() + + // The chain lock first, as every audit append takes it before touching the rows its event references. + if _, err := tx.ExecContext(ctx, "SELECT pg_advisory_xact_lock($1)", AdvisoryLockAuditChain); err != nil { + return nil, nil, fmt.Errorf("failed to acquire audit chain lock: %w", err) + } + locked := LockedUser{ID: userID, Reason: reason} + err = tx.QueryRowContext(ctx, d.rebind(`UPDATE users + SET locked_at = NOW(), locked_reason = ?, locked_by_user_id = NULL, updated_at = NOW() + WHERE id = ? AND locked_at IS NULL + RETURNING email`), nullableString(reason, 255), userID).Scan(&locked.Email) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil, nil + } + if err != nil { + return nil, nil, fmt.Errorf("failed to lock user: %w", err) + } + event := entry(locked) + if err := appendAuditLogTx(ctx, tx, event); err != nil { + return nil, nil, fmt.Errorf("%w for locking user %s: %w", ErrAuditAppend, locked.Email, err) + } + if err := tx.Commit(); err != nil { + return nil, nil, fmt.Errorf("failed to commit user lock: %w", err) + } + return &locked, []*AuditLog{event}, nil +} + +// UnlockedAdminEmails returns the emails of the admins who aren't locked, sorted. +func (d *Database) UnlockedAdminEmails() ([]string, error) { + rows, err := d.query(`SELECT email FROM users + WHERE roles::jsonb @> '["admin"]' AND locked_at IS NULL + ORDER BY email`) + if err != nil { + return nil, fmt.Errorf("failed to list admins: %w", err) + } + defer func() { _ = rows.Close() }() + var emails []string + for rows.Next() { + var email string + if err := rows.Scan(&email); err != nil { + return nil, fmt.Errorf("failed to read admin: %w", err) + } + emails = append(emails, email) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("failed to list admins: %w", err) + } + return emails, nil +} diff --git a/internal/gateway/handlers/auth_cli_logout.go b/internal/gateway/handlers/auth_cli_logout.go new file mode 100644 index 00000000..c8f08295 --- /dev/null +++ b/internal/gateway/handlers/auth_cli_logout.go @@ -0,0 +1,48 @@ +package handlers + +import ( + "net/http" + + "github.com/gin-gonic/gin" + + "github.com/DocSpring/rack-gateway/internal/gateway/audit" +) + +// CLILogoutResponse confirms a CLI logout. +type CLILogoutResponse struct { + Revoked bool `json:"revoked"` +} + +// CLILogout godoc +// @Summary Log out a CLI session +// @Description Revokes the session whose token is sent as `Authorization: Bearer `. `rack-gateway logout` +// @Description calls it before removing the rack from its config. Cookies are ignored, so a browser can't be made +// @Description to call it, and API tokens get 401: they're revoked on the API Tokens page. +// @Tags Auth +// @Produce json +// @Success 200 {object} CLILogoutResponse +// @Failure 401 {object} ErrorResponse +// @Failure 500 {object} ErrorResponse +// @Failure 503 {object} ErrorResponse +// @Router /auth/cli/logout [post] +func (h *AuthHandler) CLILogout(c *gin.Context) { + sessionToken := extractBearerToken(c) + email, name := h.sessionUser(c, sessionToken) + if !h.recordAuthAttempt(c, audit.ActionScopeLogout, "cli", email, name) { + c.JSON(http.StatusServiceUnavailable, ErrorResponse{Error: audit.RefusalMessage(audit.ActionScopeLogout)}) + return + } + if email == "" { + h.recordAuthResult(c, audit.ActionScopeLogout, "cli", "", "", audit.StatusFailed) + c.JSON(http.StatusUnauthorized, ErrorResponse{Error: "not a valid session"}) + return + } + + _, err := h.sessions.RevokeByToken(sessionToken, nil) + h.recordAuthResult(c, audit.ActionScopeLogout, "cli", email, name, auditStatus(err)) + if err != nil { + c.JSON(http.StatusInternalServerError, ErrorResponse{Error: "failed to revoke session"}) + return + } + c.JSON(http.StatusOK, CLILogoutResponse{Revoked: true}) +} diff --git a/internal/gateway/handlers/auth_helpers.go b/internal/gateway/handlers/auth_helpers.go index cc6a5e7a..9905be30 100644 --- a/internal/gateway/handlers/auth_helpers.go +++ b/internal/gateway/handlers/auth_helpers.go @@ -34,6 +34,14 @@ func extractSessionToken(c *gin.Context) string { return trimmed } } + return extractBearerToken(c) +} + +// extractBearerToken returns the token from an "Authorization: Bearer" header, or "" without one. +func extractBearerToken(c *gin.Context) string { + if c == nil { + return "" + } authHeader := strings.TrimSpace(c.GetHeader("Authorization")) if authHeader != "" { const bearerPrefix = "Bearer " @@ -318,9 +326,6 @@ func extractTrustedDeviceUserAgent(device *db.TrustedDevice) string { func shortDeviceID(id string) string { id = strings.TrimSpace(id) - if id == "" { - panic("CRITICAL: Empty device ID") - } if len(id) <= 8 { return id } diff --git a/internal/gateway/openapi/generated/swagger.json b/internal/gateway/openapi/generated/swagger.json index 4e0f7d69..17298ce6 100644 --- a/internal/gateway/openapi/generated/swagger.json +++ b/internal/gateway/openapi/generated/swagger.json @@ -1625,6 +1625,44 @@ } } }, + "/auth/cli/logout": { + "post": { + "description": "Revokes the session whose token is sent as `Authorization: Bearer \u003ctoken\u003e`. `rack-gateway logout`\ncalls it before removing the rack from its config. Cookies are ignored, so a browser can't be made\nto call it, and API tokens get 401: they're revoked on the API Tokens page.", + "produces": [ + "application/json" + ], + "tags": [ + "Auth" + ], + "summary": "Log out a CLI session", + "responses": { + "200": { + "description": "OK", + "schema": { + "$ref": "#/definitions/handlers.CLILogoutResponse" + } + }, + "401": { + "description": "Unauthorized", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + }, + "500": { + "description": "Internal Server Error", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + }, + "503": { + "description": "Service Unavailable", + "schema": { + "$ref": "#/definitions/handlers.ErrorResponse" + } + } + } + } + }, "/auth/cli/mfa": { "get": { "description": "Sends the bound browser to MFA (or MFA enrollment), or back to the CLI when MFA is satisfied.", @@ -4900,6 +4938,14 @@ } } }, + "handlers.CLILogoutResponse": { + "type": "object", + "properties": { + "revoked": { + "type": "boolean" + } + } + }, "handlers.ConfirmTOTPEnrollmentRequest": { "type": "object", "required": [ diff --git a/internal/gateway/ratelimit/ratelimit.go b/internal/gateway/ratelimit/ratelimit.go index 2529f000..f843abad 100644 --- a/internal/gateway/ratelimit/ratelimit.go +++ b/internal/gateway/ratelimit/ratelimit.go @@ -3,7 +3,6 @@ package ratelimit import ( "fmt" "net/http" - "strings" "sync" "time" @@ -20,6 +19,7 @@ type RateLimiter struct { burst int // max burst size cleanup time.Duration stop chan struct{} // channel to stop cleanup goroutine + now func() time.Time } type visitor struct { @@ -43,6 +43,7 @@ func NewRateLimiter(requestsPerSecond float64, burst int) *RateLimiter { burst: burst, cleanup: 5 * time.Minute, stop: make(chan struct{}), + now: time.Now, } // Start cleanup goroutine to remove old visitors @@ -51,26 +52,22 @@ func NewRateLimiter(requestsPerSecond float64, burst int) *RateLimiter { return rl } -// getVisitor retrieves or creates a rate limiter for the given IP -func (rl *RateLimiter) getVisitor(ip string) *rate.Limiter { +// allow reports whether the IP may make a request now, and takes a token if so. +func (rl *RateLimiter) allow(ip string) bool { rl.mu.Lock() defer rl.mu.Unlock() + now := rl.now() v, exists := rl.visitors[ip] if !exists { - limiter := rate.NewLimiter(rl.rate, rl.burst) - rl.visitors[ip] = &visitor{ - limiter: limiter, - lastSeen: time.Now(), - } - return limiter + v = &visitor{limiter: rate.NewLimiter(rl.rate, rl.burst)} + rl.visitors[ip] = v } - - v.lastSeen = time.Now() - return v.limiter + v.lastSeen = now + return v.limiter.AllowN(now, 1) } -// cleanupVisitors removes old entries from the visitors map +// cleanupVisitors removes idle visitors every cleanup interval until Stop. func (rl *RateLimiter) cleanupVisitors() { ticker := time.NewTicker(rl.cleanup) defer ticker.Stop() @@ -78,19 +75,25 @@ func (rl *RateLimiter) cleanupVisitors() { for { select { case <-ticker.C: - rl.mu.Lock() - for ip, v := range rl.visitors { - if time.Since(v.lastSeen) > rl.cleanup { - delete(rl.visitors, ip) - } - } - rl.mu.Unlock() + rl.removeIdleVisitors() case <-rl.stop: return } } } +// removeIdleVisitors forgets the IPs that haven't made a request for the cleanup interval. +func (rl *RateLimiter) removeIdleVisitors() { + rl.mu.Lock() + defer rl.mu.Unlock() + now := rl.now() + for ip, v := range rl.visitors { + if now.Sub(v.lastSeen) > rl.cleanup { + delete(rl.visitors, ip) + } + } +} + // Stop stops the cleanup goroutine func (rl *RateLimiter) Stop() { close(rl.stop) @@ -99,16 +102,13 @@ func (rl *RateLimiter) Stop() { // Middleware returns an HTTP middleware that enforces rate limiting func (rl *RateLimiter) Middleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - ip := netutil.ClientIP(r) - - limiter := rl.getVisitor(ip) - if !limiter.Allow() { + if !rl.allow(netutil.ClientIP(r)) { // Return 429 Too Many Requests w.Header().Set("Content-Type", "application/json") w.Header().Set("Retry-After", fmt.Sprintf("%d", int(1/float64(rl.rate)))) w.Header().Set("X-RateLimit-Limit", fmt.Sprintf("%d", int(rl.rate))) w.Header().Set("X-RateLimit-Remaining", "0") - w.Header().Set("X-RateLimit-Reset", fmt.Sprintf("%d", time.Now().Add(time.Second).Unix())) + w.Header().Set("X-RateLimit-Reset", fmt.Sprintf("%d", rl.now().Add(time.Second).Unix())) w.WriteHeader(http.StatusTooManyRequests) errorMsg := `{"error":"rate limit exceeded",` + @@ -122,34 +122,3 @@ func (rl *RateLimiter) Middleware(next http.Handler) http.Handler { next.ServeHTTP(w, r) }) } - -// AuthEndpointsOnly wraps the middleware to only apply to auth-related endpoints -func (rl *RateLimiter) AuthEndpointsOnly(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Only rate limit auth-related endpoints - path := r.URL.Path - if strings.Contains(path, "/auth/") || - strings.Contains(path, "/login") || - strings.Contains(path, "/logout") || - strings.Contains(path, "/callback") || - strings.Contains(path, "/tokens") && r.Method == "POST" { - // Apply rate limiting - ip := netutil.ClientIP(r) - limiter := rl.getVisitor(ip) - if !limiter.Allow() { - w.Header().Set("Content-Type", "application/json") - w.Header().Set("Retry-After", fmt.Sprintf("%d", int(1/float64(rl.rate)))) - w.WriteHeader(http.StatusTooManyRequests) - errorMsg := `{"error":"rate limit exceeded",` + - `"message":"too many authentication attempts, please try again later"}` - if _, err := w.Write([]byte(errorMsg)); err != nil { - return - } - return - } - } - - // Pass through for non-auth endpoints - next.ServeHTTP(w, r) - }) -} diff --git a/internal/gateway/ratelimit/ratelimit_test.go b/internal/gateway/ratelimit/ratelimit_test.go index 81e7701d..633d05a3 100644 --- a/internal/gateway/ratelimit/ratelimit_test.go +++ b/internal/gateway/ratelimit/ratelimit_test.go @@ -9,7 +9,6 @@ import ( "time" "github.com/stretchr/testify/require" - "golang.org/x/time/rate" "github.com/DocSpring/rack-gateway/internal/gateway/netutil" ) @@ -17,6 +16,9 @@ import ( func TestRateLimiter(t *testing.T) { // Create a rate limiter with 5 requests per second, burst of 10 rl := NewRateLimiter(5, 10) + defer rl.Stop() + clock := time.Now() + rl.now = func() time.Time { return clock } // Create a test handler testHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { @@ -63,7 +65,7 @@ func TestRateLimiter(t *testing.T) { } // Test 4: After waiting, requests should succeed again - time.Sleep(time.Second / 5) // Wait for 1/5 second (1 token at 5 req/sec) + clock = clock.Add(time.Second / 5) // 1/5 second later: 1 token at 5 req/sec req3 := httptest.NewRequest("GET", "/test", nil) req3.RemoteAddr = "192.168.1.1:1234" @@ -105,63 +107,6 @@ func TestRateLimiterKeysOnResolvedClientIPNotForwardingHeaders(t *testing.T) { require.Equal(t, http.StatusTooManyRequests, serve("100.85.250.16", "")) } -func TestAuthEndpointsOnly(t *testing.T) { - rl := NewRateLimiter(100, 2) // High rate limit but small burst for faster testing - - // Create a test handler - testHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("OK")) - }) - - // Wrap with auth-only rate limiter - handler := rl.AuthEndpointsOnly(testHandler) - - tests := []struct { - path string - method string - shouldLimit bool - }{ - {"/auth/login", "POST", true}, - {"/auth/web/callback", "GET", true}, - {"/logout", "GET", true}, - {"/api/v1/tokens", "POST", true}, - {"/api/v1/users", "GET", false}, - {"/api/v1/tokens", "GET", false}, // GET is not rate limited - {"/health", "GET", false}, - {"/apps", "GET", false}, - } - - for _, tt := range tests { - t.Run(fmt.Sprintf("%s %s", tt.method, tt.path), func(t *testing.T) { - // Use unique IP per test to avoid interference - testIP := fmt.Sprintf("10.0.%d.%d:1234", len(tt.path), len(tt.method)) - - // Make 3 requests (burst is 2) - var lastCode int - for i := 0; i < 3; i++ { - req := httptest.NewRequest(tt.method, tt.path, nil) - req.RemoteAddr = testIP - rr := httptest.NewRecorder() - - handler.ServeHTTP(rr, req) - lastCode = rr.Code - } - - if tt.shouldLimit { - if lastCode != http.StatusTooManyRequests { - t.Errorf("Expected rate limiting (429) for %s %s, got %d", tt.method, tt.path, lastCode) - } - } else { - if lastCode != http.StatusOK { - t.Errorf("Expected no rate limiting (200) for %s %s, got %d", tt.method, tt.path, lastCode) - } - } - }) - // No sleep needed since we use unique IPs - } -} - func TestConcurrentRequests(t *testing.T) { rl := NewRateLimiter(10, 20) @@ -263,45 +208,32 @@ func TestRateLimiterHeaders(t *testing.T) { } func TestVisitorCleanup(t *testing.T) { - // Create a custom rate limiter with very short cleanup interval for fast testing - rl := &RateLimiter{ - visitors: make(map[string]*visitor), - rate: rate.Limit(5), - burst: 10, - cleanup: 50 * time.Millisecond, // Very short cleanup for fast testing - stop: make(chan struct{}), - } - defer rl.Stop() // Ensure cleanup goroutine stops - - // Start cleanup goroutine - go rl.cleanupVisitors() + rl := NewRateLimiter(5, 10) + defer rl.Stop() + clock := time.Now() + rl.now = func() time.Time { return clock } - testHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + handler := rl.Middleware(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) - }) - - handler := rl.Middleware(testHandler) - - // Make a request from an IP + })) req := httptest.NewRequest("GET", "/test", nil) req.RemoteAddr = "192.168.1.1:1234" - rr := httptest.NewRecorder() - handler.ServeHTTP(rr, req) + handler.ServeHTTP(httptest.NewRecorder(), req) - // Verify visitor exists - rl.mu.RLock() - if _, exists := rl.visitors["192.168.1.1"]; !exists { - t.Error("Visitor should exist after request") + visiting := func() bool { + rl.mu.RLock() + defer rl.mu.RUnlock() + _, exists := rl.visitors["192.168.1.1"] + return exists } - rl.mu.RUnlock() - // Wait for cleanup (2x cleanup interval to ensure it runs) - time.Sleep(110 * time.Millisecond) + // Seen within the cleanup interval: kept. + clock = clock.Add(rl.cleanup) + rl.removeIdleVisitors() + require.True(t, visiting(), "a visitor seen within the cleanup interval must be kept") - // Verify visitor was cleaned up - rl.mu.RLock() - if _, exists := rl.visitors["192.168.1.1"]; exists { - t.Error("Visitor should have been cleaned up") - } - rl.mu.RUnlock() + // Idle for longer: removed. + clock = clock.Add(time.Second) + rl.removeIdleVisitors() + require.False(t, visiting(), "an idle visitor must be removed") } diff --git a/internal/gateway/rbac/main_test.go b/internal/gateway/rbac/main_test.go index 42d15596..0bf14e4d 100644 --- a/internal/gateway/rbac/main_test.go +++ b/internal/gateway/rbac/main_test.go @@ -8,13 +8,5 @@ import ( ) func TestMain(m *testing.M) { - cleanup, err := convoxguard.Setup() - if err != nil { - panic(err) - } - code := m.Run() - if err := cleanup(); err != nil { - panic("CRITICAL: " + err.Error()) - } - os.Exit(code) + os.Exit(convoxguard.Run(m)) } diff --git a/internal/gateway/routes/cli_logout_test.go b/internal/gateway/routes/cli_logout_test.go new file mode 100644 index 00000000..9e8007e0 --- /dev/null +++ b/internal/gateway/routes/cli_logout_test.go @@ -0,0 +1,92 @@ +package routes + +import ( + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/DocSpring/rack-gateway/internal/gateway/auth" + "github.com/DocSpring/rack-gateway/internal/gateway/testutil/audittest" +) + +const cliLogoutPath = "/api/v1/auth/cli/logout" + +// cliSession returns the token and bearer credentials of a new CLI session for userEmail. +func (e *authzEnv) cliSession(t *testing.T, userEmail string) (string, credentials) { + t.Helper() + user, err := e.database.GetUser(userEmail) + require.NoError(t, err) + sessionToken, _, err := e.sessions.CreateSession(user, auth.SessionMetadata{Channel: "cli"}) + require.NoError(t, err) + return sessionToken, credentials{"Authorization": "Bearer " + sessionToken} +} + +func (e *authzEnv) logoutStatuses(t *testing.T, email string) []string { + t.Helper() + rows, err := e.database.DB().Query(` + SELECT status FROM audit.audit_event + WHERE action = 'logout' AND resource = 'cli' AND user_email = $1 + ORDER BY id`, email) + require.NoError(t, err) + defer func() { require.NoError(t, rows.Close()) }() + var statuses []string + for rows.Next() { + var status string + require.NoError(t, rows.Scan(&status)) + statuses = append(statuses, status) + } + require.NoError(t, rows.Err()) + return statuses +} + +// TestCLILogoutRevokesTheSession: `rack-gateway logout` ends the session on the gateway, recorded as attempted +// before the revoke and successful after it, so a copied token stops working. +func TestCLILogoutRevokesTheSession(t *testing.T) { + e := newAuthzEnv(t) + sessionToken, creds := e.cliSession(t, "viewer@example.com") + + w := e.do(t, creds, http.MethodPost, cliLogoutPath, nil) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + require.JSONEq(t, `{"revoked":true}`, w.Body.String()) + + _, err := e.sessions.ValidateSession(sessionToken, "", "") + require.Error(t, err, "the session must be revoked") + require.Equal(t, []string{"attempted", "success"}, e.logoutStatuses(t, "viewer@example.com")) + + w = e.do(t, creds, http.MethodPost, cliLogoutPath, nil) + require.Equal(t, http.StatusUnauthorized, w.Code, w.Body.String()) + require.Contains(t, w.Body.String(), "not a valid session") +} + +// TestCLILogoutIgnoresCookiesAndAPITokens: only a bearer session token is logged out. A browser's cookie is +// ignored (so another site can't log a browser out), and an API token isn't a session. +func TestCLILogoutIgnoresCookiesAndAPITokens(t *testing.T) { + e := newAuthzEnv(t) + web := e.webSession(t, "viewer@example.com") + webToken := strings.TrimPrefix(web["Cookie"], "session_token=") + + w := e.do(t, web, http.MethodPost, cliLogoutPath, nil) + require.Equal(t, http.StatusUnauthorized, w.Code, w.Body.String()) + _, err := e.sessions.ValidateSession(webToken, "", "") + require.NoError(t, err, "the web session must still be valid") + + token := e.apiToken(t, "admin@example.com", "gateway:user:list") + w = e.do(t, token, http.MethodPost, cliLogoutPath, nil) + require.Equal(t, http.StatusUnauthorized, w.Code, w.Body.String()) +} + +// TestCLILogoutFailsClosed: with the audit log down, the logout is refused before it revokes anything. +func TestCLILogoutFailsClosed(t *testing.T) { + e := newAuthzEnv(t) + sessionToken, creds := e.cliSession(t, "viewer@example.com") + e.auditLog.SetChainWriter(audittest.FailingChainWriter{}) + + w := e.do(t, creds, http.MethodPost, cliLogoutPath, nil) + require.Equal(t, http.StatusServiceUnavailable, w.Code, w.Body.String()) + require.Contains(t, w.Body.String(), "audit log unavailable") + + _, err := e.sessions.ValidateSession(sessionToken, "", "") + require.NoError(t, err, "the refused logout must not have revoked the session") +} diff --git a/internal/gateway/routes/main_test.go b/internal/gateway/routes/main_test.go index 68c1c2b1..6dea0304 100644 --- a/internal/gateway/routes/main_test.go +++ b/internal/gateway/routes/main_test.go @@ -8,13 +8,5 @@ import ( ) func TestMain(m *testing.M) { - cleanup, err := convoxguard.Setup() - if err != nil { - panic(err) - } - code := m.Run() - if err := cleanup(); err != nil { - panic("CRITICAL: " + err.Error()) - } - os.Exit(code) + os.Exit(convoxguard.Run(m)) } diff --git a/internal/gateway/routes/route_registration.go b/internal/gateway/routes/route_registration.go index 4b6f7096..c8149f2b 100644 --- a/internal/gateway/routes/route_registration.go +++ b/internal/gateway/routes/route_registration.go @@ -33,6 +33,7 @@ func registerAuthRoutes(api *gin.RouterGroup, cfg *Config, h *handlerSet) { authGroup.HEAD("/auth/web/login", h.auth.WebLoginStart) authGroup.GET("/auth/web/callback", h.auth.WebLoginCallback) authGroup.GET("/auth/web/logout", h.auth.WebLogout) + authGroup.POST("/auth/cli/logout", h.auth.CLILogout) } func registerMFARoutes(authenticated *gin.RouterGroup, cfg *Config, h *handlerSet) { diff --git a/internal/gateway/testutil/convoxguard/convoxguard.go b/internal/gateway/testutil/convoxguard/convoxguard.go index 6fb3d3bc..1457ba0a 100644 --- a/internal/gateway/testutil/convoxguard/convoxguard.go +++ b/internal/gateway/testutil/convoxguard/convoxguard.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "runtime" + "testing" ) func configDir() (string, error) { @@ -33,41 +34,50 @@ func configDir() (string, error) { } } -// Setup enforces that tests must be run through safe-test.sh wrapper -// The wrapper handles all backup/restore, we just verify it's being used -func Setup() (func() error, error) { - // Skip all protection in CI environments - if os.Getenv("CI") != "" { - return func() error { return nil }, nil - } +// errUnsafeRun means the tests were started with 'go test' rather than through the safe-test.sh wrapper. +var errUnsafeRun = errors.New("CRITICAL SAFETY VIOLATION: tests must be run through the safe wrapper script") - // ENFORCE: Tests MUST be run through safe-test.sh wrapper - if os.Getenv("GATEWAY_SAFE_TEST") != "1" { - panic(` -CRITICAL SAFETY VIOLATION: Tests must be run through the safe wrapper script! - -You attempted to run tests directly with 'go test' which could destroy your production Convox configuration. +const unsafeRunHelp = `You attempted to run tests directly with 'go test', +which could destroy your production Convox configuration. ALWAYS use one of these commands: task test - Run all tests (web + go + e2e) task go:test - Run Go tests safely ./scripts/safe-test.sh - Run tests with custom flags -DO NOT run 'go test' directly! -`) +DO NOT run 'go test' directly!` + +// Run runs the package's tests (call it from TestMain) once it has checked they were started through the +// safe-test.sh wrapper, which backs up and restores the real Convox config around them. It returns the exit code. +func Run(m *testing.M) int { + if err := check(); err != nil { + fmt.Fprintln(os.Stderr, err) + if errors.Is(err, errUnsafeRun) { + fmt.Fprintln(os.Stderr, unsafeRunHelp) + } + return 1 + } + return m.Run() +} + +func check() error { + // Skip all protection in CI environments + if os.Getenv("CI") != "" { + return nil + } + // ENFORCE: Tests MUST be run through safe-test.sh wrapper + if os.Getenv("GATEWAY_SAFE_TEST") != "1" { + return errUnsafeRun } // Verify the wrapper has set up the test environment correctly cfg, err := configDir() if err != nil { - return nil, err + return err } - guardFile := filepath.Join(cfg, "GUARD_ACTIVE") if _, err := os.Stat(guardFile); err != nil { - return nil, fmt.Errorf("CRITICAL: Guard file not found at %s - safe wrapper may have failed", guardFile) + return fmt.Errorf("CRITICAL: Guard file not found at %s - safe wrapper may have failed", guardFile) } - - // No cleanup needed - wrapper handles everything - return func() error { return nil }, nil + return nil } diff --git a/internal/gateway/testutil/dbtest/dbtest.go b/internal/gateway/testutil/dbtest/dbtest.go index d0a0e34c..a50877e1 100644 --- a/internal/gateway/testutil/dbtest/dbtest.go +++ b/internal/gateway/testutil/dbtest/dbtest.go @@ -4,6 +4,7 @@ import ( "crypto/rand" "database/sql" "encoding/binary" + "errors" "fmt" "net/url" "os" @@ -88,9 +89,7 @@ func setupAdminConnection(t *testing.T, baseDSN string) (*sql.DB, func()) { func generateTestDBName() string { var b [8]byte - if _, err := rand.Read(b[:]); err != nil { - panic(fmt.Sprintf("failed to generate random bytes: %v", err)) - } + _, _ = rand.Read(b[:]) // never returns an error (crashes the program instead) // Mask to ensure we stay within int64 positive range (0 to MaxInt64) // G115: Masked to prevent overflow randomNum := int64(binary.LittleEndian.Uint64(b[:]) & 0x7FFFFFFFFFFFFFFF) @@ -109,22 +108,20 @@ func buildTestDSN(t *testing.T, baseDSN, dbName string) string { func waitForDatabaseReady(t *testing.T, dsn string) { t.Helper() - for i := 0; i < 300; i++ { - testConn, err := sql.Open("pgx", dsn) - if err == nil { - if err = testConn.Ping(); err == nil { - if cerr := testConn.Close(); cerr != nil { - t.Fatalf("close test connection: %v", cerr) - } - return - } - if cerr := testConn.Close(); cerr != nil { - t.Fatalf("close test connection: %v", cerr) - } - } - time.Sleep(50 * time.Millisecond) + err := retry(15*time.Second, 50*time.Millisecond, func() (bool, error) { + return true, pingDatabase(dsn) + }) + if err != nil { + t.Fatalf("database did not become ready in time: %v", err) + } +} + +func pingDatabase(dsn string) error { + conn, err := sql.Open("pgx", dsn) + if err != nil { + return err } - t.Fatal("database did not become ready in time") + return errors.Join(conn.Ping(), conn.Close()) } // testPoolConfig is the connection pool of each test database. diff --git a/internal/gateway/testutil/dbtest/retry.go b/internal/gateway/testutil/dbtest/retry.go new file mode 100644 index 00000000..88a360d4 --- /dev/null +++ b/internal/gateway/testutil/dbtest/retry.go @@ -0,0 +1,26 @@ +package dbtest + +import ( + "context" + "time" +) + +// retry calls attempt every interval until it succeeds, returns an error it can't retry, or timeout passes. It +// returns attempt's last error (nil on success). +func retry(timeout, interval time.Duration, attempt func() (retryable bool, err error)) error { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + retryable, err := attempt() + if err == nil || !retryable { + return err + } + select { + case <-ctx.Done(): + return err + case <-ticker.C: + } + } +} diff --git a/internal/gateway/testutil/dbtest/template.go b/internal/gateway/testutil/dbtest/template.go index f892bf6e..4e279676 100644 --- a/internal/gateway/testutil/dbtest/template.go +++ b/internal/gateway/testutil/dbtest/template.go @@ -93,16 +93,16 @@ func ensureTemplate(t *testing.T, admin *sql.DB, baseDSN string) string { // Postgres container restarts once after initializing (e.g. the first test run in CI). func adminConn(ctx context.Context, t *testing.T, admin *sql.DB) *sql.Conn { t.Helper() - var err error - for attempt := 0; attempt < 100; attempt++ { - var conn *sql.Conn - if conn, err = admin.Conn(ctx); err == nil { - return conn - } - time.Sleep(100 * time.Millisecond) + var conn *sql.Conn + err := retry(10*time.Second, 100*time.Millisecond, func() (bool, error) { + var err error + conn, err = admin.Conn(ctx) + return true, err + }) + if err != nil { + t.Fatalf("template connection: %v", err) } - t.Fatalf("template connection: %v", err) - return nil + return conn } // migrateTemplate runs every migration in the template and closes all its connections, so it can be copied. @@ -133,15 +133,11 @@ func migrateTemplate(t *testing.T, dsn string) { func cloneTemplate(t *testing.T, admin *sql.DB, name, template string) { t.Helper() query := strings.Join([]string{"CREATE DATABASE", pqQuoteIdent(name), "TEMPLATE", pqQuoteIdent(template)}, " ") - var err error - for attempt := 0; attempt < 50; attempt++ { - if _, err = admin.Exec(query); err == nil { - return - } - if !strings.Contains(err.Error(), "is being accessed by other users") { - break - } - time.Sleep(100 * time.Millisecond) + err := retry(5*time.Second, 100*time.Millisecond, func() (bool, error) { + _, err := admin.Exec(query) + return err != nil && strings.Contains(err.Error(), "is being accessed by other users"), err + }) + if err != nil { + t.Fatalf("create database %s from template %s: %v", name, template, err) } - t.Fatalf("create database %s from template %s: %v", name, template, err) } diff --git a/internal/integration/main_test.go b/internal/integration/main_test.go index 24999d04..1888373e 100644 --- a/internal/integration/main_test.go +++ b/internal/integration/main_test.go @@ -8,13 +8,5 @@ import ( ) func TestMain(m *testing.M) { - cleanup, err := convoxguard.Setup() - if err != nil { - panic(err) - } - code := m.Run() - if err := cleanup(); err != nil { - panic("CRITICAL: " + err.Error()) - } - os.Exit(code) + os.Exit(convoxguard.Run(m)) } diff --git a/main_test.go b/main_test.go index 873bae5c..a2face89 100644 --- a/main_test.go +++ b/main_test.go @@ -8,15 +8,5 @@ import ( ) func TestMain(m *testing.M) { - cleanup, err := convoxguard.Setup() - if err != nil { - // fail fast if we can't protect prod config - panic(err) - } - code := m.Run() - if err := cleanup(); err != nil { - // CRITICAL: Failed to restore config - this is more important than test results - panic("CRITICAL: " + err.Error()) - } - os.Exit(code) + os.Exit(convoxguard.Run(m)) } diff --git a/scripts/lib/cli-e2e/cli_helpers.sh b/scripts/lib/cli-e2e/cli_helpers.sh index c05a5f68..4bbb237b 100644 --- a/scripts/lib/cli-e2e/cli_helpers.sh +++ b/scripts/lib/cli-e2e/cli_helpers.sh @@ -147,5 +147,5 @@ verify_rgw_command_failure() { logout_cli() { echo -e "${YELLOW}Logging out...${NC}" - verify_rgw_command "logout" "Logged out from e2e" + verify_rgw_command "logout" "Logged out from e2e" "the session is ended on the gateway" } diff --git a/web/src/api/generated.ts b/web/src/api/generated.ts index 922b48ad..9f0c9aa5 100644 --- a/web/src/api/generated.ts +++ b/web/src/api/generated.ts @@ -48,6 +48,7 @@ import type { HandlersCLILoginResponse, HandlersCLILoginStartRequest, HandlersCLILoginStartResponse, + HandlersCLILogoutResponse, HandlersConfirmTOTPEnrollmentRequest, HandlersConfirmWebAuthnEnrollmentRequest, HandlersCreateAPITokenRequest, @@ -617,6 +618,23 @@ export const getRackGatewayAPI = () => { ); }; + /** + * Revokes the session whose token is sent as `Authorization: Bearer `. `rack-gateway logout` + * calls it before removing the rack from its config. Cookies are ignored, so a browser can't be made + * to call it, and API tokens get 401: they're revoked on the API Tokens page. + * @summary Log out a CLI session + */ + const postAuthCliLogout = ( + options?: SecondParameter< + typeof createGatewayClient + >, + ) => { + return createGatewayClient( + { url: `/auth/cli/logout`, method: "POST" }, + options, + ); + }; + /** * Sends the bound browser to MFA (or MFA enrollment), or back to the CLI when MFA is satisfied. * @summary Continue CLI login in the browser @@ -1528,6 +1546,7 @@ export const getRackGatewayAPI = () => { postAuthCliCancel, postAuthCliComplete, getAuthCliInitiator, + postAuthCliLogout, getAuthCliMfa, getAuthCliReturn, postAuthCliStart, @@ -1745,6 +1764,9 @@ export type GetAuthCliInitiatorResult = NonNullable< ReturnType["getAuthCliInitiator"]> > >; +export type PostAuthCliLogoutResult = NonNullable< + Awaited["postAuthCliLogout"]>> +>; export type GetAuthCliMfaResult = NonNullable< Awaited["getAuthCliMfa"]>> >; diff --git a/web/src/api/openapi.json b/web/src/api/openapi.json index 90c864b7..a96a0c44 100644 --- a/web/src/api/openapi.json +++ b/web/src/api/openapi.json @@ -1978,6 +1978,57 @@ } } }, + "/auth/cli/logout": { + "post": { + "description": "Revokes the session whose token is sent as `Authorization: Bearer `. `rack-gateway logout`\ncalls it before removing the rack from its config. Cookies are ignored, so a browser can't be made\nto call it, and API tokens get 401: they're revoked on the API Tokens page.", + "tags": [ + "Auth" + ], + "summary": "Log out a CLI session", + "responses": { + "200": { + "description": "OK", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.CLILogoutResponse" + } + } + } + }, + "401": { + "description": "Unauthorized", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + }, + "500": { + "description": "Internal Server Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + }, + "503": { + "description": "Service Unavailable", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/handlers.ErrorResponse" + } + } + } + } + } + } + }, "/auth/cli/mfa": { "get": { "description": "Sends the bound browser to MFA (or MFA enrollment), or back to the CLI when MFA is satisfied.", @@ -5912,6 +5963,14 @@ } } }, + "handlers.CLILogoutResponse": { + "type": "object", + "properties": { + "revoked": { + "type": "boolean" + } + } + }, "handlers.ConfirmTOTPEnrollmentRequest": { "type": "object", "required": [ diff --git a/web/src/api/schemas/handlersCLILogoutResponse.ts b/web/src/api/schemas/handlersCLILogoutResponse.ts new file mode 100644 index 00000000..bfbebca6 --- /dev/null +++ b/web/src/api/schemas/handlersCLILogoutResponse.ts @@ -0,0 +1,11 @@ +/** + * Generated by orval 🍺 + * Do not edit manually. + * Rack Gateway API + * API for the Rack Gateway administration and proxy services. + * OpenAPI spec version: 1.0 + */ + +export interface HandlersCLILogoutResponse { + revoked?: boolean; +} diff --git a/web/src/api/schemas/index.ts b/web/src/api/schemas/index.ts index a04ed8e2..a9dc86cb 100644 --- a/web/src/api/schemas/index.ts +++ b/web/src/api/schemas/index.ts @@ -65,6 +65,7 @@ export * from "./handlersCLILoginRedirectResponse"; export * from "./handlersCLILoginResponse"; export * from "./handlersCLILoginStartRequest"; export * from "./handlersCLILoginStartResponse"; +export * from "./handlersCLILogoutResponse"; export * from "./handlersConfirmTOTPEnrollmentRequest"; export * from "./handlersConfirmWebAuthnEnrollmentRequest"; export * from "./handlersCreateAPITokenRequest"; diff --git a/web/src/api/types.generated.ts b/web/src/api/types.generated.ts index b849cf92..f10c14a1 100644 --- a/web/src/api/types.generated.ts +++ b/web/src/api/types.generated.ts @@ -1708,6 +1708,74 @@ export interface paths { patch?: never; trace?: never; }; + "/auth/cli/logout": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + /** + * Log out a CLI session + * @description Revokes the session whose token is sent as `Authorization: Bearer `. `rack-gateway logout` + * calls it before removing the rack from its config. Cookies are ignored, so a browser can't be made + * to call it, and API tokens get 401: they're revoked on the API Tokens page. + */ + post: { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description OK */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["handlers.CLILogoutResponse"]; + }; + }; + /** @description Unauthorized */ + 401: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["handlers.ErrorResponse"]; + }; + }; + /** @description Internal Server Error */ + 500: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["handlers.ErrorResponse"]; + }; + }; + /** @description Service Unavailable */ + 503: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["handlers.ErrorResponse"]; + }; + }; + }; + }; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/auth/cli/mfa": { parameters: { query?: never; @@ -5209,6 +5277,9 @@ export interface components { "handlers.CLILoginStartResponse": { auth_url: string; }; + "handlers.CLILogoutResponse": { + revoked?: boolean; + }; "handlers.ConfirmTOTPEnrollmentRequest": { code: string; label?: string; From 2609477e654dd8b462e8e3c3b541c2a1d3f4944f Mon Sep 17 00:00:00 2001 From: Nathan Broadbent Date: Tue, 13 Oct 2026 14:49:49 +1300 Subject: [PATCH 2/2] chore: lint the whole module locally, as CI does CI runs golangci-lint on ./..., which includes scripts/test-libfido2.go (a command-line program, so fmt.Print is its output). task go:lint only linted cmd, internal and tools, so it passed locally. It now lints ./... too, with node_modules excluded (a web dependency ships Go files), and reruns when the lint config or scripts change. Co-Authored-By: Claude Opus 5.5 --- .golangci.yml | 4 +++- taskfiles/Taskfile.go.yml | 12 ++++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 4fd51ca2..61154e5f 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -35,6 +35,7 @@ linters: - reference - tmp - bin + - node_modules rules: - path: '(.+)_test\.go' linters: @@ -42,7 +43,7 @@ linters: - funlen - goconst # Command-line programs print their output. - - path: '^(cmd|internal/cli)/' + - path: '^(cmd|internal/cli|scripts)/' linters: - forbidigo text: 'fmt\.Print' @@ -123,6 +124,7 @@ formatters: - reference - tmp - bin + - node_modules settings: gofmt: simplify: true diff --git a/taskfiles/Taskfile.go.yml b/taskfiles/Taskfile.go.yml index 5d7390ea..657f44ed 100644 --- a/taskfiles/Taskfile.go.yml +++ b/taskfiles/Taskfile.go.yml @@ -232,13 +232,17 @@ tasks: sources: - Taskfile.go.yml - go.* + - "*.go" - cmd/**/*.go - internal/**/*.go - tools/**/*.go + - scripts/**/*.go + - .golangci.yml + - scripts/check_go_nolint.sh cmds: # The release variant (no build tags) is what ships. Only the testbypass package differs with the tag, so the # second run lints just that package's testbypass files. - - golangci-lint run ./cmd/... ./internal/... ./tools/... + - golangci-lint run ./... - golangci-lint run --build-tags testbypass ./internal/gateway/testbypass/... - scripts/check_go_nolint.sh @@ -248,11 +252,15 @@ tasks: sources: - Taskfile.go.yml - go.* + - "*.go" - cmd/**/*.go - internal/**/*.go - tools/**/*.go + - scripts/**/*.go + - .golangci.yml + - scripts/check_go_nolint.sh cmds: - - golangci-lint run --fix ./cmd/... ./internal/... ./tools/... + - golangci-lint run --fix ./... - golangci-lint run --fix --build-tags testbypass ./internal/gateway/testbypass/... - scripts/check_go_nolint.sh