From 529fd9d661b7b5ebeb613f159d51e878f4419513 Mon Sep 17 00:00:00 2001 From: vishesh92 Date: Wed, 11 Feb 2026 13:26:36 +0530 Subject: [PATCH] fixups --- .../command/admin/kms/RotateKMSKeyCmd.java | 4 ++ .../user/kms/hsm/AddHSMProfileCmd.java | 2 +- .../org/apache/cloudstack/kms/HSMProfile.java | 20 ++++----- .../org/apache/cloudstack/kms/KMSManager.java | 9 ---- .../pkcs11/PKCS11HSMProviderTest.java | 12 ++--- .../apache/cloudstack/kms/KMSManagerImpl.java | 44 +++++++++---------- .../cloudstack/kms/KMSManagerImplHSMTest.java | 7 --- .../kms/KMSManagerImplKeyCreationTest.java | 21 ++++----- 8 files changed, 49 insertions(+), 70 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/kms/RotateKMSKeyCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/kms/RotateKMSKeyCmd.java index 7ab47a65351..5b6125703d5 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/kms/RotateKMSKeyCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/kms/RotateKMSKeyCmd.java @@ -28,6 +28,7 @@ import org.apache.cloudstack.api.ServerApiException; import org.apache.cloudstack.api.response.AsyncJobResponse; import org.apache.cloudstack.api.response.HSMProfileResponse; import org.apache.cloudstack.api.response.KMSKeyResponse; +import org.apache.cloudstack.api.response.SuccessResponse; import org.apache.cloudstack.framework.kms.KMSException; import org.apache.cloudstack.kms.KMSKey; import org.apache.cloudstack.kms.KMSManager; @@ -81,6 +82,9 @@ public class RotateKMSKeyCmd extends BaseAsyncCmd { public void execute() { try { kmsManager.rotateKMSKey(this); + SuccessResponse response = new SuccessResponse(); + response.setResponseName(getCommandName()); + setResponseObject(response); } catch (KMSException e) { throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to rotate KMS key: " + e.getMessage()); diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/kms/hsm/AddHSMProfileCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/kms/hsm/AddHSMProfileCmd.java index 4aad0811aff..eefed29e04e 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/kms/hsm/AddHSMProfileCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/kms/hsm/AddHSMProfileCmd.java @@ -22,6 +22,7 @@ import com.cloud.exception.InsufficientCapacityException; import com.cloud.exception.NetworkRuleConflictException; import com.cloud.exception.ResourceAllocationException; import com.cloud.exception.ResourceUnavailableException; +import com.cloud.utils.StringUtils; import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ApiErrorCode; @@ -37,7 +38,6 @@ import org.apache.cloudstack.framework.kms.KMSException; import org.apache.cloudstack.kms.HSMProfile; import org.apache.cloudstack.kms.KMSManager; import org.apache.commons.collections.MapUtils; -import org.apache.commons.lang.StringUtils; import javax.inject.Inject; import java.util.Collection; diff --git a/api/src/main/java/org/apache/cloudstack/kms/HSMProfile.java b/api/src/main/java/org/apache/cloudstack/kms/HSMProfile.java index c9d8d1a9a54..68071af4a98 100644 --- a/api/src/main/java/org/apache/cloudstack/kms/HSMProfile.java +++ b/api/src/main/java/org/apache/cloudstack/kms/HSMProfile.java @@ -17,27 +17,27 @@ package org.apache.cloudstack.kms; -import java.util.Date; - import org.apache.cloudstack.api.Identity; import org.apache.cloudstack.api.InternalIdentity; +import java.util.Date; + public interface HSMProfile extends Identity, InternalIdentity { String getName(); - + String getProtocol(); - + Long getAccountId(); - + Long getDomainId(); - + Long getZoneId(); - + String getVendorName(); - + boolean isEnabled(); - + Date getCreated(); - + Date getRemoved(); } diff --git a/api/src/main/java/org/apache/cloudstack/kms/KMSManager.java b/api/src/main/java/org/apache/cloudstack/kms/KMSManager.java index ee64543b702..243569a06f5 100644 --- a/api/src/main/java/org/apache/cloudstack/kms/KMSManager.java +++ b/api/src/main/java/org/apache/cloudstack/kms/KMSManager.java @@ -161,15 +161,6 @@ public interface KMSManager extends Manager, Configurable { */ KMSProvider getKMSProvider(String name); - /** - * Get the configured provider for a zone - * - * @param zoneId the zone ID (null for global default) - * @return the configured provider - * @throws KMSException if no provider configured - */ - KMSProvider getKMSProviderForZone(Long zoneId) throws KMSException; - /** * Check if KMS is enabled for a zone * diff --git a/plugins/kms/pkcs11/src/test/java/org/apache/cloudstack/kms/provider/pkcs11/PKCS11HSMProviderTest.java b/plugins/kms/pkcs11/src/test/java/org/apache/cloudstack/kms/provider/pkcs11/PKCS11HSMProviderTest.java index 405fe3899be..1434e39b6c6 100644 --- a/plugins/kms/pkcs11/src/test/java/org/apache/cloudstack/kms/provider/pkcs11/PKCS11HSMProviderTest.java +++ b/plugins/kms/pkcs11/src/test/java/org/apache/cloudstack/kms/provider/pkcs11/PKCS11HSMProviderTest.java @@ -120,7 +120,7 @@ public class PKCS11HSMProviderTest { public void testLoadProfileConfig_DecryptsSensitiveValues() { // Setup: Profile details with encrypted pin HSMProfileDetailsVO detail1 = mock(HSMProfileDetailsVO.class); - when(detail1.getName()).thenReturn("library_path"); + when(detail1.getName()).thenReturn("library"); when(detail1.getValue()).thenReturn("/path/to/lib.so"); HSMProfileDetailsVO detail2 = mock(HSMProfileDetailsVO.class); @@ -140,7 +140,7 @@ public class PKCS11HSMProviderTest { // Verify assertNotNull("Config should not be null", config); assertEquals(3, config.size()); - assertEquals("/path/to/lib.so", config.get("library_path")); + assertEquals("/path/to/lib.so", config.get("library")); // Note: In real code, DBEncryptionUtil.decrypt would be called // Here we just verify the structure is correct assertTrue("Config should contain pin", config.containsKey("pin")); @@ -184,7 +184,7 @@ public class PKCS11HSMProviderTest { @Test public void testIsSensitiveKey_IdentifiesNonSensitiveKeys() { // Test - assertFalse(provider.isSensitiveKey("library_path")); + assertFalse(provider.isSensitiveKey("library")); assertFalse(provider.isSensitiveKey("slot_id")); assertFalse(provider.isSensitiveKey("endpoint")); assertFalse(provider.isSensitiveKey("max_sessions")); @@ -232,7 +232,7 @@ public class PKCS11HSMProviderTest { public void testLoadProfileConfig_CachesConfiguration() { // Setup HSMProfileDetailsVO detail = mock(HSMProfileDetailsVO.class); - when(detail.getName()).thenReturn("library_path"); + when(detail.getName()).thenReturn("library"); when(detail.getValue()).thenReturn("/path/to/lib.so"); when(hsmProfileDetailsDao.listByProfileId(testProfileId)).thenReturn(Arrays.asList(detail)); @@ -251,7 +251,7 @@ public class PKCS11HSMProviderTest { public void testGetSessionPool_CreatesPoolForNewProfile() { // Setup HSMProfileDetailsVO detail = mock(HSMProfileDetailsVO.class); - when(detail.getName()).thenReturn("library_path"); + when(detail.getName()).thenReturn("library"); when(detail.getValue()).thenReturn("/path/to/lib.so"); when(hsmProfileDetailsDao.listByProfileId(testProfileId)).thenReturn(Arrays.asList(detail)); @@ -270,7 +270,7 @@ public class PKCS11HSMProviderTest { public void testGetSessionPool_ReusesPoolForSameProfile() { // Setup HSMProfileDetailsVO detail = mock(HSMProfileDetailsVO.class); - when(detail.getName()).thenReturn("library_path"); + when(detail.getName()).thenReturn("library"); when(detail.getValue()).thenReturn("/path/to/lib.so"); when(hsmProfileDetailsDao.listByProfileId(testProfileId)).thenReturn(Arrays.asList(detail)); diff --git a/server/src/main/java/org/apache/cloudstack/kms/KMSManagerImpl.java b/server/src/main/java/org/apache/cloudstack/kms/KMSManagerImpl.java index 09fd6838a0e..6d6ca131188 100644 --- a/server/src/main/java/org/apache/cloudstack/kms/KMSManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/kms/KMSManagerImpl.java @@ -129,13 +129,6 @@ public class KMSManagerImpl extends ManagerBase implements KMSManager, Pluggable return provider; } - @Override - public KMSProvider getKMSProviderForZone(Long zoneId) throws KMSException { - // Default to database provider for backward compatibility - // HSM-based keys will use provider from HSM profile's protocol field - return getKMSProvider("database"); - } - @Override public boolean isKmsEnabled(Long zoneId) { if (zoneId == null) { @@ -147,8 +140,7 @@ public class KMSManagerImpl extends ManagerBase implements KMSManager, Pluggable /** * Internal method to rotate a KEK (create new version and update KMS key state) */ - String rotateKek(KMSKeyVO kmsKey, String oldKekLabel, - String newKekLabel, int keyBits, HSMProfileVO newHSMProfile) throws KMSException { + String rotateKek(KMSKeyVO kmsKey, String oldKekLabel, String newKekLabel, int keyBits, HSMProfileVO newHSMProfile) throws KMSException { validateKmsEnabled(kmsKey.getZoneId()); @@ -240,7 +232,7 @@ public class KMSManagerImpl extends ManagerBase implements KMSManager, Pluggable throw KMSException.invalidParameter("HSM Profile not found"); } - // Determine provider from HSM profile or default to database + // Determine provider from HSM profile KMSProvider provider = getKMSProvider(profile.getProtocol()); // Generate unique KEK label @@ -373,16 +365,17 @@ public class KMSManagerImpl extends ManagerBase implements KMSManager, Pluggable throw KMSException.kekNotFound("KMS key not found for wrapped key: " + wrappedKeyId); } - KMSProvider provider = getKMSProvider(kmsKey.getProviderName()); - // Try the specific version first if available if (wrappedVO.getKekVersionId() != null) { KMSKekVersionVO version = kmsKekVersionDao.findById(wrappedVO.getKekVersionId()); if (version != null && version.getStatus() != KMSKekVersionVO.Status.Archived) { try { + HSMProfileVO hsmProfile = hsmProfileDao.findById(version.getHsmProfileId()); + KMSProvider provider = getKMSProvider(hsmProfile.getProtocol()); + WrappedKey wrapped = new WrappedKey(version.getKekLabel(), kmsKey.getPurpose(), kmsKey.getAlgorithm(), wrappedVO.getWrappedBlob(), - kmsKey.getProviderName(), wrappedVO.getCreated(), kmsKey.getZoneId()); + hsmProfile.getProtocol(), wrappedVO.getCreated(), kmsKey.getZoneId()); // Pass HSM profile ID from version byte[] dek = retryOperation(() -> provider.unwrapKey(wrapped, version.getHsmProfileId())); logger.debug("Successfully unwrapped key {} with KEK version {}", wrappedKeyId, @@ -398,6 +391,9 @@ public class KMSManagerImpl extends ManagerBase implements KMSManager, Pluggable List versions = kmsKekVersionDao.getVersionsForDecryption(kmsKey.getId()); for (KMSKekVersionVO version : versions) { try { + HSMProfileVO hsmProfile = hsmProfileDao.findById(version.getHsmProfileId()); + KMSProvider provider = getKMSProvider(hsmProfile.getProtocol()); + WrappedKey wrapped = new WrappedKey(version.getKekLabel(), kmsKey.getPurpose(), kmsKey.getAlgorithm(), wrappedVO.getWrappedBlob(), kmsKey.getProviderName(), wrappedVO.getCreated(), kmsKey.getZoneId()); @@ -433,16 +429,15 @@ public class KMSManagerImpl extends ManagerBase implements KMSManager, Pluggable throw KMSException.invalidParameter("KMS key purpose is not VOLUME_ENCRYPTION: " + kmsKey); } - HSMProfileVO hsmProfile = hsmProfileDao.findById(kmsKey.getHsmProfileId()); - if (hsmProfile == null) { - throw KMSException.invalidParameter("HSM profile not found: " + kmsKey.getHsmProfileId()); - } - - KMSProvider provider = getKMSProvider(hsmProfile.getProtocol()); - // Get active KEK version KMSKekVersionVO activeVersion = getActiveKekVersion(kmsKey.getId()); + HSMProfileVO hsmProfile = hsmProfileDao.findById(activeVersion.getHsmProfileId()); + if (hsmProfile == null) { + throw KMSException.invalidParameter("HSM profile not found: " + activeVersion.getHsmProfileId()); + } + KMSProvider provider = getKMSProvider(hsmProfile.getProtocol()); + // Generate and wrap DEK using active KEK version int dekSize = KMSDekSizeBits.value(); WrappedKey wrappedKey; @@ -1081,7 +1076,7 @@ public class KMSManagerImpl extends ManagerBase implements KMSManager, Pluggable } // Get active version for this KMS key - KMSKekVersionVO activeVersion = kmsKekVersionDao.getActiveVersion(oldVersion.getKmsKeyId()); + KMSKekVersionVO activeVersion = kmsKekVersionDao.getActiveVersion(oldVersion.getKmsKeyId()); if (activeVersion == null) { logger.warn("No active KEK version found for KMS key {}, skipping", kmsKey); return; @@ -1102,7 +1097,8 @@ public class KMSManagerImpl extends ManagerBase implements KMSManager, Pluggable } // Get provider - KMSProvider provider = getKMSProviderForZone(kmsKey.getZoneId()); + HSMProfileVO hsmProfile = hsmProfileDao.findById(activeVersion.getHsmProfileId()); + KMSProvider provider = getKMSProvider(hsmProfile.getProtocol()); // Rewrap this batch using the common helper int successCount = 0; @@ -1146,15 +1142,15 @@ public class KMSManagerImpl extends ManagerBase implements KMSManager, Pluggable boolean allDeleted = true; for (KMSKeyVO key : accountKeys) { try { - KMSProvider provider = getKMSProviderForZone(key.getZoneId()); - // Step 1: Delete all KEKs from the provider first List kekVersions = kmsKekVersionDao.listByKmsKeyId(key.getId()); if (kekVersions != null && !kekVersions.isEmpty()) { logger.debug("Deleting {} KEK version(s) from provider for KMS key {}", kekVersions.size(), key.getUuid()); for (KMSKekVersionVO kekVersion : kekVersions) { + HSMProfileVO hsmProfile = hsmProfileDao.findById(kekVersion.getHsmProfileId()); try { + KMSProvider provider = getKMSProvider(hsmProfile.getProtocol()); provider.deleteKek(kekVersion.getKekLabel()); logger.debug("Deleted KEK {} (v{}) from provider", kekVersion.getKekLabel(), kekVersion.getVersionNumber()); diff --git a/server/src/test/java/org/apache/cloudstack/kms/KMSManagerImplHSMTest.java b/server/src/test/java/org/apache/cloudstack/kms/KMSManagerImplHSMTest.java index ac029ce7424..4cf94aa77ea 100644 --- a/server/src/test/java/org/apache/cloudstack/kms/KMSManagerImplHSMTest.java +++ b/server/src/test/java/org/apache/cloudstack/kms/KMSManagerImplHSMTest.java @@ -19,16 +19,11 @@ package org.apache.cloudstack.kms; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.assertEquals; -import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import java.util.ArrayList; import java.util.Arrays; import org.apache.cloudstack.api.response.HSMProfileResponse; @@ -41,9 +36,7 @@ import org.mockito.Mock; import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; -import com.cloud.exception.PermissionDeniedException; import com.cloud.user.AccountManager; -import com.cloud.utils.exception.CloudRuntimeException; /** * Unit tests for HSM-related business logic in KMSManagerImpl diff --git a/server/src/test/java/org/apache/cloudstack/kms/KMSManagerImplKeyCreationTest.java b/server/src/test/java/org/apache/cloudstack/kms/KMSManagerImplKeyCreationTest.java index c5af2a61f37..6ce1c0049c9 100644 --- a/server/src/test/java/org/apache/cloudstack/kms/KMSManagerImplKeyCreationTest.java +++ b/server/src/test/java/org/apache/cloudstack/kms/KMSManagerImplKeyCreationTest.java @@ -28,9 +28,6 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import java.util.Arrays; -import java.util.ArrayList; - import org.apache.cloudstack.framework.kms.KMSException; import org.apache.cloudstack.framework.kms.KMSProvider; import org.apache.cloudstack.framework.kms.KeyPurpose; @@ -90,9 +87,9 @@ public class KMSManagerImplKeyCreationTest { Long hsmProfileId = 10L; HSMProfileVO profile = mock(HSMProfileVO.class); - when(profile.getId()).thenReturn(hsmProfileId); when(profile.getAccountId()).thenReturn(testAccountId); - when(hsmProfileDao.findByName(hsmProfileName)).thenReturn(profile); + when(profile.getProtocol()).thenReturn(testProviderName); + when(hsmProfileDao.findById(hsmProfileId)).thenReturn(profile); // Mock provider KEK creation when(kmsProvider.createKek(any(KeyPurpose.class), anyString(), anyInt(), eq(hsmProfileId))) @@ -106,16 +103,16 @@ public class KMSManagerImplKeyCreationTest { KMSKekVersionVO mockVersion = mock(KMSKekVersionVO.class); when(kmsKekVersionDao.persist(any(KMSKekVersionVO.class))).thenReturn(mockVersion); - // Mock getKMSProviderForZone to return our mock provider - doReturn(kmsProvider).when(kmsManager).getKMSProviderForZone(testZoneId); + // Mock getKMSProvider to return our mock provider doReturn(true).when(kmsManager).isKmsEnabled(testZoneId); + doReturn(kmsProvider).when(kmsManager).getKMSProvider(testProviderName); KMSKey result = kmsManager.createUserKMSKey(testAccountId, testDomainId, testZoneId, "test-key", "Test key", KeyPurpose.VOLUME_ENCRYPTION, 256, hsmProfileId); // Verify explicit profile was used assertNotNull(result); - verify(hsmProfileDao).findByName(hsmProfileName); + verify(hsmProfileDao).findById(hsmProfileId); verify(kmsProvider).createKek(any(KeyPurpose.class), anyString(), eq(256), eq(hsmProfileId)); // Verify KMSKeyVO was created with correct profile ID @@ -135,7 +132,6 @@ public class KMSManagerImplKeyCreationTest { long hsmProfileId = 1L; when(hsmProfileDao.findById(hsmProfileId)).thenReturn(null); - doReturn(kmsProvider).when(kmsManager).getKMSProviderForZone(testZoneId); doReturn(true).when(kmsManager).isKmsEnabled(testZoneId); kmsManager.createUserKMSKey(testAccountId, testDomainId, testZoneId, @@ -151,10 +147,9 @@ public class KMSManagerImplKeyCreationTest { Long hsmProfileId = 40L; HSMProfileVO profile = mock(HSMProfileVO.class); - when(profile.getId()).thenReturn(hsmProfileId); - when(profile.isEnabled()).thenReturn(true); when(profile.getProtocol()).thenReturn(testProviderName); - when(hsmProfileDao.listByAccountId(testAccountId)).thenReturn(Arrays.asList(profile)); + when(profile.getAccountId()).thenReturn(testAccountId); + when(hsmProfileDao.findById(hsmProfileId)).thenReturn(profile); when(kmsProvider.createKek(any(KeyPurpose.class), anyString(), anyInt(), eq(hsmProfileId))) .thenReturn("test-kek-label"); @@ -166,8 +161,8 @@ public class KMSManagerImplKeyCreationTest { KMSKekVersionVO mockVersion = mock(KMSKekVersionVO.class); when(kmsKekVersionDao.persist(any(KMSKekVersionVO.class))).thenReturn(mockVersion); - doReturn(kmsProvider).when(kmsManager).getKMSProviderForZone(testZoneId); doReturn(true).when(kmsManager).isKmsEnabled(testZoneId); + doReturn(kmsProvider).when(kmsManager).getKMSProvider(testProviderName); kmsManager.createUserKMSKey(testAccountId, testDomainId, testZoneId, "test-key", "Test key", KeyPurpose.VOLUME_ENCRYPTION, 256, hsmProfileId);