-
-
Notifications
You must be signed in to change notification settings - Fork 176
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
Expose way to use SSL_CTX_set_cert_cb as general replacement of SSL_C… #388
Conversation
59ad75b
to
5325846
Compare
…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.
if (nsig <= 0) { | ||
return NULL; | ||
} | ||
array = (*e)->NewObjectArray(e, nsig, tcn_get_string_class(), 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.
need to check if the return is null 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.
done
openssl-dynamic/src/main/c/ssl.c
Outdated
|
||
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))); |
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.
Same for NewStringUTF. need to check for null
} | ||
#endif // OPENSSL_IS_BORINGSSL | ||
|
||
tcn_ssl_ctxt_t *c = tcn_SSL_get_app_data2(ssl); |
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.
This can return null, so I think you need to check 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.
let me put an asset as it should really never
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
return 0; | ||
} | ||
|
||
issuers = principalBytes(e, SSL_get_client_CA_list(ssl)); |
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.
extra space after comma
openssl-dynamic/src/main/java/io/netty/internal/tcnative/CertificateCallback.java
Show resolved
Hide resolved
* 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; |
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.
Also, why not an enum?
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.
because we really want to access the byte value later on.
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 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.
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 I will keep it as byte for now to reduce garbage / and keep it simple in JNI
if (callback == NULL) { | ||
SSL_CTX_set_cert_cb(c->ctx, NULL, NULL); | ||
} else { | ||
jclass callback_class = (*e)->GetObjectClass(e, callback); |
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.
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.
@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 :) |
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 comments, but LGTM
* 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; |
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 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.
c0a6bf1
to
27bfb90
Compare
…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.
27bfb90
to
b87b71c
Compare
…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. 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.
…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:
Result:
More correct way to hook in validation / keymaterial selection when using client / server mode and ways to retrieve addional infos.