Support key rotation for KMIP KMS#1633
Conversation
WalkthroughThe KMIP struct in the KMS utility was updated to include three new fields: UID, name, and namespace, with the constructor and Version method adjusted to initialize and use these fields. The Version method now returns a VersionRotatingSecret instead of VersionSingleSecret. Error handling in version secret methods was improved, and secret storage logic was modified to handle the "KMIPSecret" driver differently. Secret storage and retrieval methods were updated to support a new unique identifier key based on secret ID suffixes and to use dynamic cryptographic length calculation. Additionally, new tests were added to validate KMIP key rotation functionality, including operator restart and condition verification, in the test suite. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant NooBaa
participant Operator
participant KMIP KMS
TestSuite->>NooBaa: Create NooBaa with KMIP KMS (rotation enabled)
NooBaa->>KMIP KMS: Initialize with UID, name, namespace
TestSuite->>NooBaa: Verify KMS condition type "kmip"
TestSuite->>Operator: Delete operator pod (simulate restart)
Operator->>NooBaa: Operator restarts, re-initializes KMS
NooBaa->>KMIP KMS: Synchronize and rotate key
TestSuite->>NooBaa: Verify KMS sync and key rotation conditions
TestSuite->>NooBaa: Delete NooBaa resource
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (13)
✨ 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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/util/kms/test/kmip/kms_kmip_test.go (1)
123-159: Consider adding validation for actual rotation behavior.While the test validates that conditions are set correctly, it doesn't verify that actual key rotation occurs. Consider adding validation to ensure that:
- The system name and namespace are properly stored in the KMIP struct
- Keys are actually rotated according to the schedule
- The
VersionRotatingSecretis functioning as expected#!/bin/bash # Description: Check if there are other key rotation validation patterns in the codebase # Search for key rotation tests or validation logic rg -A 10 -B 5 "VersionRotatingSecret|KeyRotate" --type go
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/util/kms/kms_kmip.go(3 hunks)pkg/util/kms/test/kmip/kms_kmip_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-kms-dev-test
- GitHub Check: run-admission-test
- GitHub Check: golangci-lint
- GitHub Check: run-operator-tests
- GitHub Check: run-cli-tests
- GitHub Check: run-hac-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-azure-vault-test
🔇 Additional comments (5)
pkg/util/kms/kms_kmip.go (2)
28-29: LGTM! Fields appropriately added for key rotation support.The addition of
nameandnsfields to track the NooBaa system name and namespace is essential for the key rotation functionality. The field names are clear and well-documented.
113-113: Version method correctly updated for key rotation.The change from
VersionSingleSecrettoVersionRotatingSecretwith the newnameandnsfields properly implements the key rotation support as described in the PR objectives.pkg/util/kms/test/kmip/kms_kmip_test.go (3)
123-128: Well-structured test setup for key rotation validation.The test configuration properly enables key rotation with
EnableKeyRotation = trueand sets an appropriate test schedule. The every-minute schedule is suitable for testing purposes.
141-149: Excellent operator restart simulation.The operator restart simulation by deleting the operator pod is crucial for validating that key rotation functionality persists across operator restarts. This addresses a key reliability concern.
153-155: Key rotation condition validation is essential.The verification of the
ConditionKMSKeyRotatecondition after operator restart confirms that the key rotation feature is working correctly. This is a critical test for the new functionality.
923a258 to
a7585d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/util/kms/kms_version.go (1)
142-147: Consider architectural alternatives to driver-specific conditional logic.While this conditional logic successfully enables KMIP key rotation, embedding driver-specific behavior in the generic version handling code introduces coupling that could complicate future maintenance.
Consider these architectural alternatives:
- Strategy Pattern: Allow drivers to specify their own secret storage strategy
- Driver Interface Extension: Add a method to the driver interface that determines the storage path
- Configuration-driven: Move this logic to driver configuration
This approach works for the current requirement but may not scale well if more drivers need special handling.
// Alternative approach - let driver determine storage path - var err error - if (v.k.driver.Name() == "KMIPSecret") { - _, err = v.k.PutSecret(key, toInterfaceMap(s), v.k.driver.SetContext()) - } else { - _, err = v.k.PutSecret(v.BackendSecretName(), toInterfaceMap(s), v.k.driver.SetContext()) - } + secretPath := v.BackendSecretName() + if storagePathProvider, ok := v.k.driver.(StoragePathProvider); ok { + secretPath = storagePathProvider.GetStoragePath(key) + } + _, err := v.k.PutSecret(secretPath, toInterfaceMap(s), v.k.driver.SetContext())
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/util/kms/kms_version.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-admission-test
- GitHub Check: run-cli-tests
- GitHub Check: run-hac-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-operator-tests
- GitHub Check: golangci-lint
🔇 Additional comments (2)
pkg/util/kms/kms_version.go (2)
47-47: LGTM: Enhanced error handling for better K8s compatibility.The addition of the "does not exist" check alongside "not found" makes the error handling more robust across different Kubernetes error message formats.
110-110: LGTM: Consistent error handling improvement.Good consistency with the error handling enhancement applied to both VersionSingleSecret and VersionRotatingSecret implementations.
376b444 to
6debea2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/util/kms/kms_kmip.go(4 hunks)pkg/util/kms/kms_kmip_storage.go(6 hunks)pkg/util/kms/kms_version.go(4 hunks)pkg/util/kms/test/kmip/kms_kmip_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/util/kms/test/kmip/kms_kmip_test.go
- pkg/util/kms/kms_kmip.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.044Z
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.
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.044Z
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.
pkg/util/kms/kms_kmip_storage.go (1)
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1636
File: pkg/backingstore/reconciler.go:718-724
Timestamp: 2025-06-25T15:28:44.044Z
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 (1)
pkg/util/kms/kms_kmip.go (2)
KMIPUniqueID(16-16)NewKMIPUniqueID(17-17)
🪛 golangci-lint (1.64.8)
pkg/util/kms/kms_version.go
124-124: ineffectual assignment to err
(ineffassign)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-kmip-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-tls-token-test
🔇 Additional comments (3)
pkg/util/kms/kms_version.go (1)
49-49: Good improvement to error handling.The extended error handling to include "does not exist" makes the code more robust against different error message formats.
Also applies to: 112-112
pkg/util/kms/kms_kmip_storage.go (2)
265-274: Well-implemented backward compatibility logic.The conditional selection between
KMIPUniqueIDandNewKMIPUniqueIDbased on the suffix properly supports both single secret (legacy) and rotating secret (new) formats during the transition period.
398-401: Suffix logic consistently applied – no further action needed.A search for “-root-master-key-backend” shows the same suffix in:
- pkg/util/kms/kms_version.go (BackendSecretName)
- pkg/util/kms/kms_kmip_storage.go (DeleteSecret)
- pkg/util/kms/test/rotate/kms_rotate_test.go
Since the pattern is used uniformly in production code and tests, the conditional cleanup logic is correct and no changes are required. Approving the delete logic as is.
| return secrets.NoVersion, err | ||
| } | ||
|
|
||
| k.secret.StringData[KMIPUniqueID] = registerRespPayload.UniqueIdentifier |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding minimum key length validation for security.
While the dynamic cryptographic length calculation is more flexible, AES requires specific key lengths (128, 192, or 256 bits). Consider validating that the key length is appropriate for AES encryption.
Add validation before line 358:
// Validate key length for AES
keyLengthBits := len(valueBytes) * 8
if keyLengthBits != 128 && keyLengthBits != 192 && keyLengthBits != 256 {
return secrets.NoVersion, fmt.Errorf("Invalid AES key length: %d bits. Expected 128, 192, or 256 bits", keyLengthBits)
}🤖 Prompt for AI Agents
In pkg/util/kms/kms_kmip_storage.go at line 358, add validation before setting
CryptographicLength to ensure the key length in bits is one of the valid AES
lengths: 128, 192, or 256. Calculate keyLengthBits as len(valueBytes) * 8 and
return an error if it does not match these values to enforce proper AES key
length requirements.
Signed-off-by: jackyalbo <jacky.albo@gmail.com>
6debea2 to
ae7408d
Compare
|
I haven't worked with KMS so not sure if I missed anything but looks good in general. LGTM. |
Explain the changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
New Features
Tests