MAPREDUCE: constant-time compare when verifying the shuffle HMAC - #8687
MAPREDUCE: constant-time compare when verifying the shuffle HMAC#8687nishat-06 wants to merge 1 commit into
Conversation
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Pull request overview
This PR hardens MapReduce shuffle authentication by eliminating data-dependent timing behavior during HMAC verification in SecureShuffleUtils, addressing a potential timing-oracle on the shuffle port. It also adds targeted unit tests to ensure reply-hash verification correctly accepts valid MACs and rejects tampered/truncated inputs.
Changes:
- Replace
WritableComparator.compareByteswithMessageDigest.isEqualfor constant-time HMAC comparison inSecureShuffleUtils.verifyHash. - Add
TestSecureShuffleUtilsto validateverifyReplybehavior for matching, tampered, and truncated hashes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/security/SecureShuffleUtils.java | Switches shuffle HMAC verification to constant-time MessageDigest.isEqual. |
| hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/security/TestSecureShuffleUtils.java | Adds unit tests covering verifyReply acceptance and rejection (tamper/truncation). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
@nishat-06 I understand that from a security perspective, we should use the constant-time method |
|
Fair point, and I'll concede the framing. I don't have a real-setup PoC, and you're right that a remote byte-by-byte recovery is drowned out by the latency and jitter across the full request path, so the practical-exploit language in the description overstates it. I'd rather keep this as plain hardening than an active-exploit claim: it makes the MAC check constant-time and consistent with how the rest of the tree already compares secrets (MessageDigest.isEqual in the delegation-token and Signer paths), with no behavior change. Happy to reword the description to drop the exploitability claim if that reads better to you. |
That makes sense. Do you have a JIRA account? The Hadoop project requires each patch to have a dedicated JIRA ticket. |
|
Reworded the description to drop the exploitability claim and keep it as a plain constant-time hardening change. On JIRA: I don't have an account set up. If you're able to file a ticket for this it'd be a big help, and I'll update the PR title to reference the id once it's created. |
|
I don't have permission to approve your JIRA account application. I just created MAPREDUCE-7543 for this PR. |
Description of PR
SecureShuffleUtils.verifyHash checks the caller-supplied MAC against the recomputed HMAC with WritableComparator.compareBytes, which returns as soon as two bytes differ. This switches the check to MessageDigest.isEqual so the compare runs in constant time and no longer depends on how many leading bytes happen to match. It matches how the rest of the tree already compares secrets (MessageDigest.isEqual in the delegation-token and auth Signer paths). This is a hardening cleanup with no behavior change: a matching MAC still verifies and a wrong one is still rejected.
How was this patch tested?
Added TestSecureShuffleUtils in the mapreduce-client-core security package covering verifyReply accepting a matching hash and rejecting a tampered or truncated one, and ran it plus the existing TestFetcher against the module build on JDK 17 (both green). Checkstyle on the touched files is clean.
For code changes:
declared according to the connector-specific documentation? Note: Automated CI
testing doesn't cover all cases so manual testing with cloud storage is still
required.
LICENSE,LICENSE-binary,NOTICE-binaryfiles?AI Tooling
If an AI tool was used:
where is the name of the AI tool used.
https://www.apache.org/legal/generative-tooling.html