Ios rc signature verification - #372
Conversation
lint lint
Verify the X-Airborne-Signature header on the release config before it is parsed or applied, matching the server-side signing added in e7b8044. The integrator supplies trusted public keys through a new publicKeys() delegate method, keyed by key ID the same way namespace() is supplied, and the SDK checks the ECDSA P-256 / SHA-256 signature over the exact raw response bytes. - AJPReleaseConfigVerifier (AirborneSwiftCore): SPKI PEM -> raw x963 point extraction that stays on the iOS 13 floor, X-Airborne-Signature parsing, and P256 verification. AJPReleaseConfigTrustStore parses keys eagerly so a malformed PEM surfaces at boot instead of silently disabling verification. - Wire publicKeys() through AirborneDelegate and AJPApplicationManagerDelegate. - Hook verification into fetchReleaseConfigWithCompletionHandler before the manifest is parsed, and reject non-2xx responses outright. A rejected config is never parsed, staged to temp, or applied; the SDK falls back to the currently installed bundle via the existing failure path. - Emit signature_* fields on release_config_fetch plus dedicated release_config_signature_missing / _verification_failed / signing_key_invalid events. - Docs: native verification in the iOS API reference and the signing guide (including an iOS 13 SecKey correction), and the new events in the callbacks reference. Backward compatible: verification is skipped when no keys are configured, and a response with no signature header is tolerated (so keys can ship before signing is enabled server-side), while a present-but-invalid signature is rejected. test
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
airborne_server/src/config.rs (1)
268-271: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider rejecting/clamping a zero
RC_SIGNATURE_CACHE_TTL.
rc_signature_cache_ttlflows straight intoset_ex(SET … EX <ttl>), and Redis errors onEX 0. An operator settingRC_SIGNATURE_CACHE_TTL=0to "disable" the cache would get an error log plus aCACHE_FAILSincrement on every signed release-config response rather than a disabled cache.♻️ Clamp to a minimum
- rc_signature_cache_ttl: parse_env( - "RC_SIGNATURE_CACHE_TTL", - DEFAULT_RC_SIGNATURE_CACHE_TTL_SECS, - ), + rc_signature_cache_ttl: parse_env( + "RC_SIGNATURE_CACHE_TTL", + DEFAULT_RC_SIGNATURE_CACHE_TTL_SECS, + ) + .max(1),🤖 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 `@airborne_server/src/config.rs` around lines 268 - 271, Update the RC_SIGNATURE_CACHE_TTL parsing in the config construction to clamp parsed values to a minimum of one second before assigning rc_signature_cache_ttl, preventing zero from reaching the Redis set_ex path. Preserve the existing default and positive TTL behavior.airborne_server/src/signing/utils.rs (1)
306-308: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit:
key.map(Some).ok_or_else(...)reads oddly.
key.map(Some)on anOption<T>to getOption<Option<T>>just took_or_elseit is harder to follow than the equivalent direct form.♻️ Simpler equivalent
- key.map(Some).ok_or_else(|| { - ABError::BadRequest("Unknown or disabled signing key in X-Signing-Key-Id".to_string()) - }) + match key { + Some(key) => Ok(Some(key)), + None => Err(ABError::BadRequest( + "Unknown or disabled signing key in X-Signing-Key-Id".to_string(), + )), + }🤖 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 `@airborne_server/src/signing/utils.rs` around lines 306 - 308, In the signing-key lookup flow, replace the nested-option construction using key.map(Some).ok_or_else with the direct Result conversion for the existing key Option, while preserving the same BadRequest error for missing or disabled keys.airborne_server/src/utils/redis.rs (1)
228-253: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift
index_dropis a three-step read-modify-delete; consider chunking the DEL and closing the window.Two things worth tightening on the invalidation path:
SMEMBERSreturns the whole set into memory and the members are deleted in a singleDEL. For a busy application (one entry perconfig_version× selector) that is one unbounded, blocking command against Redis.- A
sign_release_configrunning concurrently canSADDa member afterSMEMBERSand before the finalDELof the index, leaving a signature entry that this invalidation misses; it then survives until the next invalidation or the TTL.
SSCAN+ chunkedDEL, or a small Lua script that pops and deletes atomically, addresses both.🤖 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 `@airborne_server/src/utils/redis.rs` around lines 228 - 253, Update RedisClient::index_drop to avoid loading the entire index set and to close the concurrent SADD race: replace the SMEMBERS/read-then-delete sequence with an atomic Lua-based pop-and-delete operation, or use SSCAN with bounded deletion chunks plus an atomic final cleanup that cannot miss members added during invalidation. Preserve the existing error metrics, logging, and ABError behavior for Redis failures.airborne_server/migrations/20260714120000_add_signing_keys/up.sql (1)
13-13: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant indexes alongside the
UNIQUEconstraint.
signing_keys_name_idx(org_id, app_id, name) duplicates the index Postgres already builds to enforceUNIQUE (org_id, app_id, name)(Line 13).signing_keys_org_app_idx(org_id, app_id) is a leftmost prefix of that same index, so it adds no additional query coverage either. Both add write overhead with no query-planning benefit.♻️ Proposed cleanup
-CREATE INDEX signing_keys_org_app_idx - ON hyperotaserver.signing_keys (org_id, app_id); - -CREATE INDEX signing_keys_name_idx - ON hyperotaserver.signing_keys (org_id, app_id, name); +-- The UNIQUE (org_id, app_id, name) constraint already provides a covering +-- index for lookups on (org_id, app_id) and (org_id, app_id, name).Also applies to: 21-29
🤖 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 `@airborne_server/migrations/20260714120000_add_signing_keys/up.sql` at line 13, Remove the redundant signing_keys_name_idx and signing_keys_org_app_idx index definitions from the migration, retaining the UNIQUE (org_id, app_id, name) constraint so PostgreSQL supplies the necessary index. Leave any unrelated indexes and migration behavior unchanged.
🤖 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
`@airborne_dashboard/app/dashboard/`[orgId]/[appId]/settings/integrity/page.tsx:
- Around line 16-34: The authorization guard in IntegrityPage must run during
render rather than inside useEffect. After permissions.isReady is true, call
notFound() when read_signing_keys is unavailable before rendering
SigningKeysCard; otherwise preserve the existing canRead, canCreate, and
canUpdate permission props.
In `@airborne_docs/docs/react-native-sdk/reference/callbacks-and-events.md`:
- Around line 92-100: The release_config_fetch payload example is missing the
documented signature_error field. Update the signature fields example near
signature_present, signature_verified, and signature_key_id to include
signature_error with the stable failure reason codes, while preserving the
existing success-case fields.
In `@airborne_docs/docs/server/configuration.md`:
- Line 60: Update the documented default for RC_SIGNATURE_CACHE_TTL in the
configuration table to match DEFAULT_RC_SIGNATURE_CACHE_TTL_SECS, preserving the
existing one-hour value and the surrounding cache-tuning descriptions.
In `@airborne_docs/openapi/airborne.openapi.json`:
- Line 2120: The ListSigningKeys documentation incorrectly claims the response
is never empty. Update the corresponding doc comment for ListSigningKeys in
signing.smithy to state that applications created before signing support may
have no keys until the signingkeys boot migration runs, then regenerate the
OpenAPI output.
In
`@airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManager.swift`:
- Around line 1236-1246: Update the signature-header handling around
AJPSignatureVerificationError.missingHeader to support configurable enforcement
when the trust store contains keys: allow the existing warning-and-nil path only
in an explicitly permissive rollout mode, and reject missing signatures in
strict mode. Add or reuse a delegate/configuration switch for this policy,
preserving compatibility while enabling integrators to require signatures.
In `@airborne_server/src/release.rs`:
- Around line 1474-1482: Update sign_release_config or the serve_release_handler
call site so Redis signature-cache read errors from
cache.get::<CachedSignature>(key).await are downgraded to the existing
unsigned-signing fallback instead of propagated via await?. Preserve successful
cached-signature and private-key decrypt/sign behavior, returning Ok(None) with
the existing “Serving unsigned” handling for cache failures.
In `@airborne_server/src/signing/utils.rs`:
- Around line 73-88: The ActiveSigningKey Redis cache can contain plaintext
private keys when no master encryption key is configured. Update the caching
flow around ActiveSigningKey and resolve_signing_key to skip Redis caching when
master_encryption_key is None, while preserving existing caching when encryption
is enabled; document the field only if the implementation still permits
plaintext caching.
- Around line 418-427: Add an alert-grade metric in the signing failure branch
of the release-config signing flow, alongside the existing error log and before
returning Ok(None), to count unsigned serves for applications configured for
signing. Preserve the current fail-open response behavior, and use the existing
metrics/instrumentation symbol or pattern rather than changing SDK verification
behavior.
- Around line 696-720: Add or verify a database migration declaring a unique
constraint/index on the organisation, application, and default-key condition
used by the has_default check in the signing-key provisioning flow. Ensure the
insert in the run_blocking block violates that constraint for concurrent
default-key creation, preserving the existing conflict-handling path.
- Around line 370-400: The cache lookup in sign_release_config must be scoped to
the exact body being signed. Include a digest or equivalent body-specific
context in signature_cache_key and use it consistently for lookup and storage,
or bypass caching when the body cannot be safely keyed; preserve cache reuse
only when organisation, application, signing selector, config_version, and body
bytes all match.
- Around line 553-575: Wrap the has_default existence check and the subsequent
NewSigningKey insert in the same conn.transaction closure within the signing-key
creation flow. Preserve the existing default assignment logic while ensuring
concurrent requests cannot both observe no default and insert competing default
keys, following the transaction pattern used by set_default_key.
In `@airborne_server/src/utils/moka.rs`:
- Around line 7-9: Update the decrypted signing-key cache path used by AppState
and MokaCache::new("airborne") to use a dedicated policy with a substantially
shorter TTL and idle timeout than DEFAULT_TTL and DEFAULT_TTI. Keep general
cache defaults unchanged, and ensure the key-specific cache configuration is
actually applied rather than only defining new constants.
---
Nitpick comments:
In `@airborne_server/migrations/20260714120000_add_signing_keys/up.sql`:
- Line 13: Remove the redundant signing_keys_name_idx and
signing_keys_org_app_idx index definitions from the migration, retaining the
UNIQUE (org_id, app_id, name) constraint so PostgreSQL supplies the necessary
index. Leave any unrelated indexes and migration behavior unchanged.
In `@airborne_server/src/config.rs`:
- Around line 268-271: Update the RC_SIGNATURE_CACHE_TTL parsing in the config
construction to clamp parsed values to a minimum of one second before assigning
rc_signature_cache_ttl, preventing zero from reaching the Redis set_ex path.
Preserve the existing default and positive TTL behavior.
In `@airborne_server/src/signing/utils.rs`:
- Around line 306-308: In the signing-key lookup flow, replace the nested-option
construction using key.map(Some).ok_or_else with the direct Result conversion
for the existing key Option, while preserving the same BadRequest error for
missing or disabled keys.
In `@airborne_server/src/utils/redis.rs`:
- Around line 228-253: Update RedisClient::index_drop to avoid loading the
entire index set and to close the concurrent SADD race: replace the
SMEMBERS/read-then-delete sequence with an atomic Lua-based pop-and-delete
operation, or use SSCAN with bounded deletion chunks plus an atomic final
cleanup that cannot miss members added during invalidation. Preserve the
existing error metrics, logging, and ABError behavior for Redis failures.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b25f03cc-9b25-41cd-85f5-b0fade8917d3
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockairborne_docs/static/docs_static/img/screenshots/dark/integrity-keys.pngis excluded by!**/*.pngairborne_docs/static/docs_static/img/screenshots/light/integrity-keys.pngis excluded by!**/*.png
📒 Files selected for processing (51)
airborne_dashboard/app/dashboard/[orgId]/[appId]/settings/integrity/page.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/settings/layout.tsxairborne_dashboard/app/dashboard/[orgId]/[appId]/settings/page.tsxairborne_dashboard/components/settings/integrity/create-key-dialog.tsxairborne_dashboard/components/settings/integrity/key-actions.tsxairborne_dashboard/components/settings/integrity/public-key-dialog.tsxairborne_dashboard/components/settings/integrity/signing-keys-card.tsxairborne_dashboard/components/settings/settings-tabs.tsxairborne_dashboard/components/shared-layout.tsxairborne_dashboard/lib/name-validation.tsairborne_dashboard/next.config.mjsairborne_dashboard/types/integrity.tsairborne_docs/docs/dashboard/integrity.mdxairborne_docs/docs/dashboard/overview.mdxairborne_docs/docs/guides/verify-the-release-config-signature.mdxairborne_docs/docs/react-native-sdk/reference/callbacks-and-events.mdairborne_docs/docs/react-native-sdk/reference/ios-api.mdairborne_docs/docs/server/configuration.mdairborne_docs/openapi/airborne.openapi.jsonairborne_docs/sidebars.tsairborne_sdk_iOS/hyper-ota/Airborne.xcodeproj/project.pbxprojairborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManager.swiftairborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManagerDelegate.swiftairborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/Airborne.swiftairborne_sdk_iOS/hyper-ota/Airborne/AirborneSwiftCore/AJPReleaseConfigVerifier.swiftairborne_sdk_iOS/hyper-ota/AirborneTestAppTests/AJPReleaseConfigLiveVerificationTests.swiftairborne_sdk_iOS/hyper-ota/AirborneTestAppTests/AJPReleaseConfigVerifierTests.swiftairborne_server/.env.exampleairborne_server/Cargo.tomlairborne_server/migrations/20260714120000_add_signing_keys/down.sqlairborne_server/migrations/20260714120000_add_signing_keys/up.sqlairborne_server/src/config.rsairborne_server/src/main.rsairborne_server/src/organisation/application.rsairborne_server/src/organisation/application/properties.rsairborne_server/src/release.rsairborne_server/src/release/utils.rsairborne_server/src/signing.rsairborne_server/src/signing/types.rsairborne_server/src/signing/utils.rsairborne_server/src/types.rsairborne_server/src/utils.rsairborne_server/src/utils/advisory_lock.rsairborne_server/src/utils/db/models.rsairborne_server/src/utils/db/schema.rsairborne_server/src/utils/moka.rsairborne_server/src/utils/redis.rssmithy/models/errors.smithysmithy/models/main.smithysmithy/models/release.smithysmithy/models/signing.smithy
| export default function IntegrityPage() { | ||
| const permissions = usePagePermissions(PAGE_AUTHZ); | ||
| const canRead = permissions.isReady && permissions.can("read_signing_keys"); | ||
|
|
||
| useEffect(() => { | ||
| if (permissions.isReady && !permissions.can("read_signing_keys")) { | ||
| notFound(); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [permissions.isReady, permissions.checks]); | ||
|
|
||
| return ( | ||
| <SigningKeysCard | ||
| canRead={canRead} | ||
| canCreate={permissions.can("create_signing_key")} | ||
| canUpdate={permissions.can("update_signing_key")} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does calling notFound() from next/navigation inside a React useEffect actually trigger the not-found UI in Next.js 15?
💡 Result:
No, calling notFound from next/navigation inside a React useEffect does not trigger the expected Next.js not-found UI [1][2]. The notFound function works by throwing a specific error (NEXT_NOT_FOUND) that must be caught by the React rendering process to terminate the current segment and render the not-found.js file [3][2]. Because useEffect runs after the component has already finished rendering, calling notFound inside it occurs outside of the rendering task [1][2]. Consequently, Next.js cannot catch the error in the way required to swap in the not-found UI, and the function will effectively fail to perform its intended navigation [1][2]. To correctly trigger the not-found UI, you should call notFound during the component's render phase [1][2]. If your data fetching is asynchronous or client-side, the recommended pattern is to store the result in the component's state and conditionally call notFound in the body of your component [1][2]: // Recommended pattern for client-side data export default function Page({ params }) { const [data, setData] = useState(null); const [loading, setLoading] = useState(true); useEffect( => { fetchData(params.id).then((result) => { setData(result); setLoading(false); }); }, [params.id]); if (!loading &&!data) { notFound; // Called during render } if (loading) return
Citations:
- 1: Next13 notFound() working case question vercel/next.js#49433
- 2: https://stackoverflow.com/questions/78870817/error-next-not-found-coming-when-moving-to-a-not-present-username-in-username
- 3: https://nextjs.org/docs/app/api-reference/functions/not-found
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the reported file and relevant auth state behavior without running repository code.
file="airborne_dashboard/app/dashboard/[orgId]/[appId]/settings/integrity/page.tsx"
if [ -f "$file" ]; then
wc -l "$file"
cat -n "$file"
else
echo "Reported file not found at $file"
fd -i 'page.tsx' airborne_dashboard/app/dashboard 2>/dev/null | head -50
fi
# Locate usePagePermissions implementation and related constants to see when isReady/checks stabilize.
rg -n "function usePagePermissions|const usePagePermissions|createSign|read_signing_keys|PAGE_AUTHZ|checks" airborne_dashboard -SRepository: juspay/airborne
Length of output: 11428
Call notFound() during the render phase, not from useEffect.
notFound() is ignored when called inside useEffect; an unauthorized user can finish the first render with canRead={false} and stay on the loading/empty SigningKeysCard UI instead of being routed to the 404 boundary. Move the guard into the render path after permissions.isReady is stabilized.
🤖 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 `@airborne_dashboard/app/dashboard/`[orgId]/[appId]/settings/integrity/page.tsx
around lines 16 - 34, The authorization guard in IntegrityPage must run during
render rather than inside useEffect. After permissions.isReady is true, call
notFound() when read_signing_keys is unavailable before rendering
SigningKeysCard; otherwise preserve the existing canRead, canCreate, and
canUpdate permission props.
| // Signature fields — present only when public keys are configured (iOS): | ||
| signature_present: true, // whether the response carried a signature header | ||
| signature_verified: true, // whether it verified against a trusted key | ||
| signature_key_id: "<key id>" // the key that verified it | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| When verification is configured but fails, `signature_verified` is `false` and `signature_error` carries a stable reason code (`signature_mismatch`, `untrusted_key_id`, `malformed_signature`, `unsupported_alg`, `malformed_header`, `no_trusted_keys`). The config is discarded and the app keeps its installed bundle. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
signature_error is described in prose but missing from the payload example.
Line 100 documents signature_error as part of the release_config_fetch value, yet the block at Lines 92-96 only lists signature_present, signature_verified, and signature_key_id. Readers taking the block as the authoritative shape will miss it.
📝 Proposed fix
// Signature fields — present only when public keys are configured (iOS):
signature_present: true, // whether the response carried a signature header
signature_verified: true, // whether it verified against a trusted key
- signature_key_id: "<key id>" // the key that verified it
+ signature_key_id: "<key id>", // the key that verified it
+ signature_error: "<reason>" // present only when signature_verified is false
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Signature fields — present only when public keys are configured (iOS): | |
| signature_present: true, // whether the response carried a signature header | |
| signature_verified: true, // whether it verified against a trusted key | |
| signature_key_id: "<key id>" // the key that verified it | |
| } | |
| } | |
| ``` | |
| When verification is configured but fails, `signature_verified` is `false` and `signature_error` carries a stable reason code (`signature_mismatch`, `untrusted_key_id`, `malformed_signature`, `unsupported_alg`, `malformed_header`, `no_trusted_keys`). The config is discarded and the app keeps its installed bundle. | |
| // Signature fields — present only when public keys are configured (iOS): | |
| signature_present: true, // whether the response carried a signature header | |
| signature_verified: true, // whether it verified against a trusted key | |
| signature_key_id: "<key id>", // the key that verified it | |
| signature_error: "<reason>" // present only when signature_verified is false | |
| } | |
| } |
🤖 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 `@airborne_docs/docs/react-native-sdk/reference/callbacks-and-events.md` around
lines 92 - 100, The release_config_fetch payload example is missing the
documented signature_error field. Update the signature fields example near
signature_present, signature_verified, and signature_key_id to include
signature_error with the stable failure reason codes, while preserving the
existing success-case fields.
| | Variable | Required | Default / Example | Purpose | | ||
| | --- | --- | --- | --- | | ||
| | `REDIS_URL` | No | `redis://localhost:6379` | Redis connection string. When set, the server enables PKCE/nonce protection on the OIDC flow and caches hot reads. When unset, Redis is skipped entirely. | | ||
| | `RC_SIGNATURE_CACHE_TTL` | No | `3600` | Time to live, in seconds, for each cached release-config signature. Only applies when `REDIS_URL` is set. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'RC_SIGNATURE_CACHE_TTL|rc_signature_cache_ttl' airborne_server --no-heading -C2
fd -i 'moka.rs' airborne_server --exec cat -n
rg -n 'max_capacity|time_to_live|time_to_idle' airborne_server --no-heading -C1Repository: juspay/airborne
Length of output: 10454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== configuration docs table =="
sed -n '55,70p' airborne_docs/docs/server/configuration.md
echo
echo "== release-config cache construction/usages =="
rg -n 'MokaCache::new|signature_cache_ttl|rc_signature_cache_ttl|CACHED.*SIGNATURE|cached.*signature|signature.*cache' airborne_server/src --no-heading -C 2
echo
echo "== config env parser =="
sed -n '90,280p' airborne_server/src/config.rsRepository: juspay/airborne
Length of output: 21412
Fix the default value for RC_SIGNATURE_CACHE_TTL
airborne_server/src/config.rs uses DEFAULT_RC_SIGNATURE_CACHE_TTL_SECS = 60 * 60, so the documented default should be one hour, not 3600. The other cache-tuning claims are accurate.
🤖 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 `@airborne_docs/docs/server/configuration.md` at line 60, Update the documented
default for RC_SIGNATURE_CACHE_TTL in the configuration table to match
DEFAULT_RC_SIGNATURE_CACHE_TTL_SECS, preserving the existing one-hour value and
the surrounding cache-tuning descriptions.
| } | ||
| "/api/signing-keys": { | ||
| "get": { | ||
| "description": "List the signing keys of an application, with the default key first. Every application\nis provisioned with a default key when it is created, so this is never empty. The\nprivate key is never returned — only the public key, which is enough to verify a\nsigned release config. Pass the organisation and application in the x-organisation and\nx-application headers. Requires a bearer token.", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
"this is never empty" contradicts the documented backfill path.
integrity.mdx (Line 15) and the verification guide (Line 32) both state that applications predating signing have no keys until the signingkeys boot migration runs. For those applications ListSigningKeys does return an empty list, so this description is wrong for exactly the case a reader is most likely to hit.
This file is generated — fix the doc comment in the Smithy source (smithy/models/signing.smithy) and regenerate.
🤖 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 `@airborne_docs/openapi/airborne.openapi.json` at line 2120, The
ListSigningKeys documentation incorrectly claims the response is never empty.
Update the corresponding doc comment for ListSigningKeys in signing.smithy to
state that applications created before signing support may have no keys until
the signingkeys boot migration runs, then regenerate the OpenAPI output.
| guard let signatureHeader = signatureHeader, !signatureHeader.isEmpty else { | ||
| // An unsigned response is not an error: the server omits the header when the | ||
| // application has no signing key. Tolerating it is what lets apps ship public keys | ||
| // before signing is switched on server-side. | ||
| logData["signature_present"] = false | ||
| let value = NSMutableDictionary() | ||
| value["reason"] = AJPSignatureVerificationError.missingHeader.reasonCode | ||
| value["trusted_key_ids"] = trustStore.keys.keys.sorted().joined(separator: ",") | ||
| self.tracker.trackLog("release_config_signature_missing", value: value, level: "warning") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Header-stripping downgrade is accepted even when keys are configured.
With a configured trust store, an active network attacker (or proxy) only has to drop X-Airborne-Signature to get an arbitrary body accepted — verification silently becomes opt-out. The staged-rollout rationale is sound, but it should be a temporary posture, not permanent. Consider an enforcement switch (e.g. a delegate flag or a second delegate method) so integrators can require a signature once server-side signing is on, and default it to strict in a later major version.
🤖 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
`@airborne_sdk_iOS/hyper-ota/Airborne/AirborneSwift/AJPApplicationManager.swift`
around lines 1236 - 1246, Update the signature-header handling around
AJPSignatureVerificationError.missingHeader to support configurable enforcement
when the trust store contains keys: allow the existing warning-and-nil path only
in an explicitly permissive rollout mode, and reject missing signatures in
strict mode. Add or reuse a delegate/configuration switch for this policy,
preserving compatibility while enabling integrators to require signatures.
| /// Sign a release-config body, reusing a cached signature for the same signing | ||
| /// key selector and `config.version`. A new config version must be minted | ||
| /// whenever the resolved release body changes. | ||
| pub async fn sign_release_config( | ||
| state: &AppState, | ||
| organisation: &str, | ||
| application: &str, | ||
| requested_key_id: Option<&str>, | ||
| config_version: &str, | ||
| body: &[u8], | ||
| ) -> Result<Option<String>, ABError> { | ||
| // An empty config version would collide across releases, so never key on one. | ||
| let cacheable = !config_version.is_empty(); | ||
| let selector = signature_cache_selector(requested_key_id)?; | ||
|
|
||
| let cache_key = state | ||
| .redis_cache | ||
| .as_ref() | ||
| .filter(|_| cacheable) | ||
| .map(|cache| { | ||
| ( | ||
| cache, | ||
| signature_cache_key(cache, organisation, application, &selector, config_version), | ||
| ) | ||
| }); | ||
|
|
||
| if let Some((cache, key)) = &cache_key { | ||
| if let Some(hit) = cache.get::<CachedSignature>(key).await? { | ||
| return Ok(Some(signature_header_value(&hit.key_id, &hit.signature))); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B3 -A12 'sign_release_config' airborne_server/src/release.rs
rg -nP -C4 '\bconfig\.version\b|config_version' airborne_server/src/release/utils.rsRepository: juspay/airborne
Length of output: 2328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file sizes =="
wc -l airborne_server/src/release.rs airborne_server/src/signing/utils.rs 2>/dev/null || true
echo "== release handler around signature call =="
sed -n '1430,1495p' airborne_server/src/release.rs 2>/dev/null || true
echo "== release response / body construction references =="
rg -n -B10 -A35 'serve_release_handler|release_response|body|config_version' airborne_server/src/release.rs | sed -n '1,260p'
echo "== signing helper =="
sed -n '330,450p' airborne_server/src/signing/utils.rs 2>/dev/null || true
echo "== cache helper definitions =="
rg -n -B8 -A18 'fn signature_cache_key|signature_cache_selector|CachedSignature' airborne_server/src/signing/utils.rs 2>/dev/null || true
echo "== config.version handling in release utils =="
rg -n -B8 -A25 'config_version|config\.version|config-version|version' airborne_server/src/release/utils.rs 2>/dev/null | sed -n '1,320p'Repository: juspay/airborne
Length of output: 37609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate serve release handler definition/context =="
rg -n -B5 -A180 'async fn\s+.*serve.*release|serve_release_handler|ServeReleaseResponse|ServeRelease|get_release|read.*release' airborne_server/src/release.rs | sed -n '1,280p'
echo "== build_overrides package config property extraction =="
sed -n '470,830p' airborne_server/src/release/utils.rs 2>/dev/null || true
echo "== search for BuildOverrides and config_version extraction in utils =="
rg -n -B10 -A90 'struct BuildOverrides|impl BuildOverrides|config_version|BuildOverrides|PackageConfig|release_config' airborne_server/src/release/utils.rs | sed -n '1,320p'
echo "== deterministic shape of cache key vs body dependency from source =="
python3 - <<'PY'
from pathlib import Path
import re
release = Path('airborne_server/src/release.rs').read_text()
utils = Path('airborne_server/src/release/utils.rs').read_text()
signing = Path('airborne_server/src/signing/utils.rs').read_text()
checks = {
"signing_cache_key_uses_config_version_only": 'signature_cache_key(cache, organisation, application, &selector, config_version)' in signing,
"handler_passes_release_config_version_and_body": release.count('signing::utils::sign_release_config(\n &state,') > 0 and '&release_response.config.version,\n &body,' in release,
"cache_hit_returns_entry_signature_only": 'return Ok(Some(signature_header_value(&hit.key_id, &hit.signature)));' in signing,
}
for key,val in checks.items():
print(f"{key}: {val}")
PYRepository: juspay/airborne
Length of output: 38081
Keep the release-config signature cache scoped to the exact response body.
sign_release_config now returns a cached signature for the same (org, app, signing_key, config.version), but serve_release builds and signs the whole release body, which includes resolved file URLs/checksums that can vary by x-dimension. A hit can serve a signature over a different body, causing clients to reject the config before falling off OTA updates. Cache only when the signature was computed over the exact body bytes, or include a body digest dimension/context in the cache key.
🤖 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 `@airborne_server/src/signing/utils.rs` around lines 370 - 400, The cache
lookup in sign_release_config must be scoped to the exact body being signed.
Include a digest or equivalent body-specific context in signature_cache_key and
use it consistently for lookup and storage, or bypass caching when the body
cannot be safely keyed; preserve cache reuse only when organisation,
application, signing selector, config_version, and body bytes all match.
| let signature = match private_key.and_then(|private_key| sign_payload(&private_key, body)) { | ||
| Ok(signature) => signature, | ||
| Err(e) => { | ||
| error!( | ||
| "Failed to sign release config for {organisation}/{application}: {e}. \ | ||
| Serving unsigned." | ||
| ); | ||
| return Ok(None); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Signing failures fail open, and the iOS client tolerates a missing header — together that is a silent downgrade path.
A signing error here logs and serves the config unsigned. verifyReleaseConfigSignature in airborne_sdk_iOS/.../AJPApplicationManager.swift treats an absent X-Airborne-Signature as acceptable, so anything that makes signing fail for an application (unreadable key material, a master-key rotation that breaks decryption) downgrades every client from verified to unverified without any client-visible signal.
Fail-open is a defensible availability choice, but it deserves an alert-grade signal rather than only an error! log — a metric on unsigned-serves-for-a-signing-app so this cannot go unnoticed. Worth also considering whether the SDK should distinguish "app has no keys" from "app has keys but this response was unsigned" (the client cannot tell today).
🤖 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 `@airborne_server/src/signing/utils.rs` around lines 418 - 427, Add an
alert-grade metric in the signing failure branch of the release-config signing
flow, alongside the existing error log and before returning Ok(None), to count
unsigned serves for applications configured for signing. Preserve the current
fail-open response behavior, and use the existing metrics/instrumentation symbol
or pattern rather than changing SDK verification behavior.
| run_blocking!({ | ||
| let mut conn = pool.get()?; | ||
|
|
||
| // The very first key for an application becomes its default, so an app | ||
| // is never left with keys but nothing to sign with. | ||
| let has_default = diesel::select(diesel::dsl::exists( | ||
| signing_keys_table | ||
| .filter(sk_org_id.eq(&organisation)) | ||
| .filter(sk_app_id.eq(&application)) | ||
| .filter(sk_is_default.eq(true)), | ||
| )) | ||
| .get_result::<bool>(&mut conn)?; | ||
|
|
||
| let new_key = NewSigningKey { | ||
| id: Uuid::new_v4(), | ||
| org_id: organisation.clone(), | ||
| app_id: application.clone(), | ||
| name: key_id.clone(), | ||
| algorithm: ALGORITHM.to_string(), | ||
| public_key: keypair.public_key.clone(), | ||
| private_key_encrypted: private_key_encrypted.clone(), | ||
| is_default: !has_default, | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
has_default check and the insert are not atomic.
Two concurrent POST /signing-keys for an application with no default both read has_default = false and both insert with is_default = true. Unless the migration adds a partial unique index on (org_id, app_id) WHERE is_default, the application ends up with two defaults and fetch_default_key_db returns whichever row the planner picks — different instances can then sign with different keys. Wrapping the exists-check and insert in conn.transaction (as set_default_key already does) closes it.
🤖 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 `@airborne_server/src/signing/utils.rs` around lines 553 - 575, Wrap the
has_default existence check and the subsequent NewSigningKey insert in the same
conn.transaction closure within the signing-key creation flow. Preserve the
existing default assignment logic while ensuring concurrent requests cannot both
observe no default and insert competing default keys, following the transaction
pattern used by set_default_key.
| run_blocking!({ | ||
| let mut conn = pool.get()?; | ||
|
|
||
| let has_default = diesel::select(diesel::dsl::exists( | ||
| signing_keys_table | ||
| .filter(sk_org_id.eq(&organisation)) | ||
| .filter(sk_app_id.eq(&application)) | ||
| .filter(sk_is_default.eq(true)), | ||
| )) | ||
| .get_result::<bool>(&mut conn)?; | ||
|
|
||
| if has_default { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let new_key = NewSigningKey { | ||
| id: Uuid::new_v4(), | ||
| org_id: organisation.clone(), | ||
| app_id: application.clone(), | ||
| name: DEFAULT_KEY_ID.to_string(), | ||
| algorithm: ALGORITHM.to_string(), | ||
| public_key: keypair.public_key.clone(), | ||
| private_key_encrypted: private_key_encrypted.clone(), | ||
| is_default: true, | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Correctness here is fully delegated to a unique constraint.
The has_default read and the insert are separate statements, so the comment at Line 727 ("Another request or instance provisioned it first") only holds if the migration actually declares a unique index that a second default insert violates. Without it, concurrent boots/app-creations insert two is_default = true rows and no error is raised.
🤖 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 `@airborne_server/src/signing/utils.rs` around lines 696 - 720, Add or verify a
database migration declaring a unique constraint/index on the organisation,
application, and default-key condition used by the has_default check in the
signing-key provisioning flow. Ensure the insert in the run_blocking block
violates that constraint for concurrent default-key creation, preserving the
existing conflict-handling path.
| const DEFAULT_CAPACITY: u64 = 1_024; | ||
| const DEFAULT_TTL: Duration = Duration::from_secs(60 * 60); | ||
| const DEFAULT_TTI: Duration = Duration::from_secs(15 * 60); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how AppState.moka_cache is constructed and whether decrypted-key caching uses a dedicated policy.
rg -nP -A5 'MokaCache::(new|with_policy)\(' airborne_server/srcRepository: juspay/airborne
Length of output: 2000
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== main.rs AppState construction =="
sed -n '420,455p' airborne_server/src/main.rs
echo
echo "== moka.rs defaults and new implementation =="
sed -n '1,80p' airborne_server/src/utils/moka.rs
echo
echo "== search for with_policy or key policy construction =="
rg -n 'with_policy|MokaCache::|get_or_try_set|decrypted_signing_key_cache_key|signing_key' airborne_server/srcRepository: juspay/airborne
Length of output: 9406
Use a short, key-specific TTL for decrypted signing keys.
AppState is initialized with MokaCache::new("airborne"), and decrypted signing key material is cached via that same shared instance; there is no with_policy(...) usage for this code path. With the current defaults, plaintext private key material can persist for up to 60m TTL / 15m idle time. Use a dedicated cache policy or shorter TTL for decrypted key material.
🤖 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 `@airborne_server/src/utils/moka.rs` around lines 7 - 9, Update the decrypted
signing-key cache path used by AppState and MokaCache::new("airborne") to use a
dedicated policy with a substantially shorter TTL and idle timeout than
DEFAULT_TTL and DEFAULT_TTI. Keep general cache defaults unchanged, and ensure
the key-specific cache configuration is actually applied rather than only
defining new constants.
Replace the keyid -> public-key map with one pinned (key id, public key). publicKeys() -> [String: String] on AirborneDelegate becomes publicKey() -> String plus signingKeyId() -> String. The SDK accepts a release config only when the response's keyid equals signingKeyId() and the signature verifies against publicKey(). Trusting exactly one key means changing it requires an app release, rather than rotating in place across several trusted keys. - AJPReleaseConfigTrustStore now holds a single (keyID, publicKey); verify() takes expectedKeyID + trustedKey; the .noTrustedKeys error is removed. - A half-configuration (a key id without a PEM, or a malformed PEM) is configured-but-unusable: signed configs are rejected (invalid_signing_config) rather than silently accepted, and a boot-time signing_key_invalid event fires. - Update the delegate wiring, manager hook, unit + live tests, and docs (rotation now requires an app release on iOS).
1 participant
Summary by CodeRabbit
New Features
Documentation