Skip to content

Commit ac1c629

Browse files
vishesh92vishesh92
committed
Fix missing labels and label generation
1 parent 943f19a commit ac1c629

File tree

8 files changed

+49
-46
lines changed

8 files changed

+49
-46
lines changed

‎framework/kms/src/main/java/org/apache/cloudstack/framework/kms/KeyPurpose.java‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,12 +65,15 @@ public String getDescription() {
6565
}
6666

6767
/**
68-
* Generate a KEK label with purpose prefix
68+
* Generate a globally unique, collision-resistant KEK label with context
6969
*
70-
* @param customLabel optional custom label suffix
71-
* @return formatted KEK label (e.g., "volume-kek-v1")
70+
* @param domainId the domain ID associated with this key
71+
* @param accountId the account ID associated with this key
72+
* @param uuid the unique identifier of the key entity
73+
* @param version the version number of the key
74+
* @return formatted KEK label (e.g., "volume-kek-1-2-a8054d8f-...-1")
7275
*/
73-
public String generateKekLabel(String customLabel) {
74-
return name + "-kek-" + (customLabel != null ? customLabel : "v1");
76+
public String generateKekLabel(long domainId, long accountId, String uuid, int version) {
77+
return name + "-kek-" + domainId + "-" + accountId + "-" + uuid.replace("-", "") + "-v" + version;
7578
}
7679
}

‎plugins/kms/database/src/main/java/org/apache/cloudstack/kms/provider/DatabaseKMSProvider.java‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
import java.util.Arrays;
3838
import java.util.Base64;
3939
import java.util.Date;
40-
import java.util.UUID;
4140

4241
/**
4342
* Database-backed KMS provider that stores master KEKs in a PKCS#11-like object table.
@@ -78,7 +77,7 @@ public String createKek(KeyPurpose purpose, String label, int keyBits) throws KM
7877
}
7978

8079
if (StringUtils.isEmpty(label)) {
81-
label = generateKekLabel(purpose);
80+
throw KMSException.invalidParameter("KEK label cannot be empty");
8281
}
8382

8483
if (kekObjectDao.existsByLabel(label)) {
@@ -321,9 +320,6 @@ private void updateLastUsed(String kekLabel) {
321320
}
322321
}
323322

324-
private String generateKekLabel(KeyPurpose purpose) {
325-
return purpose.getName() + "-kek-" + UUID.randomUUID().toString().substring(0, 8);
326-
}
327323

328324
@Override
329325
public String getConfigComponentName() {

‎plugins/kms/pkcs11/src/main/java/org/apache/cloudstack/kms/provider/pkcs11/PKCS11HSMProvider.java‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,10 @@ public String createKek(KeyPurpose purpose, String label, int keyBits, Long hsmP
110110
if (hsmProfileId == null) {
111111
throw KMSException.invalidParameter("HSM Profile ID is required for PKCS#11 provider");
112112
}
113-
final String kekLabel = StringUtils.isEmpty(label) ? generateKekLabel(purpose) : label;
114-
return executeWithSession(hsmProfileId, session -> session.generateKey(kekLabel, keyBits, purpose));
113+
if (StringUtils.isEmpty(label)) {
114+
throw KMSException.invalidParameter("KEK label cannot be empty");
115+
}
116+
return executeWithSession(hsmProfileId, session -> session.generateKey(label, keyBits, purpose));
115117
}
116118

117119
@Override
@@ -367,9 +369,7 @@ boolean isSensitiveKey(String key) {
367369
return KMSProvider.isSensitiveKey(key);
368370
}
369371

370-
String generateKekLabel(KeyPurpose purpose) {
371-
return purpose.getName() + "-kek-" + UUID.randomUUID().toString().substring(0, 8);
372-
}
372+
373373

374374
@Override
375375
public String getConfigComponentName() {
@@ -822,6 +822,12 @@ String generateKey(String label, int keyBits, KeyPurpose purpose) throws KMSExce
822822
return label;
823823

824824
} catch (KeyStoreException e) {
825+
if (e.getMessage() != null
826+
&& e.getMessage().contains("found multiple secret keys sharing same CKA_LABEL")) {
827+
logger.warn("Multiple duplicate keys found with label '{}' in HSM. Reusing the existing key. " +
828+
"Please purge duplicate keys manually if possible.", label);
829+
return label;
830+
}
825831
handlePKCS11Exception(e, "Failed to store key in HSM KeyStore");
826832
} catch (NoSuchAlgorithmException e) {
827833
handlePKCS11Exception(e, "AES KeyGenerator not available via PKCS#11 provider");

‎plugins/kms/pkcs11/src/test/java/org/apache/cloudstack/kms/provider/pkcs11/PKCS11HSMProviderTest.java‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -186,21 +186,6 @@ public void testIsSensitiveKey_IdentifiesNonSensitiveKeys() {
186186
assertFalse(provider.isSensitiveKey("max_sessions"));
187187
}
188188

189-
/**
190-
* Test: generateKekLabel creates valid label
191-
*/
192-
@Test
193-
public void testGenerateKekLabel_CreatesValidLabel() {
194-
// Test
195-
String label = provider.generateKekLabel(KeyPurpose.VOLUME_ENCRYPTION);
196-
197-
// Verify
198-
assertNotNull("Label should not be null", label);
199-
assertTrue("Label should start with purpose", label.startsWith(KeyPurpose.VOLUME_ENCRYPTION.getName()));
200-
assertTrue("Label should contain UUID",
201-
label.length() > (KeyPurpose.VOLUME_ENCRYPTION.getName() + "-kek-").length());
202-
}
203-
204189
/**
205190
* Test: getProviderName returns correct name
206191
*/

‎server/src/main/java/org/apache/cloudstack/kms/KMSManagerImpl.java‎

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@
8787
import java.util.HashMap;
8888
import java.util.List;
8989
import java.util.Map;
90-
import java.util.UUID;
90+
9191
import java.util.concurrent.ExecutionException;
9292
import java.util.concurrent.ExecutorService;
9393
import java.util.concurrent.Executors;
@@ -435,8 +435,11 @@ KMSKey createUserKMSKey(Long accountId, Long domainId, Long zoneId,
435435
throw KMSException.invalidParameter("HSM Profile not found");
436436
}
437437

438+
KMSKeyVO kmsKey = new KMSKeyVO(name, description, "", purpose,
439+
accountId, domainId, zoneId, "AES/GCM/NoPadding", keyBits);
440+
438441
KMSProvider provider = getKMSProvider(profile.getProtocol());
439-
String kekLabel = purpose.getName() + "-kek-" + UUID.randomUUID().toString().substring(0, 8);
442+
String kekLabel = purpose.generateKekLabel(domainId, accountId, kmsKey.getUuid(), 1);
440443

441444
String providerKekLabel;
442445
Long finalProfileId = hsmProfileId;
@@ -446,8 +449,7 @@ KMSKey createUserKMSKey(Long accountId, Long domainId, Long zoneId,
446449
throw handleKmsException(e);
447450
}
448451

449-
KMSKeyVO kmsKey = new KMSKeyVO(name, description, providerKekLabel, purpose,
450-
accountId, domainId, zoneId, "AES/GCM/NoPadding", keyBits);
452+
kmsKey.setKekLabel(providerKekLabel);
451453
kmsKey.setHsmProfileId(finalProfileId);
452454
kmsKey = kmsKeyDao.persist(kmsKey);
453455

@@ -661,8 +663,12 @@ String rotateKek(KMSKeyVO kmsKey, String oldKekLabel, String newKekLabel, int ke
661663
try {
662664
logger.info("Starting KEK rotation from {} to {} for kms key {}", oldKekLabel, newKekLabel, kmsKey);
663665

666+
final KMSKekVersionVO newVersionEntity = new KMSKekVersionVO();
664667
if (StringUtils.isEmpty(newKekLabel)) {
665-
newKekLabel = kmsKey.getPurpose().getName() + "-kek-" + UUID.randomUUID().toString().substring(0, 8);
668+
List<KMSKekVersionVO> existingVersions = kmsKekVersionDao.listByKmsKeyId(kmsKey.getId());
669+
int nextVersion = existingVersions.stream().mapToInt(KMSKekVersionVO::getVersionNumber).max().orElse(0) + 1;
670+
newKekLabel = kmsKey.getPurpose().generateKekLabel(kmsKey.getDomainId(), kmsKey.getAccountId(),
671+
kmsKey.getUuid(), nextVersion);
666672
}
667673

668674
String finalNewKekLabel = newKekLabel;
@@ -676,7 +682,9 @@ String rotateKek(KMSKeyVO kmsKey, String oldKekLabel, String newKekLabel, int ke
676682
.execute(new TransactionCallbackWithException<KMSKekVersionVO, KMSException>() {
677683
@Override
678684
public KMSKekVersionVO doInTransaction(TransactionStatus status) throws KMSException {
679-
KMSKekVersionVO version = createKekVersion(kmsKey.getId(), newKekId, newProfileId);
685+
newVersionEntity.setKmsKeyId(kmsKey.getId());
686+
newVersionEntity.setHsmProfileId(newProfileId);
687+
KMSKekVersionVO version = createKekVersion(newVersionEntity);
680688

681689
if (!newProfileId.equals(kmsKey.getHsmProfileId())) {
682690
kmsKey.setHsmProfileId(newProfileId);
@@ -712,26 +720,23 @@ public KMSKekVersionVO doInTransaction(TransactionStatus status) throws KMSExcep
712720
}
713721
}
714722

715-
private KMSKekVersionVO createKekVersion(Long kmsKeyId, String kekLabel, Long hsmProfileId) throws KMSException {
716-
List<KMSKekVersionVO> existingVersions = kmsKekVersionDao.listByKmsKeyId(kmsKeyId);
723+
private KMSKekVersionVO createKekVersion(KMSKekVersionVO newVersion) throws KMSException {
724+
List<KMSKekVersionVO> existingVersions = kmsKekVersionDao.listByKmsKeyId(newVersion.getKmsKeyId());
717725
int nextVersion = existingVersions.stream()
718726
.mapToInt(KMSKekVersionVO::getVersionNumber)
719727
.max()
720728
.orElse(0) + 1;
721729

722-
KMSKekVersionVO currentActive = kmsKekVersionDao.getActiveVersion(kmsKeyId);
730+
KMSKekVersionVO currentActive = kmsKekVersionDao.getActiveVersion(newVersion.getKmsKeyId());
723731
if (currentActive != null) {
724732
currentActive.setStatus(KMSKekVersionVO.Status.Previous);
725733
kmsKekVersionDao.update(currentActive.getId(), currentActive);
726734
}
727735

728-
KMSKekVersionVO newVersion = new KMSKekVersionVO(kmsKeyId, nextVersion, kekLabel,
729-
KMSKekVersionVO.Status.Active);
730-
newVersion.setHsmProfileId(hsmProfileId);
736+
newVersion.setVersionNumber(nextVersion);
737+
newVersion.setStatus(KMSKekVersionVO.Status.Active);
731738
newVersion = kmsKekVersionDao.persist(newVersion);
732-
733-
logger.info("Created KEK version {} for KMS key {} (label: {}, profile: {})", nextVersion, kmsKeyId, kekLabel,
734-
hsmProfileId);
739+
logger.info("Created KEK version {} for KMS key {}", nextVersion, newVersion.getKmsKeyId());
735740
return newVersion;
736741
}
737742

‎server/src/test/java/org/apache/cloudstack/kms/KMSManagerImplKeyRotationTest.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,9 @@ public void testRotateKek_GeneratesLabel() throws Exception {
253253
when(kmsKey.getId()).thenReturn(kmsKeyId);
254254
when(kmsKey.getHsmProfileId()).thenReturn(oldProfileId);
255255
when(kmsKey.getPurpose()).thenReturn(KeyPurpose.VOLUME_ENCRYPTION);
256+
when(kmsKey.getUuid()).thenReturn("test-kms-key-uuid");
257+
when(kmsKey.getDomainId()).thenReturn(1L);
258+
when(kmsKey.getAccountId()).thenReturn(2L);
256259

257260
HSMProfileVO hsmProfile = mock(HSMProfileVO.class);
258261
when(hsmProfile.getId()).thenReturn(oldProfileId);

‎ui/public/locales/en.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1419,6 +1419,7 @@
14191419
"label.keep": "Keep",
14201420
"label.kernelversion": "Kernel Version",
14211421
"label.key": "Key",
1422+
"label.keybits": "Key Bits",
14221423
"label.keyboard": "Keyboard language",
14231424
"label.keyboardtype": "Keyboard type",
14241425
"label.keypair": "SSH key pair",
@@ -1434,6 +1435,7 @@
14341435
"label.migrate.volumes.to.kms": "Migrate Volumes to KMS",
14351436
"label.hsm.profile": "HSM Profile",
14361437
"label.hsmprofile": "HSM Profile",
1438+
"label.hsmprofileid": "HSM Profile",
14371439
"label.create.hsmprofile": "Add HSM Profile",
14381440
"label.update.hsm.profile": "Update HSM Profile",
14391441
"label.delete.hsm.profile": "Delete HSM Profile",
@@ -1496,6 +1498,7 @@
14961498
"label.lbruleid": "Load balancer ID",
14971499
"label.lbtype": "Load balancer type",
14981500
"label.ldap": "LDAP",
1501+
"label.library": "Library",
14991502
"label.ldapdomain": "LDAP Domain",
15001503
"label.ldap.configuration": "LDAP Configuration",
15011504
"label.ldap.group.name": "LDAP Group",
@@ -1912,6 +1915,7 @@
19121915
"label.physicalnetworkid": "Physical Network",
19131916
"label.physicalnetworkname": "Physical Network name",
19141917
"label.physicalsize": "Physical size",
1918+
"label.pin": "PIN",
19151919
"label.ping.path": "Ping path",
19161920
"label.pkcs.private.certificate": "PKCS#8 private certificate",
19171921
"label.plannermode": "Planner mode",
@@ -2498,6 +2502,7 @@
24982502
"label.suspend.project": "Suspend Project",
24992503
"label.switch.type": "Switch type",
25002504
"label.sync.storage": "Sync Storage Pool",
2505+
"label.system": "System",
25012506
"label.system.ip.pool": "System Pool",
25022507
"label.system.offering": "System Offering",
25032508
"label.system.offerings": "System Offerings",

‎ui/src/config/section/kms.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ export default {
220220
},
221221
mapping: {
222222
details: {
223-
optionalKeys: ['pin', 'library', 'slot', 'token_label']
223+
optionalKeys: ['pin', 'library', 'slot', 'slot_list_index', 'token_label']
224224
}
225225
}
226226
},

0 commit comments

Comments
 (0)