Skip to content

utils: add UuidUtils.nameUUIDFromBytes #11136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

weizhouapache
Copy link
Member

Description

The method UUID.nameUUIDFromBytes uses MD5 (Message Digest Algorithm 5) which is considered cryptographically broken and deprecated for most security-related purposes.

This PR adds UuidUtils.nameUUIDFromBytes to replace UUID.nameUUIDFromBytes.
In the next releases we could implement the method using SHA-256 or so.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Copy link

codecov bot commented Jul 3, 2025

Codecov Report

Attention: Patch coverage is 14.00000% with 43 lines in your changes missing coverage. Please review.

Project coverage is 16.57%. Comparing base (be22bfe) to head (5546835).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
...nserver/resource/Xenserver625StorageProcessor.java 14.28% 6 Missing ⚠️
...ervisor/xenserver/resource/CitrixResourceBase.java 0.00% 4 Missing ⚠️
...cloud/storage/resource/VmwareStorageProcessor.java 0.00% 3 Missing ⚠️
.../storage/resource/NfsSecondaryStorageResource.java 0.00% 3 Missing ⚠️
...isor/hyperv/discoverer/HypervServerDiscoverer.java 0.00% 2 Missing ⚠️
...oud/hypervisor/vmware/resource/VmwareResource.java 0.00% 2 Missing ⚠️
...rvisor/kvm/discoverer/LibvirtServerDiscoverer.java 0.00% 2 Missing ⚠️
utils/src/main/java/com/cloud/utils/UuidUtils.java 33.33% 2 Missing ⚠️
...n/java/com/cloud/agent/resource/DummyResource.java 0.00% 1 Missing ⚠️
.../com/cloud/agent/api/ModifyStoragePoolCommand.java 50.00% 1 Missing ⚠️
... and 17 more
Additional details and impacted files
@@            Coverage Diff            @@
##               main   #11136   +/-   ##
=========================================
  Coverage     16.57%   16.57%           
+ Complexity    13971    13970    -1     
=========================================
  Files          5743     5743           
  Lines        510648   510651    +3     
  Branches      62105    62105           
=========================================
+ Hits          84641    84644    +3     
- Misses       416534   416535    +1     
+ Partials       9473     9472    -1     
Flag Coverage Δ
uitests 3.90% <ø> (ø)
unittests 17.47% <14.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@sureshanaparti sureshanaparti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm

@sureshanaparti
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14039

@weizhouapache
Copy link
Member Author

@blueorangutan test

@blueorangutan
Copy link

@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-13685)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 58458 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11136-t13685-kvm-ol8.zip
Smoke tests completed. 141 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

@DaanHoogland
Copy link
Contributor

@weizhouapache , 👍 but if I read this correctly it doesn’t change anything (yet), does it? I am all for proxying liberaries and even standard packages, but what are the plans for implementation?

@weizhouapache
Copy link
Member Author

@weizhouapache , 👍 but if I read this correctly it doesn’t change anything (yet), does it? I am all for proxying liberaries and even standard packages, but what are the plans for implementation?

@DaanHoogland
exactly, it does not change anything.

We will implement the method with SHA-256 instead. However, we need to make sure that there is no regression, especially for the environment upgraded from older versions, because the method will return a different value.

@weizhouapache weizhouapache marked this pull request as ready for review July 7, 2025 11:33
@sureshanaparti sureshanaparti requested review from DaanHoogland, Copilot and shwstppr and removed request for DaanHoogland July 15, 2025 12:21
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Centralize name-based UUID generation by introducing a utility wrapper and update existing calls.

  • Add UuidUtils.nameUUIDFromBytes to abstract direct calls to UUID.nameUUIDFromBytes.
  • Replace all inline UUID.nameUUIDFromBytes(...) invocations across multiple modules with UuidUtils.nameUUIDFromBytes(...).
  • Update imports and references in utility, hypervisor, storage, agent, and test code.

Reviewed Changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
utils/src/main/java/com/cloud/utils/UuidUtils.java Add wrapper method nameUUIDFromBytes for MD5‐based UUID creation
vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java Use UuidUtils instead of UUID for secondary datastore UUID
utils/src/main/java/com/cloud/utils/UuidUtils.java Remove unused imports and wrap UUID.nameUUIDFromBytes
services/secondary-storage/.../DownloadManagerImpl.java Replace direct UUID.nameUUIDFromBytes with UuidUtils
services/secondary-storage/.../NfsSecondaryStorageResource.java Replace direct UUID.nameUUIDFromBytes with UuidUtils
server/src/main/java/com/cloud/user/AccountManagerImpl.java Refactor registration token generation to use UuidUtils
server/src/test/java/com/cloud/test/DatabaseConfig.java Use UuidUtils for storage pool UUID generation
server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java Swap direct UUID calls with UuidUtils in cluster GUID logic
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java Replace direct UUID for zone token with UuidUtils
plugins/.../CloudStackPrimaryDataStoreLifeCycleImpl.java Update primary data store lifecycle to use UuidUtils
plugins/hypervisors/xenserver/.../Xenserver625StorageProcessor.java and its test Refactor SR UUID generation in code and tests to UuidUtils
plugins/hypervisors/xenserver/.../CitrixResourceBase.java Replace multiple UUID.nameUUIDFromBytes calls with UuidUtils
plugins/hypervisors/vmware/.../VmwareStorageProcessor.java Switch to UuidUtils for various NFS and template UUIDs
plugins/hypervisors/vmware/.../VmwareResource.java Use UuidUtils for datastore and template UUID derivation
plugins/hypervisors/vmware/.../VmwareServerDiscoverer.java Replace cluster GUID creation to use UuidUtils
plugins/hypervisors/simulator/.../MockStorageManagerImpl.java Refactor mount path hashing to UuidUtils
plugins/hypervisors/ovm3/.../XenTest.java and Ovm3 resources/discoverer/object classes Update test and resource code to UuidUtils for VM/SR UUIDs
plugins/hypervisors/ovm/.../OvmResourceBase.java and OvmDiscoverer.java Substitute UUID calls with UuidUtils for VM and host GUIDs
plugins/hypervisors/kvm/.../KVMStoragePoolManager.java and LibvirtComputingResource.java Use UuidUtils for storage pool and secret UUID generation
plugins/hypervisors/hyperv/.../HypervServerDiscoverer.java Replace direct UUID calls with UuidUtils for Hyper‐V GUIDs
plugins/hypervisors/baremetal/.../BareMetalDiscoverer.java Switch to UuidUtils for IPMI GUID generation
plugins/event-bus/rabbitmq/.../RabbitMQEventBus.java Use UuidUtils instead of UUID for queue naming
engine/storage/integration-test/.../VolumeTestVmware.java, VolumeServiceTest.java, SnapshotTest.java Refactor tests to reference UuidUtils instead of UUID
engine/schema/src/main/java/com/cloud/storage/VMTemplateVO.java Replace inline UUID with UuidUtils for template VO naming
core/src/main/java/com/cloud/agent/api/{Modify,Delete}StoragePoolCommand.java Update storage pool commands to use UuidUtils
agent/src/main/java/com/cloud/agent/resource/DummyResource.java Swap UUID.nameUUIDFromBytes for UuidUtils in dummy resource
Comments suppressed due to low confidence (3)

utils/src/main/java/com/cloud/utils/UuidUtils.java:27

  • Add Javadoc to nameUUIDFromBytes explaining that this method currently uses an MD5-based UUID generation and note any future plans for migrating to a stronger hash.
public class UuidUtils {

utils/src/main/java/com/cloud/utils/UuidUtils.java:27

  • Add a private constructor to UuidUtils to prevent instantiation of this utility class.
public class UuidUtils {

utils/src/main/java/com/cloud/utils/UuidUtils.java:27

  • Introduce unit tests for UuidUtils.nameUUIDFromBytes to ensure correct fallback behavior and catch future regressions when the implementation changes.
public class UuidUtils {

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants