-
-
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
Ensure X509KeyManager methods are called on the correct time when using server-side and support more methods of ExtendedSSLSession. #8283
Conversation
normanmaurer
commented
Sep 12, 2018
This depends on netty/netty-tcnative#388 and #8281 |
d867eff
to
23fcb7e
Compare
handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
Show resolved
Hide resolved
|
||
private SignatureAlgorithmConverter() { } | ||
|
||
// OpenSSL has 3 different formats it uses at the moment we will match against all of these. |
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.
Why not just enumerate them in the regex then?
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.
@carl-mastrangelo not sure I understand... Can you show me some code ?
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.
Make the regex "^(SHA256withECDSA|SHA512withRSA|SHA256withDSA)$"
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.
Ah ok... maybe the comment is misleading. It supports more then 3 algorithms. It has just 3 different way to format the supported algorithms
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 guess the comment is correct - it says 'formats' which is not 'algorithms', but it's probably better giving some examples in the comment.
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.
done
if (matcher.matches()) { | ||
String group2 = matcher.group(2); | ||
if (group2 != null) { | ||
return group2.toUpperCase(Locale.US) + "with" + matcher.group(3).toUpperCase(Locale.US); |
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.
You probably want Locale.ROOT I think
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.
addressed and rebased.
23fcb7e
to
f24e5cf
Compare
@carl-mastrangelo PTAL again |
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.
LGTM
handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
Show resolved
Hide resolved
f24e5cf
to
239dc86
Compare
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.
Mostly nits
@@ -115,7 +104,6 @@ void setKeyMaterialClientSide(ReferenceCountedOpenSslEngine engine, long certOut | |||
} | |||
} | |||
} | |||
|
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.
nit: Better leaving this as is?
@@ -349,10 +347,8 @@ private OpenSslServerContext( | |||
// Create a new SSL_CTX and configure it. | |||
boolean success = false; | |||
try { | |||
ServerContext context = newSessionContext(this, ctx, engineMap, trustCertCollection, trustManagerFactory, | |||
sessionContext = newSessionContext(this, ctx, engineMap, trustCertCollection, trustManagerFactory, | |||
keyCertChain, key, keyPassword, keyManagerFactory); |
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.
Indentation
@Override | ||
public String[] getPeerSupportedSignatureAlgorithms() { | ||
synchronized (ReferenceCountedOpenSslEngine.this) { | ||
if (peerSupportedSignatureAlgorithms == null) { |
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.
More readable by returning early when peerSupportedSignatureAlgorithms != null
?
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 dont think so
synchronized (ReferenceCountedOpenSslEngine.this) { | ||
if (peerSupportedSignatureAlgorithms == null) { | ||
if (isDestroyed()) { | ||
peerSupportedSignatureAlgorithms = EmptyArrays.EMPTY_STRINGS; |
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.
No need to clone after 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.
} else { | ||
String[] algs = SSL.getSigAlgs(ssl); | ||
if (algs == null) { | ||
peerSupportedSignatureAlgorithms = EmptyArrays.EMPTY_STRINGS; |
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.
No need to clone since the array is empty
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.
cloning an empty array is super cheap and it simplifies the flow of the method so let me leave the method like it is.
// OpenJDK SSLEngineImpl does. | ||
keyManagerHolder.setKeyMaterialServerSide(engine); | ||
} catch (Throwable cause) { | ||
logger.debug("request of key failed", cause); |
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.
How about: `"Failed to set the server-side key material:", cause
|
||
private SignatureAlgorithmConverter() { } | ||
|
||
// OpenSSL has 3 different formats it uses at the moment we will match against all of these. |
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 guess the comment is correct - it says 'formats' which is not 'algorithms', but it's probably better giving some examples in the comment.
if (matcher.matches()) { | ||
String group2 = matcher.group(2); | ||
if (group2 != null) { | ||
return group2.toUpperCase(Locale.ROOT) + "with" + matcher.group(3).toUpperCase(Locale.ROOT); |
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.
Learned Locale.ROOT
just today. 👍
@@ -24,4 +24,8 @@ private OpenSslTestUtils() { | |||
static void checkShouldUseKeyManagerFactory() { | |||
assumeTrue(OpenSsl.supportsKeyManagerFactory() && OpenSsl.useKeyManagerFactory()); | |||
} | |||
|
|||
static boolean isBoringSSL() { | |||
return "BoringSSL".equals(OpenSsl.versionString()); |
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.
Curious if we should be case-insensitive here.
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 its fine ...
|
||
@Test | ||
public void testInvalid() { | ||
assertNull(SignatureAlgorithmConverter.toJavaName("ThisIsSomeThingInvalid")); |
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.
nit: SomeThing -> Something
…ng server-side and support more methods of ExtendedSSLSession. Motivation: Before when on server-side we just called the X509KeyManager methods when handshake() was called the first time which is not quite correct as we may not have received the full SSL hello / handshake and so could not extra for example the SNI hostname that was requested. OpenSSL exposes the SSL_CTX_set_cert_cb function which allows to set a callback which is executed at the correct moment, so we should use it. This also allows us to support more methods of ExtendedSSLSession easily. Modifications: - Make use of new methods exposed by netty-tcnative since netty/netty-tcnative#388 to ensure we select the key material at the correct time. - Implement more methods of ExtendedOpenSslSession - Add unit tests to ensure we are able to retrieve various things on server-side in the X509KeyManager and so verify it is called at the correct time. - Simplify code by using new netty-tcnative methods. Result: More correct implementation for server-side usage and more complete implemented of ExtendedSSLSession.
239dc86
to
00ca995
Compare
I will merge this once the CI is done |