Bug 57767 - Websocket client proprietary configuration
Summary: Websocket client proprietary configuration
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: WebSocket (show other bugs)
Version: unspecified
Hardware: PC Linux
: P1 enhancement (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords: PatchAvailable
: 58623 59191 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-03-27 14:53 UTC by Remy Maucherat
Modified: 2017-10-19 03:25 UTC (History)
2 users (show)



Attachments
websocket upgrade redirect (8.57 KB, patch)
2017-08-01 02:42 UTC, J Fernandez
Details | Diff
Authentication support (28.18 KB, patch)
2017-09-04 19:16 UTC, J Fernandez
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Remy Maucherat 2015-03-27 14:53:32 UTC
The Websocket client does not provide the functionality usually found in HTTP clients. As a result, it cannot do anything except a straight upgrade from HTTP/1.1 to Websocket.

To handle more than this, it would need proprietary configuration to handle:
- Authentication
- Redirects

For reference about the possibilities:
https://tyrus.java.net/documentation/1.8/user-guide.html#tyrus-proprietary-config
Authentication: https://tyrus.java.net/documentation/1.8/user-guide.html#d0e1524
Redirects: https://tyrus.java.net/documentation/1.8/user-guide.html#d0e1640

This is not a critical enhancement however as users can use their own websocket client implementation, they don't have to rely on the one Tomcat provides.
Comment 1 Mark Thomas 2015-11-22 16:53:28 UTC
*** Bug 58623 has been marked as a duplicate of this bug. ***
Comment 2 Remy Maucherat 2016-03-17 16:53:07 UTC
*** Bug 59191 has been marked as a duplicate of this bug. ***
Comment 3 MikeLing 2016-09-25 03:34:19 UTC
Hey, I would like to work on this issue if it's ok :) However, as a newbie  to tomcat, could you tell me where should I look into? BTW, I had clone and set up tomcat locally and my environment is Ubuntu16.04

Thank you very much!
Comment 4 Mark Thomas 2016-09-25 09:07:50 UTC
I'd suggest supporting 302 responses as a starting point. The code should handle both absolute and relative redirects.

There is a ready made test case here:
http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/tomcat/websocket/TestWebSocketFrameClient.java?view=annotate#l100
if you restore the commented out request.

The starting point for the client code is here:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/WsWebSocketContainer.java?view=annotate#l341
Comment 5 MikeLing 2016-09-28 16:06:03 UTC
(In reply to Mark Thomas from comment #4)
> I'd suggest supporting 302 responses as a starting point. The code should
> handle both absolute and relative redirects.
> 
> There is a ready made test case here:
> http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/tomcat/websocket/
> TestWebSocketFrameClient.java?view=annotate#l100
> if you restore the commented out request.
> 
> The starting point for the client code is here:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/websocket/
> WsWebSocketContainer.java?view=annotate#l341

Sorry for the late reply. So, how do I know the Websocket client can handle redirections as expected after I make some changes? It's my first bug in Tomcat, so please tell me what should I do or where should I look into?  Thank you very much! :)
Comment 6 Mark Thomas 2016-09-29 12:58:53 UTC
Re-read my comment #4 regarding a suitable test case and how to activate it.
Comment 7 J Fernandez 2017-08-01 02:42:35 UTC
Created attachment 35193 [details]
websocket upgrade redirect

Hi, this is my first time submitting any open source contribution. Please let me know if any changes are needed or if anything was missed.

Regards
Comment 8 Mark Thomas 2017-08-02 19:12:02 UTC
Thanks for the patch. It has been applied to:
- trunk for 9.0.0.M26 onwards
- 8.5.x for 8.5.20 onwards
- 8.0.x for 8.0.46 onwards
- 7.0.x for 7.0.80 onwards

There were a few minor style things I tweaked. If you enable ChekcStyle and run the validate target it will catch most of them.

I also moved the configuration properties from System properties the user properties. Generally, we try to avoid system properties where we can as they can conflict in some use cases.

I merged the two properties so redirects are disabled by setting the number of allowed redirects to 0. The default I set to 20 which is consistent with most current browsers.

It might look like a lot of changes but they were all fairly minor. Thanks again for the patch.
Comment 9 J Fernandez 2017-08-02 22:04:13 UTC
Where can I learn more about CheckStyle? I assume, there is a formatting file involved. Also, I am interested in adding support for authentication, should I submit a patch to to this thread? Thanks for your time.
Comment 10 Mark Thomas 2017-08-02 22:08:13 UTC
Add

execute.validate=true

to your build.properties file and run

ant validate

The configuration files are in res/checkstyle.

Please open a new bugzilla enhancement for adding authentication support. If you need any pointers, the dev@ list is the place to ask.

Thanks again for your contribution and we are looking forward to the next one.
Comment 11 Remy Maucherat 2017-08-03 07:41:19 UTC
Reopening since authentication is not done and this was for both items, and possibly more.
Comment 12 J Fernandez 2017-09-04 19:16:15 UTC
Created attachment 35289 [details]
Authentication support

Please find below additional changes.

Added support for Basic and Digest Authentication.
Added support for dynamic loading of other Authentication Schemes.
Cleared redirectSet after invocation to allow for Container reuse.
Comment 13 Remy Maucherat 2017-09-05 14:57:30 UTC
Ok, so that's obviously the big item (IMO), that looks good.
I'm not convinced that digest is useful anymore, do you think it is ? On the plus side, you did it already, on the minus side we'll have to maintain the feature.
Comment 14 J Fernandez 2017-09-06 01:53:28 UTC
(In reply to Remy Maucherat from comment #13)
> Ok, so that's obviously the big item (IMO), that looks good.
> I'm not convinced that digest is useful anymore, do you think it is ? On the
> plus side, you did it already, on the minus side we'll have to maintain the
> feature.

There is not much need for it nowadays given how ubiquitous SSL is. That being said, it may prove to be useful in certain circumstances. I don't think it should require too much maintenance.
Comment 15 Christopher Schultz 2017-09-07 22:18:43 UTC
None of the Java classes in the authentication support patch have any Javadoc. I'm -1 on accepting the patch on that basis alone. I've skimmed the code and it otherwise looks good, but I have not tested it at all.

For authentication, I wonder how much code can be re-used from Tomcat's existing HTTP Basic and HTTP Digest authenticators. I'd prefer not to have competing implementations of WWW-Authenticate handling in Tomcat.
Comment 16 Mark Thomas 2017-09-08 06:13:24 UTC
If there is a possibility of reuse ( this is client side and the existing code is server side) we'd need to be careful about which package / jar we put it in to avoid adding unwanted dependencies for the WebSocket client JAR.
Comment 17 Remy Maucherat 2017-09-08 06:26:47 UTC
Yes, I would rather integrate it (if it works) then see about reuse. I also don't think javadoc is a big issue either for this kind of code.
Comment 18 J Fernandez 2017-09-08 18:57:51 UTC
I have spent some time looking for opportunities to reuse but did not find many. We could replace the WWWAuthenticate parser for digest with org.apache.tomcat.util.http.parser, but we will still need the one I added for basic. At that point I am not sure if it's worth it if we can't replace it for both schemes.
Comment 19 J Fernandez 2017-09-21 01:43:30 UTC
Are there any additional proposed changes for this patch? I would like to leverage some of the functionality for https://bz.apache.org/bugzilla/show_bug.cgi?id=59758.
Comment 20 Remy Maucherat 2017-10-13 13:46:19 UTC
I committed the patch to trunk, with a few changes:
- Adding javadocs
- Merged all client code back to WsWebSocketContainer (the new client class was taking over nearly all its code so I didn't really see the point)

If nobody complains about other mandatory improvements, I will proceed with backporting the feature addition.
Comment 21 Mark Thomas 2017-10-13 14:05:58 UTC
Wasn't the point of the new class that the redirectSet wasn't thread safe?
Comment 22 Remy Maucherat 2017-10-13 14:09:46 UTC
Yes, that would be a big problem with my "simplification" then. Ooops. I will restore the separate client class, it's a good solution for the issue.
Comment 23 Remy Maucherat 2017-10-13 14:20:51 UTC
I prefer getting rid of the field instead, the GC savings are minimal and not worth it IMO.
Comment 24 Remy Maucherat 2017-10-14 06:26:26 UTC
This should make the enhancement "done" as it adds the most important features.

This is included in 9.0.2, 8.5.24, 8.0.48, 7.0.83.
Comment 25 J Fernandez 2017-10-19 03:25:53 UTC
I believe that there are additional benefits for separating the websocket client from the container. For example, we could enhance the redirect flow when behind a proxy by caching the SocketChannel for a connected host:port. We can always, pass the structure as a method argument, but I am not sure that can be manageable long term. In my opinion, as it stands, the container limits flexibility when adding similar features.