fix(webui-auth): P1/P2 review findings — Secrets Manager, domain uniqueness, Update handling - #83
Conversation
P1 fixes:
- Domain uniqueness: append AWS::AccountId to Cognito hosted-UI domain
prefix (was 'lowkey-${PackName}-${EnvironmentName}' — could collide
globally per region). Now 'lowkey-${PackName}-${EnvironmentName}-${AWS::AccountId}'.
- Rules assertion: reject stack create when EnableWebUIAuth=true but
WebUIAdminEmail is empty. Fails at validation, not mid-deploy.
P2 fixes:
- Password → Secrets Manager: replaces CFN output WebUIAdminPassword
with WebUIAdminSecretArn. New AWS::SecretsManager::Secret resource
holds {email, password}. Lambda writes via PutSecretValue, IAM policy
scoped to that specific secret. Aligns with 'ALL secrets in Secrets
Manager' workspace rule.
- Update event handling: Lambda now handles RequestType=Update. If
AdminEmail changed on update, old user is deleted first. Otherwise
password is regenerated and both Cognito + Secrets Manager are
refreshed.
- Existing user recovery: UsernameExistsException no longer silently
succeeds with '(password unchanged)'. Falls through to
admin_set_user_password so Secrets Manager stays authoritative.
- Log safety: no password in CloudWatch. Event print excludes
ResourceProperties; exception messages truncated to 180 chars.
P3 cleanup:
- Removed dead --webui-pool-id CLI flag (installer no longer uses
existing pools — CFN always creates one).
Installer:
- Post-deploy fetches password from Secrets Manager via GetSecretValue
instead of reading from CFN output.
- Display box shows Secret ARN so operator can retrieve later.
Validated: aws cloudformation validate-template passes (38 params,
CAPABILITY_NAMED_IAM). bash -n install.sh passes. Zero 'aws cognito-idp'
calls remaining in installer.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a99b71907
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Properties: | ||
| Name: !Sub '/lowkey/${EnvironmentName}/webui-admin' | ||
| Description: !Sub 'Initial WebUI admin credentials for ${EnvironmentName}' | ||
| SecretString: !Sub '{"email":"${WebUIAdminEmail}","password":"pending-lambda-write"}' |
There was a problem hiding this comment.
When an existing auth-enabled stack is upgraded from the parent commit, CloudFormation creates this secret with pending-lambda-write, but neither the Lambda code change nor the new secret changes any property of WebUIUserCreationResource, so the custom resource receives no Update event and never replaces the placeholder or resets the Cognito password. The installer consequently reports an unusable password for every such upgraded stack; add a version/property to the custom resource that forces provisioning during this migration.
Useful? React with 👍 / 👎.
| admin_password=$(aws secretsmanager get-secret-value --secret-id "$secret_arn" \ | ||
| --region "$DEPLOY_REGION" --query SecretString --output text 2>/dev/null \ | ||
| | jq -r '.password // empty') |
There was a problem hiding this comment.
When the deployment identity can operate CloudFormation but lacks direct secretsmanager:GetSecretValue permission, this command fails; under the script's set -euo pipefail, the unguarded assignment terminates the installer after the stack was successfully deployed and before wait_for_bootstrap or show_complete runs. The permission granted to the provisioning Lambda does not apply to the installer caller, so this lookup should degrade gracefully and display the secret ARN or retrieval instructions instead of aborting.
Useful? React with 👍 / 👎.
Two literal *** artifacts leaked into the Lambda ZipFile in PR #83: Line 1599: Password=*** → Password=password, Line 1643: password = ***) → password = generate_password() These were sanitized display masks (from a tool that redacts what looks like secrets when rendering diffs) that got baked into the on-disk file during a splice operation. The Lambda failed at import with Runtime.UserCodeSyntaxError: unmatched ')' so the WebUIUserCreationResource never completed. Impact: stack kirocrew-7-233-stack got stuck in CREATE_IN_PROGRESS because CFN kept re-invoking the broken Lambda. Stack was manually deleted; no data lost. Verification: - python3 ast.parse on extracted ZipFile: OK (111 lines) - aws cloudformation validate-template: OK (38 params) - bash -n install.sh: OK Co-authored-by: Roy Osherove <575051+royosherove@users.noreply.github.com>
1 participant
Follow-up to #82. Addresses my own code review of that PR.
P1 fixes
${AWS::AccountId}— was globally unique per region, would have collided across accounts/deploys with same env name.EnableWebUIAuth=truebutWebUIAdminEmailis empty (fails at validation, not mid-deploy).P2 fixes
WebUIAdminPasswordwithWebUIAdminSecretArn. NewAWS::SecretsManager::Secretresource holds{email, password}. Lambda writes viaPutSecretValue, IAM policy scoped to that specific secret ARN. Aligns with 'ALL secrets in Secrets Manager' workspace rule.RequestType=Update. If email changed, old user is deleted first. Otherwise password is regenerated; both Cognito + Secrets Manager stay in sync.UsernameExistsExceptionno longer silently succeeds. Falls through toadmin_set_user_passwordso Secrets Manager stays authoritative.P3 cleanup
--webui-pool-idCLI flag.Installer
aws secretsmanager get-secret-valueinstead of reading from CFN output. Display box shows secret ARN.Validation
bash -n install.sh✓aws cloudformation validate-template✓ (38 params, CAPABILITY_NAMED_IAM)aws cognito-idpcalls in installer ✓