Ensure X509KeyManager methods are called on the correct time when using server-side and support more methods of ExtendedSSLSession. by normanmaurer · Pull Request #8283 · netty/netty

normanmaurer · 2018-09-12T10:41:28Z

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 https://github.com/netty/netty-tcnative/pull/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.

normanmaurer · 2018-09-12T10:42:21Z

This depends on netty/netty-tcnative#388 and #8281

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

carl-mastrangelo · 2018-09-12T21:44:59Z

handler/src/main/java/io/netty/handler/ssl/SignatureAlgorithmConverter.java

+
+    private SignatureAlgorithmConverter() { }
+
+    // OpenSSL has 3 different formats it uses at the moment we will match against all of these.


Why not just enumerate them in the regex then?

@carl-mastrangelo not sure I understand... Can you show me some code ?

Make the regex "^(SHA256withECDSA|SHA512withRSA|SHA256withDSA)$"

Ah ok... maybe the comment is misleading. It supports more then 3 algorithms. It has just 3 different way to format the supported algorithms

I guess the comment is correct - it says 'formats' which is not 'algorithms', but it's probably better giving some examples in the comment.

carl-mastrangelo · 2018-09-12T21:45:30Z

handler/src/main/java/io/netty/handler/ssl/SignatureAlgorithmConverter.java

+        if (matcher.matches()) {
+            String group2 = matcher.group(2);
+            if (group2 != null) {
+                return group2.toUpperCase(Locale.US) + "with" + matcher.group(3).toUpperCase(Locale.US);


You probably want Locale.ROOT I think

addressed and rebased.

normanmaurer · 2018-09-14T13:02:18Z

@carl-mastrangelo PTAL again

carl-mastrangelo

LGTM

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

trustin

Mostly nits

trustin · 2018-09-27T13:00:58Z

handler/src/main/java/io/netty/handler/ssl/OpenSslKeyMaterialManager.java

nit: Better leaving this as is?

trustin · 2018-09-27T13:01:14Z

handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java

        try {
-            ServerContext context = newSessionContext(this, ctx, engineMap, trustCertCollection, trustManagerFactory,
+            sessionContext = newSessionContext(this, ctx, engineMap, trustCertCollection, trustManagerFactory,
                                                      keyCertChain, key, keyPassword, keyManagerFactory);


Indentation

trustin · 2018-09-27T13:02:51Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

+                @Override
+                public String[] getPeerSupportedSignatureAlgorithms() {
+                    synchronized (ReferenceCountedOpenSslEngine.this) {
+                        if (peerSupportedSignatureAlgorithms == null) {


More readable by returning early when peerSupportedSignatureAlgorithms != null?

I dont think so

trustin · 2018-09-27T13:03:28Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

+                    synchronized (ReferenceCountedOpenSslEngine.this) {
+                        if (peerSupportedSignatureAlgorithms == null) {
+                            if (isDestroyed()) {
+                                peerSupportedSignatureAlgorithms = EmptyArrays.EMPTY_STRINGS;


No need to clone after this

See https://github.com/netty/netty/pull/8283/files#r220914035

trustin · 2018-09-27T13:03:45Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

+                            } else {
+                                String[] algs = SSL.getSigAlgs(ssl);
+                                if (algs == null) {
+                                    peerSupportedSignatureAlgorithms = EmptyArrays.EMPTY_STRINGS;


No need to clone since the array is empty

cloning an empty array is super cheap and it simplifies the flow of the method so let me leave the method like it is.

trustin · 2018-09-27T13:06:14Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java

+                // OpenJDK SSLEngineImpl does.
+                keyManagerHolder.setKeyMaterialServerSide(engine);
+            } catch (Throwable cause) {
+                logger.debug("request of key failed", cause);


How about: `"Failed to set the server-side key material:", cause

trustin · 2018-09-27T13:07:36Z

handler/src/main/java/io/netty/handler/ssl/SignatureAlgorithmConverter.java

+
+    private SignatureAlgorithmConverter() { }
+
+    // OpenSSL has 3 different formats it uses at the moment we will match against all of these.


I guess the comment is correct - it says 'formats' which is not 'algorithms', but it's probably better giving some examples in the comment.

trustin · 2018-09-27T13:08:06Z

handler/src/main/java/io/netty/handler/ssl/SignatureAlgorithmConverter.java

+        if (matcher.matches()) {
+            String group2 = matcher.group(2);
+            if (group2 != null) {
+                return group2.toUpperCase(Locale.ROOT) + "with" + matcher.group(3).toUpperCase(Locale.ROOT);


Learned Locale.ROOT just today. 👍

trustin · 2018-09-27T13:08:25Z

handler/src/test/java/io/netty/handler/ssl/OpenSslTestUtils.java

    }
+
+    static boolean isBoringSSL() {
+        return "BoringSSL".equals(OpenSsl.versionString());


Curious if we should be case-insensitive here.

I think its fine ...

trustin · 2018-09-27T13:08:50Z

handler/src/test/java/io/netty/handler/ssl/SignatureAlgorithmConverterTest.java

+
+    @Test
+    public void testInvalid() {
+        assertNull(SignatureAlgorithmConverter.toJavaName("ThisIsSomeThingInvalid"));


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.

normanmaurer · 2018-09-28T09:32:07Z

I will merge this once the CI is done

normanmaurer changed the base branch from 4.1 to extended_ssl_session September 12, 2018 10:41

normanmaurer requested review from Scottmitch, bryce-anderson, carl-mastrangelo, ejona86 and trustin September 12, 2018 10:42

normanmaurer force-pushed the correctly_use_keymanager_server_side branch 2 times, most recently from d867eff to 23fcb7e Compare September 12, 2018 10:45

carl-mastrangelo reviewed Sep 12, 2018

View reviewed changes

normanmaurer force-pushed the correctly_use_keymanager_server_side branch from 23fcb7e to f24e5cf Compare September 14, 2018 13:01

normanmaurer changed the base branch from extended_ssl_session to 4.1 September 14, 2018 13:02

normanmaurer added this to the 4.1.30.Final milestone Sep 14, 2018

normanmaurer added defect improvement feature labels Sep 14, 2018

carl-mastrangelo approved these changes Sep 14, 2018

View reviewed changes

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java Show resolved Hide resolved

normanmaurer force-pushed the correctly_use_keymanager_server_side branch from f24e5cf to 239dc86 Compare September 14, 2018 19:43

normanmaurer modified the milestones: 4.1.30.Final, 4.1.31.Final Sep 25, 2018

trustin reviewed Sep 27, 2018

View reviewed changes

normanmaurer force-pushed the correctly_use_keymanager_server_side branch from 239dc86 to 00ca995 Compare September 28, 2018 08:52

normanmaurer merged commit 59973e9 into 4.1 Sep 28, 2018

normanmaurer deleted the correctly_use_keymanager_server_side branch September 28, 2018 09:34

mihalyr mentioned this pull request Dec 10, 2019

Synchronously close pools when closing AbstractChannelPoolMap #9857

Merged


		private SignatureAlgorithmConverter() { }

		// OpenSSL has 3 different formats it uses at the moment we will match against all of these.

Uh oh!

Conversation

normanmaurer commented Sep 12, 2018

Uh oh!

normanmaurer commented Sep 12, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Sep 14, 2018

Uh oh!

carl-mastrangelo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trustin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Sep 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants