Expose way to use SSL_CTX_set_cert_cb as general replacement of SSL_C… by normanmaurer · Pull Request #388 · netty/netty-tcnative

normanmaurer · 2018-09-12T10:14:38Z

…TX_set_client_cert_cb and expose some more functions.

Motivation:

Currently we only support SSL_CTX_set_client_cert_cb which only works for client side and so we have no good way to setup certificates on the server-side in a proper way that is "safe". Beside this SSL_CTX_set_client_cert_cb should be considered as "deprecated" and SSL_CTX_set_cert_cb should be use as its replacement.

By using SSL_CTX_set_cert_cb we can also ensure the SSL Hello was send and so the SNI / algorithms etc can be accessed in all cases. Methods for doing so were also mssing.

Modifications:

Add method to make use of SSL_CTX_set_cert_cb and mark the old methods that uses SSL_CTX_set_client_cert_cb as deprecated (+ the old callback interface).
Add method to retrieve requested SNI hostname
Add methods to retrieve the supported signature algorithms by the peer.

Result:

More correct way to hook in validation / keymaterial selection when using client / server mode and ways to retrieve addional infos.

…g 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.

carl-mastrangelo · 2018-09-12T20:32:17Z

openssl-dynamic/src/main/c/ssl.c

+    if (nsig <= 0) {
+        return NULL;
+    }
+    array = (*e)->NewObjectArray(e, nsig, tcn_get_string_class(), NULL);


need to check if the return is null here.

carl-mastrangelo · 2018-09-12T20:33:17Z

openssl-dynamic/src/main/c/ssl.c

+
+    for (i = 0; i < nsig; i++) {
+        SSL_get_sigalgs(ssl_, i, NULL, NULL, &psignhash, NULL, NULL);
+        (*e)->SetObjectArrayElement(e, array, i, (*e)->NewStringUTF(e, OBJ_nid2ln(psignhash)));


Same for NewStringUTF. need to check for null

carl-mastrangelo · 2018-09-12T20:37:38Z

openssl-dynamic/src/main/c/sslcontext.c

+    }
+#endif // OPENSSL_IS_BORINGSSL
+
+    tcn_ssl_ctxt_t *c = tcn_SSL_get_app_data2(ssl);


This can return null, so I think you need to check this.

let me put an asset as it should really never

carl-mastrangelo · 2018-09-12T20:37:54Z

openssl-dynamic/src/main/c/sslcontext.c

+            return 0;
+        }
+
+        issuers = principalBytes(e,  SSL_get_client_CA_list(ssl));


extra space after comma

openssl-dynamic/src/main/java/io/netty/internal/tcnative/CertificateCallback.java

carl-mastrangelo · 2018-09-12T20:47:39Z

openssl-dynamic/src/main/java/io/netty/internal/tcnative/CertificateCallback.java

+     * The types contained in the {@code keyTypeBytes} array.
+     */
+    // Extracted from https://github.com/openssl/openssl/blob/master/include/openssl/tls1.h
+    byte TLS_CT_RSA_SIGN = 1;


Also, why not an enum?

because we really want to access the byte value later on.

I see. I was thinking (optional of course) something like:

enum KeyTypes { TLS_CT_RSA_SIGN(1), TLS_CT_RSA_FIXED_DH(4); // ... byte getByte() { // ... } }

This would allow handle() to be an EnumSet, and no risk of unsupported bytes being pased in.

I think I will keep it as byte for now to reduce garbage / and keep it simple in JNI

carl-mastrangelo · 2018-09-12T21:32:47Z

openssl-dynamic/src/main/c/sslcontext.c

+    if (callback == NULL) {
+        SSL_CTX_set_cert_cb(c->ctx, NULL, NULL);
+    } else {
+        jclass callback_class = (*e)->GetObjectClass(e, callback);


check not null

…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-14T13:06:28Z

@carl-mastrangelo @ejona86 any more comments ? I would love to pull this in as I plan to open another PR after this that will add support for TLSv1.3 when using OpenSSL 1.1.1. This will help me to not get into merge hell :)

carl-mastrangelo

Two comments, but LGTM

carl-mastrangelo · 2018-09-14T17:50:06Z

openssl-dynamic/src/main/java/io/netty/internal/tcnative/CertificateCallback.java

+     * The types contained in the {@code keyTypeBytes} array.
+     */
+    // Extracted from https://github.com/openssl/openssl/blob/master/include/openssl/tls1.h
+    byte TLS_CT_RSA_SIGN = 1;


I see. I was thinking (optional of course) something like:

enum KeyTypes { TLS_CT_RSA_SIGN(1), TLS_CT_RSA_FIXED_DH(4); // ... byte getByte() { // ... } }

This would allow handle() to be an EnumSet, and no risk of unsupported bytes being pased in.

…TX_set_client_cert_cb and expose some more functions. Motivation: Currently we only support SSL_CTX_set_client_cert_cb which only works for client side and so we have no good way to setup certificates on the server-side in a proper way that is "safe". Beside this SSL_CTX_set_client_cert_cb should be considered as "deprecated" and SSL_CTX_set_cert_cb should be use as its replacement. By using SSL_CTX_set_cert_cb we can also ensure the SSL Hello was send and so the SNI / algorithms etc can be accessed in all cases. Methods for doing so were also mssing. Modifications: - Add method to make use of SSL_CTX_set_cert_cb and mark the old methods that uses SSL_CTX_set_client_cert_cb as deprecated (+ the old callback interface). - Add method to retrieve requested SNI hostname - Add methods to retrieve the supported signature algorithms by the peer. Result: More correct way to hook in validation / keymaterial selection when using client / server mode and ways to retrieve addional infos.

…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.

…ng server-side and support more methods of ExtendedSSLSession. (#8283) 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.

…TX_set_client_cert_cb and expose some more functions. (netty#388) Motivation: Currently we only support SSL_CTX_set_client_cert_cb which only works for client side and so we have no good way to setup certificates on the server-side in a proper way that is "safe". Beside this SSL_CTX_set_client_cert_cb should be considered as "deprecated" and SSL_CTX_set_cert_cb should be use as its replacement. By using SSL_CTX_set_cert_cb we can also ensure the SSL Hello was send and so the SNI / algorithms etc can be accessed in all cases. Methods for doing so were also mssing. Modifications: - Add method to make use of SSL_CTX_set_cert_cb and mark the old methods that uses SSL_CTX_set_client_cert_cb as deprecated (+ the old callback interface). - Add method to retrieve requested SNI hostname - Add methods to retrieve the supported signature algorithms by the peer. Result: More correct way to hook in validation / keymaterial selection when using client / server mode and ways to retrieve addional infos.

normanmaurer force-pushed the cert_cb branch from 59ad75b to 5325846 Compare September 12, 2018 10:33

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

normanmaurer mentioned this pull request Sep 12, 2018

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

Merged

carl-mastrangelo reviewed Sep 12, 2018

View reviewed changes

carl-mastrangelo approved these changes Sep 14, 2018

View reviewed changes

normanmaurer self-assigned this Sep 14, 2018

normanmaurer added improvement feature labels Sep 14, 2018

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

normanmaurer force-pushed the cert_cb branch from c0a6bf1 to 27bfb90 Compare September 14, 2018 19:14

normanmaurer force-pushed the cert_cb branch from 27bfb90 to b87b71c Compare September 14, 2018 19:32

normanmaurer merged commit 1d3d6bd into master Sep 14, 2018

normanmaurer deleted the cert_cb branch September 14, 2018 20:04

Uh oh!

Conversation

normanmaurer commented Sep 12, 2018

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants