HADOOP-19306. Support user defined auth Callback in SaslRpcServer. #7140
Conversation
This file contains hidden or 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. |
|
🎊 +1 overall
This message was automatically generated. |
|
I'm not knowledgeable enough of this code to review it |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
stoty
reviewed
Nov 11, 2024
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_SECURITY_SASL_MECHANISM_DEFAULT; | ||
| import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_SECURITY_SASL_MECHANISM_KEY; | ||
|
|
There was a problem hiding this comment.
Calling these values constants is very misleading.
Something like SSLMechanismFactory, SSLMechanismHolder or similar would be a more fitting name.
There was a problem hiding this comment.
Sure, let's call it SaslMechanismFactory.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Will try rebasing this. |
7372589 to
11ac338
Compare
|
💔 -1 overall
This message was automatically generated. |
jojochuang
reviewed
Nov 13, 2024
| /** | ||
| * SASL related constants. | ||
| */ | ||
| @InterfaceAudience.LimitedPrivate({"HDFS", "MapReduce"}) |
There was a problem hiding this comment.
should we open this open for YARN too?
There was a problem hiding this comment.
@jojochuang , good point! We should also add hbase.
| LOG.debug("{} = {} (env)", SASL_MECHANISM_ENV, envValue); | ||
|
|
||
| // conf | ||
| final Configuration conf = new Configuration(); |
There was a problem hiding this comment.
I am a little concerned about this one.
If an application loads configuration from a custom path, SaslMechanismFactory initialized this way will load configurations from default path, and results in unexpected behavior to the user.
And that is a potential security issue. I had some grief over similar problems before, see: HADOOP-13638
There was a problem hiding this comment.
I seems to recall similar pr B41A oblem so that I did not use conf previously. Cc @stoty
@jojochuang , If we use new Configuration(false), the value is not set when the application loads configuration from a custom path. That application must set env variable. Is it okay?
There was a problem hiding this comment.
I don't follow the issue here.
Is it about adding config files to the classpath dynamically ?
Is SaslMechanismFactory reading the Configuration too early ?
It should be possble (with more changes) to load the mechanism lazily, when SASL is used first.
IMO having to configure this from an environment variable would be an operational nightmare.
There was a problem hiding this comment.
It should be possble (with more changes) to load the mechanism lazily, when SASL is used first.
The is already loaded mechanism lazily. The class won't be loaded unless it is used.
In our case, it should be fine since conf will be loaded during a SASL server/client is being created. The conf path must be set at that time. (UserGroupInformation has a different story since it may have to logged in before starting a server/client.)
IMO having to configure this from an environment variable would be an operational nightmare.
Hadoop security tends to use environment variable since conf may not available.
There was a problem hiding this comment.
The rest of PR looks good to me. This part I'm not sure. So what about the normal case, like YARN Resource Manager, do they also get the value from env variable? Then what's the use of the configuration object?
There was a problem hiding this comment.
... So what about the normal case, like YARN Resource Manager, do they also get the value ?
They can get it either from env variable or from conf.
Then what's the use of the configuration object?
@stoty has pointed that env variable is inconvenient since the other RPC parameters are specified using conf. He has tested it with existing code. BTW, he is working on changing HBase asyncfs for the configurable SASL mechanism.
There was a problem hiding this comment.
@jojochuang , @stoty , unfortunately, UserGroupInformation is statically referring to SaslMechanismFactory (via SaslRpcServer.AuthMethod) as @steveloughran discovered. We may need to remove the conf support from SaslMechanismFactory.
There was a problem hiding this comment.
We are able to use reflection to implement lazy loading of SaslMechanismFactory; see #7173
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/SaslConstants.java
Show resolved
Hide resolved
.../src/main/java/org/apache/hadoop/hdfs/protocol/datatransfer/sasl/SaslDataTransferServer.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
@jojochuang , @stoty , thanks a lot for reviewing this! |
|
@szetszwo now all my test run logs are full of stuff stack trace from the debugger implies this is UGI.initialize() calling SaslRpcServer, whose Please downgrade to debug ASAP. thanks |
|
@steveloughran , thanks for letting me know.
|
|
Filed HADOOP-19342. |
1 task
1 task
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.
Description of PR
HADOOP-19306
Similar to HDFS-17576, SaslRpcServer should support CustomizedCallbackHandler.
How was this patch tested?
Added new test cases
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?