Add Apple App Attest attestation for mobile clients - #4055
Conversation
Dependency Validation ResultsDependency name: github.com/fxamacker/cbor/v2 Next Steps
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (29)
🚧 Files skipped from review as they are similar to previous changes (21)
📝 WalkthroughWalkthroughAdds Apple App Attest support alongside Android Play Integrity across API schemas, backend verification and flow enforcement, console configuration, integration tests, and documentation. Applications now require exactly one attestation platform. ChangesPlatform attestation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MobileClient
participant FlowExec
participant CompositeVerifier
participant AppAttestVerifier
MobileClient->>FlowExec: POST /flow/execute with Attestation-Token
FlowExec->>CompositeVerifier: Verify configured attestation
CompositeVerifier->>AppAttestVerifier: Verify Apple App Attest token
AppAttestVerifier-->>CompositeVerifier: Verification result
CompositeVerifier-->>FlowExec: Verification result
FlowExec-->>MobileClient: Accept or reject flow initiation
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/application.yaml`:
- Around line 1375-1380: Update the attestation schema object around the android
and apple properties to require exactly one platform using mutually exclusive
oneOf branches, each requiring its corresponding property. Ensure
AndroidAttestation and AppleAttestation enforce all required platform
identifiers so empty objects such as apple: {} are rejected, while preserving
the existing platform references.
In `@backend/internal/attestation/appattest.go`:
- Around line 50-58: Extend the App Attest verification flow to require a
server-issued, per-request challenge and validate the attestation’s nonce
extension (OID 1.2.840.113635.100.8.2) against it before accepting the
attestation as authorization. Update the verifier and its callers to issue,
bind, consume, and reject missing, mismatched, or replayed challenges, removing
the current limitation described in the verification comment.
In `@backend/internal/attestation/composite.go`:
- Around line 29-60: Add unit tests for newCompositeVerifier and
compositeVerifier.Verify covering Android dispatch, Apple dispatch, and missing
or nil platform configuration returning false with
tidcommon.InternalServerError. Use mock providers to verify the selected
provider receives the request while the other is not called, and target at least
80% coverage of the dispatcher logic.
In `@docs/content/guides/guides/applications/application-settings.mdx`:
- Line 135: Clarify the attestation configuration wording in
docs/content/guides/guides/applications/application-settings.mdx:135-135 and
docs/content/guides/key-concepts/authentication/integration-models.mdx:87-87 so
attestation remains optional, while exactly one platform is required when
attestation is enabled; keep both descriptions consistent.
In `@docs/content/guides/key-concepts/authentication/integration-models.mdx`:
- Line 102: Update the authentication response description in the
integration-models guide to say that invalid or malformed tokens receive 401
Unauthorized, rather than claiming every verification failure does. Preserve the
existing 401 behavior for omitted tokens and the continuation-request exception,
while leaving verifier outage behavior accurately distinguishable.
In
`@frontend/apps/console/src/features/applications/components/edit-application/advanced-settings/AttestationSection.tsx`:
- Around line 191-195: Update every newly added i18n call in the
AttestationSection, including the platformOptions labels and all changed labels,
placeholders, hints, and actions through the referenced ranges, to provide an
appropriate fallback default string using the surrounding translation-call
convention. Do not alter the translation keys or unrelated existing calls.
In `@frontend/apps/console/src/features/applications/models/oauth.ts`:
- Around line 479-488: Enforce complete Apple attestation identity across all
listed sites: in oauth.ts, make AppleAttestationConfig.teamId and bundleId
required; in AttestationSection.tsx sites 165-176, emit Apple configuration only
when both values are populated, at 212-220 mark either missing value as a
validation error, and at 352-382 display both required-field errors and
propagate validation to the page Save state; in AttestationSection.test.tsx,
replace the Team ID-only expectation with incomplete-input and
complete-configuration cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 49971c9b-2b0e-46bf-a235-1664c1c81651
📒 Files selected for processing (24)
api/application.yamlbackend/go.modbackend/internal/application/error_constants.gobackend/internal/application/service.gobackend/internal/application/service_test.gobackend/internal/attestation/appattest.gobackend/internal/attestation/appattest_constants.gobackend/internal/attestation/appattest_test.gobackend/internal/attestation/composite.gobackend/internal/attestation/error_constants.gobackend/internal/attestation/init.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_test.gobackend/internal/system/i18n/core/defaults.gobackend/pkg/thunderidengine/providers/model.gobackend/pkg/thunderidengine/providers/model_test.godocs/content/guides/guides/applications/application-settings.mdxdocs/content/guides/key-concepts/authentication/integration-models.mdxfrontend/apps/console/src/features/applications/components/edit-application/advanced-settings/AttestationSection.tsxfrontend/apps/console/src/features/applications/components/edit-application/advanced-settings/__tests__/AttestationSection.roundtrip.test.tsxfrontend/apps/console/src/features/applications/components/edit-application/advanced-settings/__tests__/AttestationSection.test.tsxfrontend/apps/console/src/features/applications/models/oauth.tsfrontend/packages/i18n/src/locales/en-US.tstests/integration/flow/authentication/apple_attestation_flow_test.go
| // appleAppAttestRootPEM is Apple's public "Apple App Attestation Root CA" certificate, published at | ||
| // https://www.apple.com/certificateauthority/Apple_App_Attestation_Root_CA.pem. It is a public trust | ||
| // anchor, not a secret. | ||
| const appleAppAttestRootPEM = `-----BEGIN CERTIFICATE----- |
There was a problem hiding this comment.
We can't keep this hard coded in source files right?
cc: @rajithacharith
There was a problem hiding this comment.
This can be moved to get from the deployment or unless we have download it from the above url. This current cert is valid until 2045.
There was a problem hiding this comment.
Let's have a config for this. We can keep the certificate in default.json.
Let's do this in a follow up PR.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
ebbcca0 to
9aa5702
Compare
Dependency Validation ResultsDependency name: github.com/fxamacker/cbor/v2 Next Steps
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/internal/attestation/app_attest.go (1)
42-47: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftReject replayable App Attest objects.
As noted in the comment, omitting the nonce validation (OID 1.2.840.113635.100.8.2) allows a captured attestation object to be replayed indefinitely to bypass the attestation gate for the same app and device. A server-issued, challenge-bound nonce validation must be implemented before treating App Attest as a reliable authorization signal.
Do you want me to help generate a server-issued challenge mechanism and the corresponding validation logic?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/attestation/app_attest.go` around lines 42 - 47, Implement a server-issued challenge mechanism for the App Attest verification flow in the attestation verifier, and validate the nonce extension OID 1.2.840.113635.100.8.2 against the expected challenge before accepting the attestation. Ensure challenge issuance, binding, expiration or one-time use, and validation occur before treating the attested app as authorized, removing the current replay limitation.
🧹 Nitpick comments (1)
backend/internal/attestation/app_attest.go (1)
78-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant nil checks.
Based on learnings, avoid adding redundant defensive nil-guards when the dependency cannot be nil given the call graph. The
compositeVerifier(which delegates to this method) already enforcescfg != nil && cfg.Apple != nilbefore calling thisVerifymethod.♻️ Proposed refactor
- if cfg == nil || cfg.Apple == nil { - v.logger.Error(ctx, "Attestation requested without an Apple attestation configuration") - return false, &tidcommon.InternalServerError - } -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/attestation/app_attest.go` around lines 78 - 81, Remove the redundant cfg == nil || cfg.Apple == nil guard from the Verify method in app_attest.go, including its associated error log and early return. Rely on compositeVerifier’s existing validation before delegating to Verify, while preserving the remaining attestation verification behavior.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/apps/console/src/features/applications/components/edit-application/advanced-settings/__tests__/AttestationSection.test.tsx`:
- Around line 126-137: Extend the incomplete Apple attestation test coverage
around AttestationSection to assert onValidationChange(true) while required
fields are incomplete, then assert it resets to false after completing the
fields or clearing the section. Preserve the existing emission-suppression
assertions and verify the validation callback transitions for both relevant
scenarios.
In `@frontend/apps/console/src/features/applications/models/oauth.ts`:
- Around line 439-451: Update AttestationConfig to an exclusive union of
Android-only and Apple-only configuration variants, requiring exactly one
platform and disallowing both an empty object and simultaneous android/apple
properties. Preserve the existing AndroidAttestationConfig and
AppleAttestationConfig types while making the mutually exclusive shape
enforceable at compile time.
---
Duplicate comments:
In `@backend/internal/attestation/app_attest.go`:
- Around line 42-47: Implement a server-issued challenge mechanism for the App
Attest verification flow in the attestation verifier, and validate the nonce
extension OID 1.2.840.113635.100.8.2 against the expected challenge before
accepting the attestation. Ensure challenge issuance, binding, expiration or
one-time use, and validation occur before treating the attested app as
authorized, removing the current replay limitation.
---
Nitpick comments:
In `@backend/internal/attestation/app_attest.go`:
- Around line 78-81: Remove the redundant cfg == nil || cfg.Apple == nil guard
from the Verify method in app_attest.go, including its associated error log and
early return. Rely on compositeVerifier’s existing validation before delegating
to Verify, while preserving the remaining attestation verification behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e292aa11-e84b-4f14-9e22-55b6031d2abe
📒 Files selected for processing (28)
api/application.yamlbackend/cmd/server/servicemanager.gobackend/go.modbackend/internal/application/error_constants.gobackend/internal/application/service.gobackend/internal/application/service_test.gobackend/internal/attestation/app_attest.gobackend/internal/attestation/app_attest_constants.gobackend/internal/attestation/app_attest_test.gobackend/internal/attestation/composite.gobackend/internal/attestation/composite_test.gobackend/internal/attestation/error_constants.gobackend/internal/attestation/init.gobackend/internal/attestation/model.gobackend/internal/flow/flowexec/service.gobackend/internal/flow/flowexec/service_test.gobackend/internal/system/i18n/core/defaults.gobackend/pkg/thunderidengine/engine.gobackend/pkg/thunderidengine/providers/model.gobackend/pkg/thunderidengine/providers/model_test.godocs/content/guides/guides/applications/application-settings.mdxdocs/content/guides/key-concepts/authentication/integration-models.mdxfrontend/apps/console/src/features/applications/components/edit-application/advanced-settings/AttestationSection.tsxfrontend/apps/console/src/features/applications/components/edit-application/advanced-settings/__tests__/AttestationSection.roundtrip.test.tsxfrontend/apps/console/src/features/applications/components/edit-application/advanced-settings/__tests__/AttestationSection.test.tsxfrontend/apps/console/src/features/applications/models/oauth.tsfrontend/packages/i18n/src/locales/en-US.tstests/integration/flow/authentication/apple_attestation_flow_test.go
🚧 Files skipped from review as they are similar to previous changes (17)
- backend/internal/system/i18n/core/defaults.go
- backend/internal/attestation/error_constants.go
- backend/internal/flow/flowexec/service.go
- backend/internal/attestation/composite.go
- backend/internal/application/service_test.go
- backend/pkg/thunderidengine/providers/model.go
- backend/internal/application/error_constants.go
- backend/internal/application/service.go
- docs/content/guides/key-concepts/authentication/integration-models.mdx
- frontend/packages/i18n/src/locales/en-US.ts
- frontend/apps/console/src/features/applications/components/edit-application/advanced-settings/tests/AttestationSection.roundtrip.test.tsx
- tests/integration/flow/authentication/apple_attestation_flow_test.go
- backend/internal/flow/flowexec/service_test.go
- api/application.yaml
- docs/content/guides/guides/applications/application-settings.mdx
- frontend/apps/console/src/features/applications/components/edit-application/advanced-settings/AttestationSection.tsx
- backend/go.mod
9aa5702 to
01fc304
Compare
Dependency Validation ResultsDependency name: github.com/fxamacker/cbor/v2 Next Steps
|
01fc304 to
ab67c2c
Compare
Dependency Validation ResultsDependency name: github.com/fxamacker/cbor/v2 Next Steps
|
Dependency Validation ResultsDependency name: github.com/fxamacker/cbor/v2 |
ab67c2c to
99e7469
Compare
Dependency Validation ResultsDependency name: github.com/fxamacker/cbor/v2 |
99e7469 to
441f76b
Compare
Dependency Validation ResultsDependency name: github.com/fxamacker/cbor/v2 |
Description
Adds Apple App Attest as a second platform attestation provider for mobile clients initiating flows directly, alongside the existing Google Play Integrity verifier.
Approach
appAttestVerifierimplements the existingattestation.VerifierInterface. Verification is fully offline: base64 + CBOR decode the attestation object, verify the certificate chain against Apple's public App Attestation Root CA, then check the App ID hash (SHA256(teamId.bundleId)), the App Attest AAGUID (production or development), a zero signature counter, and that the credential ID matches the credential certificate's public key.teamId,bundleId) carries no secret, so no credential encryption is involved. An application may configure only one platform; configuring both is rejected on create/update.Related Issues
Related PRs
Tests
Summary by CodeRabbit