Conversation
WalkthroughA new constant for active key ID management was added to the KMIP client configuration. The KMIP secret storage logic was updated to support active root key IDs, enforce a fixed cryptographic length, and improve error handling and logging. Special handling for the "KMIPSecret" driver was removed from the versioned secret logic. Changes
Suggested labels
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
46b42e9 to
6e79344
Compare
As some KMIP doesn't support any CryptographicLength, we will revert to saving only one key in KMIP. Once we will want to support rotating secrets we will need to find a better solution Signed-off-by: jackyalbo <jacky.albo@gmail.com>
6e79344 to
5b3aa9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/util/kms/kms_kmip_storage.go (1)
331-337: Consider the architectural implications of partial rotation support.While the PR description mentions reverting to single key storage, the code still maintains rotation-related structures (active key ID, map returns). If rotation support is not currently needed, consider whether a cleaner approach would be to fully remove rotation logic until a proper solution is designed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/util/kms/kms_kmip.go(1 hunks)pkg/util/kms/kms_kmip_storage.go(8 hunks)pkg/util/kms/kms_version.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
pkg/util/kms/kms_version.go (1)
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.080Z
Learning: In pkg/backingstore/reconciler.go, the switch refactor for S3Compatible.SignatureVersion and IBMCos.SignatureVersion maintains the same behavior as the original if-else if logic - when SignatureVersion is empty or unspecified, conn.AuthMethod is left unset. There was no default to V4 in the original code.
🧬 Code Graph Analysis (1)
pkg/util/kms/kms_kmip_storage.go (2)
pkg/util/kms/kms_kmip.go (2)
NewKMIPUniqueID(17-17)NewActiveKeyID(18-18)pkg/util/kms/kms_version.go (1)
ActiveRootKey(96-96)
⏰ 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). (11)
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-azure-vault-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-cli-tests
- GitHub Check: run-admission-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-hac-test
- GitHub Check: golangci-lint
🔇 Additional comments (9)
pkg/util/kms/kms_kmip.go (1)
18-18: LGTM!The new constant follows the established naming pattern and is properly integrated with the KMIP secret storage logic.
pkg/util/kms/kms_version.go (2)
106-121: LGTM! Good refactoring to centralize KMIP-specific logic.The removal of special KMIP handling from the generic version interface is a clean architectural improvement. The encoding/decoding logic is now properly encapsulated within the KMIP storage implementation.
141-141: LGTM!Consistent with the refactoring approach - the conversion to interface map is now straightforward without special encoding.
pkg/util/kms/kms_kmip_storage.go (6)
38-40: LGTM! Good practice to define the cryptographic length as a constant.Using a constant for the 256-bit key length improves maintainability and aligns with AES-256 standards.
213-213: Good improvement to error reporting.Including the reason and message from KMIP responses will significantly help with debugging failed operations.
266-275: Verify the handling of missing active key ID.The code correctly handles the case when
NewActiveKeyIDis missing from the secret. However, ensure that this error condition is properly handled by callers, especially during initial setup or migration scenarios.
318-320: Good addition of cryptographic length validation.This validation ensures that retrieved keys match the expected 256-bit length, preventing potential security issues from malformed keys.
372-372: Good fix for the cryptographic length issue.Using the fixed constant instead of calculating from byte length addresses the main KMIP compatibility issue mentioned in the PR description.
272-272: Good security improvement in logging.Logging only the secret name instead of the entire secret object prevents potential exposure of sensitive data in logs.
Also applies to: 280-280
| activeKey := plainText[ActiveRootKey].(string) | ||
| value := plainText[activeKey].(string) |
There was a problem hiding this comment.
Add validation for required keys in the input map.
The code assumes ActiveRootKey exists in plainText and that its value is a valid key name that also exists in the map. Add validation to prevent potential panics.
Apply this diff to add validation:
-activeKey := plainText[ActiveRootKey].(string)
-value := plainText[activeKey].(string)
+activeKeyInterface, exists := plainText[ActiveRootKey]
+if !exists {
+ return secrets.NoVersion, fmt.Errorf("ActiveRootKey not found in plainText")
+}
+activeKey, ok := activeKeyInterface.(string)
+if !ok {
+ return secrets.NoVersion, fmt.Errorf("ActiveRootKey must be a string")
+}
+valueInterface, exists := plainText[activeKey]
+if !exists {
+ return secrets.NoVersion, fmt.Errorf("Active key %s not found in plainText", activeKey)
+}
+value, ok := valueInterface.(string)
+if !ok {
+ return secrets.NoVersion, fmt.Errorf("Secret value must be a string")
+}📝 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.
| activeKey := plainText[ActiveRootKey].(string) | |
| value := plainText[activeKey].(string) | |
| - activeKey := plainText[ActiveRootKey].(string) | |
| - value := plainText[activeKey].(string) | |
| + activeKeyInterface, exists := plainText[ActiveRootKey] | |
| + if !exists { | |
| + return secrets.NoVersion, fmt.Errorf("ActiveRootKey not found in plainText") | |
| + } | |
| + activeKey, ok := activeKeyInterface.(string) | |
| + if !ok { | |
| + return secrets.NoVersion, fmt.Errorf("ActiveRootKey must be a string") | |
| + } | |
| + valueInterface, exists := plainText[activeKey] | |
| + if !exists { | |
| + return secrets.NoVersion, fmt.Errorf("Active key %s not found in plainText", activeKey) | |
| + } | |
| + value, ok := valueInterface.(string) | |
| + if !ok { | |
| + return secrets.NoVersion, fmt.Errorf("Secret value must be a string") | |
| + } |
🤖 Prompt for AI Agents
In pkg/util/kms/kms_kmip_storage.go around lines 350 to 351, the code assumes
the keys ActiveRootKey and the derived activeKey exist in the plainText map
without validation, which can cause panics. Add checks to verify that
ActiveRootKey exists in plainText and that its value is a string key present in
plainText before accessing them. If either key is missing or invalid, handle the
error gracefully to prevent panics.
Explain the changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor