feat: Add ECDSA AWS KMS hosted signer backend#270
Open
zeljkoX wants to merge 6 commits into
Open
Conversation
|
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 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)
Comment |
zeljkoX
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/SERVER_AWS_DEPLOY.md`:
- Around line 155-160: Add a clarifying sentence immediately after the existing
sentence "The ECDSA Secrets Manager secret is not needed on this path." to state
that the Falcon ACK secret bootstrap is still required in production; update the
paragraph around the optional KMS signer (near the line mentioning
guardian_ack_ecdsa_kms_key_arn and the sentence about the Secrets Manager
secret) so operators know KMS mode does not remove the need to bootstrap the
Falcon ACK secret in prod.
🪄 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: 39efd924-f144-41b3-a1b6-b6aa6137b932
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
crates/server/Cargo.tomlcrates/server/src/ack/miden_ecdsa/backend/aws_kms.rscrates/server/src/ack/miden_ecdsa/backend/in_memory.rscrates/server/src/ack/miden_ecdsa/backend/mod.rscrates/server/src/ack/miden_ecdsa/mod.rscrates/server/src/ack/miden_ecdsa/signer.rscrates/server/src/ack/mod.rscrates/server/src/error.rscrates/server/src/services/push_delta.rsdocs/CONFIGURATION.mddocs/PRODUCTION.mddocs/SERVER_AWS_DEPLOY.mddocs/runbooks/secrets.mdinfra/data.tfinfra/ecs.tfinfra/iam.tfinfra/variables.tfspeckit/features/001-hosted-signer-backends/contracts/configuration.mdspeckit/features/001-hosted-signer-backends/contracts/ecdsa-signer-backend.mdspeckit/features/001-hosted-signer-backends/data-model.mdspeckit/features/001-hosted-signer-backends/spec.md
💤 Files with no reviewable changes (1)
- crates/server/src/error.rs
There was a problem hiding this comment.
Pull request overview
This PR adds a modular ECDSA ACK signer backend abstraction to the Guardian server, introducing an AWS KMS–hosted signer option so the ECDSA private key never needs to be loaded into server process memory, while keeping the existing in-memory (filesystem / Secrets Manager) path as the default.
Changes:
- Refactors ECDSA ACK signing to route through a backend trait and makes ACK signing async where needed (ECDSA path).
- Implements an AWS KMS ECDSA backend with startup validation + DER→Miden signature conversion and adds required Rust dependencies.
- Updates Terraform and operator docs to optionally provision/use a KMS-backed ECDSA ACK signer and to skip requiring the ECDSA Secrets Manager secret when KMS is configured.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
speckit/features/001-hosted-signer-backends/spec.md |
Feature spec for modular hosted ECDSA backends (AWS KMS first), including scenarios and requirements. |
speckit/features/001-hosted-signer-backends/data-model.md |
In-process type model describing backend trait, AWS KMS backend, and startup validation behavior. |
speckit/features/001-hosted-signer-backends/contracts/ecdsa-signer-backend.md |
Contract doc for the internal backend trait and conformance expectations. |
speckit/features/001-hosted-signer-backends/contracts/configuration.md |
Operator-facing configuration contract for selecting the backend and KMS requirements. |
infra/variables.tf |
Adds Terraform variable to supply a KMS key ARN for hosted ECDSA ACK signing. |
infra/iam.tf |
Conditionally grants Secrets Manager vs KMS permissions based on KMS key configuration. |
infra/ecs.tf |
Injects GUARDIAN_ACK_ECDSA_BACKEND=aws-kms and GUARDIAN_ACK_ECDSA_KMS_KEY_ID when KMS ARN is set. |
infra/data.tf |
Skips looking up the ECDSA ACK Secrets Manager secret when KMS is configured (prod). |
docs/SERVER_AWS_DEPLOY.md |
Documents the new Terraform variable and KMS requirements for production deploys. |
docs/runbooks/secrets.md |
Adds KMS-backed ECDSA ACK signer runbook guidance, including migration implications. |
docs/PRODUCTION.md |
Documents Secrets Manager vs KMS option for ECDSA ACK signer in production. |
docs/CONFIGURATION.md |
Adds env vars and semantics for selecting/configuring the hosted ECDSA backend. |
crates/server/src/services/push_delta.rs |
Updates ACK signing call to await async ack_delta. |
crates/server/src/error.rs |
Removes the ECDSA-specific error type, consolidating on existing GuardianError variants. |
crates/server/src/ack/mod.rs |
Refactors AckRegistry construction to build Falcon/ECDSA independently and support hosted ECDSA. |
crates/server/src/ack/miden_ecdsa/signer.rs |
Converts ECDSA signer to a backend-based wrapper and makes ack_delta async. |
crates/server/src/ack/miden_ecdsa/mod.rs |
Introduces backend module and updates re-exports accordingly. |
crates/server/src/ack/miden_ecdsa/backend/mod.rs |
Defines backend trait + env selector and adds trait-plugging tests. |
crates/server/src/ack/miden_ecdsa/backend/in_memory.rs |
Implements the default in-memory backend extracted from prior signer logic. |
crates/server/src/ack/miden_ecdsa/backend/aws_kms.rs |
Implements AWS KMS backend with timeouts, startup sign probe, and DER conversion. |
crates/server/Cargo.toml |
Adds aws-sdk-kms and sha3; adds k256 for dev-only KMS signature test support. |
Cargo.lock |
Lockfile updates for new dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (84.46%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #270 +/- ##
==========================================
+ Coverage 76.61% 76.66% +0.04%
==========================================
Files 139 142 +3
Lines 25124 25475 +351
==========================================
+ Hits 19250 19530 +280
- Misses 5874 5945 +71 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/aws-deploy.sh`:
- Around line 453-465: The script currently creates a KMS key (key_id) then runs
aws kms create-alias and can exit on alias failure, orphaning the key; update
the block around aws kms create-key and aws kms create-alias to check the exit
status of create-alias and, on failure, clean up the newly-created key by
scheduling its deletion (e.g., call aws kms schedule-key-deletion --key-id
"$key_id" --pending-window-in-days 7 --region "$AWS_REGION") and/or disabling it
first, log the cleanup action, then exit non-zero; reference the
variables/key_id, alias_name and the aws kms create-key/create-alias calls so
the error path reliably cleans up the created key.
🪄 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: 0a63e948-02d0-4092-be3c-b41c0992c759
📒 Files selected for processing (13)
crates/server/src/ack/miden_ecdsa/backend/mod.rscrates/server/src/ack/miden_ecdsa/mod.rscrates/server/src/ack/miden_ecdsa/signer.rscrates/server/src/ack/mod.rsdocs/PRODUCTION.mddocs/README.mddocs/SERVER_AWS_DEPLOY.mddocs/guides/README.mddocs/guides/aws-signers/.env.exampledocs/guides/aws-signers/README.mddocs/guides/aws-signers/docker-compose.ymldocs/runbooks/secrets.mdscripts/aws-deploy.sh
✅ Files skipped from review due to trivial changes (5)
- docs/guides/aws-signers/docker-compose.yml
- docs/guides/aws-signers/README.md
- docs/README.md
- docs/guides/README.md
- docs/runbooks/secrets.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/PRODUCTION.md
- crates/server/src/ack/miden_ecdsa/backend/mod.rs
- crates/server/src/ack/mod.rs
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
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
New Features
GUARDIAN_ACK_ECDSA_BACKENDenvironment variable (defaults to in-memory)bootstrap-kms-ecdsa-key) for KMS key provisioningDocumentation