Created attachment 34633 [details] Proposed patch to add SSL_CIPHER_SUITES property There is a user property "org.apache.tomcat.websocket.SSL_PROTOCOLS" that you can use to provide the list of permitted SSL protocols when connecting to a websocket with WsWebSocketContainer. I was expecting that there would be a similar property to allow you to set the list of permitted SSL cipher suites as well (e.g. "org.apache.tomcat.websocket.SSL_CIPHER_SUITES"), but such a property doesn't exist in 7.0.73. I've attached a proposed patch to add the latter.
Created attachment 34637 [details] Proposed patch to add SSL_CIPHER_SUITES property to 7.0 This is the same patch file as 34633 but with a better naming convention to prevent confusion around version numbers.
Created attachment 34638 [details] Proposed patch to add SSL_CIPHER_SUITES property to 8.5
Created attachment 34639 [details] Proposed patch to add SSL_CIPHER_SUITES property to Trunk (9.0)
Is there any particular reason for this to be a system property? I would much prefer to have TLS protocols and cipher suites configurable via direct mutators (e.g. setCipherSuites(String[])). When I write APIs that need to make outgoing connections, I always allow the client to specify an SSLSocketFactory if they want to do that. In the WS case, we are using an SSLEngine which might be awkward. I'd have to look more at how the WS client is working and how it uses the SSLEngine to decide what makes most sense. Michael, as an end-user, what configuration capabilities would YOU prefer? System properties always seem like a hack that should really be corrected in the actual API.
Hi Christopher, Thanks for the feedback. I'm just following the example set by other configuration entities in this area (e.g. the SSL_PROTOCOLS property). In general I'd much prefer the direct mutators approach as well. In this case, however, the intent is to provide additional configuration capabilities to the generic javax.websocket.* API classes that were missing from that generic API. The choice for a user would be either to use these properties or to abandon the javax.websocket.* classes and instead use Tomcat-specific implementations of them (e.g. org.apache.tomcat.websocket.WsWebSocketContainer). The latter would make the user's code much less portable overall, so I think that using properties here and sticking to the javax interfaces is the better approach. Of course, the best thing would be if protocol/ciphers configurability were to be added to the official java.websocket.* API spec, but I have no idea when that's coming... Michael
(In reply to Michael Orr from comment #5) > In general I'd much prefer the direct mutators approach as well. In this > case, however, the intent is to provide additional configuration > capabilities to the generic javax.websocket.* API classes that were missing > from that generic API. Understood. I think the system-property-based configuration is probably best, then. No need to build infrastructure only to remove it later when the public API improves. > Of course, the best thing would be if protocol/ciphers configurability were > to be added to the official java.websocket.* API spec, but I have no idea > when that's coming... After wrestling with several third-party APIs which didn't support various TLS configuration parameters, I've decided that the only sane way to accomplish it is to allow the caller to supply their own SSLSocketFactory. Otherwise, you end up re-inventing the wheel for everything: protocols, cipher suites, trust stores, key management, certificate revocation lists, hostname verifiers, certificate verifiers, etc. Apache HTTP Components (http-client) stepped on this landmine long ago and it took them several versions to climb out of the hole and remove all of the Apache-specific configuration in favor of the standard SSLSocketFactory. It didn't help that it took the Java API a while to standardize and expose that interface, too. Thanks for your patches!
Yes, SSLSocketFactory would enable you to set all of these in one fell swoop (with the exception of the hostname verifier, which I think is applied after the socket factory has produced the initial socket, but I might be wrong there). Do you think we should abandon/deprecate the current mechanism in favour of this and, if so, shall I withdraw these patches?
I think an SSLEngine is required rather than a socket factory but apart from that I think that is the way to go. The code already started down that route with SSLContext but I think SSLEngine is the right way to do this. Note: I'd deprecate the other constants in 7.0 to 8.5 and remove them entirely in 9.0. The docs will need an appropriate update as well.
Ok, thanks for the feedback. I'll work on a patch to pass in an SSLEngine instance and deprecate/remove the old constants.
Created attachment 35578 [details] Proposed patch to add SSL_ENGINE property to 7.0
Created attachment 35579 [details] Proposed patch to add SSL_ENGINE property to 8.5
Created attachment 35580 [details] Proposed patch to add SSL_ENGINE property to Trunk (9.0)
I've added three new patches for the various versions. You can now pass an SSL_ENGINE property in the user properties when calling WebSocketContainer.connectToServer(). I've marked the other properties (SSL_PROTOCOLS, SSL_TRUST*, and SSL_CONTEXT) as deprecated in all three versions; I assume it's too late to just remove them completely in trunk, now that Tomcat 9 is GA. If my assumption is wrong, or if there are any other changes that should be made, please let me know. Thanks.
Is there any feedback on the attached patch please? Thanks!
Again, can someone please review this?
With Tomcat 7 reaching EOL, move the remaining open enhancement requests to Tomcat 8.
This has been overtaken by events. The WebSocket 2.1 specification added ClientEndpointConfig.getSSLContext() for configuring client TLS connections. Tomcat has adopted this approach.