HADOOP-19152. Do not hard code security providers. #6739
Merged
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
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyProvider.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
@steveloughran , thanks for looking at this! As mentioned in +++ b/hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
@@ -3623,6 +3623,9 @@ The switch to turn S3A auditing on or off.
<value></value>
<description>
The JCE provider name used in CryptoCodec.
+ If this value is set, the corresponding provider must be added to the provider list.
+ The provider may be added statically in the java.security file, or
+ added dynamically by calling the java.security.Security.addProvider(..) method.
</description>
</property> |
It makes sense for the downstream modules adding the dependencies when they need it, although it is even better to let users set their security providers. Then, they will have the freedom to choose the providers.
hadoop-azure seems okay? The tests did not fail. |
|
🎊 +1 overall
This message was automatically generated. |
we'd have to see about the integration tests. Actually, I think it was minihdfs which needed it for tests, rather than the actual code |
Yes, there are many tests requiring |
|
@steveloughran , question to you: +++ b/hadoop-common-project/hadoop-common/pom.xml
@@ -375,6 +375,7 @@
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcprov-jdk18on</artifactId>
+ <scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.kerby</groupId>According to our Compatibility Java_Classpath doc, removing a dependency is a compatible change. The above changes the scope of Note that users currently using |
|
will this require changes everywhere? or will the default of bce still work? as if it gives a choice, fine -just as long as it doesn't force that choice on everyone |
If we change it to use java reflection (similar to
We are currently forcing everyone to add the |
|
Let me clarify in more details:
Question: is Alternative Approach 1 considered compatible? |
|
Passing by... To me Alt-1 looks functionally incompatible. It is written under Functional Compat column: Cluster behaviour changed, To make things work I need to explicitily make sure the jar is in Classpath. Regarding the Java Classpath in the compat: I think that means If tomorrow Hadoop doesn't need a Jar, means removing it has "no impact" we can remove it... |
|
yeah, looks incompatible. I'd rather it supports switching, but the existing BC jar ships and everything works |
|
Let's do Alternative Approach 2 then. We could as welll add another conf to disable the existing hardcoded provider. |
| package org.apache.hadoop.crypto; | ||
|
|
||
| import org.apache.hadoop.classification.InterfaceAudience; | ||
| import org.apache.hadoop.conf.Configuration; |
There was a problem hiding this comment.
nit: import ordering
There was a problem hiding this comment.
The ordering seems okay -- co after cl. BTW, it was automatically ordered by Inteliij.
There was a problem hiding this comment.
more the mix of apache and others, the general layout is currently
java.*
javax.*
other
org.apache*
static
things are generally messy with "other" being tainted by our move off google guava into our own stuff, but its still good to try and keep things under control
There was a problem hiding this comment.
Is there a way to set the import ordering in IDEs such as IntelliJ? We should not manually sort the imports.
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/CryptoUtils.java
Show resolved
Hide resolved
| } | ||
|
|
||
| public static String getJceProvider(Configuration conf) { | ||
| final String jceProvider = conf.get(HADOOP_SECURITY_CRYPTO_JCE_PROVIDER_KEY); |
There was a problem hiding this comment.
prefer
getTrimmed(HADOOP_SECURITY_CRYPTO_JCE_PROVIDER_KEY, "");
trims the string and guarantees that it is not null
| LOG.debug("Successfully added security provider {}", provider); | ||
| } | ||
| } catch (ClassNotFoundException e) { | ||
| LOG.warn("Failed to load " + BOUNCY_CASTLE_PROVIDER_CLASS, e); |
There was a problem hiding this comment.
this is going to log every time addProvider() is called. is that ok? otherwise a LogExactlyOnce log should be used
| </property> | ||
|
|
||
| <property> | ||
| <name>hadoop.security.crypto.jce.provider.add</name> |
There was a problem hiding this comment.
not sure about the name here.
There was a problem hiding this comment.
Let's change it to auto-add.
|
💔 -1 overall
This message was automatically generated. |
| package org.apache.hadoop.crypto; | ||
|
|
||
| import org.apache.hadoop.classification.InterfaceAudience; | ||
| import org.apache.hadoop.conf.Configuration; |
There was a problem hiding this comment.
more the mix of apache and others, the general layout is currently
java.*
javax.*
other
org.apache*
static
things are generally messy with "other" being tainted by our move off google guava into our own stuff, but its still good to try and keep things under control
| import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_SECURITY_CRYPTO_JCE_PROVIDER_ADD_KEY; | ||
| import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_SECURITY_CRYPTO_JCE_PROVIDER_KEY; | ||
|
|
||
| /** Utility methods for the crypto related features. */ | ||
| @InterfaceAudience.Private | ||
| public class CryptoUtils { |
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/CryptoUtils.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Oops, just found that I accidentally changed the whitespaces in |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Apr 26, 2024
| @@ -40,6 +41,8 @@ public class CryptoUtils { | |||
| = "org.bouncycastle.jce.provider.BouncyCastleProvider"; | |||
| private static final String PROVIDER_NAME_FIELD = "PROVIDER_NAME"; | |||
|
|
|||
| static final String PROVIDER_NAME = "BC"; | |||
There was a problem hiding this comment.
make private add a javadoc, and give it a name like BOUNCY_CASTLE_PROVIDER_NAME
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/CryptoUtils.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Apr 30, 2024
| import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.HADOOP_SECURITY_CRYPTO_JCE_PROVIDER_KEY; | ||
|
|
||
| /** Test {@link CryptoUtils}. */ | ||
| public class TestCryptoUtils { |
There was a problem hiding this comment.
if you extend AbstractHadoopTestBase you get the timeout set for all, and thread naming for the logs. no need to repeat timeout everywhere
There was a problem hiding this comment.
The default timeout 100s is too long for this. The other things may not be useful here since this is just a very simple test.
|
|
||
| final Configuration conf = new Configuration(); | ||
| Assert.assertTrue("true".equalsIgnoreCase( | ||
| conf.get(HADOOP_SECURITY_CRYPTO_JCE_PROVIDER_AUTO_ADD_KEY))); |
There was a problem hiding this comment.
for all the asserts other than assertNull and assertEqual, which generate them automatically we do need useful error messages so when a test run fails we can debug it.
this is where AssertJ assertions beat junit.
Assertions.assertThat(conf.get(HADOOP_SECURITY_CRYPTO_JCE_PROVIDER_AUTO_ADD_KEY))
.describedAs("security provider from configuration")
.isEqualToIgnoringCase("true")
| Assert.assertEquals(CryptoUtils.BOUNCY_CASTLE_PROVIDER_NAME, providerFromConf); | ||
|
|
||
| final Provider provider = Security.getProvider(BouncyCastleProvider.PROVIDER_NAME); | ||
| Assert.assertTrue(provider instanceof BouncyCastleProvider); |
There was a problem hiding this comment.
Assertions.assertThat(provider)
.isInstanceOf(BouncyCastleProvider.class);
|
💔 -1 overall
This message was automatically generated. |
steveloughran
approved these changes
May 13, 2024
|
🎊 +1 overall
This message was automatically generated. |
This has been stuck by |
|
@steveloughran , thanks a lot for reviewing this! |
K0K0V0K
pushed a commit
to K0K0V0K/hadoop
that referenced
this pull request
May 17, 2024
K0K0V0K
pushed a commit
to K0K0V0K/hadoop
that referenced
this pull request
May 17, 2024
pan3793
pushed a commit
to pan3793/hadoop
that referenced
this pull request
Feb 27, 2025
pan3793
pushed a commit
to pan3793/hadoop
that referenced
this pull request
Mar 6, 2025
cnauroth
pushed a commit
that referenced
this pull request
Mar 6, 2025
Co-authored-by: Tsz-Wo Nicholas Sze <[email protected]> Signed-off-by: Chris Nauroth <[email protected]>
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-19152 In order to support different security providers in different clusters, we should not hard code a provider in our code.
How was this patch tested?
Updating existing tests.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?