Repository navigation
feat(auth): gated Google OIDC IdP, auth-status, URL validation and auth doctor - #167
Alexanderamiri wants to merge 3 commits into
Conversation
- 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) <[email protected]>
Terraform Plan🚧 Changes detected — Plan: 0 to add, 4 to change, 0 to destroy. Plan outputLLM ReviewRisk: 🟡 MEDIUM Plan updates 4 existing resources in place (0 add, 0 destroy): an IAM role policy for app-auth-sync gains DynamoDB GetItem and two SSM parameter read paths, and three Lambda functions get new code and, for app-auth-sync, new environment variables.
|
…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) <[email protected]>
|
Notes on the risk review (MEDIUM):
|
…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) <[email protected]>
094b5e2 to
8359c32
Compare
Terraform Plan🚧 Changes detected — Plan: 2 to add, 7 to change, 0 to destroy. Plan outputLLM ReviewRisk: 🟡 MEDIUM Plan adds 2 CloudWatch resources (metric filter and alarm) and updates 7 existing resources: Cognito user pool admin-only sign-up, Lambda code and env vars for auth/provisioning, an IAM policy widening DynamoDB and SSM read access, and an SNS topic policy extension; 0 destroyed. |
|
Review round 1 addressed (pushed 8359c32, rebased on #166). Notes on the new plan and risk review (MEDIUM, 2 add / 7 change / 0 destroy):
|
Why
Part of #157. Internal-pool sign-in goes through the
GoogleSAMLapp, which always answers for the browser's active Google account. A personal Gmail gets a dead-end403 app_not_configured_for_userpage with no account chooser. The classic hosted UI dropsprompt=, so Cognito can't ask SAML for one either. The fix is a Google OIDC IdP, with an Internal-consent OAuth client andprompt=select_account.The investigation also found that teams can't see why sign-in fails, and can't recover:
redirect_mismatchin the browser;auth.groupstypo 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_googleis now gated on SSM/javabin/platform/google-oauth/internal/{client-id,client-secret}, read withaws_ssm_parameters_by_path, instead of a tfvar. Its count stays 0 until both exist.authorize_scopes = "openid email profile", and mappingemail,email_verified,name,username=sub.authorize_url = …/v2/auth?hd=java.no&prompt=select_account. You can turn this off withgoogle_authorize_url_params = falsefor the managed-login fallback.internal_identity_providers(default["GoogleSAML"], COGNITO refused) andexternal_identity_providers(["COGNITO"]), envINTERNAL_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.Googleis listed without the SSM credentials or with an emptygoogle_workspace_domain(auth-claims'REQUIRED_EMAIL_DOMAIN).EMAIL_VERIFIED_IDENTITY_PROVIDERS(defaultGoogle) are not trusted on the IdP name alone; their users also needemail_verified=true, mapped from Google's claim.google_client_idtfvar path. It is unchanged and still empty.app-auth-sync
cognito-client-id,-secretand the new-identity-providerare written only when the value changes./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 asWARN drift),unknown_groups/empty_groups(checked againstgroup#items in the memberships table),request_version,synced_at.PoolMisconfiguredskip: statuserrorin auth-status, the client is left as it is.Sync failed for …, which fix(identity): lock down the internal Cognito pool #166'sjavabin-app-auth-sync-skippedmetric filter now also counts.#fragmentare rejected by the Lambda too.dynamodb:GetItemon the memberships table, plus write access to*/cognito-client-identity-providerand*/auth-status.Team access to auth-status
/javabin/apps/{team}/*read grant.auth-statusin the team-provisioner permission-set policy (CI-applied), and interraform/org/identity-center.tf(human-applied; see user actions).app-secret-rollout
cognito-client-idandcognito-client-identity-providerare no longer ignored. A replaced client (case 9/15) or an IdP cutover now restarts the service.auth-statusandcognito-client-secretare 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.generate-modules.py
https, excepthttp://localhost/127.0.0.1;#fragment;routing.hostor listed in the new optionalauth.extra_hosts.client: nonecombined withcallback_urls,logout_urls,scopesorextra_hostsis rejected; these were silently ignored before.COGNITO_IDENTITY_PROVIDERis injected (an ECS secret frommodule.auth.identity_provider_arn) and can be aliased throughauth.env.resolve_auth(), which the doctor reuses.cognito-app-client module
identity_provider_arnandauth_status_parameter_name.scripts/auth-doctor.py
cognito-client-id;/oauth2/authorizefor every callback URL redirects to the IdP, not/error?error=redirect_mismatch(it warns if Google isn't givenprompt=select_account);/logoutfor every logout URL redirects back (a mismatch lands on the hosted UI/login, verified live);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).--wait.--jsonis the hook for a futurejavabin auth doctor. javabin-cli is not changed here.tf-apply.ymlgets a non-blockingAuth doctorstep (continue-on-error,--wait 180, skipped for apps without auth).platform-ci
scripts/reconcile-app-auth.shqueues an async reconcile. This writes the newcognito-client-identity-providerandauth-statusparameters right away, so an app redeploy that picks up the newCOGNITO_IDENTITY_PROVIDERsecret doesn't hitParameterNotFound. It is non-blocking, and the 03:30 reconcile is the backstop.Docs
docs/cognito-google-setup.mdis rewritten:https://javabin-internal.auth.eu-central-1.amazoncognito.com/oauth2/idpresponse; the old doc listed a nonexistentauth-internal.javazone.no);docs/cognito-saml-setup.mdis marked superseded. It also noted that its groups mapping never existed.docs/app-yaml-reference.md: IdP list, URL rules andextra_hosts, the/logoutexact-match rule, group denial (error_description),COGNITO_IDENTITY_PROVIDER, auth-status and the doctor.docs/lambda-functions.md,docs/reusable-modules.mdandCLAUDE.mdare updated.How verified
python3 -m unittest discover -s terraform/lambda-src/testspasses (111, including fix(identity): lock down the internal Cognito pool #166's), and-s scripts/testspasses (67, including the fmt check of generated TF). New tests cover:cognito-client-*;client_id_changed_atcarry-over, fragments rejected;email_verified=true.terraform fmt -recursive -checkandterraform validate(platform root) pass.cupcake-cupcakeclient: callback → GoogleSAML ✔,/nope→redirect_mismatch✔, logout exact ✔, logout without trailing slash → hosted UI/login(this is why the logout check looks for "redirects back").GoogleSAML_*external-provider users, no native users./javabin/apps/*has only cupcake'sauthandcognito-client-id.Rollout & user actions
Merging (after #166) changes nothing about Google sign-in:
["GoogleSAML"], which equals cupcake's live client (SupportedIdentityProviders=[GoogleSAML]). The post-apply reconcile therefore does not put COGNITO back; on main without fix(identity): lock down the internal Cognito pool #166, the old 03:30 reconcile would.cognito-client-identity-provider = GoogleSAML(Create, so no restart) and anauth-status.Expected plan: the
aws_ssm_parameters_by_pathread, 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:
terraform/org(identity-center.tf). It addsauth-statusto thejavabin-developerpermission set's platform-managed deny. Per-team permission sets get it from team-provisioner on its next run.docs/cognito-google-setup.md§1–2. Then re-run platform CI on main. The plan creates theGoogleIdP, and clients are untouched.prompt=select_account. If it doesn't, use the documented managed-login fallback.internal_identity_providers = ["Google"]. fix(identity): lock down the internal Cognito pool #166 (server-side domain check, no self sign-up) is a hard prerequisite, which stacking on it guarantees.sub. Platform memberships and groups are keyed by email, so nothing to migrate there; cupcake should check forsub-keyed data.GoogleSAML_*users and theGoogleSAMLIdP a week later, and turn off the Google Admin SAML app.Interactions
identity_providers(pool, upid). It adds to fix(identity): lock down the internal Cognito pool #166's code: the Google precondition oninternal_identity_providers,EMAIL_VERIFIED_IDENTITY_PROVIDERSin auth-claims, and a second pattern on the skipped-sync metric filter. If fix(identity): lock down the internal Cognito pool #166 is squash-merged, I rebase this branch onto main with--onto.feat/alb-login: both changeapp_auth_sync(client_settings,sync,_put_param,reconcile) andgenerate-modules.pyauth handling. Expect textual conflicts; the slot threading composes with this.auth-statusandcognito-client-identity-providernext to the slot request (…/login-N/). The rollout regex already ignores slot params.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'scognito-client-identity-provider.app_secret_rollout/handler.py, a small conflict.Follow-ups
auth-status/cognito-client-*. It is at 5,946/6,144 bytes and feat(platform): app-to-app calls with private networking and scopes #156 adds to it, so I left it alone; the limitation is now documented indocs/lambda-functions.mdanddocs/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.ALLOW_USER_SRP_AUTHfrom internal clients. With fix(identity): lock down the internal Cognito pool #166 there is no COGNITO and no self sign-up, so it is unused there, but changing client auth flows is fix(identity): lock down the internal Cognito pool #166-adjacent scope.javabin auth doctorin javabin-cli, wrappingscripts/auth-doctor.py --json.auth.envalias; canary-client probe in platform CI after identity applies.auth.accept_clientsforclient: nonebackends (case 13).🤖 Generated with Claude Code