server sni support - Githubissues

jalberti commented 3 years ago

depends on https://github.com/wildfly-security/wildfly-openssl-natives/pull/7

jalberti commented 3 years ago

jalberti commented 2 years ago

@fjuma we were running into some OOM issues due to huge amounts of sessions with client certificates in them being leaked, I updated this PR and see also https://github.com/wildfly-security/wildfly-openssl-natives/pull/7. Thanks for your consideration.

fjuma commented 2 years ago

@jalberti Thanks again for working on this and apologies for the delay in getting feedback on this one!

It would be good to add test cases for this. This will also help us to better understand how things are expected to be configured. As an example, how does one specify which certificate to use for a specific host name?

jalberti commented 2 years ago

@fjuma thanks for your response. I was looking into the test cases that exist, and I was honestly not sure how I would be able to add test cases. We can look into that, I agree, it would be good to have this.

To answer your question. The way SNI works, it is the client that uses a TLS extension to send the servername, which the callback receives, which in turn will find the match based on the certificate content.

See SNIMatcher ... https://github.com/wildfly-security/wildfly-openssl/pull/99/files#diff-960e4df8ba6ec1c7184745cba9b81f6156827aae30b902959cfb2bb362916e0cR585

jalberti commented 2 years ago

fjuma commented 2 years ago

Thanks for your response, @jalberti.

Within Elytron and within WildFly, our SNI support involves explicitly configuring which SSL context to use for a given a host name.

Some examples can be found here:

In your implementation, I noticed a x509CertificateToSSLContextMap. However, it doesn't look like that's something that's being configured explicitly. It looks like that's being created dynamically?

jalberti commented 2 years ago

@fjuma correct, it is automatic. When we load the keystore we enumerate the certificates (keys) present. We end up with a map that maps certificate to ssl context, we then during runtime provide openssl via its sni callback the ctx ptr, the snimatcher finds the best match certificate to use based on the hostname presented by the client, openssl uses the ptr as a copy template for the session to use.

That's how openssl works with SNI, nothing proprietary. We just provided the missing wires, so the flow can work end to end. Your current implementation simply never provided openssl a different cert/key to use in the callback, because the callback was not implemented correctly.

If there is only a single cert, or the SNImatcher does not find a match, the behavior is unchanged, the first cert or last cert in the keystore is used (I don't remember, I'm typing this on my phone, but I think we made sure that the behavior is essentially unchanged). This change in this PR should make things better, it should not change behavior, unless you strictly do not want SNI, but then, why would you present a keystore with multiple certificates to the openssl stack.

All the other changes are just cleanup to eliminate memory leaks due to jni references not being collected, as the GC could not reach them.

I hope this explains better what's contained in here. Thank you for your consideration.

fjuma commented 2 years ago

Thanks for that explanation, that's quite useful.

Looking closer at the existing SNI code, it was originally forked from https://github.com/apache/tomcat-native. I'm wondering if you've taken a look at the latest tomcat-native code to see how/why the SNI callback is working correctly there. Ideally, it would be nice for the updates to the SNI implementation in the wildfly-openssl code base to align with the implementation from tomcat-native since that's what it was based on.

@rmartinc Just wanted to ping you as well on this one to see if you have any thoughts on this too.

jalberti commented 2 years ago

Hi @fjuma, yes, I recognized the fact that wildfly is a fork, and yes I looked at the original source when we looked at the wildfly issues with SNI. If I recall correctly, it looked more like it was broken in the process of some refactoring, i.e. lack of testing might be the real reason why this was never working.

fjuma commented 2 years ago

Thanks @jalberti. I think I'd like to better understand if we can just add missing pieces directly from tomcat-native or if things are broken there too.

In the meantime, it would be good to add tests.

jalberti commented 2 years ago

@fjuma as mentioned, when your team forked it, you must have broken it. tc-native works, but can only be used in Tomcat, see also https://stackoverflow.com/questions/20190464/howto-setup-tomcat-serving-two-ssl-certificates-using-sni. In addition to fixing the SNI issues, our PR also fixes memory leaks/jni reference leaks in your code. If you want to make an assessment if you can fix the issues we have fixed in a different way by yourself, that would be great. Unless you are willing to accept our contributions, I'm not sure we have capacity to develop tests for your project.

fjuma commented 2 years ago

@jalberti We could consider adding a system property (say org.wildfly.openssl.sni) that could be used to enable the support for SNI. That way, we could have the system property set to false by default and it could be explicitly enabled for anyone who wants to try it out. That would allow us to merge your implementation and then follow up on the approach used when time permits.

The majority of your changes are in the OpenSSLContextSPI#init method so we could potentially introduce a new method with those changes (say initWithSniSupport) and then update OpenSSLContextSPI#engineInit so that it calls initWithSniSupport if System.getProperty(org.wildfly.openssl.sni) returns true and calls the existing init method otherwise.

The steps to move forward with this would then be as follows:

1) Update the implementation so that the SNI support is enabled when the org.wildfly.openssl.sni system property is set to true. I've described most of the changes needed for this above. SSL#sniCallBack would also need to be updated to return currentCtx if the system property is set to true. (I'm happy to help with this and can make this change if you don't have time.)

2) Some tests should be added (the tests shouldn't depend on the implementation anyway so even if we were to update this at some point, the tests shouldn't be affected).

3) The fixes for memory leaks/jni reference leaks included in this PR should be split out into a separate commit just to make things cleaner.

What do you think? Would you be interested in moving forward this way?

fjuma commented 2 years ago

@jalberti Glad this approach sounds good to you! Feel free to let us know if help is needed with any of the 3 steps.

wildfly-security / wildfly-openssl

server sni support #99