diff --git a/.github/workflows/platform-ci.yml b/.github/workflows/platform-ci.yml index 05520bf..763efe2 100644 --- a/.github/workflows/platform-ci.yml +++ b/.github/workflows/platform-ci.yml @@ -239,6 +239,15 @@ jobs: working-directory: ${{ env.TF_ROOT }} run: terraform apply -auto-approve -lock-timeout=5m tfplan + # Re-sync app Cognito clients with the Lambda/config just applied, so + # parameters it adds (cognito-client-identity-provider, auth-status) + # exist before an app redeploys. Async; unchanged values are not + # rewritten, so nothing restarts. Non-blocking: the 03:30 reconcile + # catches up anyway. + - name: Reconcile app Cognito clients + continue-on-error: true + run: sh scripts/reconcile-app-auth.sh + # -------------------------------------------------------------------------- # Drift Detection — scheduled weekly, plan-only # -------------------------------------------------------------------------- diff --git a/.github/workflows/tf-apply.yml b/.github/workflows/tf-apply.yml index e2c8d99..b69a8ea 100644 --- a/.github/workflows/tf-apply.yml +++ b/.github/workflows/tf-apply.yml @@ -93,3 +93,13 @@ jobs: - name: Terraform Apply working-directory: ${{ inputs.tf_root }} run: terraform apply -auto-approve tfplan + + # Non-blocking: checks Cognito sign-in right after apply (sync errors, + # redirect_mismatch, logout URL mismatch, unknown auth.groups). Waits + # for app-auth-sync to process the new request. Skips apps without auth. + - name: Auth doctor + if: hashFiles('app.yaml') != '' + continue-on-error: true + env: + APP_SERVICE: ${{ github.event.repository.name }} + run: python3 .platform/scripts/auth-doctor.py --app-yaml app.yaml --wait 180 diff --git a/CLAUDE.md b/CLAUDE.md index ab9f499..9ec3274 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -193,6 +193,8 @@ terraform/state/ | `scripts/invoke-apply-gate.sh` | CI: invoke gate Lambda for apply credentials | | `scripts/run-plan.sh` | CI: terraform plan with exit code handling | | `scripts/upload-plan.sh` | CI: upload plan artifact to S3 | +| `scripts/auth-doctor.py` | CI (after app apply) / local: checks an app's Cognito sign-in end to end from auth-status + HTTP probes (tests: `scripts/tests/`) | +| `scripts/reconcile-app-auth.sh` | CI (after platform apply): queue an app-auth-sync reconcile | ## Naming @@ -254,7 +256,9 @@ All parameters are in `eu-central-1`. Use `--profile javabin --region eu-central | `/javabin/platform/atlassian-api-key` | SecureString | team-provisioner (Atlassian org admin API key, unscoped — full org admin) | | `/javabin/platform-overrides/{repo}/{sha}` | SecureString | Risk gate override tokens (single-use) | | `/javabin/apps/{team}/{app}/auth` | String | app.yaml `auth:` request (written by app TF, read by app-auth-sync) | -| `/javabin/apps/{team}/{app}/cognito-client-{id,secret}` | String / SecureString | Written by app-auth-sync, read by ECS execution role | +| `/javabin/apps/{team}/{app}/cognito-client-{id,secret,identity-provider}` | String / SecureString / String | Written by app-auth-sync (only on change; a change restarts the app), read by ECS execution role | +| `/javabin/apps/{team}/{app}/auth-status` | String (JSON) | Written by app-auth-sync, read by the team and `scripts/auth-doctor.py` | +| `/javabin/platform/google-oauth/internal/{client-id,client-secret}` | String / SecureString | Internal pool's Google OIDC IdP (identity TF; IdP is created only when both exist) | ## GCP Connection @@ -296,7 +300,7 @@ The SA JSON key is at `/javabin/platform/google-admin-sa`, the impersonation tar ### Known Issues - **ECS deploy stabilization**: platform-test-app task registers but service fails health check -- **Cognito**: pools deployed; internal pool's `GoogleSAML` IdP was created outside Terraform +- **Cognito**: pools deployed; internal pool's `GoogleSAML` IdP was created outside Terraform. It is replaced by the Terraform-managed Google OIDC IdP at cutover (docs/cognito-google-setup.md), then deleted - **SSO sessions carry no `team` tag**: the Identity Center ABAC attribute (`${path:enterprise.team}`) is never set on users, so `${aws:PrincipalTag/team}` is empty for humans. Per-team permission sets (`javabin-team-{team}`) therefore use `javabin-team-{team}-guardrails` (the developer boundary rendered with the team name) instead. The ABAC-based `javabin-developer` permission set in `terraform/org` cannot reach team resources - **App databases share one network path**: service-rds allows ingress from the shared ECS tasks SG, so every team's tasks can reach every team's DB port (password still required). Fix: per-app security groups - **Team provisioner Lambda**: All sync functions working (Google/GitHub/Budget/Cognito/Identity Center). Password-set flow deployed. diff --git a/docs/app-yaml-reference.md b/docs/app-yaml-reference.md index b6f6dc9..419336f 100644 --- a/docs/app-yaml-reference.md +++ b/docs/app-yaml-reference.md @@ -200,11 +200,13 @@ Full form: auth: pool: internal # 'internal' (default) or 'external' client: confidential # confidential (default) | public | none — see below - callback_urls: # optional; https, or http://localhost for local dev + callback_urls: # optional; https on routing.host / extra_hosts, or http://localhost - https://app.javazone.no/auth/callback - http://localhost:3000/auth/callback logout_urls: # optional; same rules - https://app.javazone.no/ + extra_hosts: # optional — other hosts the app serves callbacks on + - www.app.javazone.no scopes: # optional, default ['openid', 'email', 'profile'] - openid - email @@ -214,6 +216,7 @@ auth: env: # optional — also inject values under your own names COGNITO_CLIENT_ID: NUXT_PUBLIC_OIDC_CLIENT_ID COGNITO_ISSUER_URL: NUXT_PUBLIC_OIDC_AUTHORITY + COGNITO_IDENTITY_PROVIDER: NUXT_PUBLIC_OIDC_IDENTITY_PROVIDER ``` #### `scopes` @@ -226,17 +229,17 @@ Any of `openid`, `email`, `profile`, `phone`, `aws.cognito.signin.user.admin`. T |---|---|---| | `confidential` | Server-side login (the server does the code exchange) | Client ID **and** secret | | `public` | Browser apps doing OIDC with PKCE (oidc-client-ts, etc.) — a secret can't be kept in a browser | Client ID, no secret | -| `none` | APIs that only *verify* tokens issued to another app's client | Pool/issuer values only; no client, no callbacks, no `routing.host` needed | +| `none` | APIs that only *verify* tokens issued to another app's client | Pool/issuer values only; no client, no callbacks, no `routing.host` needed. `callback_urls`, `logout_urls`, `scopes` and `extra_hosts` are rejected (they would be ignored) | A typical split frontend/backend: the frontend uses `client: public`, the API uses `client: none` and validates tokens against `COGNITO_ISSUER_URL` / `COGNITO_WELL_KNOWN_URL`. A `client: none` API has no client ID of its own to check, so it accepts tokens issued to *any* client in the pool. Check `cognito:groups` (or the token's `client_id`) for anything that matters. -Changing `client` between `confidential` and `public` replaces the Cognito client, so its ID changes. Running tasks keep the old ID until they restart: redeploy after the change. +Changing `client` between `confidential` and `public` replaces the Cognito client, so its ID changes. The platform restarts the service when the new ID is written (app-secret-rollout), so running tasks don't keep the deleted ID. SPAs that bake the client ID in at *build* time (Vite `VITE_*`, Next `NEXT_PUBLIC_*` at build) must rebuild; runtime config (Nuxt `NUXT_PUBLIC_*`) picks it up on restart. Both forms are supported and produce the same wiring. -- `internal` — Javabin heroes. The hosted UI sends users straight to Google Workspace (`GoogleSAML`), so apps don't need an `identity_provider` parameter. There is no password form or sign-up. Only `@java.no` accounts get a token; anyone else is refused at sign-in. +- `internal` — Javabin heroes. Every internal client signs in through the pool's IdP list (`internal_identity_providers` in `terraform/platform/identity/`): today Google Workspace SAML (`GoogleSAML`), after the Google cutover only `Google`. With a single IdP the hosted UI sends users straight to it, so apps don't need an `identity_provider` parameter; apps that pass one anyway should read `COGNITO_IDENTITY_PROVIDER` instead of hardcoding `GoogleSAML`. There is no password form or sign-up. Only `@java.no` accounts get a token; anyone else is refused at sign-in. - `external` — public users. - `none` or omitted — no Cognito integration. @@ -247,16 +250,42 @@ Both forms are supported and produce the same wiring. If `callback_urls` is omitted, defaults to `[https://{routing.host}/, https://{routing.host}/auth/callback]`. If `logout_urls` is omitted, defaults to `[https://{routing.host}/]`. +An explicit list *replaces* the defaults. Apps that redirect elsewhere (Spring: `/login/oauth2/code/cognito`, NextAuth: `/api/auth/callback/cognito`) must list that path. + +#### URL rules (checked when terraform/ is generated) + +- `https://`, except `http://localhost` / `http://127.0.0.1` for local development. +- The host must be `routing.host` or listed in `auth.extra_hosts` (localhost excepted), so a typo fails in the PR instead of as `redirect_mismatch` in the browser. +- No `#fragment` (also rejected by `app-auth-sync`). + +The host rule runs when `app.yaml` is expanded (`generate-modules.py`); the platform Lambda checks only the scheme and fragments, so it is a guard against typos, not a security boundary. + +#### Logout + +Cognito's `/logout?client_id=…&logout_uri=…` redirects back only when `logout_uri` matches a `logout_urls` entry **exactly** (trailing slash and path included). Anything else lands on the hosted UI's `/login` page. Cognito ignores `post_logout_redirect_uri`; send `logout_uri`. + +#### Sign-in denied + +A user in none of `auth.groups` gets no token: Cognito redirects to your callback with `error=access_denied&error_description=PreTokenGeneration failed with error You are not in a group that has access to this app.` Render `error_description` on your callback page instead of looping back to sign-in. + #### How the client is created App CI cannot create Cognito clients: the pools belong to the platform, and a grant on a pool would let any team rewrite every other team's client. So the app only *requests* one: 1. The generated Terraform writes the client config to SSM `/javabin/apps/{team}/{name}/auth`. The permission boundary lets a team write only under its own prefix, so the path proves ownership. -2. The platform Lambda `javabin-app-auth-sync` reacts to the change (EventBridge), creates or updates the client `{team}-{name}` in the requested pool, and writes `/javabin/apps/{team}/{name}/cognito-client-id` and `cognito-client-secret`. +2. The platform Lambda `javabin-app-auth-sync` reacts to the change (EventBridge), creates or updates the client `{team}-{name}` in the requested pool, and writes `/javabin/apps/{team}/{name}/cognito-client-id`, `cognito-client-secret` and `cognito-client-identity-provider` (only when a value changes; a change restarts the service). It reports the outcome in `/javabin/apps/{team}/{name}/auth-status` (JSON, readable by the team): `status` (`synced` / `invalid` / `error` + `error`), `client_id`, `identity_providers`, `drift_corrected` (client settings someone changed outside the platform, reverted by the daily reconcile), `unknown_groups` (`auth.groups` entries that don't exist in the registry — nobody can sign in through them), `client_id_changed_at`, `synced_at`. Your team's SSO role cannot write it, but your CI role can (as it can the `cognito-client-*` parameters); the next sync overwrites any such change, so treat it as a report. 3. Deleting `auth:` deletes the request parameter, and the Lambda deletes the client and its credentials. A daily reconcile repairs anything an event missed. On the very first deploy with `auth:`, the first task can start a few seconds before the credentials exist; ECS retries it. +#### auth doctor + +`scripts/auth-doctor.py` (platform repo) checks the whole chain with the team's own credentials: the request matches app.yaml, `auth-status` is `synced` with the same client ID, `/oauth2/authorize` redirects to the IdP for every callback URL (no `redirect_mismatch`), `/logout` returns to every logout URL, and (warning only) the service's current deployment started after the client ID last changed. App CI runs it after `terraform apply` (non-blocking; failures show as annotations). Locally, from the app repo: + +```bash +python3 ../platform/scripts/auth-doctor.py --app-yaml app.yaml # add --json for scripts +``` + #### Injected environment variables | Var | Source | Notes | @@ -267,6 +296,7 @@ On the very first deploy with `auth:`, the first task can start a few seconds be | `COGNITO_WELL_KNOWN_URL` | `{issuer}/.well-known/openid-configuration` | OIDC discovery document | | `COGNITO_CLIENT_ID` | SSM (ECS task `secrets`) | `confidential` and `public` only | | `COGNITO_CLIENT_SECRET` | SSM SecureString (ECS task `secrets`) | `confidential` only | +| `COGNITO_IDENTITY_PROVIDER` | SSM (ECS task `secrets`) | `confidential` and `public` only. The IdP the client signs in through (`GoogleSAML`, `Google`, or `COGNITO`), for apps that send `identity_provider` themselves. Changes at the IdP cutover, which restarts the service | | `COGNITO_GROUPS` | Comma-joined list from `auth.groups` | Only when `groups` is set | `auth.env` adds the same values under extra names (both names are injected). Only names injected for your `client` type can be aliased. diff --git a/docs/cognito-google-setup.md b/docs/cognito-google-setup.md index e21c105..f84b6df 100644 --- a/docs/cognito-google-setup.md +++ b/docs/cognito-google-setup.md @@ -1,152 +1,147 @@ -# Cognito + Google IdP Setup - -Manual steps required alongside Terraform-managed infrastructure for Cognito user pools with Google Sign-In. - -## Two Separate Google Integrations - -This platform uses Google in two distinct ways. They require separate credentials and serve different purposes. - -| Integration | Purpose | Credential Type | Stored In | -|------------|---------|----------------|-----------| -| Domain-wide delegation SA | Team provisioner manages Google Groups via Admin SDK | GCP Service Account JSON key | SSM `/javabin/platform/google-admin-sa` | -| OAuth 2.0 Client | User-facing Google Sign-In in Cognito pools | OAuth Client ID + Secret | Cognito IdP config (Terraform) | - -The domain-wide delegation SA already exists in the `javabin-platform` GCP project. The OAuth client below is a separate credential that must be created. - -## 1. Google Cloud Console: OAuth Client - -### Prerequisites -- Access to `javabin-platform` GCP project (under `java.no` org) -- Permissions: `roles/oauthconfig.editor` or project Owner - -### 1.1 Configure OAuth Consent Screen - -1. Go to **APIs & Services > OAuth consent screen** in the `javabin-platform` project -2. Select **External** user type (needed for the external Cognito pool) -3. Fill in: - - App name: `Javabin Platform` - - User support email: `platform@java.no` (or board contact) - - Developer contact: same -4. Scopes: add `openid`, `email`, `profile` -5. Test users: skip (not needed once published) -6. Publish the app (move from Testing to Production) — Google review may take a few days - -### 1.2 Create OAuth 2.0 Client ID - -1. Go to **APIs & Services > Credentials > Create Credentials > OAuth client ID** -2. Application type: **Web application** -3. Name: `javabin-cognito` -4. Authorized redirect URIs — add both Cognito pool callback URLs: - ``` - https://auth-internal.javazone.no/oauth2/idpresponse - https://auth-external.javazone.no/oauth2/idpresponse - ``` - These are the Cognito hosted UI domains. Adjust if using custom domains. -5. Click **Create** and note the **Client ID** and **Client Secret** - -These values are passed to Terraform as variables when creating the Cognito identity provider resources. They are not stored in SSM — Cognito holds them internally. - -## 2. Cognito User Pools (Terraform-managed) - -The `terraform/platform/identity/` module (not yet implemented) will create two Cognito user pools. Below is what Terraform will manage and what to verify manually. - -### 2.1 Internal Pool (`javabin-internal`) - -For Javabin heroes and board members — Google Workspace accounts only. - -**Terraform creates:** -- User pool with email as primary attribute -- Google IdP with `hd=java.no` restriction (hosted domain claim) -- Hosted UI domain: `auth-internal.javazone.no` -- Default app client for platform admin UI - -**Google IdP config (in Terraform):** -```hcl -resource "aws_cognito_identity_provider" "google_internal" { - user_pool_id = aws_cognito_user_pool.internal.id - provider_name = "Google" - provider_type = "Google" - - provider_details = { - client_id = var.google_oauth_client_id - client_secret = var.google_oauth_client_secret - authorize_scopes = "openid email profile" - attributes_url = "https://people.googleapis.com/v1/people/me?personFields=" - attributes_url_add_attributes = true - authorize_url = "https://accounts.google.com/o/oauth2/v2/auth" - oidc_issuer = "https://accounts.google.com" - token_url = "https://oauth2.googleapis.com/token" - token_request_method = "POST" - } - - attribute_mapping = { - email = "email" - name = "name" - username = "sub" - } -} +# Cognito + Google Sign-In Setup + +How the internal Cognito pool (`javabin-internal`, heroes with `@java.no` accounts) signs users in with +Google, and how to cut over from the `GoogleSAML` provider. Background: javaBin/platform#157 (personal +Gmail accounts hit a dead-end `403 app_not_configured_for_user` page on the SAML app, with no way to +pick another account). + +## Why OIDC, not SAML + +Google's SAML IdP always answers for the browser's *active* Google session and has no account chooser. +The classic hosted UI drops `prompt=`, so Cognito cannot ask for one either. The Google OIDC provider +can: its `authorize_url` carries `prompt=select_account`, and the OAuth client lives in a GCP project +with an **Internal** consent screen, so Google itself refuses non-java.no accounts on a page that lets +the user switch account. + +| Integration | Purpose | Credential | Stored in | +|---|---|---|---| +| Domain-wide delegation SA | team-provisioner manages Google Groups (Admin SDK) | Service account JSON key | SSM `/javabin/platform/google-admin-sa` | +| Internal-pool OAuth client | Google sign-in for the internal pool | OAuth client ID + secret | SSM `/javabin/platform/google-oauth/internal/{client-id,client-secret}` | +| External-pool OAuth client (later) | Google sign-in for the external pool | OAuth client ID + secret | not set up yet (`google_client_id` tfvar) | + +The internal client must be separate from any external-pool client: its consent screen is Internal, +the external one would be External. + +## 1. GCP: create the OAuth client (one-time, GCP admin) + +Project: `javabin-platform` (under the `java.no` organisation). Needs `roles/oauthconfig.editor` or Owner. + +1. **APIs & Services → OAuth consent screen** (Google Auth Platform → Branding/Audience): + - User type / Audience: **Internal**. Only `java.no` Workspace accounts can complete sign-in; Google + shows other accounts an error page *with* the account switcher. + - App name `Javabin`, support email and developer contact `platform@java.no` (or a board address). + - Scopes (Data access): `openid`, `.../auth/userinfo.email`, `.../auth/userinfo.profile`. + - Internal apps need no Google verification or publishing. +2. **APIs & Services → Credentials → Create credentials → OAuth client ID**: + - Application type: **Web application**, name `cognito-javabin-internal`. + - Authorized JavaScript origins: none. + - Authorized redirect URI — exactly one, the pool's Cognito domain: + ``` + https://javabin-internal.auth.eu-central-1.amazoncognito.com/oauth2/idpresponse + ``` + (`terraform output` → `internal_user_pool_domain` + `/oauth2/idpresponse`. There is no + `auth-internal.javazone.no` domain; an earlier version of this doc listed it by mistake.) + - Create, and copy the client ID and secret. + +## 2. AWS: store the client in SSM (platform admin) + +```bash +aws ssm put-parameter --profile javabin --region eu-central-1 \ + --name /javabin/platform/google-oauth/internal/client-id --type String \ + --value '.apps.googleusercontent.com' +aws ssm put-parameter --profile javabin --region eu-central-1 \ + --name /javabin/platform/google-oauth/internal/client-secret --type SecureString \ + --value '' ``` -**Manual verification after `terraform apply`:** -1. Visit `https://auth-internal.javazone.no/login?client_id=&response_type=code&scope=openid+email+profile&redirect_uri=` -2. Confirm only `@java.no` accounts can sign in -3. Verify the `hd` claim filtering works (non-java.no accounts get rejected) +Then re-run platform CI (any merge, or re-run the latest `main` workflow). The plan reads the path with +`aws_ssm_parameters_by_path` and creates `aws_cognito_identity_provider.internal_google` (`Google`) +once both parameters exist. Until then the plan creates nothing, which is why the Terraform could merge +first. -### 2.2 External Pool (`javabin-external`) +Adding the IdP changes **no app client**: clients use only the IdPs in `internal_identity_providers` +(`terraform/platform/identity/variables.tf`, default `["GoogleSAML"]`). GoogleSAML keeps +working. -For public users — JavaZone attendees, community members. +What Terraform configures (`terraform/platform/identity/main.tf`): -**Terraform creates:** -- User pool with email as primary attribute -- Google IdP **without** `hd` restriction (any Google account) -- Self-registration enabled -- Hosted UI domain: `auth-external.javazone.no` +- `authorize_scopes = "openid email profile"` +- `authorize_url = https://accounts.google.com/o/oauth2/v2/auth?hd=java.no&prompt=select_account` + (`hd=` preselects java.no accounts; it is only a hint) +- attribute mapping `email`, `email_verified`, `name`, `username = sub` -**Key differences from internal:** -- No hosted domain restriction on Google IdP -- Self-registration allowed -- Optional: GitHub social IdP (if configured) -- MFA optional (not enforced) +## 3. Verify the account chooser survives (after the IdP exists) -**Manual verification after `terraform apply`:** -1. Test sign-in with a non-java.no Google account -2. Test self-registration with email/password -3. Verify email verification flow works +Cognito keeping query parameters in a `Google`-type `authorize_url` is not documented. Probe it with +any internal client ID and one of its callback URLs (no credentials needed; cupcake's are in +`/javabin/apps/cupcake/cupcake/{cognito-client-id,auth}`): -## 3. App Client Registration - -Apps declare their auth needs in `app.yaml`: - -```yaml -auth: internal # or: external, both, none +```bash +DOMAIN=javabin-internal.auth.eu-central-1.amazoncognito.com +curl -s -o /dev/null -w '%{redirect_url}\n' \ + "https://$DOMAIN/oauth2/authorize?response_type=code&client_id=&redirect_uri=&scope=openid&identity_provider=Google" \ + | grep -q 'prompt=select_account' && echo OK || echo "prompt dropped" ``` -The `cognito-app-client` module (`terraform/modules/cognito-app-client/`) creates: -- Cognito user pool client(s) in the specified pool(s) -- SSM parameters with client credentials: - - `/{project}/platform-apps/{name}/cognito-internal-client-id` - - `/{project}/platform-apps/{name}/cognito-internal-client-secret` - - `/{project}/platform-apps/{name}/cognito-external-client-id` - - `/{project}/platform-apps/{name}/cognito-external-client-secret` - -Apps read these SSM parameters at runtime — no secrets in env vars or code. - -## 4. Checklist - -### Before Terraform deployment -- [ ] OAuth consent screen configured and published in GCP -- [ ] OAuth 2.0 Client ID created with correct redirect URIs -- [ ] Client ID and Secret available for Terraform variables -- [ ] Domain-wide delegation SA already in SSM (separate from OAuth client) - -### After Terraform deployment -- [ ] Internal pool: `@java.no` Google Sign-In works -- [ ] Internal pool: non-`@java.no` accounts are rejected -- [ ] External pool: any Google account can sign in -- [ ] External pool: self-registration works -- [ ] Hosted UI domains resolve correctly (Route53 + ACM) -- [ ] Test app client credentials in SSM are readable - -### DNS records needed (created by identity module) -- `auth-internal.javazone.no` — CNAME to Cognito internal pool domain -- `auth-external.javazone.no` — CNAME to Cognito external pool domain +`identity_provider=Google` works before cutover only if the client lists `Google`; before cutover, use +a test client (or run the probe right after cutover — `scripts/auth-doctor.py` does the same check and +warns when `prompt=select_account` is missing). + +**If the parameters are dropped (fallback):** set `google_authorize_url_params = false`, switch the +domain to managed login (`aws_cognito_user_pool_domain.internal` → `managed_login_version = 2`), give +every client a branding style (app-auth-sync: `CreateManagedLoginBranding(UseCognitoProvidedValues=True)`), +and send `prompt=select_account` from the app (SPA `extraQueryParams`, ALB +`authentication_request_extra_params`). Managed login forwards `prompt` to OIDC IdPs; the classic UI +does not. + +## 4. Cutover: GoogleSAML → Google + +Prerequisites: + +- The Google IdP exists (step 2) and the probe passes (step 3). +- **Hard requirement:** server-side domain enforcement is live — the internal-pool lockdown (#166: + auth-claims `REQUIRED_EMAIL_DOMAIN`, no self sign-up, no `COGNITO` on internal clients). The Internal + consent screen already blocks other Google accounts, but the pool must not rely on one GCP setting. + This branch builds on that change; the identity module also refuses `Google` in the list while + `google_workspace_domain` is empty, and auth-claims requires `email_verified=true` from Google users + (Google is never trusted on the IdP name alone). + +Steps: + +1. PR: `internal_identity_providers = ["Google"]` (identity `variables.tf` default). The same list feeds + app clients and auth-claims' `FEDERATED_IDENTITY_PROVIDERS`, so one change moves both. The plan fails + on an output precondition if the Google credentials are not in SSM. After apply, platform CI queues an + app-auth-sync reconcile: every internal client switches to `Google` only, and + `cognito-client-identity-provider` changes to `Google`, which restarts each app (app-secret-rollout) + so `COGNITO_IDENTITY_PROVIDER` is current. +2. Check: `python3 scripts/auth-doctor.py --app-yaml /app.yaml` for each auth app (cupcake), or the + doctor step in the app's next deploy. Expect `authorize … redirects to Google (accounts.google.com)`. +3. Sign in once yourself with a personal Gmail active in the browser: Google must show the account chooser. + +### Users after cutover + +Google users are new Cognito users: username `Google_`, a **new `sub`**. Today the pool has +4 users, all `GoogleSAML_` (external-provider users; there are no native users to link to — +Cognito can only link a federated identity onto a native user). + +- The platform keys everything on **email**, not `sub`: registry group membership (`group#` + members) and the auth-claims lookup use the email claim; per-client allowed groups are keyed by client + ID. Nothing in the platform needs migrating. +- Apps that stored data keyed by `sub` (check cupcake/frosting) must re-key by email or map old to new. +- Delete the 4 `GoogleSAML_*` users a week after cutover (`aws cognito-idp admin-delete-user`); they are + harmless meanwhile. If a Google sign-in ever fails because the email already exists, delete that + user's `GoogleSAML_*` entry first. + +### Decommission GoogleSAML (a week later) + +1. Delete the `GoogleSAML` provider from the pool (created outside Terraform; nothing in Terraform + references it). app-auth-sync errors if a pool's declared list still names it. +2. Turn off the Google Admin SAML app "Javabin Apps (Cognito)". +3. Mark `docs/cognito-saml-setup.md` for removal and drop the CLAUDE.md known issue. + +## External pool + +The external pool (`javabin-external`) still uses the `google_client_id` / `google_client_secret` +variables (empty, so no Google IdP) and `external_identity_providers = ["COGNITO"]`. Setting it up needs +a separate OAuth client with an **External** consent screen (Google verification for public use) and a +redirect URI on the external pool's domain. diff --git a/docs/cognito-saml-setup.md b/docs/cognito-saml-setup.md index 487c4d2..8f05fec 100644 --- a/docs/cognito-saml-setup.md +++ b/docs/cognito-saml-setup.md @@ -1,5 +1,11 @@ # Cognito SAML Setup — Google Workspace Federation +> **Superseded.** The internal pool moves to the Google OIDC provider (account chooser, Internal +> consent screen): see [cognito-google-setup.md](cognito-google-setup.md). This page describes the +> `GoogleSAML` provider that was created by hand and stays only until the cutover. The live provider maps +> `email` only — the groups attribute mapping described below was never configured; registry groups come +> from the auth-claims trigger. + ## Overview The internal Cognito pool authenticates Javabin heroes via Google Workspace SAML. diff --git a/docs/lambda-functions.md b/docs/lambda-functions.md index 9049982..d98bdce 100644 --- a/docs/lambda-functions.md +++ b/docs/lambda-functions.md @@ -125,11 +125,17 @@ Idempotent: skips resources that already have a `created-by` tag (preserves orig **Trigger:** EventBridge "Parameter Store Change" for `/javabin/apps/*/*/auth` (Create/Update/Delete), plus a daily `{"action": "reconcile"}` at 03:30 UTC **Purpose:** Creates, updates and deletes the Cognito app clients requested by `app.yaml` `auth:`. The request parameter's path identifies the team (only that team can write there), so no caller-supplied identity is trusted. -**Flow:** Re-read the request → validate (pool, client_type, https/localhost URLs, scopes, allowed_groups) → upsert client `{team}-{app}` in the requested pool with that pool's fixed sign-in IdP list (a client_type change replaces the client, since the secret setting is immutable) → remove it from the other pool → write `/javabin/apps/{team}/{app}/cognito-client-id` (String) and, for confidential clients, `cognito-client-secret` (SecureString), tagged `team={team}` → record `allowed_groups` in `javabin-auth-memberships`. A missing request deletes the client and both parameters. Reconcile also removes clients whose credential parameter exists but whose request is gone; clients created by other means are never touched. +**Flow:** Re-read the request → validate (pool, client_type, https/localhost URLs without `#fragment`, scopes, allowed_groups) → upsert client `{team}-{app}` in the requested pool with that pool's fixed sign-in IdP list (a client_type change replaces the client, since the secret setting is immutable) → remove it from the other pool → write `/javabin/apps/{team}/{app}/cognito-client-id` (String), for confidential clients `cognito-client-secret` (SecureString), and `cognito-client-identity-provider` (String), tagged `team={team}`, **only when the value changed** (secret first, ID last) → record `allowed_groups` in `javabin-auth-memberships` → write `auth-status`. A missing request deletes the client, its parameters and `auth-status`. Reconcile also removes clients whose credential parameter exists but whose request is gone; clients created by other means are never touched. The Lambda checks URL scheme and fragments only; the host rules (one of the app's own hosts or `extra_hosts`) run in `generate-modules.py` at generate time, and a team writing the request parameter directly bypasses them. -**Identity providers:** every client in a pool gets exactly the pool's list, from `INTERNAL_IDENTITY_PROVIDERS` / `EXTERNAL_IDENTITY_PROVIDERS` (comma-separated; set from the identity module's `internal_identity_providers` = `["GoogleSAML"]` and `external_identity_providers` = `["COGNITO"]`). With a single IdP the hosted UI redirects straight to it, so internal apps show no password form or sign-up link. Reconcile overwrites console edits to the list. If a configured IdP does not exist in the pool, the request is skipped with an `ERROR` log and status `error` (reconcile lists it under `error`): nothing is created, moved or changed, and the existing client keeps working. The CloudWatch alarm `javabin-app-auth-sync-skipped` (metric filter on `client left unchanged`) posts to `javabin-alerts` when this happens. Internal-pool requests asking for `aws.cognito.signin.user.admin` are rejected as invalid (that scope would let a user rewrite the email auth-claims trusts). +**Identity providers:** every client in a pool gets exactly the pool's list, from `INTERNAL_IDENTITY_PROVIDERS` / `EXTERNAL_IDENTITY_PROVIDERS` (comma-separated; set from the identity module's `internal_identity_providers` = `["GoogleSAML"]` and `external_identity_providers` = `["COGNITO"]`). With a single IdP the hosted UI redirects straight to it, so internal apps show no password form or sign-up link. Reconcile overwrites console edits to the list (reported as drift). If a configured IdP does not exist in the pool, the request is skipped with an `ERROR` log and status `error` (reconcile lists it under `error`): nothing is created, moved or changed, and the existing client keeps working. Internal-pool requests asking for `aws.cognito.signin.user.admin` are rejected as invalid (that scope would let a user rewrite the email auth-claims trusts). -**Environment vars:** `PROJECT`, `MEMBERSHIPS_TABLE`, `INTERNAL_IDENTITY_PROVIDERS`, `EXTERNAL_IDENTITY_PROVIDERS` +**Errors:** reconcile syncs every app even if one fails, and returns normally (raising would make the async EventBridge/CI invoke retry the whole run twice). The CloudWatch alarm `javabin-app-auth-sync-skipped` (metric filter on `client left unchanged` or `Sync failed for`) posts to `javabin-alerts` for a skipped request or a failed app. A failing change event still raises (Lambda `Errors`). + +**auth-status** (`/javabin/apps/{team}/{app}/auth-status`, String JSON, team-readable): `status` (`synced` / `invalid` / `error`), `error`, `client_id`, `client_id_changed`, `client_id_changed_at` (when the ID last changed, carried over between syncs), `client_type`, `pool`, `pool_domain`, `identity_providers`, `identity_provider`, `callback_urls`, `logout_urls`, `scopes`, `allowed_groups`, `unknown_groups` (no `group#` item in the memberships table), `empty_groups`, `changed_fields`, `drift_corrected` (fields a *reconcile* changed back: someone edited the client outside the platform; also logged as `WARN drift`), `request_version`, `synced_at`. When the JSON would exceed 4096 bytes, list fields are dropped (URLs first) and `truncated: true` is set; it is always valid JSON. `scripts/auth-doctor.py` reads it. + +**Who can write it:** the team SSO permission sets (`javabin-team-{team}`, and `javabin-developer` in `terraform/org`) deny writes to `auth-status`, `cognito-client-*`, `auth` and `db-master-password`. Team CI roles and gate-vended apply credentials (developer boundary, which allows `ssm:PutParameter` under `/javabin/apps/{team}/*`) can still write them, as they always could for `cognito-client-*`. The effect stays inside the team's own app: a forged auth-status, or a changed credential that the next sync puts back (and one extra restart). Treat auth-status as a report, not an authority. + +**Environment vars:** `PROJECT`, `MEMBERSHIPS_TABLE`, `INTERNAL_IDENTITY_PROVIDERS`, `EXTERNAL_IDENTITY_PROVIDERS`, `INTERNAL_POOL_DOMAIN`, `EXTERNAL_POOL_DOMAIN`. Platform CI queues a reconcile after every apply (`scripts/reconcile-app-auth.sh`). **Concurrency:** reserved concurrency 1, so two changes to one app cannot race into duplicate clients. @@ -140,9 +146,9 @@ Idempotent: skips resources that already have a `created-by` tag (preserves orig **Trigger:** Cognito pre-token-generation (V2) on the internal pool (`javabin-internal`), every sign-in and token refresh **Purpose:** Refuses tokens for accounts outside the Workspace domain, puts the user's registry groups into `cognito:groups` (ID and access token), and refuses sign-in when the app client has `allowed_groups` and the user is in none of them. -**Domain check:** with `REQUIRED_EMAIL_DOMAIN` set (internal pool: `var.google_workspace_domain`, `java.no`), a token is issued only when the email ends with `@java.no` (case-insensitive) **and** the user signed in through one of `FEDERATED_IDENTITY_PROVIDERS` (the internal pool's IdP list, `GoogleSAML`) or has `email_verified=true`. Federated users are recognised by the `identities` attribute, or by their `_` username. Anyone else gets "Sign in with your @java.no account." on the app's callback. Every IdP in that list is trusted to vouch for the email (no `email_verified` check), so it must release only Workspace accounts; see the comment on `internal_identity_providers` before adding one (for example a Google OIDC IdP). A function without `REQUIRED_EMAIL_DOMAIN` skips the check. The internal pool also has self sign-up disabled (`allow_admin_create_user_only`). +**Domain check:** with `REQUIRED_EMAIL_DOMAIN` set (internal pool: `var.google_workspace_domain`, `java.no`), a token is issued only when the email ends with `@java.no` (case-insensitive) **and** the user signed in through one of `FEDERATED_IDENTITY_PROVIDERS` (the internal pool's IdP list, `GoogleSAML`) or has `email_verified=true`. Federated users are recognised by the `identities` attribute, or by their `_` username. Anyone else gets "Sign in with your @java.no account." on the app's callback. Every IdP in that list is trusted to vouch for the email (no `email_verified` check), so it must release only Workspace accounts; see the comment on `internal_identity_providers` before adding one. Exception: IdPs in `EMAIL_VERIFIED_IDENTITY_PROVIDERS` (default `Google`, the OIDC IdP) also need `email_verified=true`, which the pool maps from Google's claim; the identity module refuses `Google` in the list until its OAuth client is in SSM and `google_workspace_domain` is set. A function without `REQUIRED_EMAIL_DOMAIN` skips the check. The internal pool also has self sign-up disabled (`allow_admin_create_user_only`). -**Environment vars:** `MEMBERSHIPS_TABLE`, `REQUIRED_EMAIL_DOMAIN`, `FEDERATED_IDENTITY_PROVIDERS` +**Environment vars:** `MEMBERSHIPS_TABLE`, `REQUIRED_EMAIL_DOMAIN`, `FEDERATED_IDENTITY_PROVIDERS`, `EMAIL_VERIFIED_IDENTITY_PROVIDERS` (optional, default `Google`) **Data:** `javabin-auth-memberships` (DynamoDB). `group#` items (members' emails) are written by team-provisioner on registry merges; `client#` items (`allowed_groups`) by app-auth-sync. Memberships are cached for 60 s per container. @@ -164,7 +170,7 @@ Team roles are denied writing `risk.json`, `review.json` and `override.json` in ## app-secret-rollout **Trigger:** EventBridge "Parameter Store Change" (Update) under `/javabin/apps/` -**Purpose:** ECS resolves task secrets only at start, so after a team sets a secret value (`javabin secret set`) this forces a new deployment of `{team}-{app}` in `javabin-platform`. Teams have no `ecs:UpdateService` themselves (it would also allow changing networking and task definitions). Platform-managed parameters (`auth`, `cognito-client-*`, `db-master-password`) are ignored; a missing service is logged and skipped. +**Purpose:** ECS resolves task secrets only at start, so after a team sets a secret value (`javabin secret set`) this forces a new deployment of `{team}-{app}` in `javabin-platform`. Teams have no `ecs:UpdateService` themselves (it would also allow changing networking and task definitions). Cognito client parameters (`cognito-client-id`, `-secret`, `-identity-provider`) also restart the app: app-auth-sync writes them only when the value changes (replaced client, IdP cutover), so running tasks never keep a deleted client ID and a no-op reconcile restarts nothing. Ignored: `auth`, `auth-status` (rewritten every sync), `db-master-password`. A missing service is logged and skipped. ## Shared Module: pricing diff --git a/docs/reusable-modules.md b/docs/reusable-modules.md index e7b37fb..24fe7c4 100644 --- a/docs/reusable-modules.md +++ b/docs/reusable-modules.md @@ -151,7 +151,7 @@ Auto-wires: resource env vars (bucket names, table names, queue URLs), secret AR Requests a Cognito OAuth client for the app. It does **not** create the client: it writes the request to SSM `/{project}/apps/{team}/{app_name}/auth`, and the platform `app-auth-sync` Lambda creates the client `{team}-{app_name}` and writes its credentials next to the request (`cognito-client-id`, `cognito-client-secret`). See [app-yaml-reference.md#auth](app-yaml-reference.md#auth) for why. **Inputs:** `app_name`, `team`, `pool` (internal/external), `client_type` (confidential/public/none), `callback_urls`, `logout_urls`, `allowed_oauth_scopes`, `allowed_groups`, `external_pool_custom_domain` -**Outputs:** `user_pool_id`, `user_pool_domain`, `issuer_url`, `well_known_url`, `client_id_arn`, `client_secret_arn` (SSM ARNs for ECS `secrets`), `request_parameter_name` +**Outputs:** `user_pool_id`, `user_pool_domain`, `issuer_url`, `well_known_url`, `client_id_arn`, `client_secret_arn`, `identity_provider_arn` (SSM ARNs for ECS `secrets`), `auth_status_parameter_name`, `request_parameter_name` ## Policy Composition Pattern diff --git a/scripts/auth-doctor.py b/scripts/auth-doctor.py new file mode 100755 index 0000000..5c3b470 --- /dev/null +++ b/scripts/auth-doctor.py @@ -0,0 +1,372 @@ +#!/usr/bin/env python3 +"""auth doctor — check that an app's Cognito sign-in works end to end. + +Uses only what the app's team can read: app.yaml, the app's own +/javabin/apps/{team}/{app}/* parameters, and unauthenticated HTTP to the +pool's hosted UI. Teams have no cognito-idp access; app-auth-sync reports +what it did in the auth-status parameter instead. + +Checks: + 1. The auth request parameter matches what app.yaml generates. + 2. auth-status says synced, for this request version, and its client ID is + the one in cognito-client-id. Reports drift the platform corrected and + allowed groups that do not exist (a typo there denies everyone). + 3. GET /oauth2/authorize for every callback URL redirects to the sign-in + IdP (not /error?error=redirect_mismatch). For Google, also that the + account chooser is forced (prompt=select_account). + 4. GET /logout for every logout URL redirects back to it. Cognito wants + logout_uri to match a logout URL exactly (trailing slash included) and + otherwise sends the user to its own /login or /error page. + 5. Best effort (WARN only): the service's PRIMARY-deployment tasks started + after the client ID last changed (auth-status client_id_changed_at). + +Usage: + python3 scripts/auth-doctor.py [--app-yaml app.yaml] [--team T] [--app A] + [--wait SECONDS] [--json] + +Runs after `terraform apply` in tf-apply.yml (non-blocking); the javabin CLI +can wrap it as `javabin auth doctor` (--json gives machine-readable output). +Needs the aws CLI with credentials that can read the app's parameters. +Exit code 1 when a check fails, 0 otherwise (warnings included). +""" + +import argparse +import datetime +import importlib.util +import json +import os +import subprocess +import sys +import time +import urllib.error +import urllib.request +from urllib.parse import parse_qs, urlencode, urlsplit + +HERE = os.path.dirname(os.path.abspath(__file__)) +PROJECT = "javabin" +CLUSTER = f"{PROJECT}-platform" + + +def _load_generator(): + spec = importlib.util.spec_from_file_location("generate_modules", os.path.join(HERE, "generate-modules.py")) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +# --------------------------------------------------------------------------- +# I/O (replaced in tests) +# --------------------------------------------------------------------------- + +def aws(*args): + """Run an aws CLI command; returns parsed JSON, or raises RuntimeError(stderr).""" + r = subprocess.run(["aws", *args, "--output", "json"], capture_output=True, text=True) + if r.returncode != 0: + raise RuntimeError(r.stderr.strip() or f"aws {' '.join(args)} failed") + return json.loads(r.stdout or "null") + + +def ssm_get(name): + """{Value, Version, LastModifiedDate} of a String parameter, or None if missing.""" + try: + return aws("ssm", "get-parameter", "--name", name)["Parameter"] + except RuntimeError as e: + if "ParameterNotFound" in str(e): + return None + raise + + +class _NoRedirect(urllib.request.HTTPRedirectHandler): + def redirect_request(self, *args, **kwargs): + return None + + +def http_get(url): + """(status, Location header) without following redirects.""" + opener = urllib.request.build_opener(_NoRedirect) + try: + with opener.open(urllib.request.Request(url, method="GET"), timeout=15) as resp: + return resp.status, resp.headers.get("Location", "") + except urllib.error.HTTPError as e: + return e.code, e.headers.get("Location", "") + + +# --------------------------------------------------------------------------- +# Checks +# --------------------------------------------------------------------------- + +class Report: + def __init__(self): + self.results = [] + + def add(self, level, check, message): + self.results.append({"level": level, "check": check, "message": message}) + + def ok(self, check, message): + self.add("PASS", check, message) + + def warn(self, check, message): + self.add("WARN", check, message) + + def fail(self, check, message): + self.add("FAIL", check, message) + + @property + def failed(self): + return any(r["level"] == "FAIL" for r in self.results) + + +def expected_request(auth): + return { + "pool": auth["pool"], + "client_type": auth["client"], + "callback_urls": auth["callback_urls"], + "logout_urls": auth["logout_urls"], + "scopes": auth["scopes"], + "allowed_groups": auth["groups"], + } + + +def check_request(report, base, auth): + param = ssm_get(f"{base}/auth") + if param is None: + report.fail("request", f"{base}/auth does not exist — has terraform apply run with the auth module?") + return None + try: + actual = json.loads(param["Value"]) + except ValueError: + report.fail("request", f"{base}/auth is not JSON") + return None + want = expected_request(auth) + diff = sorted(k for k in want if actual.get(k, [] if isinstance(want[k], list) else None) != want[k]) + if diff: + report.fail("request", f"{base}/auth differs from app.yaml in {', '.join(diff)} (not applied yet?)") + else: + report.ok("request", f"{base}/auth matches app.yaml (version {param.get('Version')})") + return param + + +# auth-status missing altogether: the platform has never synced this app (no +# reconcile since the platform change, or a failed one). Don't sit out the +# full --wait for that; a first deploy's event sync takes seconds. +MISSING_STATUS_WAIT = 30 + + +class StatusNotJSON(ValueError): + pass + + +def read_status(base): + param = ssm_get(f"{base}/auth-status") + if not param: + return None + try: + status = json.loads(param["Value"]) + except ValueError: + raise StatusNotJSON(f"{base}/auth-status is not JSON") + if not isinstance(status, dict): + raise StatusNotJSON(f"{base}/auth-status is not a JSON object") + return status + + +def wait_for_status(base, request_version, wait, sleep=time.sleep): + """auth-status for this request version, polling up to `wait` seconds.""" + start = time.monotonic() + while True: + status = read_status(base) + if status and (request_version is None or status.get("request_version") == request_version): + return status, True + limit = wait if status else min(wait, MISSING_STATUS_WAIT) + if time.monotonic() - start >= limit: + return status, False + sleep(5) + + +def check_status(report, base, status, current): + if status is None: + report.fail("status", f"{base}/auth-status does not exist — app-auth-sync has not synced this app yet") + return False + if not current: + report.warn("status", f"auth-status is for request version {status.get('request_version')}, " + f"not the current one (sync still running?) — checking the last result") + if status.get("status") != "synced": + report.fail("status", f"app-auth-sync: {status.get('status')}: {status.get('error')}") + return False + client_param = ssm_get(f"{base}/cognito-client-id") + if client_param is None or client_param["Value"] != status.get("client_id"): + report.fail("status", f"cognito-client-id ({client_param and client_param['Value']}) " + f"!= synced client {status.get('client_id')}") + return False + report.ok("status", f"synced {status.get('synced_at')}: client {status.get('client_id')} in " + f"{status.get('pool')} pool, sign-in via {', '.join(status.get('identity_providers') or [])}") + if status.get("drift_corrected"): + report.warn("drift", f"someone changed the client outside the platform; reverted: " + f"{', '.join(status['drift_corrected'])}") + if status.get("unknown_groups"): + report.fail("groups", f"auth.groups {status['unknown_groups']} do not exist in the registry " + f"memberships — nobody can sign in through them (typo?)") + if status.get("empty_groups"): + report.warn("groups", f"auth.groups {status['empty_groups']} have no members") + return True + + +def probe_authorize(report, domain, client_id, idp, scopes, callback): + query = {"response_type": "code", "client_id": client_id, "redirect_uri": callback, + "scope": " ".join(scopes)} + if idp and idp != "COGNITO": + query["identity_provider"] = idp + code, location = http_get(f"https://{domain}/oauth2/authorize?{urlencode(query)}") + loc = urlsplit(location) + if code not in (301, 302, 303, 307) or not location: + report.fail("authorize", f"{callback}: HTTP {code} instead of a redirect") + return + if loc.hostname == domain and loc.path.startswith("/error"): + err = parse_qs(loc.query) + report.fail("authorize", f"{callback}: Cognito error {err.get('error', ['?'])[0]} " + f"{err.get('error_description', [''])[0]}".rstrip()) + return + if idp == "COGNITO": + report.ok("authorize", f"{callback}: redirects to the hosted UI ({loc.path})") + return + if loc.hostname == domain: + report.fail("authorize", f"{callback}: stays on the hosted UI ({loc.path}) instead of going to {idp}") + return + report.ok("authorize", f"{callback}: redirects to {idp} ({loc.hostname})") + if loc.hostname == "accounts.google.com" and "/o/oauth2/" in loc.path: + if parse_qs(loc.query).get("prompt") != ["select_account"]: + report.warn("authorize", "Google is not asked to show the account chooser (prompt=select_account " + "missing) — see docs/cognito-google-setup.md, managed-login fallback") + + +def probe_logout(report, domain, client_id, logout_url): + """A matching logout_uri redirects straight back to it; a mismatch lands on + the hosted UI (/login or /error) — checked live against the classic UI.""" + code, location = http_get(f"https://{domain}/logout?{urlencode({'client_id': client_id, 'logout_uri': logout_url})}") + loc = urlsplit(location) + if code not in (301, 302, 303, 307) or not location: + report.fail("logout", f"{logout_url}: HTTP {code} instead of a redirect") + elif location == logout_url or location.startswith(logout_url): + report.ok("logout", f"{logout_url}: redirects back to the app") + elif loc.hostname == domain: + report.fail("logout", f"{logout_url}: Cognito sends the user to {loc.path} instead — logout_uri must " + f"match a logout URL exactly (trailing slash, path)") + else: + report.warn("logout", f"{logout_url}: redirects to {loc.scheme}://{loc.hostname}{loc.path} (IdP sign-out?)") + + +def check_tasks(report, team, app, status): + """Best effort: PRIMARY-deployment tasks that started before the client ID + last changed still hold the old one. WARN only — the timestamp is the + platform's record of a real ID change, absent for apps not resynced since.""" + changed_at = status.get("client_id_changed_at") + if not changed_at: + report.add("SKIP", "tasks", "auth-status has no client_id_changed_at yet; skipping the task check") + return + try: + services = aws("ecs", "describe-services", "--cluster", CLUSTER, "--services", f"{team}-{app}")["services"] + primary = next((d["id"] for s in services for d in s.get("deployments", []) if d.get("status") == "PRIMARY"), + None) + arns = aws("ecs", "list-tasks", "--cluster", CLUSTER, "--service-name", f"{team}-{app}")["taskArns"] + if not arns: + report.warn("tasks", f"no running tasks in {team}-{app}") + return + tasks = aws("ecs", "describe-tasks", "--cluster", CLUSTER, "--tasks", *arns)["tasks"] + except (RuntimeError, KeyError) as e: + report.add("SKIP", "tasks", f"could not read ECS tasks: {str(e).splitlines()[0][:160]}") + return + if primary: + # Tasks of a deployment that is still draining are replaced anyway + tasks = [t for t in tasks if t.get("startedBy") == primary] + changed = _ts(changed_at) + stale = [t["taskArn"].rsplit("/", 1)[-1] for t in tasks if t.get("startedAt") and _ts(t["startedAt"]) < changed] + if stale: + report.warn("tasks", f"{len(stale)} task(s) started before the client ID changed ({changed_at}) and may " + f"use the old client: {', '.join(stale)} — app-secret-rollout should be restarting them") + else: + report.ok("tasks", f"{len(tasks)} running task(s) started after the client ID last changed") + + +def _ts(value): + if isinstance(value, (int, float)): + return float(value) + return datetime.datetime.fromisoformat(str(value).replace("Z", "+00:00")).timestamp() + + +def diagnose(app, team=None, name=None, wait=0, tasks=True, sleep=time.sleep): + gen = _load_generator() + report = Report() + team = team or app.get("team") + name = name or app.get("name") + auth = gen.resolve_auth(app) + if not auth: + report.add("SKIP", "auth", "app.yaml has no auth section") + return report + if auth["client"] == "none": + report.add("SKIP", "auth", "client: none — validate-only app, no Cognito client to check") + return report + + base = f"/{PROJECT}/apps/{team}/{name}" + request = check_request(report, base, auth) + if request is None: + return report + try: + status, current = wait_for_status(base, request.get("Version"), wait, sleep) + except StatusNotJSON as e: + report.fail("status", str(e)) + return report + if not check_status(report, base, status, current): + return report + + domain = status.get("pool_domain") + if not domain: + report.warn("authorize", f"the {status.get('pool')} pool has no hosted-UI domain; skipping HTTP probes") + else: + for cb in status.get("callback_urls") or auth["callback_urls"]: + probe_authorize(report, domain, status["client_id"], status.get("identity_provider"), + status.get("scopes") or auth["scopes"], cb) + for lo in status.get("logout_urls") or auth["logout_urls"]: + probe_logout(report, domain, status["client_id"], lo) + if tasks: + check_tasks(report, team, name, status) + return report + + +def main(argv=None): + p = argparse.ArgumentParser(description=__doc__.split("\n")[0]) + p.add_argument("--app-yaml", default="app.yaml") + p.add_argument("--team", help="default: app.yaml team") + p.add_argument("--app", help="default: app.yaml name, then $APP_SERVICE, then the repository name") + p.add_argument("--wait", type=int, default=0, help="seconds to wait for app-auth-sync to finish (CI: 180)") + p.add_argument("--no-tasks", action="store_true", help="skip the ECS task check") + p.add_argument("--json", action="store_true", help="print results as JSON") + args = p.parse_args(argv) + + if not os.path.exists(args.app_yaml): + print(f"No {args.app_yaml}; nothing to check") + return 0 + gen = _load_generator() + app = gen.load_yaml(args.app_yaml) or {} + name = args.app or app.get("name") or os.environ.get("APP_SERVICE") or \ + os.environ.get("GITHUB_REPOSITORY", "").rsplit("/", 1)[-1] + try: + report = diagnose(app, args.team, name, args.wait, tasks=not args.no_tasks) + except gen.AppYamlError as e: + print(f"app.yaml: {e}", file=sys.stderr) + return 1 + + if args.json: + print(json.dumps({"ok": not report.failed, "results": report.results}, indent=2)) + else: + for r in report.results: + line = f"{r['level']:4} {r['check']:9} {r['message']}" + if r["level"] == "FAIL" and os.environ.get("GITHUB_ACTIONS"): + line = f"::error title=auth doctor ({r['check']})::{r['message']}" + elif r["level"] == "WARN" and os.environ.get("GITHUB_ACTIONS"): + line = f"::warning title=auth doctor ({r['check']})::{r['message']}" + print(line) + print("auth doctor: " + ("FAILED" if report.failed else "OK")) + return 1 if report.failed else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/generate-modules.py b/scripts/generate-modules.py index b1d4562..7d4370c 100755 --- a/scripts/generate-modules.py +++ b/scripts/generate-modules.py @@ -22,6 +22,7 @@ import re import subprocess import sys +from urllib.parse import urlsplit sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) from registry import ( @@ -63,7 +64,7 @@ class AppYamlError(ValueError): "routing": {"host", "priority", "deregistration_delay"}, "domain": None, "resources": {"buckets", "databases", "secrets", "queues"}, - "auth": {"pool", "client", "callback_urls", "logout_urls", "scopes", "groups", "env"}, + "auth": {"pool", "client", "callback_urls", "logout_urls", "scopes", "groups", "env", "extra_hosts"}, "environment": None, "alarms": {"enabled", "cpu_threshold", "memory_threshold", "error_5xx_threshold"}, } @@ -91,6 +92,8 @@ class AppYamlError(ValueError): RESOURCE_NAME_RE = re.compile(r"^[a-z0-9][a-z0-9_-]*$") ENV_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") HOST_LABEL_RE = re.compile(r"^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?$") +HOSTNAME_RE = re.compile(r"^(?=.{1,253}$)([a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?\.)+[a-z]{2,63}$") +LOCAL_HOSTS = ("localhost", "127.0.0.1") def host_zone(host): @@ -236,6 +239,98 @@ def module_block(name, source, attrs): return "\n".join(lines) +# --------------------------------------------------------------------------- +# Auth +# --------------------------------------------------------------------------- + +def _check_auth_urls(key, urls, allowed_hosts): + """Callback/logout URLs: https (http only for localhost), no fragment, and a + host the app serves, so a typo fails here instead of as redirect_mismatch.""" + if not isinstance(urls, list) or not all(isinstance(u, str) and u for u in urls): + raise AppYamlError(f"auth.{key} must be a list of URLs") + for u in urls: + parts = urlsplit(u) + host = (parts.hostname or "").lower() + local = host in LOCAL_HOSTS + if parts.scheme not in ("https", "http") or not host: + raise AppYamlError(f"auth.{key}: {u!r} is not an absolute https:// URL") + if parts.scheme == "http" and not local: + raise AppYamlError(f"auth.{key}: {u!r} must use https:// (plain http is only allowed for localhost)") + if "#" in u: + raise AppYamlError(f"auth.{key}: {u!r} must not contain a #fragment (OAuth forbids it)") + if not local and host not in allowed_hosts: + where = f"routing.host ({', '.join(sorted(allowed_hosts))})" if allowed_hosts else "routing.host" + raise AppYamlError( + f"auth.{key}: host {host!r} in {u!r} is not this app's {where}; " + f"list other hosts the app serves in auth.extra_hosts" + ) + + +def resolve_auth(app, routing=None): + """The app's validated auth settings with defaults applied, or None. + + {pool, client, callback_urls, logout_urls, scopes, scopes_set, groups, + aliases}. scripts/auth-doctor.py uses this to know what the request + parameter should contain. + """ + routing = routing if routing is not None else dict(app.get("routing") or {}) + if app.get("domain") and not routing.get("host"): + routing = {**routing, "host": app["domain"]} + auth_raw = app.get("auth") + if not auth_raw or auth_raw == "none": + return None + # Accept shorthand: auth: internal -> {pool: internal} + auth = {"pool": auth_raw} if isinstance(auth_raw, str) else auth_raw + pool = auth.get("pool", "internal") + if pool == "both": + raise AppYamlError( + "auth.pool: 'both' is not supported yet — pick 'internal' or 'external'." + ) + if pool not in ("internal", "external"): + raise AppYamlError(f"auth.pool must be 'internal' or 'external' (got {pool!r})") + + client = auth.get("client", "confidential") + if client not in AUTH_CLIENT_TYPES: + raise AppYamlError(f"auth.client must be one of {', '.join(AUTH_CLIENT_TYPES)} (got {client!r})") + + groups = auth.get("groups") or [] + if not isinstance(groups, list) or not all(isinstance(g, str) and g for g in groups): + raise AppYamlError("auth.groups must be a list of group names") + + aliases = auth.get("env") or {} + if not isinstance(aliases, dict): + raise AppYamlError("auth.env must map injected names to extra variable names") + + if client == "none": + # Validate-only apps get no client; these settings would be silently ignored + ignored = [k for k in ("callback_urls", "logout_urls", "scopes", "extra_hosts") if k in auth] + if ignored: + raise AppYamlError( + f"auth.client: none requests no Cognito client, so {', '.join(ignored)} would be ignored — " + f"remove them, or use client: public / confidential" + ) + return {"pool": pool, "client": client, "callback_urls": [], "logout_urls": [], "scopes": [], + "scopes_set": False, "groups": groups, "aliases": aliases} + + extra_hosts = auth.get("extra_hosts") or [] + if not isinstance(extra_hosts, list) or not all(isinstance(h, str) and HOSTNAME_RE.match(h) for h in extra_hosts): + raise AppYamlError("auth.extra_hosts must be a list of hostnames (e.g. [www.javazone.no]), no scheme or path") + + host = routing.get("host") + if not host and not auth.get("callback_urls"): + raise AppYamlError("auth: requires routing.host or auth.callback_urls (callback URLs default to the host)") + allowed_hosts = {h.lower() for h in [host, *extra_hosts] if h} + callback_urls = auth.get("callback_urls") or [f"https://{host}/", f"https://{host}/auth/callback"] + logout_urls = auth.get("logout_urls") or ([f"https://{host}/"] if host else []) + _check_auth_urls("callback_urls", callback_urls, allowed_hosts) + _check_auth_urls("logout_urls", logout_urls, allowed_hosts) + + scopes = auth.get("scopes") + return {"pool": pool, "client": client, "callback_urls": callback_urls, "logout_urls": logout_urls, + "scopes": scopes or ["openid", "email", "profile"], "scopes_set": bool(scopes), + "groups": groups, "aliases": aliases} + + # --------------------------------------------------------------------------- # Main generation # --------------------------------------------------------------------------- @@ -407,46 +502,23 @@ def src(mod): # Auth (Cognito app client) — optional. The module only *requests* the # client; the platform app-auth-sync Lambda creates it (see module docs). - auth_raw = app.get("auth") - if auth_raw and auth_raw != "none": - # Accept shorthand: auth: internal -> {pool: internal} - auth = {"pool": auth_raw} if isinstance(auth_raw, str) else auth_raw - pool = auth.get("pool", "internal") - if pool == "both": - raise AppYamlError( - "auth.pool: 'both' is not supported yet — pick 'internal' or 'external'." - ) - if pool not in ("internal", "external"): - raise AppYamlError(f"auth.pool must be 'internal' or 'external' (got {pool!r})") - - client = auth.get("client", "confidential") - if client not in AUTH_CLIENT_TYPES: - raise AppYamlError(f"auth.client must be one of {', '.join(AUTH_CLIENT_TYPES)} (got {client!r})") - - groups = auth.get("groups") or [] - if not isinstance(groups, list) or not all(isinstance(g, str) and g for g in groups): - raise AppYamlError("auth.groups must be a list of group names") - + auth = resolve_auth(app, routing) + if auth: + client = auth["client"] auth_attrs = { "app_name": name, "team": team, - "pool": pool, + "pool": auth["pool"], "client_type": client, "project": PROJECT, } if client != "none": - host = routing.get("host") - if not host and not auth.get("callback_urls"): - raise AppYamlError("auth: requires routing.host or auth.callback_urls (callback URLs default to the host)") - auth_attrs["callback_urls"] = auth.get("callback_urls") or [ - f"https://{host}/", - f"https://{host}/auth/callback", - ] - auth_attrs["logout_urls"] = auth.get("logout_urls") or ([f"https://{host}/"] if host else []) - if groups: - auth_attrs["allowed_groups"] = groups - if auth.get("scopes"): - auth_attrs["allowed_oauth_scopes"] = auth["scopes"] + auth_attrs["callback_urls"] = auth["callback_urls"] + auth_attrs["logout_urls"] = auth["logout_urls"] + if auth["groups"]: + auth_attrs["allowed_groups"] = auth["groups"] + if auth["scopes_set"]: + auth_attrs["allowed_oauth_scopes"] = auth["scopes"] blocks.append(module_block("auth", src("cognito-app-client"), auth_attrs)) @@ -456,20 +528,20 @@ def src(mod): "COGNITO_ISSUER_URL": Ref("module.auth.issuer_url"), "COGNITO_WELL_KNOWN_URL": Ref("module.auth.well_known_url"), } - if groups: - auth_env["COGNITO_GROUPS"] = ",".join(groups) + if auth["groups"]: + auth_env["COGNITO_GROUPS"] = ",".join(auth["groups"]) auth_secrets = {} if client != "none": auth_secrets["COGNITO_CLIENT_ID"] = Ref("module.auth.client_id_arn") + # The IdP the client signs in through (written by app-auth-sync), + # for apps that pass identity_provider themselves + auth_secrets["COGNITO_IDENTITY_PROVIDER"] = Ref("module.auth.identity_provider_arn") if client == "confidential": auth_secrets["COGNITO_CLIENT_SECRET"] = Ref("module.auth.client_secret_arn") # auth.env: expose the same values under names the app already reads, # e.g. {COGNITO_CLIENT_ID: NUXT_PUBLIC_OIDC_CLIENT_ID} - aliases = auth.get("env") or {} - if not isinstance(aliases, dict): - raise AppYamlError("auth.env must map injected names to extra variable names") - for source, alias in aliases.items(): + for source, alias in auth["aliases"].items(): if source not in auth_env and source not in auth_secrets: available = ", ".join(sorted({*auth_env, *auth_secrets})) raise AppYamlError(f"auth.env: {source} is not injected for this app (available: {available})") diff --git a/scripts/reconcile-app-auth.sh b/scripts/reconcile-app-auth.sh new file mode 100755 index 0000000..84b8a05 --- /dev/null +++ b/scripts/reconcile-app-auth.sh @@ -0,0 +1,11 @@ +#!/bin/sh +# Queue an app-auth-sync reconcile (async: a busy Lambda queues it instead of +# throttling). Used by platform-ci after apply. +set -eu +aws lambda invoke \ + --function-name "${PROJECT:-javabin}-app-auth-sync" \ + --invocation-type Event \ + --cli-binary-format raw-in-base64-out \ + --payload '{"action":"reconcile"}' \ + /dev/null +echo "Queued app-auth-sync reconcile" diff --git a/scripts/tests/test_auth_doctor.py b/scripts/tests/test_auth_doctor.py new file mode 100644 index 0000000..2e37ca9 --- /dev/null +++ b/scripts/tests/test_auth_doctor.py @@ -0,0 +1,161 @@ +"""Tests for auth-doctor.py — run: python3 -m unittest discover scripts/tests""" + +import importlib.util +import json +import os +import unittest + +HERE = os.path.dirname(os.path.abspath(__file__)) +spec = importlib.util.spec_from_file_location("auth_doctor", os.path.join(HERE, "..", "auth-doctor.py")) +doc = importlib.util.module_from_spec(spec) +spec.loader.exec_module(doc) + +DOMAIN = "javabin-internal.auth.eu-central-1.amazoncognito.com" +BASE = "/javabin/apps/cupcake/cupcake" +APP = { + "name": "cupcake", "team": "cupcake", + "routing": {"host": "cupcake.javazone.no", "priority": 100}, + "auth": {"pool": "internal", "client": "public", + "callback_urls": ["https://cupcake.javazone.no/"], "logout_urls": ["https://cupcake.javazone.no/"], + "groups": ["helter"]}, +} +REQUEST = {"pool": "internal", "client_type": "public", "callback_urls": ["https://cupcake.javazone.no/"], + "logout_urls": ["https://cupcake.javazone.no/"], "scopes": ["openid", "email", "profile"], + "allowed_groups": ["helter"]} +STATUS = {"status": "synced", "client_id": "abc", "pool": "internal", "pool_domain": DOMAIN, + "identity_providers": ["Google"], "identity_provider": "Google", "request_version": 3, + "callback_urls": REQUEST["callback_urls"], "logout_urls": REQUEST["logout_urls"], + "scopes": REQUEST["scopes"], "unknown_groups": [], "empty_groups": [], "drift_corrected": [], + "synced_at": "2026-10-24T10:00:00+00:00"} +GOOGLE = "https://accounts.google.com/o/oauth2/v2/auth?hd=java.no&prompt=select_account&client_id=x" + + +class DoctorTest(unittest.TestCase): + def setUp(self): + self.params = { + f"{BASE}/auth": {"Value": json.dumps(REQUEST), "Version": 3}, + f"{BASE}/auth-status": {"Value": json.dumps(STATUS), "Version": 9}, + f"{BASE}/cognito-client-id": {"Value": "abc", "Version": 1}, + } + self.redirects = {"authorize": (302, GOOGLE), "logout": (302, "https://cupcake.javazone.no/")} + doc.ssm_get = lambda name: self.params.get(name) + doc.http_get = lambda url: self.redirects["authorize" if "/oauth2/authorize" in url else "logout"] + + def run_doctor(self, app=APP): + return doc.diagnose(app, wait=0, tasks=False, sleep=lambda s: None) + + def levels(self, report): + return {(r["level"], r["check"]) for r in report.results} + + def set_status(self, **changes): + self.params[f"{BASE}/auth-status"]["Value"] = json.dumps({**STATUS, **changes}) + + def test_healthy_app_passes(self): + report = self.run_doctor() + self.assertFalse(report.failed, report.results) + self.assertIn(("PASS", "authorize"), self.levels(report)) + self.assertIn(("PASS", "logout"), self.levels(report)) + + def test_redirect_mismatch_fails(self): + self.redirects["authorize"] = (302, f"https://{DOMAIN}/error?error=redirect_mismatch") + report = self.run_doctor() + self.assertTrue(report.failed) + self.assertIn("redirect_mismatch", json.dumps(report.results)) + + def test_logout_mismatch_fails(self): + # Live behaviour: a non-matching logout_uri lands on the hosted UI's /login + for location in (f"https://{DOMAIN}/error?error=x", f"https://{DOMAIN}/login?client_id=abc"): + self.redirects["logout"] = (302, location) + self.assertIn(("FAIL", "logout"), self.levels(self.run_doctor())) + + def test_missing_account_chooser_warns(self): + self.redirects["authorize"] = (302, "https://accounts.google.com/o/oauth2/v2/auth?client_id=x") + report = self.run_doctor() + self.assertFalse(report.failed) + self.assertIn(("WARN", "authorize"), self.levels(report)) + + def test_staying_on_hosted_ui_fails_for_federated_idp(self): + self.redirects["authorize"] = (302, f"https://{DOMAIN}/login?client_id=abc") + self.assertIn(("FAIL", "authorize"), self.levels(self.run_doctor())) + + def test_sync_error_and_stale_client_id_fail(self): + self.set_status(status="error", error="internal pool has no identity provider ['Google']") + self.assertIn(("FAIL", "status"), self.levels(self.run_doctor())) + self.set_status(client_id="other") + self.assertIn(("FAIL", "status"), self.levels(self.run_doctor())) + + def test_unknown_groups_fail_and_drift_warns(self): + self.set_status(unknown_groups=["helters"], drift_corrected=["SupportedIdentityProviders"]) + levels = self.levels(self.run_doctor()) + self.assertIn(("FAIL", "groups"), levels) + self.assertIn(("WARN", "drift"), levels) + + def test_request_differing_from_app_yaml_fails(self): + app = {**APP, "auth": {**APP["auth"], "logout_urls": ["https://cupcake.javazone.no/bye"]}} + self.assertIn(("FAIL", "request"), self.levels(self.run_doctor(app))) + + def test_old_status_version_is_reported(self): + self.set_status(request_version=2) + self.assertIn(("WARN", "status"), self.levels(self.run_doctor())) + + def test_validate_only_and_no_auth_are_skipped(self): + for auth in ({"pool": "internal", "client": "none"}, None): + app = {**APP, "auth": auth} if auth else {k: v for k, v in APP.items() if k != "auth"} + report = self.run_doctor(app) + self.assertFalse(report.failed) + self.assertEqual(report.results[0]["level"], "SKIP") + + def test_status_that_is_not_json_fails_instead_of_crashing(self): + self.params[f"{BASE}/auth-status"]["Value"] = '{"status":"synced","callback_ur' + report = self.run_doctor() + self.assertIn(("FAIL", "status"), self.levels(report)) + self.assertIn("not JSON", json.dumps(report.results)) + + def test_missing_status_waits_briefly_not_the_full_wait(self): + del self.params[f"{BASE}/auth-status"] + clock = [0.0] + doc.time.monotonic = lambda: clock[0] + try: + def sleep(s): + clock[0] += s + doc.diagnose(APP, wait=180, tasks=False, sleep=sleep) + finally: + import time + doc.time.monotonic = time.monotonic + self.assertLessEqual(clock[0], doc.MISSING_STATUS_WAIT + 5) + + +class TaskCheckTest(unittest.TestCase): + def setUp(self): + self.responses = { + "describe-services": {"services": [{"deployments": [ + {"id": "ecs-svc/new", "status": "PRIMARY"}, {"id": "ecs-svc/old", "status": "ACTIVE"}]}]}, + "list-tasks": {"taskArns": ["arn/t1", "arn/t2"]}, + "describe-tasks": {"tasks": [ + {"taskArn": "arn/t1", "startedBy": "ecs-svc/new", "startedAt": "2026-10-24T10:05:00+00:00"}, + {"taskArn": "arn/t2", "startedBy": "ecs-svc/old", "startedAt": "2026-10-24T09:00:00+00:00"}]}, + } + doc.aws = lambda *args: self.responses[args[1]] + + def check(self, status): + report = doc.Report() + doc.check_tasks(report, "cupcake", "cupcake", status) + return report.results + + def test_draining_deployment_tasks_are_ignored(self): + results = self.check({"client_id_changed_at": "2026-10-24T10:00:00+00:00"}) + self.assertEqual([r["level"] for r in results], ["PASS"]) + + def test_stale_primary_task_is_a_warning_not_a_failure(self): + results = self.check({"client_id_changed_at": "2026-10-24T11:00:00+00:00"}) + self.assertEqual([r["level"] for r in results], ["WARN"]) + self.assertIn("t1", results[0]["message"]) + + def test_no_recorded_change_skips(self): + # Old code rewrote cognito-client-id on every reconcile, so its + # LastModifiedDate says nothing; only auth-status's timestamp counts. + self.assertEqual([r["level"] for r in self.check({})], ["SKIP"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/tests/test_generate_modules.py b/scripts/tests/test_generate_modules.py index 6236ef1..f4a43f1 100644 --- a/scripts/tests/test_generate_modules.py +++ b/scripts/tests/test_generate_modules.py @@ -178,6 +178,75 @@ def test_localhost_callbacks_are_passed_through(self): self.assertIn('"http://localhost:3000/"', generate(app)["main.tf"]) +REAL_FROSTING = { # javaBin/frosting app.yaml, verbatim + **FROSTING, + "auth": { + "pool": "internal", "client": "public", + "callback_urls": ["https://cupcake.javazone.no/", "https://cupcake.javazone.no/auth/callback", + "http://localhost:3000/"], + "logout_urls": ["https://cupcake.javazone.no/", "http://localhost:3000/"], + "groups": ["helter"], + "env": {"COGNITO_CLIENT_ID": "NUXT_PUBLIC_OIDC_CLIENT_ID", "COGNITO_ISSUER_URL": "NUXT_PUBLIC_OIDC_AUTHORITY", + "COGNITO_DOMAIN": "NUXT_PUBLIC_OIDC_LOGOUT_DOMAIN"}, + }, +} +REAL_CUPCAKE = {**CUPCAKE, "auth": {"pool": "internal", "client": "none", + "env": {"COGNITO_WELL_KNOWN_URL": "OIDC_WELL_KNOWN_URL"}}} + + +class AuthValidationTest(unittest.TestCase): + def assertRejected(self, auth, fragment, app=FROSTING): + with self.assertRaises(gen.AppYamlError) as cm: + generate({**app, "auth": {"pool": "internal", "client": "public", **auth}}) + self.assertIn(fragment, str(cm.exception)) + + def test_live_apps_still_generate(self): + for app in (REAL_FROSTING, REAL_CUPCAKE): + generate(app) + + def test_callback_on_another_host_is_rejected(self): + self.assertRejected({"callback_urls": ["https://cupcake.javazone.no.evil.com/cb"]}, "not this app's routing.host") + self.assertRejected({"logout_urls": ["https://other.javazone.no/"]}, "auth.extra_hosts") + + def test_extra_hosts_allow_other_hosts(self): + auth = {"callback_urls": ["https://www.javazone.no/cb"], "extra_hosts": ["www.javazone.no"]} + self.assertIn('"https://www.javazone.no/cb"', generate({**FROSTING, "auth": {"client": "public", **auth}})["main.tf"]) + self.assertRejected({"extra_hosts": ["https://www.javazone.no"]}, "list of hostnames") + + def test_without_routing_every_host_needs_extra_hosts(self): + app = {"name": "spa", "team": "core"} + self.assertRejected({"callback_urls": ["https://spa.javazone.no/"]}, "auth.extra_hosts", app=app) + generate({**app, "auth": {"callback_urls": ["https://spa.javazone.no/"], "extra_hosts": ["spa.javazone.no"]}}) + + def test_http_only_for_localhost_and_no_fragments(self): + self.assertRejected({"callback_urls": ["http://cupcake.javazone.no/"]}, "must use https://") + self.assertRejected({"callback_urls": ["https://cupcake.javazone.no/#/cb"]}, "#fragment") + self.assertRejected({"callback_urls": ["cupcake.javazone.no/cb"]}, "absolute https:// URL") + generate({**FROSTING, "auth": {"client": "public", "callback_urls": ["http://127.0.0.1:5173/cb", + "https://localhost:8443/cb"]}}) + + def test_client_none_rejects_settings_it_would_ignore(self): + for key, value in (("callback_urls", ["https://cupcake.javazone.no/"]), ("scopes", ["openid"]), + ("logout_urls", ["https://cupcake.javazone.no/"]), ("extra_hosts", ["a.javazone.no"])): + self.assertRejected({"client": "none", key: value}, f"would be ignored") + + def test_identity_provider_is_injected_and_aliasable(self): + app = {**FROSTING, "auth": {"client": "public", + "env": {"COGNITO_IDENTITY_PROVIDER": "NUXT_PUBLIC_OIDC_IDENTITY_PROVIDER"}}} + main = generate(app)["main.tf"] + self.assertRegex(main, r"COGNITO_IDENTITY_PROVIDER\s+= module.auth.identity_provider_arn") + self.assertRegex(main, r"NUXT_PUBLIC_OIDC_IDENTITY_PROVIDER\s+= module.auth.identity_provider_arn") + + def test_validate_only_app_gets_no_identity_provider(self): + self.assertNotIn("COGNITO_IDENTITY_PROVIDER", generate(REAL_CUPCAKE)["main.tf"]) + + def test_resolve_auth_matches_the_request(self): + auth = gen.resolve_auth(REAL_FROSTING) + self.assertEqual(auth["callback_urls"], REAL_FROSTING["auth"]["callback_urls"]) + self.assertEqual(auth["scopes"], ["openid", "email", "profile"]) + self.assertIsNone(gen.resolve_auth({**FROSTING, "auth": "none"})) + + @unittest.skipUnless(shutil.which("terraform"), "terraform not installed") class FmtTest(unittest.TestCase): """tf-plan runs `terraform fmt -check`; generated files must pass it.""" @@ -185,7 +254,7 @@ class FmtTest(unittest.TestCase): def test_generated_files_are_fmt_clean(self): public = {**FROSTING, "auth": {"pool": "internal", "client": "public", "groups": ["helter"], "env": {"COGNITO_CLIENT_ID": "NUXT_PUBLIC_OIDC_CLIENT_ID"}}} - for app in (CUPCAKE, FROSTING, FULL, public, {"name": "worker", "team": "core"}): + for app in (CUPCAKE, FROSTING, FULL, public, REAL_FROSTING, REAL_CUPCAKE, {"name": "worker", "team": "core"}): with tempfile.TemporaryDirectory() as d: for name, content in generate(app).items(): with open(os.path.join(d, name), "w") as f: diff --git a/terraform/lambda-src/app_auth_sync/handler.py b/terraform/lambda-src/app_auth_sync/handler.py index 5a5df3e..a2d92ed 100644 --- a/terraform/lambda-src/app_auth_sync/handler.py +++ b/terraform/lambda-src/app_auth_sync/handler.py @@ -8,18 +8,29 @@ For each request this Lambda keeps one client named "{team}-{app}" in the requested pool, removes it from the other pool, and writes the credentials to - /{project}/apps/{team}/{app}/cognito-client-id (String) - /{project}/apps/{team}/{app}/cognito-client-secret (SecureString) + /{project}/apps/{team}/{app}/cognito-client-id (String) + /{project}/apps/{team}/{app}/cognito-client-secret (SecureString) + /{project}/apps/{team}/{app}/cognito-client-identity-provider (String) where the app's ECS task picks them up via its team's execution role. Public clients (browser apps using PKCE) get no secret and no secret parameter. +These are written only when the value changes: app-secret-rollout restarts +the app on every Update, so a replaced client reaches the running tasks and +an unchanged reconcile restarts nothing. + +Each sync also writes /{project}/apps/{team}/{app}/auth-status (String JSON, +team-readable, platform-written): the outcome (synced | invalid | error), the +client ID and when it last changed, sign-in IdPs, fields a reconcile had to correct (drift), and +allowed groups that do not exist in the memberships table. Teams have no +cognito-idp read access, so this is how they (and scripts/auth-doctor.py) +see what the platform did with their request. Each pool has a fixed sign-in IdP list (INTERNAL_IDENTITY_PROVIDERS / EXTERNAL_IDENTITY_PROVIDERS, comma-separated, set by Terraform). Every client in the pool gets exactly that list, so with a single IdP the hosted UI sends the user straight to it (no password form, no sign-up link). If a configured -IdP does not exist in the pool, the request is skipped and logged as an error; -the existing client is left as it is. +IdP does not exist in the pool, the request is skipped and logged as an error +(status "error"); the existing client is left as it is. The request's allowed groups are written to the auth memberships table as {pk: "client#", allowed_groups: [...]}; the auth-claims trigger @@ -32,6 +43,7 @@ clients whose request is gone """ +import datetime import json import logging import os @@ -48,6 +60,7 @@ "external": os.environ.get("EXTERNAL_POOL_NAME", f"{PROJECT}-external"), } + def _idp_list(value): return [p.strip() for p in value.split(",") if p.strip()] @@ -58,6 +71,19 @@ def _idp_list(value): "external": _idp_list(os.environ.get("EXTERNAL_IDENTITY_PROVIDERS", "COGNITO")), } MEMBERSHIPS_TABLE = os.environ.get("MEMBERSHIPS_TABLE", "") +# Hosted-UI domains, reported in auth-status for the doctor's probes +POOL_DOMAINS = { + "internal": os.environ.get("INTERNAL_POOL_DOMAIN", ""), + "external": os.environ.get("EXTERNAL_POOL_DOMAIN", ""), +} +# Client fields compared for drift reporting (lists compared as sets) +DRIFT_FIELDS = ("AllowedOAuthFlows", "AllowedOAuthFlowsUserPoolClient", "AllowedOAuthScopes", "CallbackURLs", + "LogoutURLs", "SupportedIdentityProviders", "ExplicitAuthFlows", "PreventUserExistenceErrors") +# Standard-tier parameter limit: 4 KB (bytes, not characters) +STATUS_MAX_BYTES = 4096 +# Dropped from auth-status, in order, until it fits; the doctor falls back to app.yaml +STATUS_DROPPABLE = ("callback_urls", "logout_urls", "changed_fields", "drift_corrected", "scopes", + "empty_groups", "allowed_groups", "unknown_groups", "identity_providers") DEFAULT_SCOPES = ["openid", "email", "profile"] CLIENT_TYPES = ("confidential", "public") # Cognito allows plain http only for localhost, for local development @@ -114,6 +140,9 @@ def urls(key, required): isinstance(u, str) and (u.startswith("https://") or LOCAL_URL.match(u)) for u in value ): raise InvalidRequest(f"{key} must be a list of https:// (or http://localhost) URLs") + if any("#" in u for u in value): + # OAuth 2.0 forbids fragments in redirect URIs; Cognito rejects them + raise InvalidRequest(f"{key} must not contain #fragments") if required and not value: raise InvalidRequest(f"{key} must not be empty") return value @@ -174,7 +203,11 @@ def find_client(user_pool_id, client_name): def identity_providers(pool, user_pool_id): - """The pool's configured sign-in IdPs, after checking each one exists.""" + """The pool's configured sign-in IdPs, after checking each one exists. + + With exactly one IdP on a client, the hosted UI redirects straight to it, + so apps need no identity_provider parameter. + """ wanted = POOL_IDPS[pool] if not wanted: raise PoolMisconfigured(f"no identity providers configured for the {pool} pool") @@ -190,6 +223,11 @@ def identity_providers(pool, user_pool_id): return list(wanted) +def primary_identity_provider(idps): + """The IdP an app passes as identity_provider: the first federated one, else COGNITO.""" + return next((p for p in idps if p != "COGNITO"), "COGNITO") + + def client_settings(pool, user_pool_id, client_name, req): return { "UserPoolId": user_pool_id, @@ -214,6 +252,30 @@ def _cred_names(team, app): return f"{base}/cognito-client-id", f"{base}/cognito-client-secret" +def _idp_name(team, app): + return f"{APPS_PREFIX}{team}/{app}/cognito-client-identity-provider" + + +def _status_name(team, app): + return f"{APPS_PREFIX}{team}/{app}/auth-status" + + +def _current(name): + """A parameter's stored value (SecureStrings come back encrypted), or None.""" + try: + return ssm.get_parameter(Name=name, WithDecryption=False)["Parameter"]["Value"] + except ssm.exceptions.ParameterNotFound: + return None + + +def _put_if_changed(name, value, team, app): + """Write a String parameter only when its value differs. Returns True if written.""" + if _current(name) == value: + return False + _put_param(name, value, False, team, app) + return True + + def _put_param(name, value, secure, team, app): ssm.put_parameter( Name=name, @@ -233,13 +295,22 @@ def _put_param(name, value, secure, team, app): ) -def sync(team, app, req): - """Make Cognito match one request. Returns the client ID.""" +def _diff(current, settings): + """Names of DRIFT_FIELDS where the live client differs from the settings.""" + def norm(v): + return sorted(v) if isinstance(v, list) else v + return [f for f in DRIFT_FIELDS if norm(current.get(f, [] if isinstance(settings[f], list) else None)) + != norm(settings[f])] + + +def sync(team, app, req, reason="event"): + """Make Cognito match one request. Returns a status dict (see write_status).""" client_name = f"{team}-{app}" # Resolve settings first: a misconfigured pool must abort before anything # (including the pool-switch cleanup below) is changed. upid = pool_id(req["pool"]) settings = client_settings(req["pool"], upid, client_name, req) + idps = settings["SupportedIdentityProviders"] # A pool switch (internal <-> external) leaves no client behind. for other in POOL_NAMES: @@ -253,9 +324,11 @@ def sync(team, app, req): want_secret = req["client_type"] == "confidential" client_id = find_client(upid, client_name) + changed = [] if client_id: - has_secret = bool(cognito.describe_user_pool_client(UserPoolId=upid, ClientId=client_id) - ["UserPoolClient"].get("ClientSecret")) + current = cognito.describe_user_pool_client(UserPoolId=upid, ClientId=client_id)["UserPoolClient"] + has_secret = bool(current.get("ClientSecret")) + changed = _diff(current, settings) if has_secret != want_secret: # A client's secret setting is immutable; switching type means a new client cognito.delete_user_pool_client(UserPoolId=upid, ClientId=client_id) @@ -263,21 +336,119 @@ def sync(team, app, req): logger.info("Replacing %s: client_type changed to %s", client_name, req["client_type"]) client_id = None if client_id: + if changed and reason == "reconcile": + # The request did not change since its last sync, so someone + # edited the client outside the platform (e.g. the console). + logger.warning("WARN drift on %s (%s), correcting: %s", client_name, client_id, ", ".join(changed)) cognito.update_user_pool_client(ClientId=client_id, **settings) logger.info("Updated client %s (%s) in %s pool", client_name, client_id, req["pool"]) else: + changed = [] client_id = cognito.create_user_pool_client(GenerateSecret=want_secret, **settings)["UserPoolClient"]["ClientId"] logger.info("Created %s client %s (%s) in %s pool", req["client_type"], client_name, client_id, req["pool"]) + # The client ID goes last: its Update is what restarts the app, and the + # new tasks must find the matching secret and IdP already in place. + # (app-secret-rollout ignores the secret: a new one always comes with a + # new ID, so restarting on it too would deploy once with the old ID.) id_param, secret_param = _cred_names(team, app) - _put_param(id_param, client_id, False, team, app) + id_changed = _current(id_param) != client_id if want_secret: - secret = cognito.describe_user_pool_client(UserPoolId=upid, ClientId=client_id)["UserPoolClient"]["ClientSecret"] - _put_param(secret_param, secret, True, team, app) - else: + # A client's secret never changes, so a new one comes only with a new + # client ID; skipping the rewrite needs no decrypt permission. + if id_changed or _current(secret_param) is None: + secret = cognito.describe_user_pool_client(UserPoolId=upid, ClientId=client_id)["UserPoolClient"]["ClientSecret"] + _put_param(secret_param, secret, True, team, app) + elif _current(secret_param) is not None: ssm.delete_parameters(Names=[secret_param]) + _put_if_changed(_idp_name(team, app), primary_identity_provider(idps), team, app) + if id_changed: + _put_param(id_param, client_id, False, team, app) _put_client_groups(client_id, req["allowed_groups"], team, app) - return client_id + unknown, empty = check_groups(req["allowed_groups"]) + return { + "status": "synced", + "client_id": client_id, + "client_type": req["client_type"], + "pool": req["pool"], + "pool_domain": POOL_DOMAINS.get(req["pool"], ""), + "identity_providers": idps, + "identity_provider": primary_identity_provider(idps), + "callback_urls": req["callback_urls"], + "logout_urls": req["logout_urls"], + "scopes": req["scopes"], + "allowed_groups": req["allowed_groups"], + "unknown_groups": unknown, + "empty_groups": empty, + "changed_fields": changed, + "drift_corrected": changed if reason == "reconcile" else [], + "client_id_changed": id_changed, + } + + +def check_groups(groups): + """(unknown, empty): allowed groups with no memberships-table entry, or no members. + + A typo in auth.groups denies everyone; the restriction is still written + (fail closed) but reported so the team can see why. + """ + unknown, empty = [], [] + if not MEMBERSHIPS_TABLE: + return unknown, empty + for g in groups: + item = dynamodb.get_item(TableName=MEMBERSHIPS_TABLE, Key={"pk": {"S": f"group#{g}"}}).get("Item") + if item is None: + unknown.append(g) + elif not (item.get("members", {}).get("L") or item.get("members", {}).get("SS")): + empty.append(g) + return unknown, empty + + +def status_value(body): + """auth-status JSON that fits the parameter: drop STATUS_DROPPABLE fields + until it does, never cut the JSON itself (the doctor parses it).""" + def encode(b): + return json.dumps(b, separators=(",", ":")) + body = dict(body) + value = encode(body) + for key in STATUS_DROPPABLE: + if len(value.encode()) <= STATUS_MAX_BYTES: + break + if key in body: + body.pop(key) + body["truncated"] = True + value = encode(body) + if len(value.encode()) > STATUS_MAX_BYTES: + # Only an absurd error message gets here + body = {k: body[k] for k in ("status", "client_id", "client_id_changed_at", "request_version", + "synced_at") if k in body} + body["truncated"] = True + value = encode(body) + return value + + +def write_status(team, app, status, request_version=None): + """Publish the outcome of a sync to the team-readable auth-status parameter.""" + now = datetime.datetime.now(datetime.timezone.utc).isoformat(timespec="seconds") + body = {**status, "request_version": request_version, "synced_at": now} + if "client_id" in status: + # When the client ID last changed, so the doctor can tell tasks that + # still hold the old one. Carried over from the previous status. + changed_at = now if status.get("client_id_changed") else _previous_status(team, app).get("client_id_changed_at") + body["client_id_changed_at"] = changed_at + try: + _put_param(_status_name(team, app), status_value(body), False, team, app) + except Exception: + # Reporting must never turn a good sync into a failure + logger.exception("Could not write auth-status for %s-%s", team, app) + + +def _previous_status(team, app): + try: + prev = json.loads(_current(_status_name(team, app)) or "{}") + return prev if isinstance(prev, dict) else {} + except Exception: + return {} def _put_client_groups(client_id, groups, team, app): @@ -311,14 +482,38 @@ def remove(team, app): cognito.delete_user_pool_client(UserPoolId=upid, ClientId=client_id) _delete_client_groups(client_id) logger.info("Deleted client %s from %s pool", client_name, pool) - ssm.delete_parameters(Names=list(_cred_names(team, app))) + ssm.delete_parameters(Names=[*_cred_names(team, app), _idp_name(team, app), _status_name(team, app)]) def read_request(name): + """(value, version) of a request parameter, or (None, None).""" try: - return ssm.get_parameter(Name=name)["Parameter"]["Value"] + p = ssm.get_parameter(Name=name)["Parameter"] + return p["Value"], p.get("Version") except ssm.exceptions.ParameterNotFound: - return None + return None, None + + +def sync_and_report(team, app, raw, version, reason): + """Parse + sync one request and publish auth-status. Returns the status dict. + + Invalid requests return status "invalid"; a misconfigured pool (missing + IdP) returns status "error" with the client untouched. Other failures are + reported as status "error" and re-raised so the Lambda still errors. + """ + try: + status = sync(team, app, parse_request(raw), reason) + except InvalidRequest as e: + logger.error("Invalid auth request for %s-%s: %s", team, app, e) + status = {"status": "invalid", "error": str(e)} + except PoolMisconfigured as e: + logger.error("Skipped %s-%s, client left unchanged: %s", team, app, e) + status = {"status": "error", "error": str(e)} + except Exception as e: + write_status(team, app, {"status": "error", "error": f"{type(e).__name__}: {e}"[:500]}, version) + raise + write_status(team, app, status, version) + return status def handle_change(name, operation): @@ -330,23 +525,16 @@ def handle_change(name, operation): # Re-read instead of trusting the event: the parameter is the source of # truth, and a stale Delete event must not remove a live client. - raw = read_request(name) + raw, version = read_request(name) if raw is None: remove(team, app) return {"status": "deleted", "client": f"{team}-{app}"} if operation == "Delete": logger.info("Delete event for %s but the parameter exists again — syncing", name) - try: - req = parse_request(raw) - except InvalidRequest as e: - logger.error("Invalid auth request %s: %s", name, e) - return {"status": "invalid", "error": str(e)} - try: - client_id = sync(team, app, req) - except PoolMisconfigured as e: - logger.error("Skipped %s-%s, client left unchanged: %s", team, app, e) - return {"status": "error", "error": str(e)} - return {"status": "synced", "client": f"{team}-{app}", "client_id": client_id} + status = sync_and_report(team, app, raw, version, "event") + if status["status"] != "synced": + return {"status": status["status"], "error": status.get("error")} + return {"status": "synced", "client": f"{team}-{app}", "client_id": status["client_id"]} def reconcile(): @@ -357,19 +545,22 @@ def reconcile(): for p in page["Parameters"]: owner = parse_name(p["Name"]) if owner: - wanted[owner] = p["Value"] + wanted[owner] = (p["Value"], p.get("Version")) - results = {"synced": [], "invalid": [], "error": [], "deleted": []} - for (team, app), raw in sorted(wanted.items()): + results = {"synced": [], "invalid": [], "deleted": [], "error": [], "drift": []} + for (team, app), (raw, version) in sorted(wanted.items()): try: - sync(team, app, parse_request(raw)) - results["synced"].append(f"{team}-{app}") - except InvalidRequest as e: - logger.error("Invalid auth request for %s-%s: %s", team, app, e) - results["invalid"].append(f"{team}-{app}") - except PoolMisconfigured as e: - logger.error("Skipped %s-%s, client left unchanged: %s", team, app, e) + status = sync_and_report(team, app, raw, version, "reconcile") + except Exception: + # One broken app must not stop the others from reconciling. Not + # re-raised: async invokes would retry the whole reconcile twice. + # The javabin-app-auth-sync-skipped alarm counts this line. + logger.exception("Sync failed for %s-%s", team, app) results["error"].append(f"{team}-{app}") + continue + results[status["status"] if status["status"] in ("synced", "invalid") else "error"].append(f"{team}-{app}") + if status.get("drift_corrected"): + results["drift"].append(f"{team}-{app}") # Orphans: clients whose credential parameter exists (so this Lambda made # them) but whose request is gone. Clients created by other means are diff --git a/terraform/lambda-src/app_secret_rollout/handler.py b/terraform/lambda-src/app_secret_rollout/handler.py index 42193d1..87e1205 100644 --- a/terraform/lambda-src/app_secret_rollout/handler.py +++ b/terraform/lambda-src/app_secret_rollout/handler.py @@ -9,8 +9,16 @@ SSM "Parameter Store Change" Update on /javabin/apps/{team}/{app}/{name} -> force a new deployment of service {team}-{app} in the platform cluster -Platform-managed parameters (auth request, Cognito credentials, RDS master -password) are ignored: their consumers redeploy through their own paths. +Cognito client parameters (cognito-client-id, -identity-provider) also +restart the app: app-auth-sync writes them only when the value changes (a +replaced client, or an IdP cutover), so running tasks never keep a deleted +client ID and an unchanged reconcile restarts nothing. + +Ignored: the auth request (app Terraform redeploys with it), auth-status +(rewritten on every sync), cognito-client-secret (a new secret only comes +with a new client, and app-auth-sync writes the ID right after it, so +restarting on the secret would deploy once with the old ID) and the RDS +master password (the database keeps the original). """ import logging @@ -25,7 +33,7 @@ PROJECT = os.environ.get("PROJECT", "javabin") CLUSTER = os.environ.get("CLUSTER", f"{PROJECT}-platform") SECRET_RE = re.compile(rf"^/{re.escape(PROJECT)}/apps/(?P[a-z]{{1,12}})/(?P[a-z][a-z0-9-]{{0,19}})/(?P[^/]+)$") -PLATFORM_MANAGED = re.compile(r"^(auth|cognito-client-.*|db-master-password)$") +PLATFORM_MANAGED = re.compile(r"^(auth|auth-status|cognito-client-secret|db-master-password)$") ecs = boto3.client("ecs") diff --git a/terraform/lambda-src/auth_claims/handler.py b/terraform/lambda-src/auth_claims/handler.py index 4508405..29f33c7 100644 --- a/terraform/lambda-src/auth_claims/handler.py +++ b/terraform/lambda-src/auth_claims/handler.py @@ -23,6 +23,10 @@ Workspace accounts, e.g. GoogleSAML) or have email_verified=true. This is the server-side guard against self-registered or foreign-IdP users in the internal pool; the IdP's hosted-domain hint is only a UI hint. + +IdPs in EMAIL_VERIFIED_IDENTITY_PROVIDERS (default: Google, the OIDC IdP) are +not trusted on their own: their users also need email_verified=true, which +the pool maps from Google's own claim. SAML releases no such claim. """ import json @@ -40,6 +44,9 @@ REQUIRED_DOMAIN = os.environ.get("REQUIRED_EMAIL_DOMAIN", "").strip().lower().lstrip("@") FEDERATED_IDPS = {p.strip() for p in os.environ.get("FEDERATED_IDENTITY_PROVIDERS", "").split(",") if p.strip() and p.strip() != "COGNITO"} +# Federated IdPs whose users must still have email_verified=true +VERIFY_EMAIL_IDPS = {p.strip() for p in os.environ.get("EMAIL_VERIFIED_IDENTITY_PROVIDERS", "Google").split(",") + if p.strip()} dynamodb = boto3.client("dynamodb") @@ -112,7 +119,7 @@ def domain_allowed(event, attrs, email): return True if not email.endswith("@" + REQUIRED_DOMAIN): return False - if _providers(event, attrs) & FEDERATED_IDPS: + if _providers(event, attrs) & (FEDERATED_IDPS - VERIFY_EMAIL_IDPS): return True return str(attrs.get("email_verified", "")).lower() == "true" diff --git a/terraform/lambda-src/team_provisioner/handler.py b/terraform/lambda-src/team_provisioner/handler.py index fe0b98f..333c4aa 100644 --- a/terraform/lambda-src/team_provisioner/handler.py +++ b/terraform/lambda-src/team_provisioner/handler.py @@ -1409,8 +1409,9 @@ def _ensure_team_permission_set(team_name, group_id): }, { # Platform-managed parameters in the team path: Cognito - # credentials (app-auth-sync), the auth request (app - # Terraform), RDS master passwords (the DB keeps the original). + # credentials and auth-status (app-auth-sync), the auth + # request (app Terraform), RDS master passwords (the DB + # keeps the original). Teams can still read them. "Sid": "DenyPlatformManagedParameters", "Effect": "Deny", "Action": ["ssm:PutParameter", "ssm:DeleteParameter", "ssm:DeleteParameters", @@ -1418,6 +1419,7 @@ def _ensure_team_permission_set(team_name, group_id): "Resource": [ f"arn:aws:ssm:*:{ACCOUNT_ID}:parameter/{PROJECT}/apps/*/*/cognito-client-*", f"arn:aws:ssm:*:{ACCOUNT_ID}:parameter/{PROJECT}/apps/*/*/auth", + f"arn:aws:ssm:*:{ACCOUNT_ID}:parameter/{PROJECT}/apps/*/*/auth-status", f"arn:aws:ssm:*:{ACCOUNT_ID}:parameter/{PROJECT}/apps/*/*/db-master-password", ], }, diff --git a/terraform/lambda-src/tests/test_app_auth_sync.py b/terraform/lambda-src/tests/test_app_auth_sync.py index 01f9d98..4639817 100644 --- a/terraform/lambda-src/tests/test_app_auth_sync.py +++ b/terraform/lambda-src/tests/test_app_auth_sync.py @@ -27,15 +27,19 @@ class FakeSSM: def __init__(self): self.params = {} self.tags = {} + self.writes = [] # parameter names in write order + self.versions = {} self.exceptions = types.SimpleNamespace(ParameterNotFound=ParameterNotFound) - def get_parameter(self, Name): + def get_parameter(self, Name, WithDecryption=False): if Name not in self.params: raise ParameterNotFound(Name) - return {"Parameter": {"Name": Name, "Value": self.params[Name]}} + return {"Parameter": {"Name": Name, "Value": self.params[Name], "Version": self.versions.get(Name, 1)}} def put_parameter(self, Name, Value, Type, Overwrite, Description): self.params[Name] = Value + self.versions[Name] = self.versions.get(Name, 0) + 1 + self.writes.append(Name) def add_tags_to_resource(self, ResourceType, ResourceId, Tags): self.tags[ResourceId] = {t["Key"]: t["Value"] for t in Tags} @@ -47,7 +51,8 @@ def delete_parameters(self, Names): def get_paginator(self, name): assert name == "get_parameters_by_path" return _Paginator(lambda Path, Recursive: [ - {"Name": k, "Value": v} for k, v in sorted(self.params.items()) if k.startswith(Path) + {"Name": k, "Value": v, "Version": self.versions.get(k, 1)} + for k, v in sorted(self.params.items()) if k.startswith(Path) ], "Parameters") @@ -61,13 +66,21 @@ def put_item(self, TableName, Item): def delete_item(self, TableName, Key): self.items.pop(Key["pk"]["S"], None) + def get_item(self, TableName, Key): + item = self.items.get(Key["pk"]["S"]) + return {"Item": item} if item else {} + + def add_group(self, name, members): + self.items[f"group#{name}"] = {"pk": {"S": f"group#{name}"}, "members": {"L": [{"S": m} for m in members]}} + class FakeCognito: POOLS = {"pool-int": "javabin-internal", "pool-ext": "javabin-external"} - def __init__(self): + def __init__(self, internal_idps=("GoogleSAML",)): self.clients = {} # client_id -> dict self.n = 0 + self.internal_idps = list(internal_idps) def get_paginator(self, name): if name == "list_user_pools": @@ -79,7 +92,7 @@ def get_paginator(self, name): ], "UserPoolClients") if name == "list_identity_providers": return _Paginator(lambda UserPoolId, MaxResults: ( - [{"ProviderName": "GoogleSAML"}] if UserPoolId == "pool-int" else [] + [{"ProviderName": p} for p in self.internal_idps] if UserPoolId == "pool-int" else [] ), "Providers") raise AssertionError(name) @@ -99,14 +112,15 @@ def describe_user_pool_client(self, UserPoolId, ClientId): return {"UserPoolClient": self.clients[ClientId]} -IDP_ENV = ("INTERNAL_IDENTITY_PROVIDERS", "EXTERNAL_IDENTITY_PROVIDERS") +ENV_KEYS = ("INTERNAL_IDENTITY_PROVIDERS", "EXTERNAL_IDENTITY_PROVIDERS", "INTERNAL_POOL_DOMAIN", "EXTERNAL_POOL_DOMAIN") +IDP_ENV = ENV_KEYS -def load_handler(ssm, cognito, dynamo=None, **idps): +def load_handler(ssm, cognito, dynamo=None, **env): os.environ["MEMBERSHIPS_TABLE"] = "javabin-auth-memberships" - for key in IDP_ENV: + for key in ENV_KEYS: os.environ.pop(key, None) - os.environ.update(idps) + os.environ.update(env) boto3 = types.ModuleType("boto3") boto3.client = lambda svc: {"ssm": ssm, "cognito-idp": cognito, "dynamodb": dynamo or FakeDynamo()}[svc] sys.modules["boto3"] = boto3 @@ -349,5 +363,174 @@ def test_localhost_callbacks_allowed_but_not_other_http(self): self.assertEqual(self.h.handler(event(REQ, "Update"), None)["status"], "invalid") +STATUS = "/javabin/apps/cupcake/frosting/auth-status" +CLIENT_ID = "/javabin/apps/cupcake/frosting/cognito-client-id" +IDP = "/javabin/apps/cupcake/frosting/cognito-client-identity-provider" + + +class SyncHardeningTest(AppAuthSyncTest): + def status(self): + return json.loads(self.ssm.params[STATUS]) + + def test_unchanged_sync_writes_no_credentials(self): + self.put_request() + self.h.handler(event(REQ, "Create"), None) + self.ssm.writes.clear() + + self.h.handler({"action": "reconcile"}, None) + self.h.handler(event(REQ, "Update"), None) + + # Only the status is rewritten; app-secret-rollout restarts on cognito-client-* updates + self.assertEqual(set(self.ssm.writes), {STATUS}) + + def test_replaced_client_rewrites_id_and_secret(self): + self.put_request({**REQUEST, "client_type": "public"}) + self.h.handler(event(REQ, "Create"), None) + self.ssm.writes.clear() + self.put_request() # confidential: a new client with a secret + self.h.handler(event(REQ, "Update"), None) + secret = "/javabin/apps/cupcake/frosting/cognito-client-secret" + # The secret lands before the ID, whose Update restarts the app + self.assertLess(self.ssm.writes.index(secret), self.ssm.writes.index(CLIENT_ID)) + self.assertTrue(self.status()["client_id_changed"]) + + def test_status_reports_sync_result_and_request_version(self): + self.put_request({**REQUEST, "allowed_groups": ["helter"]}) + self.dynamo.add_group("helter", ["a@java.no"]) + self.h.handler(event(REQ, "Create"), None) + cid, _ = self.only_client() + status = self.status() + self.assertEqual(status["status"], "synced") + self.assertEqual(status["client_id"], cid) + self.assertEqual(status["request_version"], 1) + self.assertEqual(status["unknown_groups"], []) + self.assertIn("synced_at", status) + self.assertEqual(self.ssm.tags[STATUS]["team"], "cupcake") + + def test_invalid_request_is_reported_in_status(self): + self.put_request({**REQUEST, "callback_urls": ["http://insecure"]}) + self.h.handler(event(REQ, "Create"), None) + self.assertEqual(self.status()["status"], "invalid") + self.assertIn("callback_urls", self.status()["error"]) + + def test_unknown_and_empty_groups_are_reported_but_still_enforced(self): + self.dynamo.add_group("pkom", []) + self.put_request({**REQUEST, "allowed_groups": ["helters", "pkom"]}) + self.h.handler(event(REQ, "Create"), None) + cid, _ = self.only_client() + self.assertEqual(self.status()["unknown_groups"], ["helters"]) + self.assertEqual(self.status()["empty_groups"], ["pkom"]) + self.assertIn(f"client#{cid}", self.dynamo.items) + + def test_reconcile_reports_console_drift_but_request_changes_are_not_drift(self): + self.put_request() + self.h.handler(event(REQ, "Create"), None) + self.put_request({**REQUEST, "logout_urls": ["https://cupcake.javazone.no/bye"]}) + self.h.handler(event(REQ, "Update"), None) + self.assertEqual(self.status()["drift_corrected"], []) + self.assertEqual(self.status()["changed_fields"], ["LogoutURLs"]) + + cid, client = self.only_client() + client["SupportedIdentityProviders"] = ["GoogleSAML", "COGNITO"] # console edit + result = self.h.handler({"action": "reconcile"}, None) + self.assertEqual(result["drift"], ["cupcake-frosting"]) + self.assertEqual(self.status()["drift_corrected"], ["SupportedIdentityProviders"]) + self.assertEqual(client["SupportedIdentityProviders"], ["GoogleSAML"]) + + def test_default_idps_and_idp_parameter(self): + self.put_request() + self.h.handler(event(REQ, "Create"), None) + self.assertEqual(self.ssm.params[IDP], "GoogleSAML") + self.assertEqual(self.status()["identity_providers"], ["GoogleSAML"]) + + def test_client_id_changed_at_is_set_on_change_and_carried_over(self): + self.put_request() + self.h.handler(event(REQ, "Create"), None) + first = self.status()["client_id_changed_at"] + self.assertTrue(first) + self.ssm.params[STATUS] = json.dumps({**self.status(), "client_id_changed_at": "2026-01-01T00:00:00+00:00"}) + self.h.handler({"action": "reconcile"}, None) + self.assertEqual(self.status()["client_id_changed_at"], "2026-01-01T00:00:00+00:00") + + def test_fragment_in_callback_url_is_invalid(self): + self.put_request({**REQUEST, "callback_urls": ["https://cupcake.javazone.no/#/cb"]}) + self.assertEqual(self.h.handler(event(REQ, "Create"), None)["status"], "invalid") + + def test_oversized_status_stays_valid_json_under_the_byte_limit(self): + many = [f"https://cupcake.javazone.no/{'å' * 40}/{i}" for i in range(60)] + value = self.h.status_value({"status": "synced", "client_id": "c1", "callback_urls": many, + "logout_urls": many, "allowed_groups": ["g" * 50] * 30, + "unknown_groups": ["u" * 50] * 30}) + self.assertLessEqual(len(value.encode()), self.h.STATUS_MAX_BYTES) + body = json.loads(value) + self.assertTrue(body["truncated"]) + self.assertEqual(body["client_id"], "c1") + self.assertNotIn("callback_urls", body) + + def test_small_status_is_not_truncated(self): + body = json.loads(self.h.status_value({"status": "synced", "callback_urls": ["https://a/"]})) + self.assertNotIn("truncated", body) + self.assertEqual(body["callback_urls"], ["https://a/"]) + + +class PoolIdpListTest(unittest.TestCase): + def setUp(self): + self.ssm, self.dynamo = FakeSSM(), FakeDynamo() + self.ssm.params[REQ] = json.dumps(REQUEST) + + def load(self, cognito, **env): + return load_handler(self.ssm, cognito, self.dynamo, **env) + + def test_declared_list_decides_the_client_idps(self): + cognito = FakeCognito(internal_idps=("GoogleSAML", "Google")) + h = self.load(cognito, INTERNAL_IDENTITY_PROVIDERS="GoogleSAML", INTERNAL_POOL_DOMAIN="javabin-internal.auth.x") + h.handler(event(REQ, "Create"), None) + client = next(iter(cognito.clients.values())) + self.assertEqual(client["SupportedIdentityProviders"], ["GoogleSAML"]) + self.assertEqual(json.loads(self.ssm.params[STATUS])["pool_domain"], "javabin-internal.auth.x") + + def test_cutover_switches_idp_and_updates_the_idp_parameter(self): + cognito = FakeCognito(internal_idps=("GoogleSAML", "Google")) + self.load(cognito, INTERNAL_IDENTITY_PROVIDERS="GoogleSAML").handler(event(REQ, "Create"), None) + self.ssm.writes.clear() + self.load(cognito, INTERNAL_IDENTITY_PROVIDERS="Google").handler({"action": "reconcile"}, None) + client = next(iter(cognito.clients.values())) + self.assertEqual(client["SupportedIdentityProviders"], ["Google"]) + self.assertEqual(self.ssm.params[IDP], "Google") + self.assertIn(IDP, self.ssm.writes) + self.assertNotIn(CLIENT_ID, self.ssm.writes) # same client, no restart for the ID + + def test_declared_idp_missing_from_pool_is_reported(self): + cognito = FakeCognito(internal_idps=("GoogleSAML",)) + h = self.load(cognito, INTERNAL_IDENTITY_PROVIDERS="Google") + with self.assertLogs(level="ERROR"): + self.assertEqual(h.handler(event(REQ, "Create"), None)["status"], "error") + self.assertEqual(cognito.clients, {}) + status = json.loads(self.ssm.params[STATUS]) + self.assertEqual(status["status"], "error") + self.assertIn("Google", status["error"]) + + def test_reconcile_continues_past_a_failing_app_without_raising(self): + # Not raised: async invokes would retry the whole reconcile twice + cognito = FakeCognito(internal_idps=("GoogleSAML",)) + ext = "/javabin/apps/core/web/auth" + self.ssm.params[ext] = json.dumps({**REQUEST, "pool": "external"}) + h = self.load(cognito, INTERNAL_IDENTITY_PROVIDERS="Google") + with self.assertLogs(level="ERROR"): + result = h.handler({"action": "reconcile"}, None) + self.assertEqual(result["error"], ["cupcake-frosting"]) + self.assertEqual([c["ClientName"] for c in cognito.clients.values()], ["core-web"]) + + def test_unexpected_failure_in_reconcile_is_logged_for_the_alarm(self): + cognito = FakeCognito(internal_idps=("GoogleSAML",)) + h = self.load(cognito) + cognito.create_user_pool_client = lambda **kw: (_ for _ in ()).throw(RuntimeError("boom")) + with self.assertLogs(level="ERROR") as logs: + result = h.handler({"action": "reconcile"}, None) + self.assertEqual(result["error"], ["cupcake-frosting"]) + self.assertIn("Sync failed for cupcake-frosting", "\n".join(logs.output)) + self.assertEqual(json.loads(self.ssm.params[STATUS])["status"], "error") + + if __name__ == "__main__": unittest.main() diff --git a/terraform/lambda-src/tests/test_app_secret_rollout.py b/terraform/lambda-src/tests/test_app_secret_rollout.py index 19b69b9..906b334 100644 --- a/terraform/lambda-src/tests/test_app_secret_rollout.py +++ b/terraform/lambda-src/tests/test_app_secret_rollout.py @@ -51,12 +51,23 @@ def test_secret_update_restarts_the_apps_service(self): def test_platform_managed_and_non_update_events_are_ignored(self): ecs = FakeECS() h = load(ecs) - for name in ("/javabin/apps/cupcake/frosting/auth", "/javabin/apps/cupcake/frosting/cognito-client-id", - "/javabin/apps/core/api/db-master-password", "/javabin/platform/github-app-key"): + for name in ("/javabin/apps/cupcake/frosting/auth", "/javabin/apps/cupcake/frosting/auth-status", + # A new secret always comes with a new ID; the ID's Update restarts + "/javabin/apps/cupcake/frosting/cognito-client-secret", + "/javabin/apps/core/api/db-master-password", "/javabin/platform/github-app-key", + "/javabin/apps/cupcake/frosting/login-1/cognito-client-id"): self.assertEqual(h.handler(event(name), None)["status"], "ignored") self.assertEqual(h.handler(event("/javabin/apps/cupcake/frosting/x", "Create"), None)["status"], "ignored") self.assertEqual(ecs.calls, []) + def test_cognito_client_changes_restart_the_app(self): + # app-auth-sync writes these only when the value changed (new client, IdP cutover) + for param in ("cognito-client-id", "cognito-client-identity-provider"): + ecs = FakeECS() + result = load(ecs).handler(event(f"/javabin/apps/cupcake/cupcake/{param}"), None) + self.assertEqual(result["status"], "restarted", param) + self.assertEqual(ecs.calls, [("javabin-platform", "cupcake-cupcake", True)]) + def test_missing_service_is_not_an_error(self): ecs = FakeECS(missing={"cupcake-frosting"}) self.assertEqual(load(ecs).handler(event("/javabin/apps/cupcake/frosting/key"), None)["status"], "no-service") diff --git a/terraform/lambda-src/tests/test_auth_claims.py b/terraform/lambda-src/tests/test_auth_claims.py index 366518e..3ecb431 100644 --- a/terraform/lambda-src/tests/test_auth_claims.py +++ b/terraform/lambda-src/tests/test_auth_claims.py @@ -30,7 +30,7 @@ def client(cid, *groups): return {"pk": {"S": f"client#{cid}"}, "allowed_groups": {"L": [{"S": g} for g in groups]}} -DOMAIN_ENV = ("REQUIRED_EMAIL_DOMAIN", "FEDERATED_IDENTITY_PROVIDERS") +DOMAIN_ENV = ("REQUIRED_EMAIL_DOMAIN", "FEDERATED_IDENTITY_PROVIDERS", "EMAIL_VERIFIED_IDENTITY_PROVIDERS") def load(dynamo, **env): @@ -117,6 +117,18 @@ def test_other_idp_needs_a_verified_email(self): self.assertDenied(user("grace.hopper@java.no", provider="SomeOtherIdP", verified="false")) self.assertAllowed(user("grace.hopper@java.no", provider="Google", verified="true")) + def test_google_oidc_users_need_email_verified_even_when_trusted(self): + # After cutover Google is in FEDERATED_IDENTITY_PROVIDERS; it still has + # to vouch for the address with email_verified (mapped from Google). + h = load(self.dynamo, REQUIRED_EMAIL_DOMAIN="java.no", FEDERATED_IDENTITY_PROVIDERS="Google") + with self.assertRaises(h.AccessDenied): + h.handler(user("ada.lovelace@java.no", provider="Google", verified="false"), None) + with self.assertRaises(h.AccessDenied): + h.handler(user("ada.lovelace@java.no", provider="Google"), None) + h.handler(user("ada.lovelace@java.no", provider="Google", verified="true"), None) + with self.assertRaises(h.AccessDenied): + h.handler(user("someone@gmail.com", provider="Google", verified="true"), None) + def test_group_rule_still_applies_after_the_domain_check(self): self.assertDenied(user("grace.hopper@java.no", provider="GoogleSAML", client_id="frosting")) diff --git a/terraform/lambda-src/tests/test_team_provisioner_memberships.py b/terraform/lambda-src/tests/test_team_provisioner_memberships.py index 4d159df..4465a22 100644 --- a/terraform/lambda-src/tests/test_team_provisioner_memberships.py +++ b/terraform/lambda-src/tests/test_team_provisioner_memberships.py @@ -88,6 +88,10 @@ def test_team_permission_set_uses_team_guardrails_and_reprovisions(self): self.assertEqual(result["provisioned"], {"status": "SUCCEEDED"}) deny = next(st for st in policy["Statement"] if st["Sid"] == "DenyCreatingParameters") self.assertEqual(deny["Condition"], {"BoolIfExists": {"ssm:Overwrite": "false"}}) + # app-auth-sync's outputs are readable but not writable by the team + managed = next(st for st in policy["Statement"] if st["Sid"] == "DenyPlatformManagedParameters") + self.assertTrue(any(r.endswith("/apps/*/*/auth-status") for r in managed["Resource"])) + self.assertTrue(any(r.endswith("/apps/*/*/cognito-client-*") for r in managed["Resource"])) def _guardrails_with(self, tags=None, missing=False): h = load(FakeDynamo([])) diff --git a/terraform/modules/cognito-app-client/main.tf b/terraform/modules/cognito-app-client/main.tf index 8f34e8d..83329b1 100644 --- a/terraform/modules/cognito-app-client/main.tf +++ b/terraform/modules/cognito-app-client/main.tf @@ -11,8 +11,10 @@ # platform's app-auth-sync Lambda reacts to the parameter (EventBridge) and # creates/updates/deletes the client named -, then writes: # -# //apps///cognito-client-id (String) -# //apps///cognito-client-secret (SecureString) +# //apps///cognito-client-id (String) +# //apps///cognito-client-secret (SecureString) +# //apps///cognito-client-identity-provider (String) +# //apps///auth-status (String JSON) # # Those parameters are owned by the Lambda, not by this Terraform. Their ARNs # are deterministic, so the ECS task definition can reference them up front; diff --git a/terraform/modules/cognito-app-client/outputs.tf b/terraform/modules/cognito-app-client/outputs.tf index 5d4a506..0aa9d9e 100644 --- a/terraform/modules/cognito-app-client/outputs.tf +++ b/terraform/modules/cognito-app-client/outputs.tf @@ -28,6 +28,16 @@ output "client_secret_arn" { value = "${local.param_arn}/cognito-client-secret" } +output "identity_provider_arn" { + description = "ARN of the SSM parameter the platform writes the client's sign-in IdP to (e.g. Google, GoogleSAML, COGNITO)" + value = "${local.param_arn}/cognito-client-identity-provider" +} + +output "auth_status_parameter_name" { + description = "SSM parameter where the platform reports the sync result (JSON; read by scripts/auth-doctor.py)" + value = "${local.param_prefix}/auth-status" +} + output "request_parameter_name" { description = "SSM parameter holding the client request (empty for client_type = none)" value = one(aws_ssm_parameter.auth_request[*].name) diff --git a/terraform/org/identity-center.tf b/terraform/org/identity-center.tf index 3da81a6..9f18d29 100644 --- a/terraform/org/identity-center.tf +++ b/terraform/org/identity-center.tf @@ -193,9 +193,9 @@ resource "aws_ssoadmin_permission_set_inline_policy" "developer" { } } }, - # Platform-managed parameters in the team path: Cognito credentials - # (app-auth-sync), the auth request (app Terraform) and RDS master - # passwords (the database keeps the original; changing it breaks the app). + # Platform-managed parameters in the team path: Cognito credentials and + # auth-status (app-auth-sync), the auth request (app Terraform) and RDS + # master passwords (the database keeps the original; changing it breaks the app). { Sid = "DenyPlatformManagedParameters" Effect = "Deny" @@ -203,6 +203,7 @@ resource "aws_ssoadmin_permission_set_inline_policy" "developer" { Resource = [ "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/*/*/cognito-client-*", "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/*/*/auth", + "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/*/*/auth-status", "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/*/*/db-master-password", ] }, diff --git a/terraform/platform/identity/main.tf b/terraform/platform/identity/main.tf index d08739b..137612b 100644 --- a/terraform/platform/identity/main.tf +++ b/terraform/platform/identity/main.tf @@ -1,7 +1,11 @@ ################################################################################ # Cognito User Pools — Internal (java.no) + External (public) # -# Pools are always created. Google IdP is added when google_client_id is set. +# Pools are always created. The internal pool's Google IdP is added once its +# OAuth client is stored in SSM (see below); the external pool's when +# google_client_id is set. Which IdPs app clients use is declared per pool +# (internal_identity_providers / external_identity_providers), so adding an IdP +# changes no client until the list names it. # Custom domains require certificate_arn (must be in us-east-1 for CloudFront). # # Identity Center lives in terraform/org/ (human-applied, not CI). @@ -62,31 +66,56 @@ resource "aws_cognito_user_pool" "internal" { } } +# Google OIDC for the internal pool. The OAuth client belongs to a GCP project +# whose consent screen is Internal, so Google itself refuses non-java.no +# accounts, on a page that lets the user pick another account (the GoogleSAML +# app shows a dead-end 403 instead — javaBin/platform#157). Setup: +# docs/cognito-google-setup.md. +# +# An admin stores the client under //platform/google-oauth/internal/ +# (client-id String, client-secret SecureString). Until both exist this +# creates nothing, so the code can merge before the credentials do. +data "aws_ssm_parameters_by_path" "google_internal" { + path = "/${var.project}/platform/google-oauth/internal" + with_decryption = true +} + +locals { + google_internal_names = [for n in data.aws_ssm_parameters_by_path.google_internal.names : basename(n)] + google_internal = zipmap(local.google_internal_names, data.aws_ssm_parameters_by_path.google_internal.values) + google_internal_ready = contains(local.google_internal_names, "client-id") && contains(local.google_internal_names, "client-secret") +} + resource "aws_cognito_identity_provider" "internal_google" { - count = var.google_client_id != "" ? 1 : 0 + count = local.google_internal_ready ? 1 : 0 user_pool_id = aws_cognito_user_pool.internal.id provider_name = "Google" provider_type = "Google" provider_details = { - client_id = var.google_client_id - client_secret = var.google_client_secret + client_id = local.google_internal["client-id"] + client_secret = local.google_internal["client-secret"] authorize_scopes = "openid email profile" attributes_url = "https://people.googleapis.com/v1/people/me?personFields=" attributes_url_add_attributes = "true" - # TODO: hd= is a UI hint only, not server-side enforcement. Add a pre-sign-up - # Lambda trigger to validate @java.no domain, or use separate GCP OAuth clients. - authorize_url = "https://accounts.google.com/o/oauth2/v2/auth?hd=${var.google_workspace_domain}" + # hd= preselects java.no accounts and prompt=select_account always shows + # Google's account chooser. hd= is a hint only: the Internal consent + # screen (and auth-claims) enforce the domain. Cognito keeping query + # parameters here is not documented — verify after apply with the probe + # in docs/cognito-google-setup.md; if they are dropped, set + # google_authorize_url_params = false and use the managed-login fallback. + authorize_url = "https://accounts.google.com/o/oauth2/v2/auth${var.google_authorize_url_params ? "?hd=${var.google_workspace_domain}&prompt=select_account" : ""}" oidc_issuer = "https://accounts.google.com" token_url = "https://www.googleapis.com/oauth2/v4/token" token_request_method = "POST" } attribute_mapping = { - email = "email" - username = "sub" - name = "name" + email = "email" + email_verified = "email_verified" + username = "sub" + name = "name" } } diff --git a/terraform/platform/identity/outputs.tf b/terraform/platform/identity/outputs.tf index a2eb944..eb866cb 100644 --- a/terraform/platform/identity/outputs.tf +++ b/terraform/platform/identity/outputs.tf @@ -28,6 +28,11 @@ output "internal_saml_entity_id" { value = "urn:amazon:cognito:sp:${aws_cognito_user_pool.internal.id}" } +output "internal_google_enabled" { + description = "Whether the internal pool's Google OIDC IdP exists (its OAuth client is in SSM)" + value = local.google_internal_ready +} + # External pool output "external_user_pool_id" { description = "ID of the external Cognito user pool" @@ -57,6 +62,13 @@ output "auth_memberships_table_arn" { output "internal_identity_providers" { description = "Sign-in IdPs app-auth-sync puts on internal-pool clients" value = var.internal_identity_providers + + # Google is trusted by auth-claims only with the domain rule in place and + # only once the IdP exists (else app-auth-sync would skip every request). + precondition { + condition = !contains(var.internal_identity_providers, "Google") || (local.google_internal_ready && var.google_workspace_domain != "") + error_message = "internal_identity_providers names Google, but //platform/google-oauth/internal/client-id and client-secret are not in SSM yet, or google_workspace_domain is empty (auth-claims' REQUIRED_EMAIL_DOMAIN). See docs/cognito-google-setup.md." + } } output "external_identity_providers" { diff --git a/terraform/platform/identity/variables.tf b/terraform/platform/identity/variables.tf index d6d0e73..ccb8484 100644 --- a/terraform/platform/identity/variables.tf +++ b/terraform/platform/identity/variables.tf @@ -14,14 +14,20 @@ variable "google_workspace_domain" { default = "java.no" } +variable "google_authorize_url_params" { + description = "Add hd=&prompt=select_account to the internal pool's Google authorize_url (false = plain URL, for the managed-login fallback)" + type = bool + default = true +} + variable "google_client_id" { - description = "Google OAuth 2.0 client ID for Cognito IdP (from GCP console)" + description = "Google OAuth 2.0 client ID for the external pool's Google IdP (from GCP console)" type = string default = "" } variable "google_client_secret" { - description = "Google OAuth 2.0 client secret for Cognito IdP (from GCP console)" + description = "Google OAuth 2.0 client secret for the external pool's Google IdP (from GCP console)" type = string sensitive = true default = "" @@ -56,6 +62,11 @@ variable "org_boundary_arn" { # (Workspace-only); otherwise hd= is just a hint and a consumer Google account # with an @java.no address would pass. In that case map email_verified/hd and # check them in auth-claims before adding the IdP here. +# +# Google (OIDC, internal pool) is the exception auth-claims knows about: its +# users must also have email_verified=true (mapped from Google), and the +# identity module refuses "Google" here until its OAuth client is in SSM. +# Cutover from GoogleSAML: ["Google"] (docs/cognito-google-setup.md). variable "internal_identity_providers" { description = "Identity providers on internal-pool app clients (GoogleSAML is created outside Terraform)" type = list(string) diff --git a/terraform/platform/lambdas/main.tf b/terraform/platform/lambdas/main.tf index 00c101c..4102476 100644 --- a/terraform/platform/lambdas/main.tf +++ b/terraform/platform/lambdas/main.tf @@ -1638,9 +1638,10 @@ resource "aws_iam_role_policy" "app_auth_sync" { ] }, { + # GetItem: report allowed groups that do not exist (auth-status) Sid = "WriteClientAllowedGroups" Effect = "Allow" - Action = ["dynamodb:PutItem", "dynamodb:DeleteItem"] + Action = ["dynamodb:PutItem", "dynamodb:DeleteItem", "dynamodb:GetItem"] Resource = var.auth_memberships_table_arn }, { @@ -1655,6 +1656,8 @@ resource "aws_iam_role_policy" "app_auth_sync" { Resource = [ "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/*/*/cognito-client-id", "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/*/*/cognito-client-secret", + "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/*/*/cognito-client-identity-provider", + "arn:aws:ssm:${var.region}:${var.aws_account_id}:parameter/${var.project}/apps/*/*/auth-status", ] }, ] @@ -1686,6 +1689,8 @@ resource "aws_lambda_function" "app_auth_sync" { MEMBERSHIPS_TABLE = var.auth_memberships_table_name INTERNAL_IDENTITY_PROVIDERS = join(",", var.internal_identity_providers) EXTERNAL_IDENTITY_PROVIDERS = join(",", var.external_identity_providers) + INTERNAL_POOL_DOMAIN = var.internal_user_pool_domain + EXTERNAL_POOL_DOMAIN = var.external_user_pool_domain } } } @@ -1744,11 +1749,13 @@ resource "aws_lambda_permission" "app_auth_sync_reconcile" { # A skipped request (configured IdP missing from the pool) leaves the client # unchanged and returns normally, so Lambda's Errors metric does not move. Count # the ERROR line instead and post to javabin-alerts, so a missing IdP does not -# silently freeze every client in the pool. +# silently freeze every client in the pool. Reconcile also logs "Sync failed +# for" for an app it could not sync and returns normally (raising would make +# async invokes retry the whole reconcile twice), so count that too. resource "aws_cloudwatch_log_metric_filter" "app_auth_sync_skipped" { name = "${var.project}-app-auth-sync-skipped" log_group_name = "/aws/lambda/${aws_lambda_function.app_auth_sync.function_name}" - pattern = "\"client left unchanged\"" + pattern = "?\"client left unchanged\" ?\"Sync failed for\"" metric_transformation { name = "AppAuthSyncSkipped" @@ -1760,7 +1767,7 @@ resource "aws_cloudwatch_log_metric_filter" "app_auth_sync_skipped" { resource "aws_cloudwatch_metric_alarm" "app_auth_sync_skipped" { alarm_name = "${var.project}-app-auth-sync-skipped" - alarm_description = "app-auth-sync skipped an app.yaml auth request because a configured identity provider is missing from the pool. See /aws/lambda/${aws_lambda_function.app_auth_sync.function_name}." + alarm_description = "app-auth-sync skipped an app.yaml auth request (a configured identity provider is missing from the pool) or reconcile could not sync an app. See /aws/lambda/${aws_lambda_function.app_auth_sync.function_name}." comparison_operator = "GreaterThanThreshold" evaluation_periods = 1 metric_name = aws_cloudwatch_log_metric_filter.app_auth_sync_skipped.metric_transformation[0].name diff --git a/terraform/platform/lambdas/variables.tf b/terraform/platform/lambdas/variables.tf index 467cba4..974b4d0 100644 --- a/terraform/platform/lambdas/variables.tf +++ b/terraform/platform/lambdas/variables.tf @@ -148,6 +148,17 @@ variable "external_identity_providers" { type = list(string) } +variable "internal_user_pool_domain" { + description = "Hosted-UI domain of the internal pool (reported in auth-status for probes)" + type = string +} + +variable "external_user_pool_domain" { + description = "Hosted-UI domain of the external pool, empty if none" + type = string + default = "" +} + variable "atlassian_sync_apply" { description = "Let team-provisioner write Atlassian group memberships; false = report the plan only" type = bool diff --git a/terraform/platform/main.tf b/terraform/platform/main.tf index b6e8b35..ab0e7bd 100644 --- a/terraform/platform/main.tf +++ b/terraform/platform/main.tf @@ -93,6 +93,8 @@ module "lambdas" { auth_memberships_table_arn = module.identity.auth_memberships_table_arn internal_identity_providers = module.identity.internal_identity_providers external_identity_providers = module.identity.external_identity_providers + internal_user_pool_domain = module.identity.internal_user_pool_domain + external_user_pool_domain = module.identity.external_user_pool_domain identity_store_id = var.identity_store_id sso_instance_arn = var.sso_instance_arn alert_dedup_table_name = module.monitoring.alert_dedup_table_name