-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Potential Regression in 8.1.3 (#4c16ca8) #362
Comments
Thank you for the detailed issue description. This issue is related to the changes from the following PR: #360 I have cloned javalin-ssl to analyse the issue on my machine and I can confirm the same issue. Although the failing test on your side is now occuring duo the latest version of this library it is not actually a bug or a regression. Let me try to explain it. The test is failing because it is saying By default with the default security provider of Java these two Ciphers are supported, the default supported list contains the following ciphers:
The failing ones are actually supported, however the custom security provider which is configured in
The second list of ciphers does not contain Probably you are curious why it was not failing before. The main reason is that the configured ciphers within the SSLFactory was not being used within the SSLContext. It was provided as a separate object as SSLParameters and it was provided within the SSLEngine, SSLSocketFactory and SSLServerSockerFactory however these 3 objects are being constructed within the SSLFactory. The latest code changes delegated the creation of these 3 objects to the SSLContext which will now use the custom ciphers suite. So your TLSConfig with a list of ciphers and protocols was being ignored when creating a server with TLS because it was creating the Server with the default SSLContext which was not able to use the custom ciphers in the earlier releases. So what can I do on my side? I can filter out the not supported ciphers by explicitly validating which are not supported and log it. This will prevent you and the end-users of having a runtime exception and if they want to analyse the ssl configuration they can still see the not supported and filtered out ciphers in the logs. What do you think? How shall we proceed and does it make sense to you? |
I don't want to change my TLSConfig because it's just a copy of Mozilla's ciphers recommendations. I was under the impression Conscrypt was internally used by your library, it this is not the case I don't see why should I have a custom security provider instead of relying on the default one. I remember having some issue, but not what it was. I will have a look at it tomorrow and get back to you.
Thank you very much for your detailed and quick response. What I understand is that this was a bug (or rather an unwanted/strange) behavior where ciphers were being ignored. My intended use would be to provide a list of allowed ciphers rather than a strict set that must be present. It is fine if you don't want to change the API, I don't really mind how you implement it. It might be an option adding some kind of verb to specify when they're strict or not, something like |
What I will do atleast is to ignore the not supported ciphers if a custom security provider is configured which does not have matching ciphers. That will prevent a runtime exception and atleast attempt a ssl handshake with the remaining possible ciphers. I will attempt to release it this weak. I will keep you updated.
Actually the current setup works like that already. If you specify a cipher it will only use those option you have specified.
No worries, I will make sure it will be smooth as before and more solid then ever. |
I made a new release yesterday and the issue should be resolved. The only suggestion I would provide is if you want to keep using the same ciphers then you should drop using that custom security provider. Can you give a try with the latest version? |
Still, it is my understanding that different JVM implementations offer different security providers, with no standard set of supported ciphers and protocols that could be known in advance. Sometimes JVMs don't even follow the RFC names! Why would a runtime exception be raised when a cipher is not available? The reason for having a set of ciphers instead of one is for compatibility and flexibility with the server and client, it is fine if some of them aren't implemented in the server. It would be the opposite case, only when using a custom security provider, that one could actually check in advance what ciphers are available to use. I understand providing an API to have a strict check, but that can easily be done by the user by just checking the provider. Am I missing something here that I don't understand? |
Great to hear, thank you for confirming!
I think Oracle just provides specifications (JEPs) and a reference implementation (the OpenJDK) other vendors can provide their implementation for those specifications and in that way they are free to choose what part to implement. I would assume some have more or maybe less supported ciphers. So I am not sure what is best for the end-user... I would say having proper integration test would be really helpfull to just validate whether the server of client is actually using the requested cipher or else use a different security provider or another JDK from a different vendor.
Yes it feels a bit redundant. It would be better to ignore the unsupported ones and just stick with the ones which are supported and attempt an ssl handshake. I am also seeing this runtime exception for the first time. |
I understand it's the end user responsibility to validate their own JVM, as library developers we can't possibly make assumptions about what will be supported by the implementation and whatnot, I just wanted to provide a simple configuration following Mozilla's recommendations. Thank you very much for your detailed and quick responses, I am glad this is solved now! |
Yes it is indeed the responsibility of the end user as we as a library provider don't have the controle of this particular topic. The only thin we might be able to do is inform the user with either debug or warn log messages in my opinion. I think you did well to have a default configuration based on Mozilla's recommendation which will be used if available or it will use now a subset of whatever is available which is still fine as it is using your configuration. |
Describe the bug
Previously supported cipher suites are no longer supported since 8.1.3. I have not had time to debug this yet, but I am opening this issue to track the problem, it did not exist in 8.1.2.
To Reproduce
Running the test suite in the actions runner fails with the following error:
Environmental Data:
Additional context
Actions Runner Logs: https://github.com/javalin/javalin-ssl/actions/runs/5606563554?pr=90
Ubuntu JDK18 runner logs: logs_491.zip
The text was updated successfully, but these errors were encountered: