feat: introduce turnkey migration engine and apple audiences migration - #392
Conversation
![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: def43f3698
ℹ️ 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".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Proceeding with the multi-audience approach for iOS requires:
|
There was a problem hiding this comment.
Pull request overview
Introduces a new Turnkey migration engine in Bedrock (exposed via UniFFI) and implements the first migration to reconcile Apple OIDC audiences on auth_user_main, using host-held P-256 keys for request stamping so private key material never crosses the FFI boundary.
Changes:
- Add a foreign
KeypairSignertrait and integrate it into Turnkey request stamping (KeypairSignerStamper). - Introduce
TurnkeyManager::run_migrationsplus an ordered migration framework withTurnkeyMigrationOutcome. - Implement the
apple_audiencemigration and environment-specific Turnkey policies; add unit tests and an ignored real-API integration test.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.lock | Adds turnkey_client / turnkey_api_key_stamper 0.12.0 lock entries. |
| bedrock/Cargo.toml | Pins Turnkey SDK + stamper deps to 0.12.0. |
| bedrock/src/primitives/signer.rs | Adds foreign KeypairSigner + error type for host-backed P-256 signing. |
| bedrock/src/primitives/mod.rs | Re-exports signer primitives and registers the module. |
| bedrock/src/lib.rs | Re-exports KeypairSigner types at crate root. |
| bedrock/src/backup/turnkey/mod.rs | Adds TurnkeyManager::run_migrations and wires in new migration modules. |
| bedrock/src/backup/turnkey/api.rs | Implements SDK wrapper with retry/jitter, per-run user caching, and stamping adapter. |
| bedrock/src/backup/turnkey/error.rs | Defines internal TurnkeyApiError and maps SDK errors for retry/logging. |
| bedrock/src/backup/turnkey/migrations/mod.rs | Adds migration framework + orchestration tests. |
| bedrock/src/backup/turnkey/migrations/apple_audience.rs | Implements Apple audience reconciliation planning + execution. |
| bedrock/src/backup/turnkey/policies.rs | Adds environment-specific parent org IDs + Apple audience policy lists. |
| bedrock/src/backup/turnkey/test.rs | Adds shared test signer + ignored integration test against real Turnkey. |
Comments suppressed due to low confidence (7)
bedrock/src/backup/turnkey/api.rs:149
- Docstring appears outdated: this caching note references a
check_migrationsrun, but the entrypoint introduced in this PR isTurnkeyManager::run_migrations.
/// `get_users` responses are cached for the lifetime of the client (a single
/// `check_migrations` run), keyed by sub-organization id.
pub struct TurnkeyApiClient {
bedrock/src/backup/turnkey/api.rs:195
- These retry logs currently include
err={error}. For HTTP status errors,TurnkeyApiError’s Display includes the full upstream response body, anderror.rsnotes Turnkey bodies may contain public keys or sub-organization IDs. Consider logging a safe summary (class/status) here to avoid leaking sensitive identifiers into warn-level logs.
Err(error) => {
attempt += 1;
if attempt >= self.retry.max_attempts || !is_retryable(&error) {
warn!("turnkey.request.failed op={operation} attempts={attempt} err={error}");
return Err(error);
bedrock/src/backup/turnkey/mod.rs:95
- This error log prints
err={error}. For HTTP status errors, the error’s Display includes the full upstream response body (whicherror.rsnotes may include public keys or sub-organization IDs). Consider logging a safe summary here to avoid leaking sensitive identifiers into error-level logs.
This issue also appears on line 115 of the same file.
bedrock/src/backup/turnkey/mod.rs:118
- This log prints
err={error}. For HTTP status errors, the error’s Display includes the full upstream response body (which may include sensitive identifiers). Consider logging a safe summary (class/status) here and relying on lower-level debug logging (with redaction) for full bodies if needed.
bedrock/src/backup/turnkey/migrations/mod.rs:130 - This failure log prints
err={error}. For HTTP status errors,TurnkeyApiError’s Display includes the full upstream response body (whichturnkey/error.rsnotes may include public keys or sub-organization IDs). Consider logging a safe summary here to reduce sensitive data exposure in error logs.
Err(error) => {
error!(
"turnkey.migration.failed migration={} err={error}",
migration.id()
);
return Err(error);
bedrock/src/backup/turnkey/api.rs:226
- Spelling/grammar in this warning comment makes it harder to read quickly in a high-risk module ("is is", "retry looks"). Suggest tightening the wording.
/// For any activities (i.e. requests that change the state) is is imperative that
/// the `timestamp_ms` is computed once outside any retry looks. Turnkey submissions
/// are idempotent on a fingerprint, maintaining the same `timestamp_ms` ensures a
/// request is not executed twice.
bedrock/src/backup/turnkey/api.rs:334
ntp_timestamp_ms()silently falls back to0on conversion failure. If the host NTP provider ever returns a pre-epoch timestamp (negative millis), this will turn into a constanttimestamp_ms=0, risking idempotency/fingerprint collisions for state-changing activities. Consider detecting negative millis and falling back to the device clock (and logging once) rather than returning 0.
/// Current NTP time in milliseconds, for Turnkey activity timestamps.
fn ntp_timestamp_ms() -> u128 {
u128::try_from(now_with_ntp().timestamp_millis()).unwrap_or(0)
}
|
@codex review |
|
cursor review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ced2c79106
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
bedrock/src/backup/turnkey/mod.rs:127
- Severity P2 (operational):
run_migrationslogs a success message atinfolevel on every invocation, and includes emoji characters. Since callers are expected to run this after every login and periodically, this will generate high-volume logs and emojis can break/complicate log parsing in some pipelines. Consider logging this atdebug(or only logging atinfowhen migrations are applied/deferred).
This issue also appears on line 129 of the same file.
bedrock/src/backup/turnkey/mod.rs:132
- Severity P2 (operational): The deferred-outcome log is currently emitted at
infolevel and includes emoji. Givenrun_migrationsis intended to run frequently, preferdebughere to avoid log spam, and keepinfofor cases where a migration is actually applied.
bedrock/src/backup/turnkey/error.rs:36 - Severity P1 (security): Same issue as above for 4xx/5xx variants—
Displayincludes{body}, so log lines that interpolate the error will emit response bodies. To reduce accidental data exposure, remove{body}from the#[error(...)]string (keep thebodyfield for diagnostics) and log it only when explicitly needed.
#[error("Turnkey server error: status {status}: {body}")]
ServerError {
/// The HTTP status code returned.
status: u16,
/// The upstream response body, for diagnostics.
bedrock/src/backup/turnkey/error.rs:30
- Severity P1 (security): These error variants include the upstream response body in their
Displayoutput via#[error(... {body})]. SinceTurnkeyApiErroris logged in retry/failure paths (e.g.,err={error}), this can leak sensitive identifiers (the comment below notes bodies may contain public keys or sub-organization IDs). Consider removing{body}from the displayed error string and logging the truncated body only in tightly-scoped debug logs when needed.
This issue also appears on line 32 of the same file.
#[error("Turnkey rate limited the request: {body}")]
RateLimited {
/// The upstream response body, for diagnostics.
body: String,
},
bedrock/src/backup/turnkey/migrations/apple_audience.rs:60
- Severity P2 (operational): This migration logs a skip reason at
infolevel even when no changes are needed. Since migrations are expected to run after every login/periodically, this can become high-volume noise; preferdebugfor the skip path and reserveinfofor applied/deferred/failed outcomes.
Plan::SkipNoAppleProvider | Plan::SkipReady => {
crate::info!("apple_audience skipped: {plan}");
Ok(MigrationOutcome::Skipped)
}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ced2c79. Configure here.
|
@codex review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c084097. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0840974b1
ℹ️ 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".
ketzusaka
left a comment
ketzusaka
left a comment
There was a problem hiding this comment.
Should have at least one other person review it but lgtm
| let unrecognized: Vec<&str> = existing.difference(&configured).copied().collect(); | ||
| if !unrecognized.is_empty() { | ||
| crate::warn!( | ||
| "auth_user_main has unrecognized Apple aduences: {}", |
| let providers = missing | ||
| .iter() | ||
| .map(|audience| OauthProviderParamsV2 { | ||
| provider_name: audience.provider_name.to_string(), | ||
| token_or_claims: Some(TokenOrClaims::OidcClaims(OidcClaims { | ||
| iss: APPLE_ISSUER.to_string(), | ||
| sub: subject.clone(), | ||
| aud: audience.client_id.to_string(), | ||
| })), | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
surprised to see this as I thought the upsert was atomically replacing all providers, therefore we'd need to build the provider list from the configured hashset. Answer might be below I will keep reading.
There was a problem hiding this comment.
looks like it's additive from the turnkey docs, but not super clear
There was a problem hiding this comment.
it is indeed additive, will add a comment clarifying in a small follow up PR
Changes
This PR introduces a migration engine to monitor a user's Turnkey account and perform any housekeeping operations to ensure the account is in an expected state. The primary entrypoint introduced is
TurnkeyManager::run_migrations, clients are expected to run this after every log in and periodically to ensure the user's Turnkey account is always properly configured.The system is designed so migrations are pure functions and logic can be unit tested. Separate tests cover API stamping and retry logic. As these are sensitive operations, having comprehensive tests to ensure the right actions are taken is imperative.
This PR also introduces the first migration: the apple audience migration which ensures the user can log in all clients that TFH supports.
Testing Flow
Aside from the unit tests, this flow is tested against the actual Turnkey infrastructure.
auth_user_mainafter the user's Turnkey account gets created and registered a new persistent keypair in the user with Turnkey's rust SDKNote
High Risk
The PR mutates live Turnkey auth configuration (OAuth providers) using privileged main-factor writes and new account-reconciliation logic on the backup/auth path; mistakes could break Sign in with Apple across clients.
Overview
Adds
TurnkeyManager::run_migrationsas the UniFFI entry point for reconciling a user’s Turnkey sub-org after login: resolves sub-org viawhoamiwhen needed, runs an ordered migration list under a single-flight lock and 180s timeout, and returns coarseTurnkeyMigrationOutcome/TurnkeyMigrationErrorwhile rich errors stay internal.Introduces a Turnkey SDK–based API layer (
turnkey_client0.12) withP256Signer/KeypairSignerstamping (sync vs main factor newtypes), bounded transport retries, per-runget_userscache, and NTP-backed activity timestamps fixed outside retry loops for idempotent writes.Ships the migration framework (fail-fast runner, defer writes when main factor is absent) and the first migration
apple_audience: ifauth_user_mainalready has Sign in with Apple, atomically create missing environment-pinned Apple OIDC audiences reusing the existingsub; no-op with no Apple provider; consistency errors on mismatchedsubs. Environment tables for parent org IDs and Appleaudvalues live inpolicies.rs.Adds unit and wiremock functional tests plus an ignored real-Turnkey integration test; bumps dev deps (
wiremock,tokiotest-util).Reviewed by Cursor Bugbot for commit fa51063. Bugbot is set up for automated code reviews on this repo. Configure here.