Use SHA3-512

[pluckyswan]

Description

Generate a SHA3-512 hash for orders and results instead of using hashCode. Added unit tests for the new method generateHash.

Issue

#824

Checklist

• I have added tests to cover my changes
• I have added logging where useful (with appropriate log level)

Note: You may remove items that are not applicable

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 824 - Fully compliant Fully compliant requirements: Implement a secure, non-reversible hash for storing message metadata.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The generateHash method returns an empty string when an exception occurs, which might not be an ideal error handling strategy.

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Improve error handling in the hash generation to prevent silent failures Add a fallback mechanism or throw an exception when the generateHash method fails to compute the hash due to a NoSuchAlgorithmException, instead of returning an empty string. etor/src/main/java/gov/hhs/cdc/trustedintermediary/etor/orders/SendOrderUseCase.java [68-70] catch (NoSuchAlgorithmException e) { logger.logError("Algorithm does not exist!", e); - return ""; + throw new RuntimeException("Failed to compute hash", e); } Suggestion importance[1-10]: 8 Why: The suggestion to throw an exception instead of returning an empty string when encountering a NoSuchAlgorithmException is crucial for robust error handling and avoiding silent failures in the system.	8
	Modify the test to ensure different object states are used to validate hash uniqueness Ensure that the test 'generateHash generates unique hash for the same object' uses different object states for mockOrder and mockOrder2 to validate the uniqueness of the hash properly. etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/orders/SendOrderUseCaseTest.groovy [145-146] def mockOrder = Mock(Order) +mockOrder.someProperty = 'value1' def mockOrder2 = Mock(Order) +mockOrder2.someProperty = 'value2' Suggestion importance[1-10]: 7 Why: This suggestion is valid as it ensures that the test case for generating unique hashes uses different states for each object, which is crucial for accurately testing the hash function's ability to generate unique hashes for different objects.	7
	Ensure the Order object's string representation is suitable for hash generation Ensure that the Order class has a properly overridden toString method that uniquely represents its state, as it is used for generating the hash in generateHash. etor/src/main/java/gov/hhs/cdc/trustedintermediary/etor/orders/SendOrderUseCase.java [64] -byte[] objBytes = order.toString().getBytes(StandardCharsets.UTF_8); +byte[] objBytes = order.customToStringMethod().getBytes(StandardCharsets.UTF_8); Suggestion importance[1-10]: 6 Why: This suggestion is relevant as it ensures the Order class's toString method provides a unique representation for hash generation. However, it suggests using a non-existent method, which reduces its score.	6
General	Implement the test case to verify error handling and logging when hashing fails Complete the implementation of the commented-out test case 'generateHash logs error when object cannot be hashed' to ensure proper error handling and logging. etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/orders/SendOrderUseCaseTest.groovy [157-167] -// Need to check failure edge case -// def "generateHash logs error when object cannot be hashed"() { +def "generateHash logs error when object cannot be hashed"() { + given: + def mockOrder = Mock(Order) + when: + String mockHash = SendOrderUseCase.getInstance().generateHash(mockOrder) + then: + mockHash == "" + 1 * mockLogger.logError("Algorithm does not exist!", _) +} Suggestion importance[1-10]: 7 Why: Completing the implementation of the commented-out test case is important for ensuring that error handling and logging are correctly implemented when the hash generation fails. This suggestion effectively addresses a commented-out section, pushing for its completion.	7

[pluckyswan]

@halprin Two questions: 1. Is MessageDigest safe for multi-threading (i, ii)? 2. There are instances of hashCode elsewhere in the repo that are overridden. Do these need to be swapped out too?

[pluckyswan]

/review

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 824 - Fully compliant Fully compliant requirements: Implement a secure, non-reversible hash for storing metadata.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ No major issues detected

[halprin]

@halprin Two questions: 1. Is MessageDigest safe for multi-threading (i, ii)? 2. There are instances of hashCode elsewhere in the repo that are overridden. Do these need to be swapped out too?

An instance of MessageDigest is not thread safe. This means the object you get back from calling MessageDigest.getInstance("whatever") should not be used in multiple threads. And you aren't using it in multiple threads. Each call to HashHelper#generateHash could be on a separate thread but it creates its own instance of MessageDigest, so there is no shared instance of MessageDigest and we're all good.

Can you give me examples of what you're talking about? Is it something like HttpEndpoint.java where we override public int hashCode()?

[pluckyswan]

@halprin Two questions: 1. Is MessageDigest safe for multi-threading (i, ii)? 2. There are instances of hashCode elsewhere in the repo that are overridden. Do these need to be swapped out too?

An instance of MessageDigest is not thread safe. This means the object you get back from calling MessageDigest.getInstance("whatever") should not be used in multiple threads. And you aren't using it in multiple threads. Each call to HashHelper#generateHash could be on a separate thread but it creates its own instance of MessageDigest, so there is no shared instance of MessageDigest and we're all good.

Can you give me examples of what you're talking about? Is it something like HttpEndpoint.java where we override public int hashCode()?

MessageLink and HttpEndpoint both have hashCode() that is overrode. Just wanted to double check it those don't need altering too.

[halprin]

@halprin Two questions: 1. Is MessageDigest safe for multi-threading (i, ii)? 2. There are instances of hashCode elsewhere in the repo that are overridden. Do these need to be swapped out too?

An instance of MessageDigest is not thread safe. This means the object you get back from calling MessageDigest.getInstance("whatever") should not be used in multiple threads. And you aren't using it in multiple threads. Each call to HashHelper#generateHash could be on a separate thread but it creates its own instance of MessageDigest, so there is no shared instance of MessageDigest and we're all good.
Can you give me examples of what you're talking about? Is it something like HttpEndpoint.java where we override public int hashCode()?

MessageLink and HttpEndpoint both have hashCode() that is overrode. Just wanted to double check it those don't need altering too.

Those do not need to be changed. The way hashCode works today in the context of how it is normally used (comparing whether two objects are equal, IIRC) is totally fine. But subsequently using hashCode for storing in a database where the original data is sensitive is perhaps not the best and why we are moving away from using hashCode in this context.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
81.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[halprin]

I know this has been merged, but let's make one more improvement, @pluckyswan.

[halprin]

Mind making a follow-up PR to add a private constructor? I'm happy to collaborate with you on that.