signing and encryption keys in gcp, aws, azure kms#307
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAwsKMS, AzureKMS, and GcpKMS now each use a separate configured encryption key for encrypt/decrypt operations. Initialization, client wiring, and runtime guards were updated so missing encryption-key settings raise ValueError. ChangesDedicated KMS encryption key support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wavefront/server/packages/flo_cloud/flo_cloud/azure/key_vault.py (1)
40-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new encryption-key configuration.
enc_key_name/enc_key_versionadd new setup knobs, but the class docstring still only lists the primary signing key env vars. Please addAZURE_KEY_VAULT_ENC_KEY_NAMEas required for encrypt/decrypt andAZURE_KEY_VAULT_ENC_KEY_VERSIONas optional.🤖 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 `@wavefront/server/packages/flo_cloud/flo_cloud/azure/key_vault.py` around lines 40 - 56, Update the KeyVault class documentation to include the new encryption-key settings introduced alongside enc_key_name and enc_key_version. In the docstring for the Azure Key Vault configuration, add AZURE_KEY_VAULT_ENC_KEY_NAME as a required environment variable for encrypt/decrypt and AZURE_KEY_VAULT_ENC_KEY_VERSION as an optional one, matching the existing descriptions for the primary key settings.
🤖 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 `@wavefront/server/packages/flo_cloud/flo_cloud/aws/kms.py`:
- Line 25: AwsKMS.encrypt() and AwsKMS.decrypt() are returning the full boto3
response dict instead of the raw payload bytes, which makes them inconsistent
with FloKMS and the other providers. Update the AwsKMS methods to extract and
return the actual byte fields from the KMS response: use CiphertextBlob from
encrypt() and Plaintext from decrypt(), keeping the public behavior aligned with
the other KMS implementations.
---
Nitpick comments:
In `@wavefront/server/packages/flo_cloud/flo_cloud/azure/key_vault.py`:
- Around line 40-56: Update the KeyVault class documentation to include the new
encryption-key settings introduced alongside enc_key_name and enc_key_version.
In the docstring for the Azure Key Vault configuration, add
AZURE_KEY_VAULT_ENC_KEY_NAME as a required environment variable for
encrypt/decrypt and AZURE_KEY_VAULT_ENC_KEY_VERSION as an optional one, matching
the existing descriptions for the primary key settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43de534a-8e16-4dd9-a8dc-e56db5cadab6
📒 Files selected for processing (3)
wavefront/server/packages/flo_cloud/flo_cloud/aws/kms.pywavefront/server/packages/flo_cloud/flo_cloud/azure/key_vault.pywavefront/server/packages/flo_cloud/flo_cloud/gcp/kms.py
Summary by CodeRabbit
New Features
Bug Fixes