Repository navigation
dolphin-maps: MusicKit key container (dolphin-maps/musickit), loaded into prod and stage Workers - #715
Conversation
- dolphin-maps-musickit.tf: the Secrets Manager container dolphin-maps/musickit (JSON: MUSICKIT_KEY_ID, MUSICKIT_TEAM_ID, MUSICKIT_PRIVATE_KEY), readable by administration only; dev-server-role may put and describe it, never read it. No value in Terraform. - scripts/workers-prod-secrets.sh and workers-stage-secrets.sh: for dolphin-maps, read it with the caller's own credentials (this account) and merge its three names into dolphin-maps-prod / -stage. No value yet: load without it. Any other read failure, a wrong set of names, a non-PEM key or a name in two containers refuses the load before Wrangler runs. - Tests for both loaders (fake aws gains the container); AGENTS.md entry.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 54 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 13fb6bf417
ℹ️ 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".
dev-server-role is at IAM's 20 managed-policy attachments (Codex on #715): the PutSecretValue/DescribeSecret statement on dolphin-maps/musickit moves into dev-dolphin-maps-pems's document instead of a policy and attachment of its own. The policy's description is unchanged (a change would replace it).
|
@codex review |
|
@codex security review |
|
@coderabbitai review |
|
@greptileai review |
|
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
@codex security review |
|
@coderabbitai review |
|
@greptileai review |
|
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Ready at ab29e0c. Two independent HIGH code/security reviews pass, all 63 focused ARM tests pass, both required CI workflows are green, and exact-head Codex code/security reviews are clean. The prior managed-policy-attachment P1 is resolved by reusing the existing PeMS policy; the corrected per-SID regression verifies MusicKit write-only access and rejects a read grant. No unresolved review threads. CodeRabbit is quota-limited and Greptile did not respond; neither is counted as approval. Read-only live metadata confirms the MusicKit container is absent. Terraform can safely create it empty and never holds its payload. Out-of-band population and Worker loads require the correct existing MusicKit app key, which is being verified separately; no unverified key will be used. Private key material remains outside Terraform, output, and Git. |
|
Applied at merged main 156cf71. The reviewed full plan applied successfully (2 added, 6 changed, 0 destroyed), and the full 2,526-resource follow-up returned exit 0 / no changes. Post-apply verification passed: the MusicKit container exists with zero secret versions, its read-deny policy matches the reviewed administration boundary, the shared PeMS policy has exactly the three scoped statements, and dev-server-role remains at 20 attachments. Simulations allow only the intended MusicKit Put/Describe grant; an actual read attempt from the running ARM dev role returned AccessDenied. Runtime bootstrap remains pending the correct MusicKit app key. Neither Worker currently has MusicKit bindings. A key file found on the Mac could not be verified as MusicKit, so it was not read or used; no payload was stored and neither Worker loader was run. Apple Music activation is not claimed. The owner has been asked for the machine/path of the correct .p8 key. |
dolphin-labs-hq/dolphin-maps is adding Apple Music (its
docs/design/music.md, stage 7). The car screen plays it through MusicKit JS, which needs a developer token: an ES256 JWT signed with a MusicKit private key. The site's Worker mints short-lived ones at/api/music/apple/token, so the Worker needs the key. This PR adds a place to keep it and loads it into both Workers.What changes
dolphin-maps-musickit.tf: a Secrets Manager container,dolphin-maps/musickit(us-west-1). Its JSON holdsMUSICKIT_KEY_ID,MUSICKIT_TEAM_IDandMUSICKIT_PRIVATE_KEY(the PEM text of the .p8 file).GetSecretValueto anyone outsidegames_mp_admin_principals.PutSecretValueandDescribeSecreton this one secret, and noGetSecretValue, as a statement in the existingdev-dolphin-maps-pemspolicy (the role is at IAM's 20 managed-policy attachments). The key was made from a dev box's session, so the box can store it but never read it back.scripts/workers-prod-secrets.shandscripts/workers-stage-secrets.sh: fordolphin-maps, the loader also reads this container and merges its three names.ResourceNotFoundException), the loader says so and loads without it. The site then keeps Apple Music off on the car screen.scripts/test-dolphin-maps-prod.pyandscripts/test-workers-stage-secrets.py.awsnow answers for the container.AGENTS.md: an entry next to the PeMS login.One key for prod and stage. The owner chose this. A developer token identifies the app to Apple, not a person, and carries no one's data. To rotate it, make a second MusicKit key for the same Media ID (Apple allows two), put it here, load it, then revoke the first.
After this is applied
jq … | aws secretsmanager put-secret-value … file:///dev/stdincommand in the.tfheader.scripts/workers-stage-secrets.sh dolphin-mapsandscripts/workers-prod-secrets.sh dolphin-maps.Tests run:
python3 scripts/test-dolphin-maps-prod.py(24 OK),python3 scripts/test-workers-stage-secrets.py(22 OK), and every otherscripts/test-*.py. Two of those need root or a Terraform init and fail the same way on main.terraform fmt -checkpasses.