feat: release config signing + moka cache - #370
Open
yuvrajjsingh0 wants to merge 3 commits into
Open
Conversation
yuvrajjsingh0
force-pushed
the
rc-signing
branch
from
July 16, 2026 15:11
492afe1 to
4d5dc25
Compare
WalkthroughAdds end-to-end ES256 release-config signing, signing-key lifecycle APIs and persistence, cache invalidation, dashboard management screens, authorization checks, and documentation. ChangesSigning contracts, persistence, and runtime state
Signing-key operations and provisioning
Release response signing and invalidation
Dashboard integrity settings
Documentation and generated API descriptions
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
lint lint
yuvrajjsingh0
force-pushed
the
rc-signing
branch
from
July 17, 2026 06:46
fe7c04d to
e87ab88
Compare
Contributor
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
smithy/models/release.smithy (1)
358-364: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove
@requiresauthfrom these public release operations.Both operations are documented and routed as unauthenticated SDK endpoints, but the Smithy trait still declares authentication mandatory. Generated clients and OpenAPI consumers may consequently require credentials for public boot requests.
Proposed contract fix
`@http`(method: "GET", uri: "/release/{organisation}/{application}") `@readonly` -@requiresauth operation ServeRelease { @@ `@http`(method: "GET", uri: "/release/v2/{organisation}/{application}") `@readonly` -@requiresauth operation ServeReleaseV2 {Also applies to: 375-381
🤖 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 `@smithy/models/release.smithy` around lines 358 - 364, Remove the `@requiresauth` trait from both public release operations: the operation shown in the diff and the corresponding operation near the second referenced location. Preserve their existing HTTP routes and documentation so generated clients treat these SDK boot endpoints as unauthenticated.airborne_server/src/main.rs (1)
185-195: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRun database migrations before the
signingkeysbackfill.
signingkeystriggers the backfill but does not setshould_run_db_migrations. WithMIGRATIONS_TO_RUN_ON_BOOT=signingkeys, startup skips the table migration, the backfill queries a missingsigning_keystable, logs the failure, and continues without provisioning keys.Proposed fix
let should_run_db_migrations = migrations_to_run_on_boot.iter().any(|m| m == "db"); +let should_run_signing_key_backfill = + migrations_to_run_on_boot.iter().any(|m| m == "signingkeys"); -if should_run_db_migrations || should_run_keycloak_to_casbin { +if should_run_db_migrations + || should_run_keycloak_to_casbin + || should_run_signing_key_backfill +{ @@ -if migrations_to_run_on_boot.iter().any(|m| m == "signingkeys") { +if should_run_signing_key_backfill {Also applies to: 499-513
🤖 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/main.rs` around lines 185 - 195, Update the startup migration selection in main so the database migration block also runs when migrations_to_run_on_boot contains "signingkeys", ensuring the signing_keys table exists before the signingkeys backfill executes. Preserve the existing behavior for "db" and "keycloaktocasbin" entries and keep the backfill flow 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/components/settings/integrity/signing-keys-card.tsx`:
- Around line 48-53: Update the useSWR state in the signing-keys card to capture
error alongside data, isLoading, and mutate, then render a dedicated error state
with a retry action using mutate when the request fails. Ensure the “No signing
keys” empty state is shown only after a successful response with no keys, not
when data is undefined because of an error.
In `@airborne_docs/docs/dashboard/integrity.mdx`:
- Around line 39-43: Update the key ID validation list in the integrity
documentation to state that IDs must be no longer than 50 characters, alongside
the existing character, dash, and boundary constraints.
In `@airborne_docs/openapi/airborne.openapi.json`:
- Line 2120: Update the signing-key list description associated with the
relevant OpenAPI endpoint to remove the guarantee that the list is never empty.
State that automatic default-key provisioning applies to newly created or
backfilled applications, while preserving the existing details about ordering,
public-key-only responses, headers, and authentication.
- Line 2865: Add operation-level security overrides with an empty array to both
ServeReleaseV2 at airborne_docs/openapi/airborne.openapi.json:2865-2865 and
ServeRelease at airborne_docs/openapi/airborne.openapi.json:2989-2989, so these
public operations do not inherit the document-level bearer authentication
requirement.
- Around line 2891-2897: Update the signing-key parameter schemas for
ServeReleaseV2 at airborne_docs/openapi/airborne.openapi.json:2891-2897 and
ServeRelease at airborne_docs/openapi/airborne.openapi.json:3015-3021 to accept
either an empty string or a valid non-empty key ID. Replace the conflicting
minLength/pattern constraints with a schema expression that preserves the
existing key-ID validation while allowing the documented empty-value default
behavior.
In `@airborne_server/src/config.rs`:
- Around line 173-175: Update the master_encryption_key handling near its
initialization and the encrypt_private_key provisioning flow so signing
private-key encryption remains mandatory regardless of USE_ENCRYPTED_SECRETS.
Require a dedicated encryption key for signing keys, or reject provisioning when
none is configured; never allow encrypt_private_key to persist plaintext PEM
data.
In `@airborne_server/src/release.rs`:
- Around line 1465-1469: Update the signing-key ID extraction in the request
handling flow to distinguish a missing header from a present header containing
invalid UTF-8. Reject the request when x-signing-key-id cannot be converted to
text, rather than passing None to signing::utils::requested_key_id and selecting
the default key; preserve existing behavior for valid and absent headers.
In `@airborne_server/src/release/utils.rs`:
- Around line 496-509: Update invalidate_release_cache to invalidate both the
existing /release/{organisation}/{application}* path and the v2 endpoint path
/release/v2/{organisation}/{application}. Submit both paths together through
invalidate_cf as a single invalidation batch, preserving the current best-effort
error logging.
In `@airborne_server/src/signing/utils.rs`:
- Around line 323-336: Update signature_cache_key and the surrounding signing
flow to resolve the authoritative active signing key before reading the cache,
then include the resolved key identity and a digest of body in the cache key
alongside the existing release context. Ensure cache hits cannot bypass key
resolution or return signatures for disabled/replaced keys, and update all
affected call sites in the signing and invalidation paths. Add regression
coverage for body mutation and concurrent mutation/invalidation so stale
in-flight signatures are not served.
- Around line 153-168: Require a configured master encryption key in
encrypt_private_key and fail with the existing ABError mechanism when it is
absent, rather than returning the plaintext key. Update the corresponding
decrypt_private_key behavior as needed to preserve encrypted-key handling, and
ensure provisioning propagates the failure instead of persisting or exposing the
raw private key.
In `@airborne_server/src/utils/db/models.rs`:
- Line 250: Remove the Debug derive from both private-key-bearing model structs
near the Queryable/Selectable declarations, including the model containing
private_key_encrypted, so debug formatting cannot expose encrypted private keys;
preserve the other derives unchanged.
In `@airborne_server/src/utils/redis.rs`:
- Around line 200-253: Make index_add and index_drop atomic by replacing their
multi-command Redis flows with Lua scripts executed through the existing Redis
connection. The index_add script must perform SADD and EXPIRE together, while
the index_drop script must read the set members, delete those members, and
delete the index within one atomic operation. Preserve the current error metrics
and ABError handling around the script executions.
In `@smithy/models/signing.smithy`:
- Around line 32-34: Update the signing model documentation around the
default-key description and the list guarantees at the referenced symbols to
state that an application may temporarily have no keys and therefore no default
key when provisioning fails; remove any claim that the key list is never empty
or that exactly one default always exists, while preserving the behavior for
applications with a configured default.
---
Outside diff comments:
In `@airborne_server/src/main.rs`:
- Around line 185-195: Update the startup migration selection in main so the
database migration block also runs when migrations_to_run_on_boot contains
"signingkeys", ensuring the signing_keys table exists before the signingkeys
backfill executes. Preserve the existing behavior for "db" and
"keycloaktocasbin" entries and keep the backfill flow unchanged.
In `@smithy/models/release.smithy`:
- Around line 358-364: Remove the `@requiresauth` trait from both public release
operations: the operation shown in the diff and the corresponding operation near
the second referenced location. Preserve their existing HTTP routes and
documentation so generated clients treat these SDK boot endpoints as
unauthenticated.
🪄 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
Run ID: f601f139-b710-49ec-8e89-00e3e44b228a
⛔ 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 (42)
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/server/configuration.mdairborne_docs/openapi/airborne.openapi.jsonairborne_docs/sidebars.tsairborne_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
Comment on lines
+48
to
+53
| const { data, isLoading, mutate } = useSWR(canRead && token && org && app ? ["/signing-keys", org, app] : null, () => | ||
| apiFetch<SigningKeysResponse>("/signing-keys", {}, { token, org, app }) | ||
| ); | ||
|
|
||
| const keys = data?.data ?? []; | ||
| const loading = !canRead || isLoading; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first
ast-grep outline airborne_dashboard/components/settings/integrity/signing-keys-card.tsx --view expanded || true
# Read the relevant section with line numbers
sed -n '1,260p' airborne_dashboard/components/settings/integrity/signing-keys-card.tsx | cat -n
# Look for related signing-keys UI and error handling patterns
rg -n "signing-keys|No signing keys|error|retry|mutate\\(" airborne_dashboard/components/settings/integrity -SRepository: juspay/airborne
Length of output: 16114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' airborne_dashboard/components/settings/integrity/create-key-dialog.tsx | cat -n
printf '\n---\n'
rg -n "error|retry|No signing keys|useSWR\\(" airborne_dashboard/components/settings/integrity -SRepository: juspay/airborne
Length of output: 6529
Show a dedicated error state instead of “No signing keys” on list failure.
When the SWR request errors, data stays undefined and this branch falls through to the empty state, which implies release configs are unsigned and invites key creation. Handle error separately and offer a retry action; only render the empty state after a successful empty response.
🤖 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/components/settings/integrity/signing-keys-card.tsx`
around lines 48 - 53, Update the useSWR state in the signing-keys card to
capture error alongside data, isLoading, and mutate, then render a dedicated
error state with a retry action using mutate when the request fails. Ensure the
“No signing keys” empty state is shown only after a successful response with no
keys, not when data is undefined because of an error.
Source: Learnings
Comment on lines
+39
to
+43
| Key IDs must be unique within the application and are immutable. They must: | ||
|
|
||
| - contain only lowercase letters (`a-z`), digits (`0-9`), and dashes (`-`); | ||
| - use no consecutive dashes; and | ||
| - neither start nor end with a dash. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the 50-character key ID limit.
The server rejects key IDs longer than 50 characters, but this validation list omits that constraint.
Proposed fix
Key IDs must be unique within the application and are immutable. They must:
+- be at most 50 characters long;
- contain only lowercase letters (`a-z`), digits (`0-9`), and dashes (`-`);📝 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.
Suggested change
| Key IDs must be unique within the application and are immutable. They must: | |
| - contain only lowercase letters (`a-z`), digits (`0-9`), and dashes (`-`); | |
| - use no consecutive dashes; and | |
| - neither start nor end with a dash. | |
| Key IDs must be unique within the application and are immutable. They must: | |
| - be at most 50 characters long; | |
| - contain only lowercase letters (`a-z`), digits (`0-9`), and dashes (`-`); | |
| - use no consecutive dashes; and | |
| - neither start nor end with a dash. |
🤖 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/dashboard/integrity.mdx` around lines 39 - 43, Update the
key ID validation list in the integrity documentation to state that IDs must be
no longer than 50 characters, alongside the existing character, dash, and
boundary constraints.
| } | ||
| "/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.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not promise that the signing-key list is never empty.
Applications created before signing can return an empty list until the signingkeys backfill runs. Describe automatic provisioning as applying to new or backfilled applications.
🤖 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, Update the
signing-key list description associated with the relevant OpenAPI endpoint to
remove the guarantee that the list is never empty. State that automatic
default-key provisioning applies to newly created or backfilled applications,
while preserving the existing details about ordering, public-key-only responses,
headers, and authentication.
| }, | ||
| "/release/v2/{organisation}/{application}": { | ||
| "get": { | ||
| "description": "Version 2 of the release-resolution endpoint: resolves and returns the active release configuration for an application based on the caller's targeting dimensions. This is the endpoint newer SDKs call at boot. Public — no auth token required.\n\nThe response is signed: the x-airborne-signature response header carries an ES256 signature over the exact raw JSON response body bytes, so a client can verify the config was served by Airborne and not tampered with in transit. Send the x-signing-key-id header to choose which key signs the response; omit it to use the application's default key. See the \"Signing keys\" endpoints for managing keys and downloading the public key to verify against.", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Override global bearer authentication for the public release endpoints.
Both operations inherit the document-level bearer requirement despite being public. Generated clients and API documentation will therefore incorrectly require credentials.
airborne_docs/openapi/airborne.openapi.json#L2865-L2865: add operation-level"security": []toServeReleaseV2.airborne_docs/openapi/airborne.openapi.json#L2989-L2989: add operation-level"security": []toServeRelease.
📍 Affects 1 file
airborne_docs/openapi/airborne.openapi.json#L2865-L2865(this comment)airborne_docs/openapi/airborne.openapi.json#L2989-L2989
🤖 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 2865, Add
operation-level security overrides with an empty array to both ServeReleaseV2 at
airborne_docs/openapi/airborne.openapi.json:2865-2865 and ServeRelease at
airborne_docs/openapi/airborne.openapi.json:2989-2989, so these public
operations do not inherit the document-level bearer authentication requirement.
Comment on lines
+2891
to
+2897
| "description": "ID of the signing key to sign the response with. This is the readable ID chosen\nwhen the key was created. Optional — when omitted, or sent with an empty value,\nthe application's default signing key is used. A non-empty key ID that is\ninvalid, unknown, disabled, or belongs to another application is rejected with\na 400.", | ||
| "schema": { | ||
| "type": "string", | ||
| "maxLength": 50, | ||
| "minLength": 1, | ||
| "pattern": "^[a-z0-9]+(-[a-z0-9]+)*$", | ||
| "description": "ID of the signing key to sign the response with. This is the readable ID chosen\nwhen the key was created. Optional — when omitted, or sent with an empty value,\nthe application's default signing key is used. A non-empty key ID that is\ninvalid, unknown, disabled, or belongs to another application is rejected with\na 400." |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Align the empty-header behavior with the parameter schemas.
The descriptions say an empty value selects the default key, but minLength: 1 and the regex reject it. Generated validators will reject a request the server accepts.
airborne_docs/openapi/airborne.openapi.json#L2891-L2897: allow either an empty string or a valid key ID forServeReleaseV2.airborne_docs/openapi/airborne.openapi.json#L3015-L3021: apply the same schema toServeRelease.
📍 Affects 1 file
airborne_docs/openapi/airborne.openapi.json#L2891-L2897(this comment)airborne_docs/openapi/airborne.openapi.json#L3015-L3021
🤖 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` around lines 2891 - 2897, Update
the signing-key parameter schemas for ServeReleaseV2 at
airborne_docs/openapi/airborne.openapi.json:2891-2897 and ServeRelease at
airborne_docs/openapi/airborne.openapi.json:3015-3021 to accept either an empty
string or a valid non-empty key ID. Replace the conflicting minLength/pattern
constraints with a schema expression that preserves the existing key-ID
validation while allowing the documented empty-value default behavior.
Comment on lines
+153
to
+168
| /// Encrypt a private key for at-rest storage with the same master key used for | ||
| /// encrypted environment secrets. | ||
| async fn encrypt_private_key(plaintext: &str, key: Option<&str>) -> Result<String, ABError> { | ||
| match key { | ||
| Some(key) => crate::utils::encryption::encrypt_string(plaintext, key).await, | ||
| None => Ok(plaintext.to_string()), | ||
| } | ||
| } | ||
|
|
||
| /// Decrypt a private key produced by [`encrypt_private_key`]. | ||
| async fn decrypt_private_key(ciphertext: &str, key: Option<&str>) -> Result<String, ABError> { | ||
| match key { | ||
| Some(key) => crate::utils::encryption::decrypt_string(ciphertext, key).await, | ||
| None => Ok(ciphertext.to_string()), | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Never fall back to plaintext private-key storage.
When master_encryption_key is absent, the raw PKCS#8 key is persisted and may also enter Redis through ActiveSigningKey. Fail provisioning instead of silently removing at-rest protection.
Proposed fix
async fn encrypt_private_key(plaintext: &str, key: Option<&str>) -> Result<String, ABError> {
- match key {
- Some(key) => crate::utils::encryption::encrypt_string(plaintext, key).await,
- None => Ok(plaintext.to_string()),
- }
+ let key = key.ok_or_else(|| {
+ ABError::InternalServerError(
+ "A master encryption key is required for release-config signing".to_string(),
+ )
+ })?;
+ crate::utils::encryption::encrypt_string(plaintext, key).await
}📝 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.
Suggested change
| /// Encrypt a private key for at-rest storage with the same master key used for | |
| /// encrypted environment secrets. | |
| async fn encrypt_private_key(plaintext: &str, key: Option<&str>) -> Result<String, ABError> { | |
| match key { | |
| Some(key) => crate::utils::encryption::encrypt_string(plaintext, key).await, | |
| None => Ok(plaintext.to_string()), | |
| } | |
| } | |
| /// Decrypt a private key produced by [`encrypt_private_key`]. | |
| async fn decrypt_private_key(ciphertext: &str, key: Option<&str>) -> Result<String, ABError> { | |
| match key { | |
| Some(key) => crate::utils::encryption::decrypt_string(ciphertext, key).await, | |
| None => Ok(ciphertext.to_string()), | |
| } | |
| } | |
| /// Encrypt a private key for at-rest storage with the same master key used for | |
| /// encrypted environment secrets. | |
| async fn encrypt_private_key(plaintext: &str, key: Option<&str>) -> Result<String, ABError> { | |
| let key = key.ok_or_else(|| { | |
| ABError::InternalServerError( | |
| "A master encryption key is required for release-config signing".to_string(), | |
| ) | |
| })?; | |
| crate::utils::encryption::encrypt_string(plaintext, key).await | |
| } | |
| /// Decrypt a private key produced by [`encrypt_private_key`]. | |
| async fn decrypt_private_key(ciphertext: &str, key: Option<&str>) -> Result<String, ABError> { | |
| match key { | |
| Some(key) => crate::utils::encryption::decrypt_string(ciphertext, key).await, | |
| None => Ok(ciphertext.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 153 - 168, Require a
configured master encryption key in encrypt_private_key and fail with the
existing ABError mechanism when it is absent, rather than returning the
plaintext key. Update the corresponding decrypt_private_key behavior as needed
to preserve encrypted-key handling, and ensure provisioning propagates the
failure instead of persisting or exposing the raw private key.
Comment on lines
+323
to
+336
| /// Cache key for one (release config, signing key) pair. | ||
| fn signature_cache_key( | ||
| cache: &RedisCache, | ||
| organisation: &str, | ||
| application: &str, | ||
| selector: &str, | ||
| config_version: &str, | ||
| ) -> RedisKey { | ||
| cache.key_unlabeled( | ||
| organisation, | ||
| application, | ||
| &["release_sig", selector, config_version], | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Bind cached signatures to the exact body and authoritative key state.
The cache key only contains the selector and config_version, while property edits can change the signed body without changing that version. An in-flight writer after invalidation can therefore cache an old-body signature. Additionally, a cache hit bypasses key resolution, so failed invalidation can continue serving a disabled or replaced key.
Resolve the active key before lookup and key entries by the resolved key identity plus a digest of body; add regression coverage for concurrent mutation/invalidation.
Also applies to: 370-405, 429-441, 447-500
🤖 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 323 - 336, Update
signature_cache_key and the surrounding signing flow to resolve the
authoritative active signing key before reading the cache, then include the
resolved key identity and a digest of body in the cache key alongside the
existing release context. Ensure cache hits cannot bypass key resolution or
return signatures for disabled/replaced keys, and update all affected call sites
in the signing and invalidation paths. Add regression coverage for body mutation
and concurrent mutation/invalidation so stale in-flight signatures are not
served.
| /// The internal row UUID, `org_id`, `app_id`, and `updated_at` are omitted: every | ||
| /// query is already scoped to one application, and clients identify keys by | ||
| /// `name`, so selecting them back would be dead weight. | ||
| #[derive(Queryable, Selectable, Debug, Clone)] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not derive Debug for private-key-bearing models.
Both structs expose private_key_encrypted through debug formatting. Remove Debug or implement a redacted formatter.
Proposed fix
-#[derive(Queryable, Selectable, Debug, Clone)]
+#[derive(Queryable, Selectable, Clone)]
...
-#[derive(Insertable, Debug)]
+#[derive(Insertable)]Also applies to: 263-274
🤖 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/db/models.rs` at line 250, Remove the Debug derive
from both private-key-bearing model structs near the Queryable/Selectable
declarations, including the model containing private_key_encrypted, so debug
formatting cannot expose encrypted private keys; preserve the other derives
unchanged.
Comment on lines
+200
to
+253
| /// Record `member` in the SET at `index`, so a family of cache entries can be | ||
| /// dropped together later. The SET is given the same TTL as its members, so a | ||
| /// forgotten index cannot outlive them. | ||
| pub async fn index_add( | ||
| &self, | ||
| index: &RedisKey, | ||
| member: &RedisKey, | ||
| ttl_secs: usize, | ||
| ) -> Result<(), ABError> { | ||
| let mut r = (*self.conn).clone(); | ||
| let (index_key, member_key) = (index.key.clone(), member.key.clone()); | ||
|
|
||
| let _: () = r.sadd(&index_key, &member_key).await.map_err(|e| { | ||
| error!("Failed to SADD {member_key} to {index_key}: {e}"); | ||
| CACHE_FAILS.with_label_values(&index.labels).inc(); | ||
| ABError::InternalServerError("service error".to_string()) | ||
| })?; | ||
|
|
||
| let _: () = r.expire(&index_key, ttl_secs as i64).await.map_err(|e| { | ||
| error!("Failed to EXPIRE {index_key}: {e}"); | ||
| CACHE_FAILS.with_label_values(&index.labels).inc(); | ||
| ABError::InternalServerError("service error".to_string()) | ||
| })?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Delete every key recorded in the SET at `index`, then the index itself. | ||
| pub async fn index_drop(&self, index: &RedisKey) -> Result<(), ABError> { | ||
| let mut r = (*self.conn).clone(); | ||
| let index_key = index.key.clone(); | ||
|
|
||
| let members: Vec<String> = r.smembers(&index_key).await.map_err(|e| { | ||
| error!("Failed to SMEMBERS {index_key}: {e}"); | ||
| CACHE_FAILS.with_label_values(&index.labels).inc(); | ||
| ABError::InternalServerError("service error".to_string()) | ||
| })?; | ||
|
|
||
| if !members.is_empty() { | ||
| let _: () = r.del(members).await.map_err(|e| { | ||
| error!("Failed to DEL members of {index_key}: {e}"); | ||
| CACHE_FAILS.with_label_values(&index.labels).inc(); | ||
| ABError::InternalServerError("service error".to_string()) | ||
| })?; | ||
| } | ||
|
|
||
| let _: () = r.del(&index_key).await.map_err(|e| { | ||
| error!("Failed to DEL {index_key}: {e}"); | ||
| CACHE_FAILS.with_label_values(&index.labels).inc(); | ||
| ABError::InternalServerError("service error".to_string()) | ||
| })?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make signature indexing and invalidation atomic.
index_add can add a member after index_drop reads SMEMBERS but before it deletes the index. The new cached signature then survives invalidation while its index entry is removed, allowing stale signatures after key rotation or disabling. Use Redis Lua scripts to atomically perform SADD+EXPIRE and member/index deletion.
🤖 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 200 - 253, Make index_add
and index_drop atomic by replacing their multi-command Redis flows with Lua
scripts executed through the existing Redis connection. The index_add script
must perform SADD and EXPIRE together, while the index_drop script must read the
set members, delete those members, and delete the index within one atomic
operation. Preserve the current error metrics and ABError handling around the
script executions.
Comment on lines
+32
to
+34
| /// Whether this is the application's default key. Exactly one key per application is | ||
| /// the default: it signs every release config served without an explicit | ||
| /// x-signing-key-id header. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not guarantee that every application already has a default key.
Lines 164-166 say the list is never empty, but add_application deliberately succeeds when provisioning fails. Document that an application may temporarily have no keys/default instead of exposing a false API invariant.
Also applies to: 164-166, 242-244
🤖 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 `@smithy/models/signing.smithy` around lines 32 - 34, Update the signing model
documentation around the default-key description and the list guarantees at the
referenced symbols to state that an application may temporarily have no keys and
therefore no default key when provisioning fails; remove any claim that the key
list is never empty or that exactly one default always exists, while preserving
the behavior for applications with a configured default.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit