Copilot commented on code in PR #14453:
URL: https://github.com/apache/cloudstack/pull/14453#discussion_r4534949500
##########
server/src/main/java/org/apache/cloudstack/kms/KMSManagerImpl.java:
##########
@@ -1781,12 +1899,174 @@ void rewrapSingleKey(KMSWrappedKeyVO wrappedKeyVO,
KMSKeyVO kmsKey,
}
}
+ /**
+ * 4.23 migrated legacy volume passphrases to KMS as their base64 text
instead of their raw bytes, so those
+ * volumes got a wrong passphrase. The upgrade flags every wrapped key
that existed at that point; this task
+ * checks each one once and rewraps the affected ones with the raw bytes.
A key that can't be unwrapped (e.g. the
+ * HSM is down) stays flagged and is retried. The task stops once no
flagged key is left.
+ */
+ private void scheduleKeyFormatRepair() {
+ if (kmsWrappedKeyDao.listFormatCheckPending(0, 1).isEmpty()) {
+ return;
+ }
+ ScheduledExecutorService executor =
Executors.newSingleThreadScheduledExecutor(r -> {
+ Thread t = new Thread(r, "KMSKeyFormatRepair");
+ t.setDaemon(true);
+ return t;
+ });
+ keyFormatRepairExecutor = executor;
+ executor.scheduleWithFixedDelay(new ManagedContextRunnable() {
+ @Override
+ protected void runInContext() {
+ try {
+ if (repairKeyFormatBatch()) {
+ logger.info("All KMS wrapped keys flagged at the
upgrade from 4.23 have been checked");
+ executor.shutdown();
+ }
+ } catch (final Exception e) {
+ logger.error("Error while checking the format of KMS
wrapped keys", e);
+ }
+ }
+ }, 30, 60, TimeUnit.SECONDS);
+ }
+
+ /**
+ * Checks the next batch of flagged keys. A batch ends at the first key
that can't be checked (e.g. its HSM is
+ * down), so the lock shared with the rewrap worker isn't held through
retries; the next batch resumes after that
+ * key so it doesn't block the others.
+ *
+ * @return true when no flagged key is left
+ */
+ boolean repairKeyFormatBatch() {
+ GlobalLock lock = GlobalLock.getInternLock("kms.rewrap.worker");
+ try {
+ if (!lock.lock(5)) {
+ return false;
+ }
+ try {
+ List<KMSWrappedKeyVO> keys =
kmsWrappedKeyDao.listFormatCheckPending(keyFormatRepairCursor,
+ KMSRewrapBatchSize.value());
+ if (keys.isEmpty()) {
+ boolean done = keyFormatRepairCursor == 0;
+ keyFormatRepairCursor = 0;
+ return done;
+ }
+ for (KMSWrappedKeyVO key : keys) {
+ keyFormatRepairCursor = key.getId();
+ try {
+ repairKeyFormat(key);
+ } catch (Exception e) {
+ logger.warn("Could not check the format of KMS wrapped
key {}, will retry: {}", key, e.getMessage());
+ logger.debug("Failed to check the format of KMS
wrapped key {}", key, e);
+ break;
+ }
+ }
+ return false;
+ } finally {
+ lock.unlock();
+ }
+ } finally {
+ lock.releaseRef();
+ }
+ }
+
+ private void repairKeyFormat(KMSWrappedKeyVO wrappedKeyVO) {
+ byte[] key;
+ try {
+ key = unwrapKey(wrappedKeyVO.getId());
+ } catch (KMSException e) {
+ if (e.getErrorType() != KMSException.ErrorType.KEK_NOT_FOUND) {
+ throw e;
+ }
+ // Gone for good, e.g. its KMS key or KEK was deleted, so there is
nothing to repair
+ logger.warn("KMS wrapped key {} can't be unwrapped anymore, not
checking its format: {}", wrappedKeyVO,
+ e.getMessage());
+ markFormatChecked(wrappedKeyVO);
+ return;
+ }
+ byte[] passphrase = decodeLegacyMigratedKey(key);
+ try {
+ if (passphrase != null) {
+ rewrapLegacyMigratedKey(wrappedKeyVO, passphrase);
+ }
+ markFormatChecked(wrappedKeyVO);
+ } finally {
+ Arrays.fill(key, (byte) 0);
+ if (passphrase != null) {
+ Arrays.fill(passphrase, (byte) 0);
+ }
+ }
+ }
+
+ private void rewrapLegacyMigratedKey(KMSWrappedKeyVO wrappedKeyVO, byte[]
passphrase) {
+ List<VolumeVO> volumes =
volumeDao.findByKmsWrappedKeyId(wrappedKeyVO.getId());
+ List<VolumeVO> createdAfterMigration = volumes.stream()
+ .filter(volume ->
volume.getCreated().after(wrappedKeyVO.getCreated()))
Review Comment:
The creation-time guard excludes removed and expunged volumes, including
4.23 storage-migration copies whose snapshots still need the double-encoded
passphrase. Repairing their shared key changes that passphrase and makes those
snapshots unreadable. Include historical volume references with non-destroyed
snapshots in this guard, while ignoring historical copies that no longer need
the key.
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeObject.java:
##########
@@ -995,20 +1008,26 @@ public void
doInTransactionWithoutResult(TransactionStatus status) {
public byte[] getPassphrase() {
// First check for KMS-encrypted volume
if (volumeVO.getKmsWrappedKeyId() != null) {
+ byte[] dekBytes;
try {
// Unwrap the DEK from KMS (returns raw binary bytes)
- byte[] dekBytes =
kmsManager.unwrapKey(volumeVO.getKmsWrappedKeyId());
- // Base64-encode the DEK for consistency with legacy
passphrases
- // and for use with qemu-img which expects base64 format
- String base64Dek =
java.util.Base64.getEncoder().encodeToString(dekBytes);
- // Zeroize the raw DEK bytes
- java.util.Arrays.fill(dekBytes, (byte) 0);
- // Return UTF-8 bytes of the base64 string
- return
base64Dek.getBytes(java.nio.charset.StandardCharsets.UTF_8);
+ dekBytes = kmsManager.unwrapKey(volumeVO.getKmsWrappedKeyId());
} catch (KMSException e) {
+ if (volumeVO.getRemoved() != null && e.getErrorType() ==
KMSException.ErrorType.KEK_NOT_FOUND) {
+ // The key of a removed volume can be gone, e.g. released
by an older release while snapshots of
+ // the volume still existed. Deleting such a snapshot
doesn't need the key, so don't fail it.
+ // Other errors (e.g. the HSM is unreachable) must not
look like an unencrypted volume.
Review Comment:
When a KEK is missing from the HSM but its database rows remain,
`unwrapKey()` converts the provider's `KEK_NOT_FOUND` into
`WRAP_UNWRAP_FAILED`, even for CBC where no fallback is attempted. This
bypasses the removed-volume fallback here, so snapshot deletion still fails and
format repair keeps retrying the missing key. Preserve the missing-key
classification when all usable candidates are definitively missing, without
masking transient failures, and cover this path through the manager.
##########
server/src/main/java/org/apache/cloudstack/kms/KMSManagerImpl.java:
##########
@@ -773,6 +837,40 @@ String rotateKek(KMSKeyVO kmsKey, String oldKekLabel,
String newKekLabel, int ke
}
}
+ /**
+ * Wraps and unwraps a random key with a newly created KEK, so an HSM that
doesn't support the profile's wrap
+ * algorithm (e.g. GCM with a caller-supplied IV in FIPS mode) fails here
rather than at the first volume
+ * creation or rewrap. On failure the KEK is deleted again.
+ */
+ private void verifyKekCanWrap(KMSProvider provider, KeyPurpose purpose,
String kekLabel, HSMProfileVO profile) {
+ String algorithm = null;
+ byte[] testKey = new byte[32];
+ new SecureRandom().nextBytes(testKey);
+ byte[] unwrapped = null;
+ try {
+ algorithm = provider.getWrapAlgorithm(profile.getId());
+ WrappedKey wrapped = provider.wrapKey(testKey, purpose, kekLabel,
profile.getId());
+ unwrapped = provider.unwrapKey(wrapped, profile.getId());
+ if (!Arrays.equals(testKey, unwrapped)) {
+ throw KMSException.wrapUnwrapFailed("unwrapped key does not
match the original");
+ }
+ } catch (Exception e) {
+ try {
+ provider.deleteKek(kekLabel, profile.getId());
Review Comment:
These new HSM calls execute directly on the create/rotate request thread,
bypassing kms.operation.timeout.sec and the bounded retryOperation() executor
used for KEK creation. If the token stalls during the wrap test, creating or
rotating a key can now hang indefinitely; the direct deletion in the failure
handler can hang as well. Apply the existing timeout mechanism to the probe and
its cleanup.
##########
server/src/main/java/org/apache/cloudstack/kms/KMSManagerImpl.java:
##########
@@ -1781,12 +1899,174 @@ void rewrapSingleKey(KMSWrappedKeyVO wrappedKeyVO,
KMSKeyVO kmsKey,
}
}
+ /**
+ * 4.23 migrated legacy volume passphrases to KMS as their base64 text
instead of their raw bytes, so those
+ * volumes got a wrong passphrase. The upgrade flags every wrapped key
that existed at that point; this task
+ * checks each one once and rewraps the affected ones with the raw bytes.
A key that can't be unwrapped (e.g. the
+ * HSM is down) stays flagged and is retried. The task stops once no
flagged key is left.
+ */
+ private void scheduleKeyFormatRepair() {
+ if (kmsWrappedKeyDao.listFormatCheckPending(0, 1).isEmpty()) {
+ return;
+ }
+ ScheduledExecutorService executor =
Executors.newSingleThreadScheduledExecutor(r -> {
+ Thread t = new Thread(r, "KMSKeyFormatRepair");
+ t.setDaemon(true);
+ return t;
+ });
+ keyFormatRepairExecutor = executor;
+ executor.scheduleWithFixedDelay(new ManagedContextRunnable() {
+ @Override
+ protected void runInContext() {
+ try {
+ if (repairKeyFormatBatch()) {
+ logger.info("All KMS wrapped keys flagged at the
upgrade from 4.23 have been checked");
+ executor.shutdown();
+ }
+ } catch (final Exception e) {
+ logger.error("Error while checking the format of KMS
wrapped keys", e);
+ }
+ }
+ }, 30, 60, TimeUnit.SECONDS);
+ }
+
+ /**
+ * Checks the next batch of flagged keys. A batch ends at the first key
that can't be checked (e.g. its HSM is
+ * down), so the lock shared with the rewrap worker isn't held through
retries; the next batch resumes after that
+ * key so it doesn't block the others.
+ *
+ * @return true when no flagged key is left
+ */
+ boolean repairKeyFormatBatch() {
+ GlobalLock lock = GlobalLock.getInternLock("kms.rewrap.worker");
+ try {
+ if (!lock.lock(5)) {
+ return false;
+ }
+ try {
+ List<KMSWrappedKeyVO> keys =
kmsWrappedKeyDao.listFormatCheckPending(keyFormatRepairCursor,
+ KMSRewrapBatchSize.value());
+ if (keys.isEmpty()) {
+ boolean done = keyFormatRepairCursor == 0;
+ keyFormatRepairCursor = 0;
+ return done;
+ }
+ for (KMSWrappedKeyVO key : keys) {
+ keyFormatRepairCursor = key.getId();
+ try {
+ repairKeyFormat(key);
+ } catch (Exception e) {
+ logger.warn("Could not check the format of KMS wrapped
key {}, will retry: {}", key, e.getMessage());
+ logger.debug("Failed to check the format of KMS
wrapped key {}", key, e);
+ break;
+ }
+ }
+ return false;
+ } finally {
+ lock.unlock();
+ }
+ } finally {
+ lock.releaseRef();
+ }
+ }
+
+ private void repairKeyFormat(KMSWrappedKeyVO wrappedKeyVO) {
+ byte[] key;
+ try {
+ key = unwrapKey(wrappedKeyVO.getId());
+ } catch (KMSException e) {
+ if (e.getErrorType() != KMSException.ErrorType.KEK_NOT_FOUND) {
+ throw e;
+ }
+ // Gone for good, e.g. its KMS key or KEK was deleted, so there is
nothing to repair
+ logger.warn("KMS wrapped key {} can't be unwrapped anymore, not
checking its format: {}", wrappedKeyVO,
+ e.getMessage());
+ markFormatChecked(wrappedKeyVO);
+ return;
+ }
+ byte[] passphrase = decodeLegacyMigratedKey(key);
+ try {
+ if (passphrase != null) {
+ rewrapLegacyMigratedKey(wrappedKeyVO, passphrase);
+ }
+ markFormatChecked(wrappedKeyVO);
+ } finally {
+ Arrays.fill(key, (byte) 0);
+ if (passphrase != null) {
+ Arrays.fill(passphrase, (byte) 0);
+ }
+ }
+ }
+
+ private void rewrapLegacyMigratedKey(KMSWrappedKeyVO wrappedKeyVO, byte[]
passphrase) {
+ List<VolumeVO> volumes =
volumeDao.findByKmsWrappedKeyId(wrappedKeyVO.getId());
+ List<VolumeVO> createdAfterMigration = volumes.stream()
+ .filter(volume ->
volume.getCreated().after(wrappedKeyVO.getCreated()))
+ .collect(Collectors.toList());
+ if (!createdAfterMigration.isEmpty()) {
+ // e.g. the copy made by a storage migration on 4.23, which was
encrypted with the passphrase 4.23 derived
+ // from this key. Repairing the key would make such a volume
unreadable, so leave it to the admin.
+ logger.warn("Not repairing KMS wrapped key {}, which 4.23 migrated
in a wrong format: volume(s) {} were "
+ + "created after the migration and may be encrypted with
the passphrase 4.23 used. Check these "
+ + "volumes manually.", wrappedKeyVO,
createdAfterMigration);
+ return;
+ }
+
+ KMSKeyVO kmsKey = kmsKeyDao.findById(wrappedKeyVO.getKmsKeyId());
+ KMSKekVersionVO activeVersion = getActiveKekVersion(kmsKey.getId());
+ HSMProfileVO hsmProfile =
hsmProfileDao.findById(activeVersion.getHsmProfileId());
+ WrappedKey rewrapped =
getKMSProvider(hsmProfile.getProtocol()).wrapKey(passphrase,
+ kmsKey.getPurpose(), activeVersion.getKekLabel(),
activeVersion.getHsmProfileId());
Review Comment:
The repair unwrap is timeout-protected, but this wrap calls the HSM
synchronously while repairKeyFormatBatch() holds the cluster-wide
kms.rewrap.worker lock. A token that stops responding during encryption can
therefore hold that lock indefinitely, blocking both format repair and normal
KEK rewrapping on every management server. Route this operation through
retryOperation() as well so a failed batch can release the lock.
##########
plugins/kms/pkcs11/src/main/java/org/apache/cloudstack/kms/provider/pkcs11/PKCS11HSMProvider.java:
##########
@@ -925,60 +979,112 @@ private void validateKeySize(int keyBits) throws
KMSException {
}
}
+ /**
+ * Generates random bytes with the token's random number generator
(C_GenerateRandom), so that keys come from
+ * the HSM. Falls back to the JVM's {@link SecureRandom} if the token
has no random number generator.
+ *
+ * @param length Number of bytes to generate
+ * @return Random bytes; the caller zeroizes them once they're no
longer needed
+ */
+ byte[] generateRandom(int length) {
+ SecureRandom random;
+ try {
+ random = SecureRandom.getInstance("PKCS11", provider);
+ } catch (NoSuchAlgorithmException e) {
+ // SunPKCS11 only offers SecureRandom when the token reports a
random number generator
+ logger.debug("HSM token has no random number generator, using
the JVM's SecureRandom");
+ random = SECURE_RANDOM;
+ }
+ byte[] bytes = new byte[length];
+ random.nextBytes(bytes);
+ return bytes;
Review Comment:
Token random-generation failures from `SecureRandom.nextBytes()` escape as
`ProviderException`, bypassing the `KMSException` recovery in
`executeWithSession()`. After errors such as `CKR_DEVICE_ERROR`, retries can
reuse stale provider state because pool invalidation and the restart counter
update are skipped. Route the exception through `handlePKCS11Exception()` and
wipe the partially generated buffer on failure.
--
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]
