Copilot commented on code in PR #12711:
URL: https://github.com/apache/cloudstack/pull/12711#discussion_r2871992971
##########
engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java:
##########
@@ -934,4 +971,12 @@ public VolumeVO findByLastIdAndState(long lastVolumeId,
State ...states) {
sc.and(sc.entity().getState(), SearchCriteria.Op.IN, (Object[])
states);
return sc.find();
}
+
+ @Override
+ public boolean existsWithKmsKey(long kmsKeyId) {
+ SearchCriteria<VolumeVO> sc = AllFieldsSearch.create();
+ sc.setParameters("kmsKeyId", kmsKeyId);
+ sc.setParameters("notDestroyed", Volume.State.Expunged,
Volume.State.Destroy);
+ return findOneBy(sc) != null;
+ }
Review Comment:
`existsWithKmsKey()` sets a `kmsKeyId` parameter on `AllFieldsSearch`, but
`AllFieldsSearch` is not configured with a `kmsKeyId` criterion in the
constructor. This will trigger an assertion/runtime error when the method is
called. Add `AllFieldsSearch.and("kmsKeyId", ...getKmsKeyId(), Op.EQ)` (or use
a dedicated SearchBuilder) before using `setParameters("kmsKeyId", ...)`.
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeObject.java:
##########
@@ -929,9 +955,29 @@ public void doInTransactionWithoutResult(TransactionStatus
status) {
/**
* Looks up passphrase from underlying volume.
- * @return passphrase as bytes
+ * Supports both legacy passphrase-based encryption and KMS-based
encryption.
+ * @return passphrase/DEK as base64-encoded bytes (UTF-8 bytes of base64
string)
*/
public byte[] getPassphrase() {
+ // First check for KMS-encrypted volume
+ if (volumeVO.getKmsWrappedKeyId() != null) {
+ 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);
+ } catch (org.apache.cloudstack.framework.kms.KMSException e) {
+ logger.error("Failed to unwrap KMS key for volume {}: {}",
volumeVO.getId(), e.getMessage());
+ return new byte[0];
+ }
Review Comment:
`getPassphrase()` swallows `KMSException` during unwrap and returns an empty
byte array. Returning an empty passphrase can cause downstream
encryption/secret setup to proceed as if the volume is unencrypted or fail in
non-obvious ways. Prefer failing the operation by rethrowing (e.g., as
`CloudRuntimeException`) so callers don't accidentally continue with invalid
encryption material.
##########
engine/schema/src/main/java/org/apache/cloudstack/kms/dao/KMSWrappedKeyDaoImpl.java:
##########
@@ -0,0 +1,71 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.cloudstack.kms.dao;
+
+import com.cloud.utils.db.Filter;
+import com.cloud.utils.db.GenericDaoBase;
+import com.cloud.utils.db.SearchBuilder;
+import com.cloud.utils.db.SearchCriteria;
+import org.apache.cloudstack.kms.KMSWrappedKeyVO;
+import org.springframework.stereotype.Component;
+
+import java.util.List;
+
+@Component
+public class KMSWrappedKeyDaoImpl extends GenericDaoBase<KMSWrappedKeyVO,
Long> implements KMSWrappedKeyDao {
+
+ private final SearchBuilder<KMSWrappedKeyVO> allFieldSearch;
+
+ public KMSWrappedKeyDaoImpl() {
+ super();
+
+ allFieldSearch = createSearchBuilder();
+ allFieldSearch.and("kmsKeyId", allFieldSearch.entity().getKmsKeyId(),
SearchCriteria.Op.EQ);
+ allFieldSearch.and("kekVersionId",
allFieldSearch.entity().getKekVersionId(), SearchCriteria.Op.EQ);
+ allFieldSearch.and("zoneId", allFieldSearch.entity().getZoneId(),
SearchCriteria.Op.EQ);
+ allFieldSearch.and("kmsKeyId", allFieldSearch.entity().getKmsKeyId(),
SearchCriteria.Op.EQ);
+ allFieldSearch.done();
Review Comment:
`allFieldSearch` adds the same criterion key (`"kmsKeyId"`) twice. Besides
being redundant, reusing the same parameter name can lead to unexpected
behavior depending on how `SearchBuilder` stores criteria. Drop the duplicate
`and("kmsKeyId", ...)` entry and keep a single definition.
##########
tools/apidoc/gen_toc.py:
##########
@@ -56,6 +56,8 @@
'HypervisorGuestOsNames': 'Guest OS',
'Domain': 'Domain',
'Template': 'Template',
+ 'KMS': 'KMS',
+ 'HSM': 'KMS',
Review Comment:
In the TOC mapping, `'HSM': 'KMS'` appears to mislabel the HSM API section
as KMS. This will likely group HSM docs under the wrong heading; set the
display label for `HSM` to something like `'HSM'` / `'HSM Profiles'` instead.
##########
ui/src/views/compute/DeployVM.vue:
##########
@@ -1993,6 +2018,30 @@ export default {
const param = this.params.networks
this.fetchOptions(param, 'networks')
},
+ fetchKmsKeys () {
+ if (!this.zoneId) {
+ return
+ }
+ this.loading.kmsKeys = true
+ this.options.kmsKeys = []
+ getAPI('listKMSKeys', {
+ zoneid: this.zoneId,
+ account: this.owner.account,
+ domainid: this.owner.domainid,
+ projectid: this.owner.projectid
+ }).then(response => {
Review Comment:
`fetchKmsKeys()` calls `listKMSKeys` without a `purpose` filter, so
non-volume keys (e.g., TLS purpose) may be shown in the Deploy VM wizard and
selected for disk encryption. Pass `purpose: 'volume'` (matching Create Volume)
to keep the UI aligned with volume-encryption usage and reduce server-side
rejections.
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -519,6 +520,8 @@ public class ApiResponseHelper implements ResponseGenerator
{
private ASNumberRangeDao asNumberRangeDao;
@Inject
private ASNumberDao asNumberDao;
+ @Inject
+ private HSMProfileDao hsmProfileDao;
Review Comment:
`HSMProfileDao` is added as an injected dependency but isn't referenced
anywhere in this class. If it's not needed for response generation, remove the
unused injection (and import) to avoid dead code and potential style/check
failures.
##########
server/src/main/java/com/cloud/api/query/dao/VolumeJoinDaoImpl.java:
##########
@@ -284,6 +295,18 @@ public VolumeResponse newVolumeResponse(ResponseView view,
VolumeJoinVO volume)
volResponse.setObjectName("volume");
volResponse.setExternalUuid(volume.getExternalUuid());
volResponse.setEncryptionFormat(volume.getEncryptionFormat());
+ volResponse.setKmsKeyId(volume.getKmsKeyUuid());
+ volResponse.setKmsKey(volume.getKmsKeyName());
+
+ if (volume.getKmsWrappedKeyId() != null) {
+ KMSWrappedKeyVO wrappedKey =
kmsWrappedKeyDao.findById(volume.getKmsWrappedKeyId());
+ if (wrappedKey != null) {
+ KMSKekVersionVO kekVersion =
kmsKekVersionDao.findById(wrappedKey.getKekVersionId());
+ if (kekVersion != null) {
+
volResponse.setKmsKeyVersion(kekVersion.getVersionNumber());
+ }
+ }
+ }
Review Comment:
`newVolumeResponse()` performs per-volume DAO lookups
(`kmsWrappedKeyDao.findById` and then `kmsKekVersionDao.findById`) to compute
`kmsKeyVersion`. This introduces an N+1 query pattern when listing many
volumes. Consider extending `volume_view` to join the wrapped key and KEK
version number (or batch-fetch these IDs once per response) to avoid per-row DB
hits.
##########
server/src/main/java/com/cloud/api/query/dao/VolumeJoinDaoImpl.java:
##########
@@ -58,6 +63,12 @@ public class VolumeJoinDaoImpl extends
GenericDaoBaseWithTagInformation<VolumeJo
@Inject
private PrimaryDataStoreDao primaryDataStoreDao;
@Inject
+ private KMSKeyDao kmsKeyDao;
+ @Inject
+ private KMSWrappedKeyDao kmsWrappedKeyDao;
+ @Inject
+ private KMSKekVersionDao kmsKekVersionDao;
Review Comment:
`kmsKeyDao` is injected but never used in this class. If it's not needed for
the response, remove the injection to avoid confusion and keep the dependency
surface minimal.
--
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]
