Skip to content

fix(identity): lock down the internal Cognito pool - #166

Merged
Alexanderamiri merged 2 commits into
mainfrom
fix/cognito-internal-lockdown
Oct 24, 2026
Merged

Alexanderamiri merged 2 commits into
mainfrom
fix/cognito-internal-lockdown

Conversation

@Alexanderamiri

@Alexanderamiri Alexanderamiri commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

Why

Part of #157. The internal pool (javabin-internal, eu-central-1_Icikv3dtD) is open to the internet today:

  • AllowAdminCreateUserOnly is false, so self sign-up is on.
  • app-auth-sync puts COGNITO on every client, so the hosted UI shows a sign-up link.

Together, anyone can create a user in the internal pool with their own Gmail address. An app with auth: internal and no groups accepts that user. Nothing checks the email domain on the server side.

The COGNITO entry also puts a password form in front of Google. The frosting client was hand-edited in the console to [GoogleSAML] to remove the form. The 03:30 UTC reconcile undoes that edit.

What

  • identity/main.tf

    • The internal pool gets admin_create_user_config { allow_admin_create_user_only = true }.
    • The internal auth-claims function gets two new env vars:
      • REQUIRED_EMAIL_DOMAIN = var.google_workspace_domain (java.no);
      • FEDERATED_IDENTITY_PROVIDERS, set from the internal IdP list.
  • New variables internal_identity_providers (default ["GoogleSAML"]) and external_identity_providers (default ["COGNITO"]).

    • They are declared in the identity module, output from it, and passed to app-auth-sync as INTERNAL_IDENTITY_PROVIDERS / EXTERNAL_IDENTITY_PROVIDERS.
    • Validation: the internal list must not include COGNITO and must not be empty.
  • app-auth-sync

    • Each client gets exactly its pool's IdP list.
    • If a configured IdP doesn't exist in the pool, the request is skipped: an ERROR log and status: error (reconcile lists it under error). Nothing is created, moved or wiped, and the existing client keeps working.
    • Settings are resolved before the pool-switch cleanup, so a misconfigured target pool can't delete the client in the old pool.
  • auth-claims: when REQUIRED_EMAIL_DOMAIN is set, a token is refused unless both of these hold:

    • the email ends with @java.no (case-insensitive);
    • the user is federated through a listed IdP (from the identities attribute, or the <Provider>_ username), or has email_verified=true.

    The user sees "Sign in with your @java.no account." Logs mask the address as e***@java.no.

  • Review follow-ups

    • app-auth-sync rejects aws.cognito.signin.user.admin for pool: internal (status invalid, client not created or changed). That scope would let a signed-in user call UpdateUserAttributes and point their email at another @java.no user, whose registry groups auth-claims would then grant. No live request uses it (only cupcake, with openid email profile). The external pool still allows it.
    • New aws_cloudwatch_log_metric_filter + aws_cloudwatch_metric_alarm javabin-app-auth-sync-skipped on client left unchanged in /aws/lambda/javabin-app-auth-sync, posting to javabin-alerts (Slack via slack-alert). The javabin-alerts topic policy gets an AllowCloudWatchAlarmsPublish statement scoped to this account's alarms.
    • internal_identity_providers documents that each entry is trusted to vouch for email, so it must release only Workspace accounts (Google OIDC needs an Internal-consent client, or an email_verified/hd check first).
    • auth-claims masks the email in the group-denial log too.
  • Docs: docs/lambda-functions.md (both functions) and the docs/app-yaml-reference.md auth section, which covers no password form or sign-up, the @java.no rule, and how a denial arrives as error_description.

How verified

  • Unit tests: python3 -m unittest discover -s terraform/lambda-src/tests (79 OK) and scripts/tests (43 OK). New tests cover:
    • the fixed IdP list, and the reconcile reverting a console edit to the list;
    • env config, the missing-IdP skip on create, update, pool switch and reconcile, and an empty list;
    • for the domain rule: SAML users with email_verified=false, lookalike domains, native users with and without a verified email, other IdPs, and a pool without the rule.
  • terraform fmt -check -recursive and terraform validate pass.
  • Read-only checks against live state:
    • Users: the pool has 4 users. All are GoogleSAML_…, @java.no, EXTERNAL_PROVIDER, email_verified=false, identities provider GoogleSAML. All 4 pass through the federated-IdP branch. This exact shape is a unit test.
    • Auth requests: there is one, /javabin/apps/cupcake/cupcake/auth (frosting, public client, groups [helter]). After this change its client gets [GoogleSAML], the same as the console edit, so the reconcile no longer reverts it. cupcake-backend is client: none and is unaffected.
    • IdPs: the internal pool has exactly one IdP, GoogleSAML, so the configured list resolves. The external pool keeps ["COGNITO"], which is what it gets today (it has no IdPs).
  • Plan expectation:
    • in-place updates: aws_cognito_user_pool.internal (admin_create_user_config is not ForceNew), aws_lambda_function.auth_claims (env) and aws_lambda_function.app_auth_sync (env + code), aws_sns_topic_policy.alerts (new statement);
    • creates: the log metric filter and the alarm (the log group already exists);
    • no destroys, no pool replacement.

Rollout & user actions

  • None needed after merge. The CI apply updates the pool and both Lambdas.
  • The next auth-request change, or the 03:30 UTC reconcile, sets every internal client to [GoogleSAML]. To apply it right away, run aws lambda invoke --function-name javabin-app-auth-sync --payload '{"action":"reconcile"}' --cli-binary-format raw-in-base64-out /dev/stdout.
  • Existing sessions keep working. Refresh tokens pass the domain check because all current users qualify.

Follow-ups

  • Interaction with feat(routing): require sign-in on app.yaml paths at the load balancer #158 (feat/alb-login). That PR edits the same functions in app-auth-sync (client_settings, sync, reconcile) and adds auth_claims_external from the same handler. The external function doesn't set REQUIRED_EMAIL_DOMAIN, so it skips the domain check, which is intended.
    • Rebase on top of this PR: client_settings() now takes pool first.
    • service-routing's hardcoded internal_identity_provider = "GoogleSAML" can be dropped once clients have a single IdP. The hosted UI already redirects.
  • Overlap with fix(platform): harden ALB, Cognito pools, subnets and bucket TLS #165. It adds deletion_protection = "ACTIVE" to both pools (raised in review here too; left to fix(platform): harden ALB, Cognito pools, subnets and bucket TLS #165 to avoid a duplicate). The change is a different attribute, so expect a trivial text merge at most.
  • Not done here: user_attribute_update_settings { attributes_require_verification_before_update = ["email"] } on the internal pool. Blocking the user-admin scope closes the reported path; the pool setting would also affect how SAML attribute mapping updates email, which can't be checked without applying, so it is left for a separate, tested change.
  • Internal login fails with Google 403 app_not_configured_for_user when another Google account is active #157 next step: switching to Google OIDC.
    • Set internal_identity_providers = ["Google"] once the IdP exists.
    • Google users then count as federated. That is safe only with an Internal-consent GCP client (java.no accounts only), as the design note proposes. Otherwise, map email_verified and drop Google from the trusted list.
  • Bring GoogleSAML under Terraform with an import block, or remove it after cutover.

🤖 Generated with Claude Code

- Disable self sign-up on the internal pool (allow_admin_create_user_only).
- app-auth-sync: give each pool a fixed IdP list from Terraform instead of
  every pool IdP plus COGNITO; skip and log when a configured IdP is missing.
- auth-claims: on the internal pool, refuse tokens unless the email is
  @java.no and the user is federated (GoogleSAML) or email_verified.

Part of #157

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@Alexanderamiri
Alexanderamiri requested a review from a team as a code owner October 24, 2026 14:02
@github-actions

Copy link
Copy Markdown

Terraform Plan

🚧 Changes detected — Plan: 0 to add, 3 to change, 0 to destroy.

Plan output
Acquiring state lock. This may take a few moments...
aws_ssm_parameter.atlassian_api_key: Refreshing state... [id=/javabin/platform/atlassian-api-key]
aws_ssm_parameter.atlassian_org_id: Refreshing state... [id=/javabin/platform/atlassian-org-id]

Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # module.identity.aws_cognito_user_pool.internal will be updated in-place
  ~ resource "aws_cognito_user_pool" "internal" {
        id                        = "eu-central-1_Icikv3dtD"
        name                      = "javabin-internal"
        tags                      = {
            "Name" = "javabin-internal"
        }
        # (12 unchanged attributes hidden)

      ~ admin_create_user_config {
          ~ allow_admin_create_user_only = false -> true
        }

        # (7 unchanged blocks hidden)
    }

  # module.identity.aws_lambda_function.auth_claims will be updated in-place
  ~ resource "aws_lambda_function" "auth_claims" {
        id                             = "javabin-auth-claims"
      ~ last_modified                  = "2026-10-23T15:36:35.282+0000" -> (known after apply)
      ~ source_code_hash               = "T0XK8BQ05sxP5MVCsIGH+JuY++sBeQohaGTkvMdm81M=" -> "g5gt69/9UeCPElGSkbf6DkPPSFNXH02JSXQGtqm4c/I="
        tags                           = {}
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              + "FEDERATED_IDENTITY_PROVIDERS" = "GoogleSAML"
              + "REQUIRED_EMAIL_DOMAIN"        = "java.no"
                # (1 unchanged element hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.lambdas.aws_lambda_function.app_auth_sync will be updated in-place
  ~ resource "aws_lambda_function" "app_auth_sync" {
        id                             = "javabin-app-auth-sync"
      ~ last_modified                  = "2026-10-23T15:31:34.000+0000" -> (known after apply)
      ~ source_code_hash               = "sIha3Wb5dyMLBep00WQdQ/yhswtf4SXSuDzJX076WoE=" -> "e3Gaxh2v+oXw+ghxdo3QLl6JjAfHNJOB0Tk+RP8GRmc="
        tags                           = {}
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              + "EXTERNAL_IDENTITY_PROVIDERS" = "COGNITO"
              + "INTERNAL_IDENTITY_PROVIDERS" = "GoogleSAML"
                # (2 unchanged elements hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

Plan: 0 to add, 3 to change, 0 to destroy.

─────────────────────────────────────────────────────────────────────────────

Saved the plan to: tfplan

To perform exactly these actions, run the following command to apply:
    terraform apply "tfplan"

LLM Review

Risk: 🟡 MEDIUM

Plan updates 3 existing resources in place (0 add, 0 destroy): a Cognito user pool setting change that stops self sign-up, and two Lambda code and environment updates for federated identity provider configuration.

  • 🔒 [security] Cognito user pool javabin-internal: admin_create_user_config.allow_admin_create_user_only changes false -> true. This disables self sign-up, so only administrators can create users. This is a tightening change, but it affects who can access the internal pool and should be confirmed as intended alongside the federation changes.
  • 🔒 [security] Lambda javabin-auth-claims gains environment variables FEDERATED_IDENTITY_PROVIDERS=GoogleSAML and REQUIRED_EMAIL_DOMAIN=java.no, and its code hash changes. The email domain restriction is a useful access control, but the new code is not visible in the plan, so the logic it runs cannot be reviewed here. Verify the code change in the commit or artifact that produced it.
  • ✅ [routine] Lambda javabin-app-auth-sync gains INTERNAL_IDENTITY_PROVIDERS=GoogleSAML and EXTERNAL_IDENTITY_PROVIDERS=COGNITO, plus a new code hash. Environment-only and code updates, with no change to IAM roles, triggers or networking in this plan.
  • 💰 [cost] No new billable resources are created. Cost impact is negligible.
  • 💥 [destruction] No resources are destroyed or replaced. All three changes are in-place updates, so the Cognito pool keeps its ID and its users.

…on skipped syncs

Review follow-up for the internal pool lockdown:
- app-auth-sync rejects aws.cognito.signin.user.admin for pool=internal, so a
  signed-in user cannot rewrite the email auth-claims trusts for group lookup
- alarm javabin-app-auth-sync-skipped (log metric filter) posts to
  javabin-alerts when a request is skipped for a missing IdP; the alerts topic
  policy now lets CloudWatch alarms in this account publish
- document that every internal_identity_providers entry vouches for email
- auth-claims masks the email in the group denial log too; tidy imports

Part of #157

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Terraform Plan

🚧 Changes detected — Plan: 2 to add, 4 to change, 0 to destroy.

Plan output
aws_ssm_parameter.atlassian_org_id: Refreshing state... [id=/javabin/platform/atlassian-org-id]
aws_ssm_parameter.atlassian_api_key: Refreshing state... [id=/javabin/platform/atlassian-api-key]

Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
  + create
  ~ update in-place

Terraform will perform the following actions:

  # module.identity.aws_cognito_user_pool.internal will be updated in-place
  ~ resource "aws_cognito_user_pool" "internal" {
        id                        = "eu-central-1_Icikv3dtD"
        name                      = "javabin-internal"
        tags                      = {
            "Name" = "javabin-internal"
        }
        # (12 unchanged attributes hidden)

      ~ admin_create_user_config {
          ~ allow_admin_create_user_only = false -> true
        }

        # (7 unchanged blocks hidden)
    }

  # module.identity.aws_lambda_function.auth_claims will be updated in-place
  ~ resource "aws_lambda_function" "auth_claims" {
        id                             = "javabin-auth-claims"
      ~ last_modified                  = "2026-10-23T15:36:35.282+0000" -> (known after apply)
      ~ source_code_hash               = "T0XK8BQ05sxP5MVCsIGH+JuY++sBeQohaGTkvMdm81M=" -> "7KJRUIQbOY5MEZQ2WTZkRPiF0xtT6GURFhNiTZGZo6o="
        tags                           = {}
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              + "FEDERATED_IDENTITY_PROVIDERS" = "GoogleSAML"
              + "REQUIRED_EMAIL_DOMAIN"        = "java.no"
                # (1 unchanged element hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.lambdas.aws_cloudwatch_log_metric_filter.app_auth_sync_skipped will be created
  + resource "aws_cloudwatch_log_metric_filter" "app_auth_sync_skipped" {
      + id             = (known after apply)
      + log_group_name = "/aws/lambda/javabin-app-auth-sync"
      + name           = "javabin-app-auth-sync-skipped"
      + pattern        = "\"client left unchanged\""

      + metric_transformation {
          + default_value = "0"
          + name          = "AppAuthSyncSkipped"
          + namespace     = "Javabin/AppAuthSync"
          + unit          = "None"
          + value         = "1"
        }
    }

  # module.lambdas.aws_cloudwatch_metric_alarm.app_auth_sync_skipped will be created
  + resource "aws_cloudwatch_metric_alarm" "app_auth_sync_skipped" {
      + actions_enabled                       = true
      + alarm_actions                         = [
          + "arn:aws:sns:eu-central-1:553637109631:javabin-alerts",
        ]
      + alarm_description                     = "app-auth-sync skipped an app.yaml auth request because a configured identity provider is missing from the pool. See /aws/lambda/javabin-app-auth-sync."
      + alarm_name                            = "javabin-app-auth-sync-skipped"
      + arn                                   = (known after apply)
      + comparison_operator                   = "GreaterThanThreshold"
      + evaluate_low_sample_count_percentiles = (known after apply)
      + evaluation_periods                    = 1
      + id                                    = (known after apply)
      + metric_name                           = "AppAuthSyncSkipped"
      + namespace                             = "Javabin/AppAuthSync"
      + period                                = 300
      + statistic                             = "Sum"
      + tags_all                              = {
          + "environment" = "production"
          + "managed-by"  = "terraform"
          + "repo"        = "javaBin/platform"
          + "service"     = "platform"
          + "team"        = "platform"
        }
      + threshold                             = 0
      + treat_missing_data                    = "notBreaching"
    }

  # module.lambdas.aws_lambda_function.app_auth_sync will be updated in-place
  ~ resource "aws_lambda_function" "app_auth_sync" {
        id                             = "javabin-app-auth-sync"
      ~ last_modified                  = "2026-10-23T15:31:34.000+0000" -> (known after apply)
      ~ source_code_hash               = "sIha3Wb5dyMLBep00WQdQ/yhswtf4SXSuDzJX076WoE=" -> "d1t3s0DwtOCO4J/h0uTpYtHYVnp6f/5eRhipC6lyDvI="
        tags                           = {}
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              + "EXTERNAL_IDENTITY_PROVIDERS" = "COGNITO"
              + "INTERNAL_IDENTITY_PROVIDERS" = "GoogleSAML"
                # (2 unchanged elements hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.monitoring.aws_sns_topic_policy.alerts will be updated in-place
  ~ resource "aws_sns_topic_policy" "alerts" {
        id     = "arn:aws:sns:eu-central-1:553637109631:javabin-alerts"
      ~ policy = jsonencode(
          ~ {
              ~ Statement = [
                    # (2 unchanged elements hidden)
                    {
                        Action    = "SNS:Publish"
                        Effect    = "Allow"
                        Principal = {
                            Service = "events.amazonaws.com"
                        }
                        Resource  = "arn:aws:sns:eu-central-1:553637109631:javabin-alerts"
                        Sid       = "AllowEventsPublish"
                    },
                  + {
                      + Action    = "SNS:Publish"
                      + Condition = {
                          + ArnLike      = {
                              + "aws:SourceArn" = "arn:aws:cloudwatch:eu-central-1:553637109631:alarm:*"
                            }
                          + StringEquals = {
                              + "aws:SourceAccount" = "553637109631"
                            }
                        }
                      + Effect    = "Allow"
                      + Principal = {
                          + Service = "cloudwatch.amazonaws.com"
                        }
                      + Resource  = "arn:aws:sns:eu-central-1:553637109631:javabin-alerts"
                      + Sid       = "AllowCloudWatchAlarmsPublish"
                    },
                ]
                # (1 unchanged attribute hidden)
            }
        )
        # (2 unchanged attributes hidden)
    }

Plan: 2 to add, 4 to change, 0 to destroy.

─────────────────────────────────────────────────────────────────────────────

Saved the plan to: tfplan

To perform exactly these actions, run the following command to apply:
    terraform apply "tfplan"

LLM Review

Risk: 🟡 MEDIUM

Plan adds two CloudWatch resources (metric filter and alarm) for app-auth-sync skipped requests and updates the Cognito user pool, two Lambda functions, and an SNS topic policy; nothing is destroyed, but enabling admin-only user creation and the new Lambda environment variables change authentication behaviour and should be confirmed before apply.

  • 🔒 [security] Cognito user pool javabin-internal changes allow_admin_create_user_only from false to true, so end users can no longer self-sign-up. This is an intentional-looking hardening but changes how accounts are created and should be confirmed with the identity owners.
  • 🔒 [security] auth_claims Lambda gains FEDERATED_IDENTITY_PROVIDERS=GoogleSAML and REQUIRED_EMAIL_DOMAIN=java.no, which restricts which logins are accepted. Verify the domain check and provider name are correct, since a typo could lock out users or weaken the restriction.
  • 🔒 [security] SNS topic policy javabin-alerts gains a statement letting cloudwatch.amazonaws.com publish, scoped by aws:SourceArn to alarms in account 553637109631 and aws:SourceAccount. This is a properly constrained grant, not a public one. The existing events.amazonaws.com statement is unchanged.
  • ✅ [routine] app_auth_sync and auth_claims Lambda code updates (source_code_hash changes) and new environment variables for internal/external identity provider routing. These are routine code and config updates with no resources replaced.
  • ✅ [routine] New metric filter on /aws/lambda/javabin-app-auth-sync matching "client left unchanged", feeding the AppAuthSyncSkipped metric, plus an alarm that fires on any count above 0 and notifies javabin-alerts. This is observability only and has negligible cost.
  • 💥 [destruction] Plan has 0 destroys and no resource replacements. All changes are in-place updates or creates.

Alexanderamiri added a commit that referenced this pull request Oct 24, 2026
…th doctor

Builds on the internal-pool lockdown (#166) and reuses its per-pool IdP
list (internal_identity_providers, INTERNAL_IDENTITY_PROVIDERS,
identity_providers(pool, upid), PoolMisconfigured), so one list drives
both the app clients and auth-claims' trusted IdPs.

- identity: Google OIDC IdP on the internal pool, created only once its
  OAuth client is in SSM (/javabin/platform/google-oauth/internal/*);
  authorize_url forces the account chooser (hd=java.no&prompt=select_account),
  email_verified mapped. Precondition: "Google" in
  internal_identity_providers needs the credentials and
  google_workspace_domain.
- auth-claims: Google users also need email_verified=true
  (EMAIL_VERIFIED_IDENTITY_PROVIDERS, default Google).
- app-auth-sync: write cognito-client-* only on change, add
  cognito-client-identity-provider, publish team-readable auth-status
  (status, client, client_id_changed_at, IdPs, drift corrected, unknown
  groups; always valid JSON under 4096 bytes); reject URL fragments;
  reconcile keeps going past a failing app and returns normally (logged
  for the skipped alarm) instead of raising into async retries.
- app-secret-rollout: restart on cognito-client-id / -identity-provider
  changes; ignore auth-status and cognito-client-secret (a new secret
  always comes with a new ID).
- generate-modules: validate callback/logout hosts (routing.host or
  auth.extra_hosts), https except localhost, no fragments; reject
  client: none with URLs/scopes; inject COGNITO_IDENTITY_PROVIDER.
- scripts/auth-doctor.py, run non-blocking after app apply (FAIL on a
  non-JSON status, short wait when auth-status is missing, task check is
  a WARN against client_id_changed_at for PRIMARY tasks only); platform
  CI queues a reconcile after apply.
- Docs: GCP setup with the correct redirect URI, cutover plan with the
  lockdown as a hard prerequisite; team CI roles can still write
  platform-managed parameters (documented); SAML doc marked superseded.

Part of #157

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@Alexanderamiri
Alexanderamiri merged commit 663d247 into main Oct 24, 2026
4 checks passed
@Alexanderamiri
Alexanderamiri deleted the fix/cognito-internal-lockdown branch October 24, 2026 22:45
Alexanderamiri added a commit that referenced this pull request Oct 24, 2026
…th doctor

Builds on the internal-pool lockdown (#166) and reuses its per-pool IdP
list (internal_identity_providers, INTERNAL_IDENTITY_PROVIDERS,
identity_providers(pool, upid), PoolMisconfigured), so one list drives
both the app clients and auth-claims' trusted IdPs.

- identity: Google OIDC IdP on the internal pool, created only once its
  OAuth client is in SSM (/javabin/platform/google-oauth/internal/*);
  authorize_url forces the account chooser (hd=java.no&prompt=select_account),
  email_verified mapped. Precondition: "Google" in
  internal_identity_providers needs the credentials and
  google_workspace_domain.
- auth-claims: Google users also need email_verified=true
  (EMAIL_VERIFIED_IDENTITY_PROVIDERS, default Google).
- app-auth-sync: write cognito-client-* only on change, add
  cognito-client-identity-provider, publish team-readable auth-status
  (status, client, client_id_changed_at, IdPs, drift corrected, unknown
  groups; always valid JSON under 4096 bytes); reject URL fragments;
  reconcile keeps going past a failing app and returns normally (logged
  for the skipped alarm) instead of raising into async retries.
- app-secret-rollout: restart on cognito-client-id / -identity-provider
  changes; ignore auth-status and cognito-client-secret (a new secret
  always comes with a new ID).
- generate-modules: validate callback/logout hosts (routing.host or
  auth.extra_hosts), https except localhost, no fragments; reject
  client: none with URLs/scopes; inject COGNITO_IDENTITY_PROVIDER.
- scripts/auth-doctor.py, run non-blocking after app apply (FAIL on a
  non-JSON status, short wait when auth-status is missing, task check is
  a WARN against client_id_changed_at for PRIMARY tasks only); platform
  CI queues a reconcile after apply.
- Docs: GCP setup with the correct redirect URI, cutover plan with the
  lockdown as a hard prerequisite; team CI roles can still write
  platform-managed parameters (documented); SAML doc marked superseded.

Part of #157

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Alexanderamiri added a commit that referenced this pull request Oct 24, 2026
…th doctor (#167)

## Why

Part of #157. Internal-pool sign-in goes through the `GoogleSAML` app,
which always answers for the browser's active Google account. A personal
Gmail gets a dead-end `403 app_not_configured_for_user` page with no
account chooser. The classic hosted UI drops `prompt=`, so Cognito can't
ask SAML for one either. The fix is a Google OIDC IdP, with an
Internal-consent OAuth client and `prompt=select_account`.

The investigation also found that teams can't see why sign-in fails, and
can't recover:
- invalid requests and console drift are visible only in CloudWatch;
- a replaced client leaves running tasks on a deleted client ID;
- callback typos show up only as `redirect_mismatch` in the browser;
- an `auth.groups` typo denies everyone.

**Merge #166 (internal-pool lockdown) first.** This branch is rebased on
it and reuses its per-pool IdP list, so until #166 merges this PR's diff
and plan also contain #166's two commits. After #166 lands, GitHub shows
only the last commit.

This PR is safe to merge before the Google credentials exist. Every
Google behaviour change is gated, and with #166 the internal default is
`["GoogleSAML"]` (what cupcake's client has live today).

## What

**identity (Terraform)**
- `aws_cognito_identity_provider.internal_google` is now gated on SSM
`/javabin/platform/google-oauth/internal/{client-id,client-secret}`,
read with `aws_ssm_parameters_by_path`, instead of a tfvar. Its count
stays 0 until both exist.
- `authorize_scopes = "openid email profile"`, and mapping `email`,
`email_verified`, `name`, `username=sub`.
- `authorize_url = …/v2/auth?hd=java.no&prompt=select_account`. You can
turn this off with `google_authorize_url_params = false` for the
managed-login fallback.
- Uses #166's per-pool lists `internal_identity_providers` (default
`["GoogleSAML"]`, COGNITO refused) and `external_identity_providers`
(`["COGNITO"]`), env
`INTERNAL_IDENTITY_PROVIDERS`/`EXTERNAL_IDENTITY_PROVIDERS`. The same
internal list is auth-claims' `FEDERATED_IDENTITY_PROVIDERS`, so the
cutover changes clients and the trust list together.
  - Adding the Google IdP changes no client until this list names it.
- An output precondition fails the plan if `Google` is listed without
the SSM credentials or with an empty `google_workspace_domain`
(auth-claims' `REQUIRED_EMAIL_DOMAIN`).
- auth-claims: IdPs in `EMAIL_VERIFIED_IDENTITY_PROVIDERS` (default
`Google`) are not trusted on the IdP name alone; their users also need
`email_verified=true`, mapped from Google's claim.
- The external pool keeps its `google_client_id` tfvar path. It is
unchanged and still empty.

**app-auth-sync**
- `cognito-client-id`, `-secret` and the new `-identity-provider` are
written **only when the value changes**.
  - The secret is not decrypted. It changes only with a new client ID.
- The client ID is written last, so restarted tasks find the matching
secret.
- Writes `/javabin/apps/{team}/{app}/auth-status` (String JSON, always
valid and under 4096 bytes: list fields are dropped, never sliced).
Fields: `status` (synced/invalid/error), `error`, `client_id`,
`client_id_changed_at` (carried over between syncs),
`identity_providers`, `pool_domain`, URLs, `drift_corrected` (fields a
reconcile reverted; also logged as `WARN drift`),
`unknown_groups`/`empty_groups` (checked against `group#` items in the
memberships table), `request_version`, `synced_at`.
- A declared IdP missing from the pool is #166's `PoolMisconfigured`
skip: status `error` in auth-status, the client is left as it is.
- Reconcile no longer stops at a failing app, and returns normally
instead of raising (an async invoke would retry the whole run twice).
The failure is logged as `Sync failed for …`, which #166's
`javabin-app-auth-sync-skipped` metric filter now also counts.
- Callback/logout URLs with a `#fragment` are rejected by the Lambda
too.
- IAM: `dynamodb:GetItem` on the memberships table, plus write access to
`*/cognito-client-identity-provider` and `*/auth-status`.

**Team access to auth-status**
- Teams read it through their existing `/javabin/apps/{team}/*` read
grant.
- Writes are denied for `auth-status` in the team-provisioner
permission-set policy (CI-applied), and in
`terraform/org/identity-center.tf` (human-applied; see user actions).

**app-secret-rollout**
- `cognito-client-id` and `cognito-client-identity-provider` are no
longer ignored. A replaced client (case 9/15) or an IdP cutover now
restarts the service.
- `auth-status` and `cognito-client-secret` are ignored: auth-status is
rewritten on every sync, and a new secret always comes with a new ID
written right after it, so restarting on the secret would deploy once
with the old ID.
- There is no restart loop, because unchanged values are never
rewritten.

**generate-modules.py**
- Callback/logout URLs must:
  - use `https`, except `http://localhost` / `127.0.0.1`;
  - have no `#fragment`;
- have a host equal to `routing.host` or listed in the new optional
`auth.extra_hosts`.
- `client: none` combined with `callback_urls`, `logout_urls`, `scopes`
or `extra_hosts` is rejected; these were silently ignored before.
- `COGNITO_IDENTITY_PROVIDER` is injected (an ECS secret from
`module.auth.identity_provider_arn`) and can be aliased through
`auth.env`.
- Auth resolution is factored into `resolve_auth()`, which the doctor
reuses.
- Both live apps (frosting, cupcake-backend) still generate. Tests use
their verbatim app.yaml.

**cognito-app-client module**
- New outputs `identity_provider_arn` and `auth_status_parameter_name`.

**scripts/auth-doctor.py**
- Uses team credentials only: app.yaml, the app's own SSM parameters,
and unauthenticated HTTP.
- Checks:
  - the request matches app.yaml;
- auth-status is synced for the current request version, and its client
ID matches `cognito-client-id`;
  - drift and unknown/empty groups are reported;
- `/oauth2/authorize` for every callback URL redirects to the IdP, not
`/error?error=redirect_mismatch` (it warns if Google isn't given
`prompt=select_account`);
- `/logout` for every logout URL redirects back (a mismatch lands on the
hosted UI `/login`, verified live);
- best effort, WARN only: the service's PRIMARY-deployment tasks started
after `client_id_changed_at` (skipped until auth-status records one;
`cognito-client-id`'s LastModifiedDate is useless because the old code
rewrote it on every reconcile).
- A non-JSON auth-status is a FAIL, not a traceback. When auth-status is
missing altogether, the doctor waits at most 30 s instead of the full
`--wait`.
- `--json` is the hook for a future `javabin auth doctor`. javabin-cli
is not changed here.
- `tf-apply.yml` gets a non-blocking `Auth doctor` step
(`continue-on-error`, `--wait 180`, skipped for apps without auth).

**platform-ci**
- After apply, `scripts/reconcile-app-auth.sh` queues an async
reconcile. This writes the new `cognito-client-identity-provider` and
`auth-status` parameters right away, so an app redeploy that picks up
the new `COGNITO_IDENTITY_PROVIDER` secret doesn't hit
`ParameterNotFound`. It is non-blocking, and the 03:30 reconcile is the
backstop.

**Docs**
- `docs/cognito-google-setup.md` is rewritten:
- exact GCP steps (Internal consent screen, a separate Web OAuth client,
redirect
`https://javabin-internal.auth.eu-central-1.amazoncognito.com/oauth2/idpresponse`;
the old doc listed a nonexistent `auth-internal.javazone.no`);
  - SSM commands;
  - the post-apply curl probe and the managed-login fallback;
  - cutover and user plan.
- `docs/cognito-saml-setup.md` is marked superseded. It also noted that
its groups mapping never existed.
- `docs/app-yaml-reference.md`: IdP list, URL rules and `extra_hosts`,
the `/logout` exact-match rule, group denial (`error_description`),
`COGNITO_IDENTITY_PROVIDER`, auth-status and the doctor.
- `docs/lambda-functions.md`, `docs/reusable-modules.md` and `CLAUDE.md`
are updated.

## How verified

- `python3 -m unittest discover -s terraform/lambda-src/tests` passes
(111, including #166's), and `-s scripts/tests` passes (67, including
the fmt check of generated TF). New tests cover:
  - no-op syncs write only auth-status;
  - secret written before ID on replacement;
- status content: invalid, error, drift vs request change, unknown/empty
groups;
- declared IdP list, cutover switching the IdP param without touching
the client ID, a missing IdP failing cleanly;
  - reconcile continuing past a failing app;
  - rollout restarting on `cognito-client-*`;
  - URL validation;
- doctor scenarios, including non-JSON status, the short wait for a
missing status, and draining/stale tasks;
- auth-status size limit (bytes, valid JSON), `client_id_changed_at`
carry-over, fragments rejected;
  - auth-claims refusing Google users without `email_verified=true`.
- `terraform fmt -recursive -check` and `terraform validate` (platform
root) pass.
- Live, read-only: the doctor against frosting's app.yaml.
- Request check passes. auth-status doesn't exist yet, as expected
before deploy.
- Probes against the live `cupcake-cupcake` client: callback →
GoogleSAML ✔, `/nope` → `redirect_mismatch` ✔, logout exact ✔, logout
without trailing slash → hosted UI `/login` (this is why the logout
check looks for "redirects back").
- Live pool state checked: 4 users, all `GoogleSAML_*` external-provider
users, no native users. `/javabin/apps/*` has only cupcake's `auth` and
`cognito-client-id`.

## Rollout & user actions

Merging (after #166) changes nothing about Google sign-in:
- The Google IdP isn't created without SSM credentials.
- The internal IdP list default is #166's `["GoogleSAML"]`, which equals
cupcake's live client (`SupportedIdentityProviders=[GoogleSAML]`). The
post-apply reconcile therefore does not put COGNITO back; on main
without #166, the old 03:30 reconcile would.
- On the first reconcile, cupcake gets `cognito-client-identity-provider
= GoogleSAML` (Create, so no restart) and an `auth-status`.

Expected plan: the `aws_ssm_parameters_by_path` read, and in-place
updates to the app-auth-sync role policy, function env/code,
app-secret-rollout code and team-provisioner code. No destroys.

User actions:
1. **Human apply `terraform/org`** (identity-center.tf). It adds
`auth-status` to the `javabin-developer` permission set's
platform-managed deny. Per-team permission sets get it from
team-provisioner on its next run.
2. **GCP (when ready):** create the Internal-consent OAuth client and
store it in SSM, following `docs/cognito-google-setup.md` §1–2. Then
re-run platform CI on main. The plan creates the `Google` IdP, and
clients are untouched.
3. **Verify:** run the §3 curl probe (or the doctor after cutover) to
confirm Cognito keeps `prompt=select_account`. If it doesn't, use the
documented managed-login fallback.
4. **Cutover (separate PR):** `internal_identity_providers =
["Google"]`. #166 (server-side domain check, no self sign-up) is a hard
prerequisite, which stacking on it guarantees.
- Users get a new `sub`. Platform memberships and groups are keyed by
email, so nothing to migrate there; cupcake should check for `sub`-keyed
data.
- Delete the 4 `GoogleSAML_*` users and the `GoogleSAML` IdP a week
later, and turn off the Google Admin SAML app.

## Interactions

- **#166 (lockdown):** this PR is stacked on it and reuses its variable,
output, env names and `identity_providers(pool, upid)`. It adds to
#166's code: the Google precondition on `internal_identity_providers`,
`EMAIL_VERIFIED_IDENTITY_PROVIDERS` in auth-claims, and a second pattern
on the skipped-sync metric filter. If #166 is squash-merged, I rebase
this branch onto main with `--onto`.
- **#158 / `feat/alb-login`:** both change `app_auth_sync`
(`client_settings`, `sync`, `_put_param`, `reconcile`) and
`generate-modules.py` auth handling. Expect textual conflicts; the slot
threading composes with this.
- Login slots should write `auth-status` and
`cognito-client-identity-provider` next to the slot request
(`…/login-N/`). The rollout regex already ignores slot params.
- service-routing's hardcoded `internal_identity_provider =
"GoogleSAML"` should go away: with a single IdP per client it is
unnecessary, and after cutover it points at a deleted IdP. Otherwise
read the slot's `cognito-client-identity-provider`.
- **#156:** also edits `app_secret_rollout/handler.py`, a small
conflict.

## Follow-ups

- The developer boundary (team CI roles, gate-vended apply credentials)
doesn't deny writing `auth-status`/`cognito-client-*`. It is at
5,946/6,144 bytes and #156 adds to it, so I left it alone; the
limitation is now documented in `docs/lambda-functions.md` and
`docs/app-yaml-reference.md`. The impact stays inside the team's own
app: a forged status, or one extra restart before the next sync puts a
credential back. A Deny in the team CI role policies is the follow-up if
we want it enforced.
- Drop `ALLOW_USER_SRP_AUTH` from internal clients. With #166 there is
no COGNITO and no self sign-up, so it is unused there, but changing
client auth flows is #166-adjacent scope.
- `javabin auth doctor` in javabin-cli, wrapping `scripts/auth-doctor.py
--json`.
- Doctor check that the task definition contains every `auth.env` alias;
canary-client probe in platform CI after identity applies.
- `auth.accept_clients` for `client: none` backends (case 13).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Alexanderamiri added a commit that referenced this pull request Oct 26, 2026
New app.yaml routing.login puts paths behind a Cognito sign-in at the ALB,
with per-rule allowed groups, on the internal or external pool.

- generate-modules.py validates routing.login and emits login_rules.
- service-routing: per protected rule a cognito-app-client in slot login-N,
  a wait for app-auth-sync, and an authenticate-oidc + forward listener rule.
  No identity_provider parameter: every client gets the pool's sign-in IdP
  list, and with one IdP the hosted UI goes straight to it.
- cognito-app-client: optional slot; external pool falls back to its prefix
  domain; new parameter-name and request_value outputs.
- app-auth-sync: accepts /apps/{team}/{app}/login-N/auth and keeps client
  {team}-{app}.login-N, with credentials, IdP and auth-status next to the
  request; reconcile handles slot orphans.
- identity: external pool gets the javabin-external prefix domain, email
  verification before update, and javabin-auth-claims-external
  (REGISTRY_GROUPS=false).

Rebased onto the internal-pool lockdown (#166) and the per-pool IdP lists
(#167); their design is kept.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant