feat(mobile-infra): support development App Attest - #7744
Conversation
Signed-off-by: Tom Brow <tomb@block.xyz>
🔐 Codex Security Review
|
|
@codex review |
|
@builderbot review |
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. |
|
@buzz-security-review 360a8e7 |
|
Codex Review: Didn't find any major issues. 🎉 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". |
brow
changed the title
brow
changed the title
![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: 360a8e74b4
ℹ️ 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".
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES on exact head 360a8e74b49e3c7969fddb9e41f033753f9a4557 (base/merge-base 8953cbfff58ed768d996677fed3af0e3bac64a20).
Author-actionable defect
P1 — the opt-in feature’s production build/runtime seam is not regression-gated.
The normal gateway test paths run without personal-dev-app-attest (Justfile:401, scripts/run-tests.sh:118-119), and the public-image workflow builds only the default image without BUZZ_PUSH_CARGO_FEATURES (.github/workflows/docker.yml:495-557). CI therefore never compiles or executes the new Development variant, its appattest/testing dependency closure, or its feature-gated tests.
The only development-mode behavior test also bypasses the production wiring: it constructs a private production helper and mutates environment directly (crates/buzz-push-gateway/src/app_attest.rs:369-430). Replacing the production call at crates/buzz-push-gateway/src/main.rs:46-50 with AppAttestVerifier::new left the complete feature-enabled package suite green (51/51 non-ignored tests). A future break in config → constructor wiring—the seam that makes this PR work—would therefore ship green. This violates the repository’s explicit falsifiability requirement in TESTING.md and AGENTS.md.
Author action:
- Add a CI lane that runs the full gateway package with
--features personal-dev-app-attest; preferably also build the feature-enabled Docker target so the documented image path is gated. - Add a production-shaped regression seam that exercises parsed configuration through the environment-aware constructor/wiring and fails when the
main.rscall is reverted to the production-only constructor. Testing the private helper again is insufficient.
Verification owner: the author should provide failing-before/passing-after or equivalent mutation proof; the next review round will rerun the new exact-head feature lane and repeat the call-site-bypass mutation.
What was verified
No additional trust-chain defect was found. The implementation selects exactly one signed AAGUID before the dependency verifier (app_attest.rs:137-169); ordinary builds cannot represent Development and reject that config (config.rs:21-29,158-166); the special build still defaults to production. APNs environment selection remains server-owned and maps sandbox/production to the corresponding Apple origin (config.rs:169-179, apns.rs:94-99). Documentation correctly scopes this to personal development and does not introduce a UI contract.
On clean exact head with the pinned Hermit toolchain:
cargo test -p buzz-push-gateway: 50 passed, 11 ignored.cargo test -p buzz-push-gateway --features personal-dev-app-attest: 51 passed, 11 ignored.- Clippy
--all-targets -D warnings, default and feature: PASS. - Feature-enabled release binary build: PASS.
cargo fmt --all --checkandgit diff --check: PASS.- Environment-fence mutation testing correctly failed when the signed-AAGUID check was removed; the separate
main.rsconstructor-bypass mutation incorrectly survived, establishing the defect above. - Live GitHub checks were green and the PR was mergeable when reviewed; green default CI does not exercise the missing feature lane.
Confidence gaps (not additional author defects)
- No local Docker daemon was available, so the Docker ARG/cache path was not independently executed.
- The claimed personal Railway deployment, APNs sandbox TLS/HTTP2 probe, and physical-iPhone notification/deep-link journey were not independently observed. The PR reports those results; the release operator owns preserving or repeating them if required for rollout.
- The personal-only feature intentionally adds synthetic-attestation helpers plus OpenSSL/P-256 to that binary. The exact signed-AAGUID fence is consequently load-bearing; current mutation-sensitive coverage of that fence materially reduces, but does not erase, this residual attack-surface risk.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 8953cbfff58ed768d996677fed3af0e3bac64a20..360a8e74b49e3c7969fddb9e41f033753f9a4557 (exact head 360a8e74b49e3c7969fddb9e41f033753f9a4557)
Risk: high — this introduces an opt-in build/runtime trust boundary for development App Attest enrollment and sandbox push delivery.
Blocking finding
The PR's sole new production path can break while every required gate remains green.
The development feature is absent from standard gateway and Docker CI, so all #[cfg(feature = "personal-dev-app-attest")] code and tests are skipped by required automation (crates/buzz-push-gateway/Cargo.toml:18-20, Justfile:401, scripts/run-tests.sh:118-119, .github/workflows/docker.yml:547-557). Separately, development_mode_preserves_verification_and_rejects_production constructs a private test helper and mutates its environment field (crates/buzz-push-gateway/src/app_attest.rs:369-430); it does not exercise config → executable wiring or AppAttestVerifier::with_environment.
This is not theoretical: replacing the production environment-aware construction at crates/buzz-push-gateway/src/main.rs:46-50 with production-only AppAttestVerifier::new still left the complete feature-enabled package suite green (51 non-ignored tests passed). A configured development gateway would then reject every development enrollment while the regression suite certified it.
Author action:
- Add a required CI lane that runs the complete gateway package with
--features personal-dev-app-attest; preferably also build the feature-enabled Docker target soBUZZ_PUSH_CARGO_FEATURESand its dependency/runtime closure are covered. - Add a production-shaped regression seam that binds parsed development configuration to construction of the environment-aware verifier and fails if the executable path falls back to
AppAttestVerifier::new. Mutation-prove that seam; testing the private helper again is insufficient.
Verification owner: author supplies failing-before/passing-after or equivalent mutation proof; reviewer reruns the new exact-head feature lane and call-site mutation.
Contracts traced
No additional blocking defect was found in the trust-chain implementation. Ordinary builds cannot represent Development and reject that setting; the special build still defaults to production (config.rs:21-29,158-166). The wrapper selects exactly one signed AAGUID before the dependency verifies certificate chain, nonce, app ID, zero attestation counter, public-key hash, AAGUID, and credential ID (app_attest.rs:68-90,137-169). APNs environment selection remains independently server-controlled (config.rs:169-179, apns.rs:94-99). Documentation accurately distinguishes a personal stack from distributed dogfood validation.
Exact-head validation
At clean 360a8e74b49e3c7969fddb9e41f033753f9a4557 with the pinned Hermit toolchain:
cargo test -p buzz-push-gateway— 50 passed, 11 ignored, 0 failed.cargo test -p buzz-push-gateway --features personal-dev-app-attest— 51 passed, 11 ignored, 0 failed.cargo clippy -p buzz-push-gateway --all-targets -- -D warnings— passed.cargo clippy -p buzz-push-gateway --all-targets --features personal-dev-app-attest -- -D warnings— passed.cargo fmt --all -- --checkandgit diff --check— passed.- AAGUID-fence mutation (development mode expecting production AAGUID) — targeted feature test failed with rc 101, then the tree was restored clean.
- Production-constructor bypass mutation — feature package suite incorrectly remained green, establishing the blocker.
- Exact-head repository CI and ordinary multi-arch push-gateway image builds are green.
Confidence gaps
- The feature-enabled Docker build was not independently run because the local Docker daemon was unavailable. Author action: none beyond the CI coverage requested above. Verification owner: CI/reviewer tooling.
- The reported Railway deployment and physical iPhone/APNs journey were not independently observed. Author action: none. Verification owner: rollout/release operator if independent workflow evidence is required.
- Enabling
appattest/testingadds synthetic-attestation/OpenSSL/P-256 code to the personal-development binary. The explicit personal-only build boundary makes this residual attack surface a documented tradeoff, not a separate defect.
🧹 workspace cleaned: disposable PR checkout and lane artifacts removed.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
Reviewed head 360a8e74b49e3c7969fddb9e41f033753f9a4557 against base 8953cbfff58ed768d996677fed3af0e3bac64a20.
P1: Bind the environment fence to the authData actually verified
Anchor: crates/buzz-push-gateway/src/app_attest.rs:147–155.
With personal-dev-app-attest enabled, an otherwise valid development attestation can pass a production-configured verifier, and the reverse also works. The precheck and appattest 0.1.1 can select different authData slices from the same CBOR buffer. This defeats the new exact-environment guarantee; ordinary feature-disabled binaries still reject development attestations.
Source-traced reproduction: take a valid development attestation for the configured app ID and enrollment transcript. Keep its signed authData and attestation statement unchanged. Encode this ordered, definite five-entry root map, using an indefinite map only for the outer attStmt value:
map(5):
"fmt": "apple-appattest"
"authData": <53-byte decoy with production AAGUID at bytes 37..53>
"attStmt": indefinite-map:
"attStmt": <original definite statement containing x5c and receipt>
"authData": <original signed development authData>
break
"padding1": 0
"padding2": 0
The wrapper skips the whole indefinite attStmt, validates only the outer decoy, and reaches EOF. However, the locked dependency’s attestation.rs:202–206 converts that indefinite length to zero with unwrap_or(0) without consuming its contents. Its fourth and fifth root iterations therefore read the two inner entries as root entries, replacing authData with the signed development bytes and loading the original certificates/receipt. from_cbor returns without checking EOF (:175–283). Full verification validates that inner material, and testing accepts its development AAGUID (authenticator.rs:61–75). No signature, app-ID, nonce, or root forgery is required. Swapping the environments produces the symmetric bypass.
Please ensure the environment check and cryptographic verifier use the same parsed authData, or reject the parser-confusing container shapes before calling the dependency. Add both adversarial cross-environment cases to the feature-enabled verifier tests while preserving valid production/development and feature-disabled rejection coverage. Checking only duplicate root authData, indefinite root maps, and wrapper EOF does not catch this construction.
Other review comments: Mongo found no additional concrete deployment defect. The existing Codex comments identify useful coverage improvements: run the opt-in feature in CI and cover configuration/constructor wiring. I am not treating those as two additional demonstrated runtime defects. The required regression for this blocker must execute the feature-enabled production verifier; retain the default-feature package tests as well. Environment switching against shared development/production authority is outside the documented isolated-stack contract, not an additional requested migration feature.
Validation: source/metadata only on Wes’s laptop; no checkout, build, test, or PR/dependency-code execution. The dependency sources were compared byte-for-byte with cached archives whose SHA-256 values match this head’s Cargo.lock (appattest 0.1.1 and minicbor 0.25.1). Mordecai independently confirmed the parser trace. Existing green CI and the author-reported physical-device happy path do not exercise this malformed envelope.
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
…-dev * origin/main: fix(desktop): bound startup request bursts and recover quota refusals (#7790) fix(audit): frame hash inputs with TLV (#7492) fix(admin): allow cold storage worker DB startup (#7770) feat(relay): add admin HTTP routes for member restriction management (#7302) fix(relay): fire kick live side effects at convergence; persist target; fence re-add race with held lock (#7298) feat(relay): add atomic complete read-state snapshots (#7572) fix(desktop): register macOS badges for new and existing installs (#7783) fix(mobile): avoid opening empty threads on message tap (#7756) fix(workflows): make deletion persistent and retryable (#7735) fix(mobile): preserve thread replies through refresh failures (#7757) fix(mobile): keep iOS message menu actions responsive after rebuilds (#7758) fix(relay): exclude ephemeral activity from message quota (#7736) release: push gateway chart 0.3.1 (#7749) fix(push): label plaintext push gateway service as HTTP (#7717) Replace personal and internal data in desktop test fixtures (#7748) Add mobile VISION (#7710) fix(mobile): keep relay sessions stable during push lease updates (#7745) Signed-off-by: Tom Brow <tomb@block.xyz>
|
🤖 Addressed the three automated review findings in this update:
Local validation: default gateway suite 51 passed; feature-enabled suite 53 passed; 11 infrastructure tests ignored in each. Both gateway Clippy configurations, formatting, and workflow syntax checks passed. The full repository gate is running. Docker is unavailable locally, so the new CI image check owns that validation. No Railway changes or new physical-device validation were performed. |
blockers resolved
|
@codex 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". |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE
Reviewed exact head 66d9340124cad95329b6f39b02172aeebae768cf against base c213d90d5c450e2e579505da4a4145f528ecbcf2.
The prior production-seam blocker is resolved. The parsed App Attest environment now reaches the verifier constructed by the real gateway executable, and the black-box startup regression is mutation-sensitive: reverting that call site to the production-only constructor fails the feature-enabled package suite with Production vs Development. Ordinary builds still cannot represent development mode, while feature builds retain a production default.
The trust boundaries remain fail closed: the wrapper accepts only the closed attestation envelope shape, binds the signed AAGUID to the selected environment before dependency verification, rejects cross-environment attestations in both directions, and keeps APNs sandbox/production selection independently server-controlled.
Required coverage now exercises the feature path. just test-unit and its shell fallback run the complete gateway package with and without personal-dev-app-attest; the Dockerfile applies the same feature argument to dependency cooking and the locked release build. The dedicated PR job built, loaded, and started the documented personal-development image successfully. Public-image jobs pass no feature argument, preserving production isolation. Documentation matches the actual environment, image, signing, APNs sandbox, and physical-device workflow.
Exact-head evidence:
- Default gateway package: 51 passed, 11 ignored.
- Feature-enabled gateway package: 53 passed, 11 ignored.
- Feature-enabled clippy (
--all-targets -D warnings): passed. - Feature-enabled locked release build: passed.
- Production-constructor bypass mutation: failed causally with rc 101; restored control passed.
git diff --check: passed.- GitHub
Rust / Unit Tests: passed. - GitHub
Build personal development push gateway: passed and exercised the loaded runtime image.
No author-actionable defects remain.
Residual risk: appattest/testing expands only the opt-in personal-image attack surface. The environment fence, production-shaped startup test, feature package lane, and real feature-image build are independent guards. The reported Railway/APNs/physical-iPhone journey was not independently repeated; the rollout/release owner retains that verification if required.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
REVIEW CLEAR: the prior environment-fence blocker is resolved. No actionable code, product, or security defect found in the corrective changes at 66d9340124cad95329b6f39b02172aeebae768cf against base c213d90d5c450e2e579505da4a4145f528ecbcf2. This is a non-approving review comment.
- Parser repair closes the demonstrated bypass. The wrapper now requires definite, unique-field root/statement maps, a bounded definite certificate array, and end-of-input before accepting the selected AAGUID (
crates/buzz-push-gateway/src/app_attest.rs:142–214). Comparison with checksum-verified, lockedappattest 0.1.1source confirms both parsers select the same signedauthDatafor every admitted envelope. The regression at lines 476–569 constructs the prior nested-statement attack in both directions, verifies its signed material through the dependency, and rejects it through the production wrapper. Valid production/development and cryptographic negative cases remain covered. - The actual startup and enrollment path preserves the contract.
main.rs:41–55passes the parsed environment to the verifier, logs that constructed verifier’s environment, and installs that same object inProfileRuntime; enrollment verifies before authority writes.tests/startup_app_attest.rslaunches the real executable, so hardcoding production breaks the development case. Default builds reject development; feature builds still default to production. APNs environment selection remains independent. The documented isolated personal stack is the supported boundary, not shared-stack migration. - Validation evidence and limits. Existing exact-head Rust / Unit Tests is successful; its
just test-unitpath includes both complete gateway feature configurations. The personal-development Docker job is successful and smoke-checks the feature binary in the runtime image; public-image builds also succeeded. This review was source-only, independently cross-checked with parser and deployment reviewers: no checkout, build, test, device enrollment, or CI rerun. The Docker smoke is not full enrollment validation. At the CI snapshot, Desktop Smoke E2E (2) was still running, and the separate security-review bot still marked the current range review-required. Neither is represented here as completed or waived.
3 participants
Development-signed iOS apps cannot enroll with the current gateway because their Apple App Attest AAGUID is rejected.
Add an explicit personal development build feature and environment setting so a development gateway can use a developer's own application identity and sandbox APNs credentials.
Ordinary gateway builds remain production-only. Special builds also default to production, enforce exactly the selected signed AAGUID, and retain certificate-chain, pinned-root, nonce, app-ID, key, credential-ID, and counter verification.
Document the matching mobile signing configuration and expose the feature through an optional Docker build argument.
Validation
The development image deployed successfully to a personal Railway stack; private readiness, enrollment challenges, authenticated relay access, and APNs sandbox TLS/HTTP2 connectivity passed.
End-to-end validation on a physical iPhone 12 mini exercised this gateway’s development App Attest configuration: device enrollment, relay delegation, and a fresh push lease succeeded; a desktop mention produced a sandbox APNs notification containing the actual message text, and tapping it opened the correct community/channel and highlighted the exact message. Production-default behavior was covered by automated tests.