-
-
Notifications
You must be signed in to change notification settings - Fork 15.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for specifying SecureRandom in SSLContext initialization #14058
Add support for specifying SecureRandom in SSLContext initialization #14058
Conversation
/** | ||
* @param secureRandom the source of randomness for SslContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | |
* @param secureRandom the source of randomness for SslContext | |
/** | |
* Specify a non-default source of randomness for the {@link SSLContext}. | |
* <p> | |
* In general, the best practice is to leave this unspecified, or to assign a new random source using the | |
* default {@code new SecureRandom()} constructor. | |
* Only assign this something when you have a good reason to. | |
* | |
* @param secureRandom the source of randomness for {@link SSLContext} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this explanation is very good. 56bee2e
My understanding is that this is only used in the JDK SslProvider (JdkSslServerContext JdkSslClientContext), is that correct? If so, what do you think about adding this to the comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thxwelchs please add this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added!
4408ced
netty/handler/src/main/java/io/netty/handler/ssl/SslContextBuilder.java
Lines 606 to 612 in 047d95a
* Specify a non-default source of randomness for the {@link JdkSslContext} | |
* <p> | |
* In general, the best practice is to leave this unspecified, or to assign a new random source using the | |
* default {@code new SecureRandom()} constructor. | |
* Only assign this something when you have a good reason to. | |
* | |
* @param secureRandom the source of randomness for {@link JdkSslContext} |
handler/src/test/java/io/netty/handler/ssl/SslContextBuilderTest.java
Outdated
Show resolved
Hide resolved
The JDK 17+ builds are actually failing on the new test, because Mockito can't mock
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid mocking SecureRandom
939a083
to
608bf20
Compare
handler/src/test/java/io/netty/handler/ssl/SslContextBuilderTest.java
Outdated
Show resolved
Hide resolved
handler/src/test/java/io/netty/handler/ssl/SslContextBuilderTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small comments, but otherwise looks good
} | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
/** | |
} | |
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
@@ -20,6 +20,7 @@ | |||
import io.netty.util.CharsetUtil; | |||
import org.junit.jupiter.api.Test; | |||
import org.junit.jupiter.api.function.Executable; | |||
import org.mockito.Mockito; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import org.mockito.Mockito; |
We don't need to import mockito anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
@chrisvest @normanmaurer |
@thxwelchs thanks a lot! |
…14058) Motivation: Enhance security by supporting specific secure randomness source in SSLContext initialization Modification: Support building SecureRandom in `SslContextBuilder`. Allow passing SecureRandom as a parameter when creating an instance of `JdkSslServerContext` through its constructor. Result: Enhance security Fixes #14026 --------- Co-authored-by: Norman Maurer <norman_maurer@apple.com>
…14058) Motivation: Enhance security by supporting specific secure randomness source in SSLContext initialization Modification: Support building SecureRandom in `SslContextBuilder`. Allow passing SecureRandom as a parameter when creating an instance of `JdkSslServerContext` through its constructor. Result: Enhance security Fixes #14026 --------- Co-authored-by: Norman Maurer <norman_maurer@apple.com>
### What changes were proposed in this pull request? The pr aims to upgrade `netty` from `4.1.109.Final` to `4.1.110.Final`. ### Why are the changes needed? - https://netty.io/news/2024/05/22/4-1-110-Final.html This version has brought some bug fixes and improvements, such as: Fix Zstd throws Exception on read-only volumes (netty/netty#13982) Add unix domain socket transport in netty 4.x via JDK16+ ([#13965](netty/netty#13965)) Backport #13075: Add the AdaptivePoolingAllocator ([#13976](netty/netty#13976)) Add no-value key handling only for form body ([#13998](netty/netty#13998)) Add support for specifying SecureRandom in SSLContext initialization ([#14058](netty/netty#14058)) - https://github.com/netty/netty/issues?q=milestone%3A4.1.110.Final+is%3Aclosed ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46744 from panbingkun/SPARK-48420. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
### What changes were proposed in this pull request? The pr aims to upgrade `netty` from `4.1.109.Final` to `4.1.110.Final`. ### Why are the changes needed? - https://netty.io/news/2024/05/22/4-1-110-Final.html This version has brought some bug fixes and improvements, such as: Fix Zstd throws Exception on read-only volumes (netty/netty#13982) Add unix domain socket transport in netty 4.x via JDK16+ ([apache#13965](netty/netty#13965)) Backport apache#13075: Add the AdaptivePoolingAllocator ([apache#13976](netty/netty#13976)) Add no-value key handling only for form body ([apache#13998](netty/netty#13998)) Add support for specifying SecureRandom in SSLContext initialization ([apache#14058](netty/netty#14058)) - https://github.com/netty/netty/issues?q=milestone%3A4.1.110.Final+is%3Aclosed ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46744 from panbingkun/SPARK-48420. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
Motivation:
Enhance security by supporting specific secure randomness source in SSLContext initialization
Modification:
Support building SecureRandom in
SslContextBuilder
.Allow passing SecureRandom as a parameter when creating an instance of
JdkSslServerContext
through its constructor.Result:
Enhance security
Fixes #14026