HDFS-17668 Treat null SASL negotiated QOP as auth in DataTransferSasl… #7171
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
jojochuang
approved these changes
Nov 20, 2024
|
Thanks, In itself, this is likely to break the SCRAM + encrypted*Stream case. This should only be committed after after #7175 (which still needs work) |
…Util#checkSaslComplete()
|
On closer inspection, breaking the SCRAM + encrypted*Stream case is exactly what we need to do. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
szetszwo
reviewed
Dec 2, 2024
There was a problem hiding this comment.
@stoty , thanks for working on this! The change looks good.
I have some ideas to improve the current code below. Could you also include them in this PR?
- Change conf to have a higher precedence than env (you are right that conf is more convenient:
+++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/SaslMechanismFactory.java
@@ -48,9 +48,9 @@ private static synchronized String getSynchronously() {
HADOOP_SECURITY_SASL_MECHANISM_DEFAULT);
LOG.debug("{} = {} (conf)", HADOOP_SECURITY_SASL_MECHANISM_KEY, confValue);
- // env has a higher precedence than conf
- mechanism = envValue != null ? envValue
- : confValue != null ? confValue
+ // conf has a higher precedence than env
+ mechanism = confValue != null ? confValue
+ : envValue != null ? envValue
: HADOOP_SECURITY_SASL_MECHANISM_DEFAULT;
LOG.debug("SASL_MECHANISM = {} (effective)", mechanism);
return mechanism;- Add a new
isDigestMechanismmethod toSaslMechanismFactoryand use it instead ofisDefaultMechanism:
diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
index cda502681c4..5b7abdb2a52 100644
--- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
+++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
@@ -2313,7 +2313,7 @@ private RpcSaslProto buildSaslNegotiateResponse()
// accelerate token negotiation by sending initial challenge
// in the negotiation response
if (enabledAuthMethods.contains(AuthMethod.TOKEN)
- && SaslMechanismFactory.isDefaultMechanism(AuthMethod.TOKEN.getMechanismName())) {
+ && SaslMechanismFactory.isDigestMechanism(AuthMethod.TOKEN.getMechanismName())) {
saslServer = createSaslServer(AuthMethod.TOKEN);
byte[] challenge = saslServer.evaluateResponse(new byte[0]);
RpcSaslProto.Builder negotiateBuilder =
diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/SaslMechanismFactory.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/SaslMechanismFactory.java
index 5b529e2a605..c12126844cd 100644
--- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/SaslMechanismFactory.java
+++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/SaslMechanismFactory.java
@@ -65,5 +65,9 @@ public static boolean isDefaultMechanism(String saslMechanism) {
return HADOOP_SECURITY_SASL_MECHANISM_DEFAULT.equals(saslMechanism);
}
+ public static boolean isDigestMechanism(String saslMechanism) {
+ return saslMechanism.startsWith("DIGEST-");
+ }
+
private SaslMechanismFactory() {}
}
\ No newline at end of file
diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/datatransfer/sasl/SaslParticipant.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/datatransfer/sasl/SaslParticipant.java
index 8743c0ccfa9..24691d74f18 100644
--- a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/datatransfer/sasl/SaslParticipant.java
+++ b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/datatransfer/sasl/SaslParticipant.java
@@ -107,7 +107,7 @@ private SaslParticipant(SaslClient saslClient) {
}
byte[] createFirstMessage() throws SaslException {
- return SaslMechanismFactory.isDefaultMechanism(MECHANISM_ARRAY[0]) ? EMPTY_BYTE_ARRAY
+ return SaslMechanismFactory.isDigestMechanism(MECHANISM_ARRAY[0]) ? EMPTY_BYTE_ARRAY
: evaluateChallengeOrResponse(EMPTY_BYTE_ARRAY);
}
|
Your suggested changes look fine to me, @szetszwo , but they are only tangetially related to this ticket, and there is no dependency either way. IMO those changes should be tracked in a different JIRA. I could open a JIRA for you, but since you have the patch ready, it would be better if it was in your name. |
|
Also, the heuristics to treat the DIGEST mechanisms differently is unneccessary. SaslClient has a dedicated method exactly for this purpose. |
|
Opened #7201 to replace the DIGEST name based heuristic. |
|
I agree with the config priority change, but again, that is outside the scope of this patch. |
|
Sure, let's do it separately. |
szetszwo
approved these changes
Dec 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…Util#checkSaslComplete()
Description of PR
HDFS-17668
Fixes handling of null negotiated QOP value for HDFS
How was this patch tested?
The specific test was not tested beyond the test suite, but the issues was detected on a cluster configured with a broken SASL mechanism.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?