Copilot commented on code in PR #14453:
URL: https://github.com/apache/cloudstack/pull/14453#discussion_r4548064709
##########
server/src/main/java/org/apache/cloudstack/kms/KMSManagerImpl.java:
##########
@@ -442,10 +475,10 @@ KMSKey createUserKMSKey(Long accountId, Long domainId,
Long zoneId,
throw new InvalidParameterValueException("A KMS key with name " +
name + " already exists in this account");
}
+ KMSProvider provider = getKMSProvider(profile.getProtocol());
KMSKeyVO kmsKey = new KMSKeyVO(name, description, "", purpose,
- accountId, domainId, zoneId, "AES/GCM/NoPadding", keyBits);
+ accountId, domainId, zoneId,
provider.getWrapAlgorithm(hsmProfileId), keyBits);
Review Comment:
`provider.getWrapAlgorithm(hsmProfileId)` is called with the raw command
parameter, but the method later resolves/uses the persisted profile (`profile`
/ `finalProfileId`). If `hsmProfileId` is null (or differs from the resolved
profile), this can break PKCS#11 (which requires a profile ID) and/or store the
wrong `kmsKey.algorithm`. Use the resolved profile ID (e.g. `profile.getId()`
or `finalProfileId`) when computing the wrap algorithm.
##########
server/src/main/java/org/apache/cloudstack/kms/KMSManagerImpl.java:
##########
@@ -218,65 +235,77 @@ public byte[] unwrapKey(Long wrappedKeyId) throws
KMSException {
Exception lastException = null;
- if (wrappedVO.getKekVersionId() != null) {
- KMSKekVersionVO version =
kmsKekVersionDao.findById(wrappedVO.getKekVersionId());
- if (version != null && version.getStatus() !=
KMSKekVersionVO.Status.Archived) {
- HSMProfileVO hsmProfile =
hsmProfileDao.findById(version.getHsmProfileId());
- if (!hsmProfile.isEnabled()) {
- throw KMSException.providerNotInitialized("HSM profile is
not enabled for wrapped key: " + version);
- }
- try {
- byte[] dek = getUnwrappedKey(wrappedVO, kmsKey, version,
hsmProfile);
-
- CallContext.current().setEventResourceId(kmsKey.getId());
-
CallContext.current().setEventResourceType(ApiCommandResourceType.KmsKey);
- CallContext.current().setEventDetails(String.format(
- "Unwrapped %s key (wrapped key uuid: %s) using KMS
key: %s (uuid: %s) with KEK version %d",
- kmsKey.getPurpose().getName(),
wrappedVO.getUuid(), kmsKey.getName(), kmsKey.getUuid(),
version.getVersionNumber()));
+ KMSKekVersionVO recordedVersion = wrappedVO.getKekVersionId() != null
+ ? kmsKekVersionDao.findById(wrappedVO.getKekVersionId()) :
null;
+ if (recordedVersion != null && recordedVersion.getStatus() ==
KMSKekVersionVO.Status.Archived) {
+ // The rewrap worker deleted its KEK; no other KEK can unwrap the
key
+ throw KMSException.kekNotFound(String.format("KEK version %d of
wrapped key %s is archived",
+ recordedVersion.getVersionNumber(), wrappedVO.getUuid()));
+ }
+ if (recordedVersion != null) {
+ HSMProfileVO hsmProfile = getEnabledHsmProfile(recordedVersion);
+ try {
+ return unwrapKeyWithVersion(wrappedVO, kmsKey,
recordedVersion, hsmProfile, "");
+ } catch (Exception e) {
+ lastException = e;
+ logger.warn("Failed to unwrap with version {}: {}",
recordedVersion.getVersionNumber(), e.getMessage(), e);
+ }
+ }
- logger.debug("Successfully unwrapped key {} with KEK
version {}", wrappedKeyId, version.getVersionNumber());
- return dek;
+ // Other versions only for an authenticated algorithm: with CBC
another KEK can unwrap the blob to a wrong
+ // key without an error, and a rewrap would then lose the real key for
good.
+ if
(AUTHENTICATED_WRAP_ALGORITHMS.contains(wrappedVO.getWrapAlgorithm())) {
+ List<KMSKekVersionVO> otherVersions =
kmsKekVersionDao.getVersionsForDecryption(kmsKey.getId()).stream()
+ .filter(version -> recordedVersion == null ||
!version.getId().equals(recordedVersion.getId()))
+
.sorted(Comparator.comparing(KMSKekVersionVO::getVersionNumber).reversed())
+ .collect(Collectors.toList());
Review Comment:
`Collectors` is referenced here; ensure `java.util.stream.Collectors` is
imported in this file. Without the import, this will fail to compile.
##########
server/src/main/java/org/apache/cloudstack/kms/KMSManagerImpl.java:
##########
@@ -591,18 +625,35 @@ public boolean deleteKMSWrappedKey(Volume vol) throws
KMSException {
if (CollectionUtils.isNotEmpty(volumes)) {
for (VolumeVO volume : volumes) {
if (volume.getId() != vol.getId()) {
- logger.warn("KMS Wrapped Key {} is still in use by
volume {}, cannot delete",
+ // Normal after a volume migration, the source and the
new volume share the key
+ logger.debug("KMS Wrapped Key {} is still in use by
volume {}, cannot delete",
wrappedKey, volume);
return false;
}
}
}
+ if (isNeededBySnapshots(wrappedKey)) {
+ logger.debug("KMS Wrapped Key {} is still needed by snapshots
of volume {} or of another volume "
+ + "sharing it, keeping it", wrappedKey, vol);
+ return false;
+ }
logger.info("Deleted KMS Wrapped Key {} for volume {}",
wrappedKey, vol);
return kmsWrappedKeyDao.remove(wrappedKey.getId());
}
return true;
}
+ /**
+ * Whether a snapshot that isn't destroyed exists for any volume (removed
volumes included) that uses the
+ * wrapped key. Restoring or deleting such a snapshot needs the key of its
volume.
+ */
+ private boolean isNeededBySnapshots(KMSWrappedKeyVO wrappedKey) {
+ List<Long> volumeIds =
volumeDao.listIncludingRemovedByKmsWrappedKeyId(wrappedKey.getId()).stream()
+ .map(VolumeVO::getId).collect(Collectors.toList());
Review Comment:
Consider short-circuiting when `volumeIds` is empty to avoid an unnecessary
DAO call (and to avoid potential SQL edge cases in implementations that
generate `IN ()`). For example, return `false` immediately if
`volumeIds.isEmpty()`.
##########
engine/schema/src/main/java/org/apache/cloudstack/kms/KMSWrappedKeyVO.java:
##########
@@ -61,6 +61,16 @@ public class KMSWrappedKeyVO {
@Column(name = "wrapped_blob", nullable = false)
private byte[] wrappedBlob;
+ @Column(name = "wrap_algorithm", nullable = false)
+ private String wrapAlgorithm;
Review Comment:
`wrapAlgorithm` is non-nullable at the ORM level, but it has no default
initialization in the entity. This makes it easy for future call sites that
create/persist `KMSWrappedKeyVO` to forget `setWrapAlgorithm(...)` and hit DB
constraint/insert failures (the DB default won't apply if ORM includes the
column with NULL). Consider enforcing this more centrally (e.g., initialize to
the schema default, validate in DAO before persist, or require it via
constructors) to prevent regressions.
##########
server/src/main/java/org/apache/cloudstack/kms/KMSManagerImpl.java:
##########
@@ -638,12 +689,21 @@ void deleteUserKMSKey(KMSKeyVO key) throws KMSException {
kekVersion.getKekLabel(),
kekVersion.getVersionNumber(), provider.getProviderName());
}
} catch (Exception e) {
+ if (e instanceof KMSException && ((KMSException)
e).getErrorType() == KMSException.ErrorType.KEK_NOT_FOUND) {
+ // e.g. an archived version whose KEK the rewrap worker
already deleted
+ logger.info("KEK {} (v{}) is already gone from the
provider", kekVersion.getKekLabel(),
+ kekVersion.getVersionNumber());
+ continue;
+ }
logger.warn("Failed to delete KEK {} (v{}) from provider
during KMS key deletion: {}",
kekVersion.getKekLabel(),
kekVersion.getVersionNumber(), e.getMessage());
ActionEventUtils.onCompletedActionEvent(CallContext.current().getCallingUserId(),
key.getAccountId(), EventVO.LEVEL_WARN,
EventTypes.EVENT_KMS_KEY_DELETE, true,
String.format("Failed to delete KEK %s from provider
during KMS key deletion", kekVersion.getKekLabel()),
key.getId(), ApiCommandResourceType.KmsKey.toString(),
CallContext.current().getStartEventId());
+ // Keep the KMS key so the KEK material isn't left in the HSM
without any reference to it
+ throw new CloudRuntimeException(String.format("Failed to
delete KEK %s from provider, KMS key %s was "
Review Comment:
`deleteUserKMSKey` is declared to throw `KMSException`, but this path throws
`CloudRuntimeException` instead, which can bypass existing KMS error handling
and produce inconsistent API responses. Consider throwing a `KMSException`
(e.g., using `handleKmsException(e)` / a dedicated `kekOperationFailed` error)
while still keeping the DB row intact.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
