Skip to content

Conversation

@NicoMolinaOZ
Copy link
Contributor

@NicoMolinaOZ NicoMolinaOZ commented Jan 5, 2026

Summary

This PR enhances the security of Google Cloud KMS signer configurations stored in Redis by encrypting all sensitive configuration fields and binding encryption to storage keys using Additional Authenticated Data (AAD). By encrypting these fields and binding them to storage keys with AAD, the configuration is now protected both at rest and against tampering.

  • Backwards compatibility: Maintained compatibility with existing Redis data through proper deserialization handling

Testing Process

Checklist

  • Add a reference to related issues in the PR description.
  • Add unit tests if applicable.

Note

If you are using Relayer in your stack, consider adding your team or organization to our list of Relayer Users in the Wild!

Summary by CodeRabbit

  • New Features

    • Added Additional Authenticated Data (AAD) support for encryption operations, enabling tamper-proof encryption with context binding
    • Introduced task-local encryption context for secure credential handling
  • Security

    • Sensitive configuration fields now encrypted at rest during storage
    • Enhanced protection of Google Cloud KMS signer credentials with field-level encryption validation
    • Improved data integrity through AAD-protected encryption with backward compatibility for legacy data

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 5, 2026

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Multiple string fields in Google Cloud KMS signer configurations are converted to SecretString wrappers for sensitive data protection. The SignerConfig::GoogleCloudKms enum variant is boxed for heap allocation. New AAD-based encryption context and v2 encryption methods are added to support authenticated encryption. Repository and service layers are updated to use SecretString fields with AAD-bound serialization.

Changes

Cohort / File(s) Summary
Google Cloud KMS Signer Models
src/models/signer/config.rs, src/models/signer/mod.rs
String fields (project_id, client_id, auth URIs, location, key IDs) wrapped in SecretString. SignerConfig::GoogleCloudKms variant boxed. Custom validators added for SecretString fields and URLs. Tests updated to access SecretString values via to_str().
Repository Storage Models
src/models/signer/repository.rs
New GoogleCloudKmsSignerServiceAccountConfigStorage and GoogleCloudKmsSignerKeyConfigStorage structs with SecretString fields and serde encryption wrappers. SignerConfigStorage::GoogleCloudKms variant boxed. Conversion logic updated for boxed and SecretString types.
Request/Response Models
src/models/signer/request.rs, src/models/signer/response.rs
Request: project_id, client_id, location, key IDs converted to SecretString; GoogleCloudKms variant boxed. Response: Fields accessed via to_str() for string conversion.
Redis Signer Repository
src/repositories/signer/signer_redis.rs
Serialization/deserialization operations wrapped with EncryptionContext to bind encryption/decryption to storage keys. Encryption context applied consistently across create, read, update operations.
Google Cloud KMS Service
src/services/google_cloud_kms/mod.rs
Fields changed to SecretString; initialization logic uses to_str() to extract string values. URL construction and key path building updated to call to_str() on SecretString fields.
EVM & Solana & Stellar Signers
src/services/signer/evm/mod.rs, src/services/signer/evm/google_cloud_kms_signer.rs, src/services/signer/solana/mod.rs, src/services/signer/stellar/mod.rs
GoogleCloudKms variants boxed in EvmSigner, SolanaSigner, StellarSigner enums. Test configs updated to construct SecretString values and wrap signer types in Box::new(...).
Encryption Utilities
src/utils/encryption.rs, src/utils/encryption_context.rs, src/utils/mod.rs
New AAD-protected encryption (v2) methods added: encrypt_with_aad(), decrypt_with_aad(), decrypt_auto(). New EncryptionContext struct provides task-local AAD scoping with async/sync APIs. Error enum extended with MissingAAD and UnsupportedVersion variants.
Repository Encryption Helpers
src/utils/serde/repository_encryption.rs
Serialization/deserialization helpers for SecretVec and SecretString updated to retrieve and apply EncryptionContext. Decryption uses auto-detection for v1 (legacy) and v2 (AAD) encrypted data. Error handling added for missing context.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Suggested reviewers

  • tirumerla
  • zeljkoX
  • dylankilkenny

Poem

🐰 Hop, hop! Secrets now wrapped tight,
String fields don SecretString's protective light,
Box'd configs dance in memory heap,
AAD encryption binds secrets deep—
From Redis to Stellar, the journey's complete!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: Encrypt google kms key' is related to the main changes (encrypting Google KMS configuration), but is vague and uses non-standard casing, making it less clear than it could be.
Description check ✅ Passed The PR description includes a Summary section explaining the changes and their security benefits, but the Testing Process section is empty and checklist items remain unchecked without providing related issue references.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch encrypt-google-kms-key

Comment @coderabbitai help to get the list of available commands and usage tips.

@NicoMolinaOZ NicoMolinaOZ changed the title Encrypt google kms key fix: Encrypt google kms key Jan 5, 2026
@NicoMolinaOZ NicoMolinaOZ marked this pull request as ready for review January 6, 2026 14:19
@NicoMolinaOZ NicoMolinaOZ requested a review from a team as a code owner January 6, 2026 14:19
@codecov
Copy link

codecov bot commented Jan 6, 2026

Codecov Report

❌ Patch coverage is 97.65432% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.34%. Comparing base (0f9b288) to head (f4fbc90).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/utils/encryption.rs 95.92% 16 Missing ⚠️
src/models/signer/mod.rs 98.88% 1 Missing ⚠️
src/services/google_cloud_kms/mod.rs 96.42% 1 Missing ⚠️
src/services/signer/stellar/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
Flag Coverage Δ
ai 0.31% <0.00%> (-0.01%) ⬇️
dev 92.32% <97.65%> (+0.01%) ⬆️
properties 0.02% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@            Coverage Diff             @@
##             main     #603      +/-   ##
==========================================
+ Coverage   92.32%   92.34%   +0.01%     
==========================================
  Files         258      259       +1     
  Lines       94352    95013     +661     
==========================================
+ Hits        87112    87740     +628     
- Misses       7240     7273      +33     
Files with missing lines Coverage Δ
src/api/controllers/api_key.rs 84.11% <100.00%> (+0.45%) ⬆️
src/models/signer/config.rs 92.74% <100.00%> (+0.04%) ⬆️
src/models/signer/repository.rs 89.67% <100.00%> (+0.06%) ⬆️
src/models/signer/request.rs 93.53% <100.00%> (ø)
src/models/signer/response.rs 99.51% <100.00%> (+0.01%) ⬆️
src/services/signer/evm/google_cloud_kms_signer.rs 95.71% <100.00%> (ø)
src/services/signer/evm/mod.rs 97.48% <100.00%> (ø)
src/services/signer/solana/mod.rs 93.34% <100.00%> (ø)
src/utils/encryption_context.rs 100.00% <100.00%> (ø)
src/utils/serde/repository_encryption.rs 99.58% <100.00%> (+0.15%) ⬆️
... and 4 more

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/models/signer/response.rs (1)

122-138: Consider simplifying the to_str().clone() pattern.

The pattern (*field.to_str()).clone() is verbose. If to_str() returns &str (which it typically does for SecretString), you can simplify to field.to_str().to_string() or field.to_str().to_owned().

🔎 Suggested simplification
             service_account: GoogleCloudKmsSignerServiceAccountResponseConfig {
-                    project_id: (*c.service_account.project_id.to_str()).clone(),
-                    client_id: (*c.service_account.client_id.to_str()).clone(),
-                    auth_uri: (*c.service_account.auth_uri.to_str()).clone(),
-                    token_uri: (*c.service_account.token_uri.to_str()).clone(),
-                    auth_provider_x509_cert_url: (*c
-                        .service_account
-                        .auth_provider_x509_cert_url
-                        .to_str())
-                    .clone(),
-                    client_x509_cert_url: (*c.service_account.client_x509_cert_url.to_str())
-                        .clone(),
-                    universe_domain: (*c.service_account.universe_domain.to_str()).clone(),
+                    project_id: c.service_account.project_id.to_str().to_string(),
+                    client_id: c.service_account.client_id.to_str().to_string(),
+                    auth_uri: c.service_account.auth_uri.to_str().to_string(),
+                    token_uri: c.service_account.token_uri.to_str().to_string(),
+                    auth_provider_x509_cert_url: c.service_account.auth_provider_x509_cert_url.to_str().to_string(),
+                    client_x509_cert_url: c.service_account.client_x509_cert_url.to_str().to_string(),
+                    universe_domain: c.service_account.universe_domain.to_str().to_string(),
                 },
                 key: GoogleCloudKmsSignerKeyResponseConfig {
-                    location: (*c.key.location.to_str()).clone(),
-                    key_ring_id: (*c.key.key_ring_id.to_str()).clone(),
-                    key_id: (*c.key.key_id.to_str()).clone(),
+                    location: c.key.location.to_str().to_string(),
+                    key_ring_id: c.key.key_ring_id.to_str().to_string(),
+                    key_id: c.key.key_id.to_str().to_string(),
                     key_version: c.key.key_version,
                 },
src/models/signer/config.rs (1)

363-364: Prefer header imports over function-level imports.

The use crate::models::SecretString; statement is placed inside the function body. As per the coding guidelines, prefer header imports over function-level imports. Move this import to the top of the file with other imports from the crate::models module.

🔎 Suggested fix

Remove the function-level import at lines 363-364 and add SecretString to the existing import at line 12-20:

 use crate::{
     config::ConfigFileError,
     models::signer::{
         AwsKmsSignerConfig, CdpSignerConfig, GoogleCloudKmsSignerConfig,
         GoogleCloudKmsSignerKeyConfig, GoogleCloudKmsSignerServiceAccountConfig, LocalSignerConfig,
         Signer, SignerConfig, TurnkeySignerConfig, VaultSignerConfig, VaultTransitSignerConfig,
     },
-    models::PlainOrEnvValue,
+    models::{PlainOrEnvValue, SecretString},
 };

Then remove lines 363-364 from the function body.

src/models/signer/repository.rs (1)

550-682: Consider adding test coverage for GoogleCloudKms storage conversions.

The test suite covers LocalSignerConfig, AwsKmsSignerConfig, and CdpSignerConfig conversions but lacks tests for GoogleCloudKmsSignerConfigStorage. Given the complexity of the config (many SecretString fields, nested structs) and the backwards compatibility requirements mentioned in the PR, a round-trip conversion test similar to test_cdp_config_storage_conversion would help catch deserialization issues.

Would you like me to generate a test case for the GoogleCloudKms storage conversion?

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between caa5803 and 07defbb.

📒 Files selected for processing (15)
  • src/models/signer/config.rs
  • src/models/signer/mod.rs
  • src/models/signer/repository.rs
  • src/models/signer/request.rs
  • src/models/signer/response.rs
  • src/repositories/signer/signer_redis.rs
  • src/services/google_cloud_kms/mod.rs
  • src/services/signer/evm/google_cloud_kms_signer.rs
  • src/services/signer/evm/mod.rs
  • src/services/signer/solana/mod.rs
  • src/services/signer/stellar/mod.rs
  • src/utils/encryption.rs
  • src/utils/encryption_context.rs
  • src/utils/mod.rs
  • src/utils/serde/repository_encryption.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (.cursor/rules/rust_standards.mdc)

**/*.rs: Follow official Rust style guidelines using rustfmt (edition = 2021, max_width = 100)
Follow Rust naming conventions: snake_case for functions/variables, PascalCase for types, SCREAMING_SNAKE_CASE for constants and statics
Order imports alphabetically and group by: std, external crates, local crates
Include relevant doc comments (///) on public functions, structs, and modules. Use comments for 'why', not 'what'. Avoid redundant doc comments
Document lifetime parameters when they're not obvious
Avoid unsafe code unless absolutely necessary; justify its use in comments
Write idiomatic Rust: Prefer Result over panic, use ? operator for error propagation
Avoid unwrap; handle errors explicitly with Result and custom Error types for all async operations
Prefer header imports over function-level imports of dependencies
Prefer borrowing over cloning when possible
Use &str instead of &String for function parameters
Use &[T] instead of &Vec for function parameters
Avoid unnecessary .clone() calls
Use Vec::with_capacity() when size is known in advance
Use explicit lifetimes only when necessary; infer where possible
Always use Tokio runtime for async code. Await futures eagerly; avoid blocking calls in async contexts
Streams: Use futures::StreamExt for processing streams efficiently
Always use serde for JSON serialization/deserialization with #[derive(Serialize, Deserialize)]
Use type aliases for complex types to improve readability
Implement common traits (Debug, Clone, PartialEq) where appropriate, using derive macros when possible
Implement Display for user-facing types
Prefer defining traits when implementing services to make mocking and testing easier
For tests, prefer existing utils for creating mocks instead of duplicating code
Use assert_eq! and assert! macros appropriately
Keep tests minimal, deterministic, and fast
Use tracing for structured logging, e.g., tracing::info! for request and error logs
When optimizing, prefer clarity first, then performance
M...

Files:

  • src/utils/mod.rs
  • src/utils/encryption_context.rs
  • src/repositories/signer/signer_redis.rs
  • src/utils/serde/repository_encryption.rs
  • src/models/signer/response.rs
  • src/services/google_cloud_kms/mod.rs
  • src/models/signer/request.rs
  • src/models/signer/config.rs
  • src/models/signer/repository.rs
  • src/utils/encryption.rs
  • src/models/signer/mod.rs
  • src/services/signer/solana/mod.rs
  • src/services/signer/stellar/mod.rs
  • src/services/signer/evm/google_cloud_kms_signer.rs
  • src/services/signer/evm/mod.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: sammccord
Repo: OpenZeppelin/openzeppelin-relayer PR: 457
File: src/models/signer/mod.rs:184-203
Timestamp: 2025-09-11T22:32:27.610Z
Learning: SecretString fields in signer configurations do not require explicit serialize_with redaction attributes, as SecretString handles redaction automatically during serialization. Only SecretVec<u8> fields like LocalSignerConfig's raw_key require explicit serialize_with = "serialize_secret_redacted".
📚 Learning: 2025-09-11T22:32:27.610Z
Learnt from: sammccord
Repo: OpenZeppelin/openzeppelin-relayer PR: 457
File: src/models/signer/mod.rs:184-203
Timestamp: 2025-09-11T22:32:27.610Z
Learning: SecretString fields in signer configurations do not require explicit serialize_with redaction attributes, as SecretString handles redaction automatically during serialization. Only SecretVec<u8> fields like LocalSignerConfig's raw_key require explicit serialize_with = "serialize_secret_redacted".

Applied to files:

  • src/repositories/signer/signer_redis.rs
  • src/utils/serde/repository_encryption.rs
  • src/models/signer/response.rs
  • src/services/google_cloud_kms/mod.rs
  • src/models/signer/request.rs
  • src/models/signer/config.rs
  • src/models/signer/repository.rs
  • src/models/signer/mod.rs
  • src/services/signer/solana/mod.rs
  • src/services/signer/evm/google_cloud_kms_signer.rs
📚 Learning: 2025-11-05T12:25:41.714Z
Learnt from: MCarlomagno
Repo: OpenZeppelin/openzeppelin-relayer PR: 479
File: src/utils/encryption.rs:88-90
Timestamp: 2025-11-05T12:25:41.714Z
Learning: In the openzeppelin-relayer codebase with aes-gcm 0.10, avoid suggesting Key::from_slice() for aes_gcm::Key construction as it generates clippy warnings. The preferred pattern is to use Key::from_iter() with an iterator, e.g., `Key::<Aes256Gcm>::from_iter(key_array.iter().copied())` after validating the key length.

Applied to files:

  • src/utils/encryption.rs
🧬 Code graph analysis (12)
src/repositories/signer/signer_redis.rs (1)
src/utils/encryption_context.rs (1)
  • with_aad_sync (82-87)
src/utils/serde/repository_encryption.rs (3)
src/utils/base64.rs (2)
  • base64_decode (8-10)
  • base64_encode (5-7)
src/utils/encryption.rs (5)
  • decrypt_sensitive_field_auto (399-429)
  • encrypt_sensitive_field_with_aad (373-393)
  • new (63-67)
  • serde_json (353-353)
  • serde_json (414-414)
src/utils/encryption_context.rs (2)
  • get (96-98)
  • with_aad_sync (82-87)
src/services/google_cloud_kms/mod.rs (1)
src/models/secret_string.rs (1)
  • to_str (72-78)
src/models/signer/request.rs (1)
src/services/google_cloud_kms/mod.rs (1)
  • new (133-157)
src/models/signer/config.rs (2)
src/services/google_cloud_kms/mod.rs (1)
  • new (133-157)
src/models/secret_string.rs (2)
  • to_str (72-78)
  • as_str (57-66)
src/models/signer/repository.rs (1)
src/services/google_cloud_kms/mod.rs (1)
  • new (133-157)
src/utils/encryption.rs (1)
src/utils/base64.rs (2)
  • base64_encode (5-7)
  • base64_decode (8-10)
src/models/signer/mod.rs (1)
src/services/google_cloud_kms/mod.rs (1)
  • new (133-157)
src/services/signer/solana/mod.rs (2)
src/services/google_cloud_kms/mod.rs (1)
  • new (133-157)
src/services/signer/solana/google_cloud_kms_signer.rs (1)
  • new (45-49)
src/services/signer/stellar/mod.rs (1)
src/services/signer/stellar/google_cloud_kms_signer.rs (1)
  • new (39-43)
src/services/signer/evm/google_cloud_kms_signer.rs (1)
src/services/google_cloud_kms/mod.rs (1)
  • new (133-157)
src/services/signer/evm/mod.rs (2)
src/services/google_cloud_kms/mod.rs (1)
  • new (133-157)
src/services/signer/evm/google_cloud_kms_signer.rs (1)
  • new (39-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: clippy
  • GitHub Check: Unit Tests
  • GitHub Check: msrv
  • GitHub Check: Properties Tests
🔇 Additional comments (45)
src/utils/mod.rs (1)

44-46: LGTM!

The new encryption_context module is properly declared and re-exported, following the established pattern in this file.

src/services/signer/stellar/mod.rs (1)

61-61: LGTM!

Boxing GoogleCloudKmsSigner reduces the StellarSigner enum size and aligns with the consistent pattern applied across EVM, Solana, and Stellar signer modules. The autoderef mechanism ensures method calls continue to work transparently.

Also applies to: 158-158

src/services/signer/evm/mod.rs (2)

284-284: LGTM!

The boxing of GoogleCloudKmsSigner in the EVM module is consistent with the pattern applied across all signer modules. Construction and pattern matching are handled correctly.

Also applies to: 398-398


731-750: Test properly updated for new types.

The test correctly uses Box::new() for the GoogleCloudKms variant and SecretString::new() for all sensitive configuration fields, matching the new type signatures.

src/repositories/signer/signer_redis.rs (4)

119-130: AAD-bound deserialization correctly implemented for batch fetch.

The deserialization now correctly binds decryption to the storage key using EncryptionContext::with_aad_sync. This ensures that encrypted data can only be decrypted when associated with its original storage location, protecting against data tampering or key swapping.


195-198: AAD-bound serialization correctly implemented for create.

The encryption is properly bound to the storage key, ensuring the ciphertext is authenticated against its storage location.


227-230: AAD-bound deserialization correctly implemented for get_by_id.

Consistent with the batch fetch implementation, ensuring authenticated decryption.


288-291: AAD-bound serialization correctly implemented for update.

Consistent with the create implementation. Both create and update paths now use the same AAD-bound encryption pattern.

src/services/signer/solana/mod.rs (3)

73-73: LGTM!

The boxing of GoogleCloudKmsSigner in the Solana module is consistent with the pattern applied across all signer modules. The factory method correctly wraps the signer in Box::new().

Also applies to: 317-319


460-479: Test properly updated for new types.

The test correctly constructs SignerConfig::GoogleCloudKms with Box::new() and uses SecretString::new() for all sensitive configuration fields.


582-601: Second test also properly updated.

Consistent with the first test, this test correctly uses the new boxed and SecretString-wrapped types.

src/utils/serde/repository_encryption.rs (5)

1-14: Excellent module documentation.

The documentation clearly explains the AAD requirements for serialization vs. deserialization, and the backwards compatibility strategy. This is critical for maintainers to understand the encryption migration path.


38-44: Serialization now enforces AAD context.

Requiring EncryptionContext for serialization ensures all new data is encrypted with AAD binding. The error message is clear and actionable.


59-63: Deserialization supports backwards compatibility.

Using EncryptionContext::get() (optional) with decrypt_sensitive_field_auto allows decryption of both legacy (v1) and new (v2) data. This is the correct approach for migration.


496-515: Good test coverage for missing context error.

This test verifies that serialization fails with a clear error when EncryptionContext is not set, which is essential for catching configuration issues early.


86-90: Consistent pattern across all serialize/deserialize helpers.

The same AAD pattern is correctly applied to SecretString and Option<SecretString> variants, maintaining consistency across the module.

Also applies to: 116-120, 142-146, 178-182

src/models/signer/config.rs (3)

389-414: LGTM!

The conversion logic correctly wraps all sensitive Google Cloud KMS configuration fields in SecretString, ensuring they will be encrypted at rest. The key_version remains as u32 since it's not sensitive.


441-443: LGTM!

Boxing GoogleCloudKmsSignerConfig is a good choice to reduce the size of the SignerConfig enum, as the GCP config contains many fields and wrapping them in SecretString increases the struct size.


675-679: LGTM!

Test assertions correctly access SecretString fields using to_str().as_str() pattern, which properly extracts the underlying string for comparison.

src/utils/encryption_context.rs (3)

22-31: LGTM!

The use of tokio::task_local! is the correct choice for async contexts, ensuring the AAD context follows the task rather than the thread. The EncryptionContext struct provides a clean API for setting and retrieving AAD during encryption/decryption operations.


53-87: LGTM!

The dual API (with_aad for async and with_aad_sync for sync) is well-designed. The with_aad_sync variant is particularly useful for serde serializers that need AAD context but run synchronously within an async task.


106-219: LGTM!

Excellent test coverage including context propagation, behavior outside scopes, nested context shadowing, and sync/async interop. The tests validate the core invariants of the task-local context mechanism.

src/models/signer/request.rs (2)

178-205: LGTM!

The From implementations correctly wrap all sensitive Google Cloud KMS fields in SecretString when converting from request models to domain models. This ensures sensitive data is protected as soon as it enters the system.

Based on learnings, SecretString handles redaction automatically during serialization, so no explicit serialize_with attributes are needed.


264-269: LGTM!

The SignerConfig::GoogleCloudKms variant correctly boxes the configuration, consistent with the domain model changes in mod.rs.

src/services/signer/evm/google_cloud_kms_signer.rs (1)

155-178: LGTM!

Test configuration correctly uses SecretString wrappers for all Google Cloud KMS configuration fields. The test setup properly reflects the updated type definitions.

src/utils/encryption.rs (6)

34-37: LGTM!

The new error variants MissingAAD and UnsupportedVersion provide clear, specific error messages for AAD-related failures, improving debuggability.


158-190: LGTM!

The encrypt_with_aad implementation correctly uses AES-256-GCM with Additional Authenticated Data. The Payload struct properly binds the AAD to the ciphertext, and version 2 is correctly set to distinguish from v1 encrypted data.


192-241: LGTM!

The decrypt_with_aad implementation correctly requires version 2 and uses the AAD for authenticated decryption. If the AAD doesn't match what was used during encryption, GCM will fail authentication, preventing ciphertext swap attacks.


243-263: LGTM!

The decrypt_auto method provides excellent backwards compatibility:

  • Version 1 data decrypts without AAD (legacy support)
  • Version 2 data requires AAD, failing clearly with MissingAAD if not provided
  • Unknown versions are rejected with a specific error

This enables safe migration from v1 to v2 encrypted data.


369-429: LGTM!

The high-level functions encrypt_sensitive_field_with_aad and decrypt_sensitive_field_auto provide a clean API that:

  • Handles JSON serialization and base64 encoding
  • Supports fallback mode when encryption is not configured
  • Maintains backwards compatibility with v1 encrypted data

762-786: LGTM!

The test_ciphertext_swap_prevention test explicitly validates the core security property: encrypted data bound to one AAD cannot be decrypted with a different AAD. This test documents and enforces the protection against ciphertext swap attacks.

src/services/google_cloud_kms/mod.rs (4)

132-156: LGTM!

The credentials JSON construction correctly extracts string values from SecretString fields using .to_str().to_string(). The Zeroizing<String> returned by to_str() ensures sensitive data is zeroed when dropped, and converting to String for the JSON is appropriate for the GCP credentials builder.


196-203: LGTM!

The get_base_url method correctly handles SecretString by calling .to_str() to get a Zeroizing<String>, then dereferencing to access the inner string value. The clone on line 199 is necessary since we need to return an owned String.


252-261: LGTM!

The get_key_path method correctly uses the deref pattern (*self.config.key.location.to_str()) to access the underlying string values for formatting. This is consistent with the get_base_url approach.


535-556: LGTM!

The create_test_config helper correctly wraps all configuration fields in SecretString, ensuring test configurations match the updated type definitions.

src/models/signer/mod.rs (6)

210-266: LGTM!

The GoogleCloudKmsSignerServiceAccountConfig struct correctly uses SecretString for all sensitive fields. The doc comment at lines 211-213 clearly explains the rationale for encrypting these fields at rest in Redis.

Each field has appropriate validation:

  • project_id, client_id, universe_domain: non-empty validation via validate_secret_string
  • auth_uri, token_uri, cert URLs: URL validation via validate_secret_url

268-290: LGTM!

The GoogleCloudKmsSignerKeyConfig correctly protects key identifiers (location, key_ring_id, key_id) as SecretString. The doc comment at lines 269-271 explains the security rationale: preventing attackers from modifying key identifiers to point to a different key.


309-318: LGTM!

The validate_secret_url function correctly validates that a SecretString contains a valid URL. It uses the as_str callback pattern to safely access the secret content without exposing it, and provides distinct error codes for empty vs. invalid URLs.


383-383: LGTM!

The SignerConfig::GoogleCloudKms variant correctly uses Box<GoogleCloudKmsSignerConfig> to reduce enum size, and the validation correctly uses config.as_ref() to validate the boxed configuration.

Also applies to: 421-421


828-857: LGTM!

The test_valid_google_cloud_kms_signer test correctly constructs the configuration using Box::new and SecretString wrappers for all fields. The test validates that proper configuration passes validation.


859-894: LGTM!

The test_invalid_google_cloud_kms_urls test verifies that URL validation works correctly for SecretString fields, catching invalid URLs in the auth_uri field.

src/models/signer/repository.rs (4)

45-45: LGTM on boxing the GoogleCloudKms variant.

Boxing the large nested config struct is appropriate here to avoid bloating the SignerConfigStorage enum size. The conversion implementations correctly handle the Box indirection.


147-200: Encryption pattern is consistent with existing storage configs.

The use of serialize_secret_string/deserialize_secret_string for encryption at rest aligns with the AAD binding approach described in the PR objectives and matches the pattern used by other storage configs in this file (VaultSignerConfigStorage, CdpSignerConfigStorage, etc.).


202-224: Good documentation of the security rationale.

The doc comment clearly explains that encryption protects integrity (preventing modification of key identifiers), not just confidentiality. The implementation correctly applies SecretString to all string fields while leaving key_version: u32 unencrypted.


467-469: Conversion implementations correctly handle Box indirection.

Both From implementations properly dereference the source Box, convert the inner config, and wrap the result in a new Box. The symmetry between the two conversions is correct.

Also applies to: 485-487

Copy link
Contributor

@LuisUrrutia LuisUrrutia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@tirumerla tirumerla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks @NicoMolinaOZ for working on this

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances security for Google Cloud KMS signer configurations by implementing field-level encryption with Additional Authenticated Data (AAD) binding. The changes encrypt all sensitive configuration fields when storing to Redis and bind the encryption to storage keys to prevent tampering and ciphertext swap attacks. The implementation maintains backward compatibility with existing unencrypted and v1 encrypted data.

Changes:

  • Added AAD-based encryption infrastructure using task-local context for automatic encryption key binding
  • Updated Google Cloud KMS configuration models to encrypt all sensitive string fields including key identifiers
  • Enhanced serialization/deserialization utilities to support v1 (legacy) and v2 (AAD-protected) encrypted data with plain text fallback

Reviewed changes

Copilot reviewed 19 out of 21 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/utils/encryption_context.rs New module providing task-local AAD context for encryption operations
src/utils/encryption.rs Extended encryption utilities with AAD support (v2) and auto-detection for backward compatibility
src/utils/serde/repository_encryption.rs Updated serde helpers to use AAD-based encryption with backward compatibility
src/models/signer/mod.rs Changed Google Cloud KMS config fields from String to SecretString with validation
src/models/signer/repository.rs Added encryption annotations to all Google Cloud KMS sensitive fields
src/repositories/signer/signer_redis.rs Wrapped serialization/deserialization with EncryptionContext for AAD binding
src/repositories/api_key/api_key_redis.rs Wrapped API key operations with EncryptionContext
src/services/google_cloud_kms/mod.rs Updated to extract values from SecretString fields and improved HTTP protocol handling
src/utils/url.rs Fixed typo in comment ("unparseable" → "unparsable")
plugins/pnpm-lock.yaml Formatting changes (whitespace cleanup)
openapi.json Added missing newline at end of file
.github/workflows/ci.yaml Added disk space cleanup step in CI workflow
Files not reviewed (1)
  • plugins/pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@zeljkoX zeljkoX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks for working on this one!

@NicoMolinaOZ NicoMolinaOZ merged commit e6faf5a into main Jan 15, 2026
37 of 42 checks passed
@NicoMolinaOZ NicoMolinaOZ deleted the encrypt-google-kms-key branch January 15, 2026 15:36
@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants