Bug 60597 - Add ability to set cipher suites for websocket client connections
Summary: Add ability to set cipher suites for websocket client connections
Status: NEW
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: WebSocket (show other bugs)
Version: 7.0.73
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2017-01-17 17:22 UTC by Michael Orr
Modified: 2020-01-02 16:03 UTC (History)
1 user (show)



Attachments
Proposed patch to add SSL_CIPHER_SUITES property (1.78 KB, patch)
2017-01-17 17:22 UTC, Michael Orr
Details | Diff
Proposed patch to add SSL_CIPHER_SUITES property to 7.0 (1.78 KB, patch)
2017-01-18 11:00 UTC, Michael Orr
Details | Diff
Proposed patch to add SSL_CIPHER_SUITES property to 8.5 (2.25 KB, patch)
2017-01-18 11:00 UTC, Michael Orr
Details | Diff
Proposed patch to add SSL_CIPHER_SUITES property to Trunk (9.0) (2.26 KB, patch)
2017-01-18 11:01 UTC, Michael Orr
Details | Diff
Proposed patch to add SSL_ENGINE property to 7.0 (3.11 KB, patch)
2017-12-02 22:27 UTC, Michael Orr
Details | Diff
Proposed patch to add SSL_ENGINE property to 8.5 (3.19 KB, patch)
2017-12-02 22:27 UTC, Michael Orr
Details | Diff
Proposed patch to add SSL_ENGINE property to Trunk (9.0) (3.19 KB, patch)
2017-12-02 22:28 UTC, Michael Orr
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Orr 2017-01-17 17:22:04 UTC
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.
Comment 1 Michael Orr 2017-01-18 11:00:19 UTC
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.
Comment 2 Michael Orr 2017-01-18 11:00:39 UTC
Created attachment 34638 [details]
Proposed patch to add SSL_CIPHER_SUITES property to 8.5
Comment 3 Michael Orr 2017-01-18 11:01:01 UTC
Created attachment 34639 [details]
Proposed patch to add SSL_CIPHER_SUITES property to Trunk (9.0)
Comment 4 Christopher Schultz 2017-01-18 20:33:33 UTC
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.
Comment 5 Michael Orr 2017-01-18 23:49:03 UTC
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
Comment 6 Christopher Schultz 2017-01-19 19:56:49 UTC
(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!
Comment 7 Michael Orr 2017-01-24 12:52:01 UTC
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?
Comment 8 Mark Thomas 2017-01-27 14:03:27 UTC
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.
Comment 9 Michael Orr 2017-02-06 18:11:00 UTC
Ok, thanks for the feedback.  I'll work on a patch to pass in an SSLEngine instance and deprecate/remove the old constants.
Comment 10 Michael Orr 2017-12-02 22:27:01 UTC
Created attachment 35578 [details]
Proposed patch to add SSL_ENGINE property to 7.0
Comment 11 Michael Orr 2017-12-02 22:27:45 UTC
Created attachment 35579 [details]
Proposed patch to add SSL_ENGINE property to 8.5
Comment 12 Michael Orr 2017-12-02 22:28:12 UTC
Created attachment 35580 [details]
Proposed patch to add SSL_ENGINE property to Trunk (9.0)
Comment 13 Michael Orr 2017-12-02 22:35:52 UTC
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.
Comment 14 Michael Orr 2018-03-08 19:58:07 UTC
Is there any feedback on the attached patch please?  Thanks!
Comment 15 Michael Orr 2018-09-19 11:25:05 UTC
Again, can someone please review this?