Copilot commented on code in PR #14453:
URL: https://github.com/apache/cloudstack/pull/14453#discussion_r4547078662
##########
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());
Review Comment:
This lookup excludes removed and expunged volumes, so the repair guard
misses a 4.23 storage-migration copy that was later expunged but still has
backed-up snapshots. Rewriting its shared key to the decoded format changes the
passphrase those snapshots receive and makes them unreadable. Include removed
and expunged volume references in the creation-time check before rewriting the
key.
##########
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:
SunPKCS11 reports C_GenerateRandom failures through ProviderException, but
this call does not translate them with handlePKCS11Exception.
executeWithSession only invalidates/discards sessions for KMSException, so an
RNG failure bypasses that recovery path; in particular,
CKR_CRYPTOKI_NOT_INITIALIZED does not increment hsmRestartCount to refresh the
cached native module. Route RNG failures through the existing exception handler
so volume-key generation can recover after a token restart.
--
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]
