Skip to content
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

Use websocket from another origin and without existing session #2598

Merged
merged 17 commits into from
Apr 7, 2021

Conversation

garrettjstevens
Copy link
Contributor

This is a change that would allow users to connect to the websocket from another origin without an existing session.

The first commit is based off this: zyro23/grails-spring-websocket#35 (comment), which is from the author of the grails-spring-websocket library. It adds a custom websocket handshake handler. It also opens up websockets so you can connect with them from other origins. In the custom handshake handler, if the user can't be authenticated, it causes the handshake to fail, guaranteeing that even on the same origin, you have to authenticate in order to establish the web socket connection.

The second commit adds the option to authenticate without an existing session by sending the username and password. If session authentication fails, this username and password are authenticated and a user is assigned to the websocket. The username and password are sent in the query params, but from what I can tell that's not really different from sending them in the body of a post request.

Since everything is done in the handshake itself and a websocket cannot be opened without authenticating, this meets the best practices for websocket authentication.

You can test this out with the websocket_login branch of this repo: https://github.com/garrettjstevens/apollo-websocket-test/tree/websocket_login. No need to put it in the Apollo app.

@nathandunn
Copy link
Contributor

@garrettjstevens If I can't take a look at it tomorrow, I'll take a look early next week.

@nathandunn
Copy link
Contributor

@rbuels
Copy link
Collaborator

rbuels commented Mar 23, 2021

This solution passes the username and password to the websocket handshake in the HTTP request (the query string in this case), so it is equivalent security-wise to the existing REST endpoints accepting username and password as part of the REST request. This does not open any additional security risks from what are already present.

@nathandunn
Copy link
Contributor

@rbuels Understood. I still need to some a couple hours testing it and going through the code as it will be distributed on the main branch. I tried to do it sooner, but it doesn't look like it will happen until Monday. You can definitely use this fork while developing to see if you uncover anything else.

Copy link
Contributor

@nathandunn nathandunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some testing notes (still going through as I find time today):

when I logout I get this:

image

Probably fixable as the proper messages do get sent.

Also, @garrettjstevens Do we have a testing web socket client implementation for this?

@nathandunn
Copy link
Contributor

@garrettjstevens Sorry, you said you had a testing mechanism. Long day will look now.

@nathandunn
Copy link
Contributor

@garrettjstevens I added a logout command to the web socket test code. However, I think it exposes another problem.

body
======
{"error_message":"No SecurityManager accessible to the calling code, either bound to the org.apache.shiro.util.ThreadContext or as a vm static singleton.  This is an invalid application configuration.","operation":"ERROR","username":"ndunn@me.com"}
====
type: binary
header:
{"content-length":"248","message-id":"13-27","subscription":"sub-0","content-type":"text/plain;charset=UTF-8","destination":"/topic/AnnotationNotification"}
=====

Also, when I disconnect, it somehow keeps the header in place (so if I disconnect and reconnect it success with a bad username and/or password).

That said, this might still be workable. I still think it might be of limited utility at this injuncture as there are other higher priorities, but I am fine helping you continue to pursue it.

@nathandunn
Copy link
Contributor

The logout function will be important as we invalidate all sessions regardless of where logout occurs.

@nathandunn
Copy link
Contributor

nathandunn commented Mar 30, 2021

Next steps:

@nathandunn
Copy link
Contributor

As an FYI: https://github.com/zyro23/grails-spring-websocket#security. Good to know In 4.0 branch.

However, I don't see the options in the current version 1.3 meaning that it may have to be done separately. Possibly shiro (what we use), might provide some additional support.

@nathandunn
Copy link
Contributor

nathandunn commented Mar 30, 2021

looks like there is a proxy method: https://create-react-app.dev/docs/proxying-api-requests-in-development . . . though this is all on the server side. . . Looking for grails, etc.

@nathandunn
Copy link
Contributor

@nathandunn
Copy link
Contributor

A global setting:

 ./grailsw add-proxy reactdev --host=localhost --port=3000 
 ./grailsw set-proxy reactdev

In theory something like that should work and we should get a cookie returned this way.

@nathandunn
Copy link
Contributor

I think you can maybe specify it this way: https://create-react-app.dev/docs/advanced-configuration/ by setting:
WDS_SOCKET_HOST WDS_SOCKET_PORT WDS_SOCKET_PATH

@nathandunn
Copy link
Contributor

@nathandunn
Copy link
Contributor

nathandunn commented Mar 30, 2021

  • plan A:get stuff working as-is (get threads to bind)
  • plan B: get hot-dev working to mimic Apollo web-page to separate build environments. Might be doable with a proxy.
  • plan C: use something like this: https://guides.grails.org/using-the-react-profile/guide/index.html where it uses CRA, but a built-in web-pack watch to compile as an asset. We know that this WILL work.
  • plan D: use a yarn build script to push into the appropriate directory from either inside or outside the Apollo directory. We've already shown that this works.

@nathandunn
Copy link
Contributor

nathandunn commented Mar 31, 2021

As a quick note, grails uses spring under the hood. This is their section on authentication: https://docs.spring.io/spring-framework/docs/current/reference/html/web.html#websocket-stomp-authentication

Note that the STOMP protocol does have login and passcode headers on the CONNECT frame. Those were originally designed for and are still needed, for example, for STOMP over TCP. However, for STOMP over WebSocket, by default, Spring ignores authorization headers at the STOMP protocol level, assumes that the user is already authenticated at the HTTP transport level, and expects that the WebSocket or SockJS session contain the authenticated user.

Using tokens you can use the ChannelInterceptor, which is ind of like what we've done. However, in this case it doesn't bind to the same session as for as I can tell, so the session is never invalidated.

@nathandunn
Copy link
Contributor

nathandunn commented Mar 31, 2021

Fro plan B we want something like this: https://stackoverflow.com/a/50056294/1739366 or better yet this: https://medium.com/analytics-vidhya/how-to-package-your-react-app-with-spring-boot-41432be974bc

Remembering that Grails is essentially Spring Boot. After this it should hopefully be pretty easy to simply add a web socket server.

@nathandunn
Copy link
Contributor

I added this as an example. based on the fork from the medium article:

https://github.com/nathandunn/Spring-Boot-ReactJS-Starter

Using mvn spring-book:run and npm start in the client directory it works great. One simply need to add the proxy path in package.json

 "proxy": {
    "/api": {
      "target": "http://localhost:8080",
      "ws": true
    }

@nathandunn
Copy link
Contributor

nathandunn commented Mar 31, 2021

Next steps:

@nathandunn
Copy link
Contributor

Actually this works and we get a JSESSIONID:

http://localhost:3000/apollo/apollo-websocket-test

I just added a symlink and a proxy and a proxy into package.json like this:

"proxy": "http://localhost:8080",

and it seemed to work though n to bringing through the session. The same is true with the build.

The problem now is that even when we send them through the security manager seems to not be on the thread even in the pre-built version.

@nathandunn nathandunn self-requested a review April 5, 2021 17:27
Copy link
Contributor

@nathandunn nathandunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@garrettjstevens / @rbuels if you have time to read back through the documentation on best-practices within the conversation links in this PR that would be helpful. Should be pretty quick links / best-practices about using web-sockets and security, but let me know if you want me to identify anything explicitly.

I would rather have someone review the literature as well prior to implementation in case I've missed something, but I think it would be valuable to have someone who has also read it so we can have a more informed discussion (can be in here as well).

I know that running over the current development mode in Apollo2 using web-pack works.

I'm fairly sure that using this: https://create-react-app.dev/docs/advanced-configuration/ will also work, but haven't tried it yet.

@garrettjstevens
Copy link
Contributor Author

I've read over the documentation. This method does authenticate before connecting (instead of authenticating after connection like with the stomp CONNECT message), so I think it is acceptable. The thing I think it needs is to associate the correct user during the handshake. It feels like I'm close to that, and maybe we can find a way to finish it off.

Basically if determineUser in AuthenticatingHandshakeHandler.groovy can return a proper user, I think it might work. Currently it uses this to get a subject:

Subject subject = SecurityUtils.getSubject();
subject.login(authToken)

But if I return that subject from determineUser, I get this error: Cannot cast object 'org.apache.shiro.web.subject.support.WebDelegatingSubject@659f72e0' with class 'org.apache.shiro.web.subject.support.WebDelegatingSubject' to class 'java.security.Principal'

This is the diff from the current branch I used to test that out:

diff --git a/grails-app/services/org/bbop/apollo/authenticator/UsernamePasswordAuthenticatorService.groovy b/grails-app/services/org/bbop/apollo/authenticator/UsernamePasswordAuthenticatorService.groovy
--- a/grails-app/services/org/bbop/apollo/authenticator/UsernamePasswordAuthenticatorService.groovy
+++ b/grails-app/services/org/bbop/apollo/authenticator/UsernamePasswordAuthenticatorService.groovy
@@ -47,7 +47,7 @@ class UsernamePasswordAuthenticatorService implements AuthenticatorService{
                 log.error "Failed to authenticate user ${authToken.username}"
                 return null
             }
-            return subject.getPrincipal()
+            return subject
         } catch (Exception ae) {
             log.error("Problem authenticating: " + ae.fillInStackTrace())
             return null
diff --git a/src/groovy/org/bbop/apollo/websocket/AuthenticatingHandshakeHandler.groovy b/src/groovy/org/bbop/apollo/websocket/AuthenticatingHandshakeHandler.groovy
--- a/src/groovy/org/bbop/apollo/websocket/AuthenticatingHandshakeHandler.groovy
+++ b/src/groovy/org/bbop/apollo/websocket/AuthenticatingHandshakeHandler.groovy
@@ -35,7 +35,7 @@ public class AuthenticatingHandshakeHandler extends DefaultHandshakeHandler {
         if (!newUser) {
             return null
         }
-        return new StompPrincipal(newUser)
+        return newUser
     }
 
 }

Do you know of a way to get a proper Principal from SecurityUtils? If we could get that, I think this method would work and also integrate with the existing methods well (for logout, etc.).

@nathandunn
Copy link
Contributor

@garrettjstevens Sounds good. We can probably pair program that tomorrow.

@nathandunn
Copy link
Contributor

nathandunn commented Apr 6, 2021

Validate:

@nathandunn
Copy link
Contributor

@garrettjstevens there is also a mismatch between ServerHttpRequest and HttpServletRequest which is annoying as well. Not sure why Handshake requires ServerHttpRequest when nothing else does.

@nathandunn
Copy link
Contributor

nathandunn commented Apr 6, 2021

But in terms of fixing your error above you want something like below.

That being said, if that worked, I would be surprised if it worked as its not associated with a thread, explicitly, but let me know.

diff --git a/src/groovy/org/bbop/apollo/websocket/AuthenticatingHandshakeHandler.groovy b/src/groovy/org/bbop/apollo/websocket/AuthenticatingHandshakeHandler.groovy
index 4ded9a308..56beca9a2 100644
--- a/src/groovy/org/bbop/apollo/websocket/AuthenticatingHandshakeHandler.groovy
+++ b/src/groovy/org/bbop/apollo/websocket/AuthenticatingHandshakeHandler.groovy
@@ -2,7 +2,9 @@ package org.bbop.apollo.websocket
 
 
 import grails.util.Holders
+import org.apache.shiro.SecurityUtils
 
+import javax.security.auth.Subject
 import java.security.Principal
 
 import org.apache.shiro.authc.UsernamePasswordToken
@@ -12,7 +14,7 @@ import org.springframework.web.socket.WebSocketHandler
 import org.springframework.web.socket.server.HandshakeHandler
 import org.springframework.web.socket.server.support.DefaultHandshakeHandler
 
-public class AuthenticatingHandshakeHandler extends DefaultHandshakeHandler {
+class AuthenticatingHandshakeHandler extends DefaultHandshakeHandler {
     def usernamePasswordAuthenticatorService = Holders.grailsApplication.mainContext.getBean('usernamePasswordAuthenticatorService')
 
     @Override
@@ -35,7 +37,8 @@ public class AuthenticatingHandshakeHandler extends DefaultHandshakeHandler {
         if (!newUser) {
             return null
         }
-        return new StompPrincipal(newUser)
+        return request.getPrincipal()   // used by default handshake
+//        return (Principal) SecurityUtils.subject.principal
+//        return new StompPrincipal(newUser)
     }
 
 }

@nathandunn
Copy link
Contributor

How are you getting the exception to generate?

@nathandunn nathandunn self-requested a review April 6, 2021 20:18
@garrettjstevens garrettjstevens merged commit 84eee66 into develop Apr 7, 2021
@garrettjstevens garrettjstevens deleted the websocket_handshake branch April 7, 2021 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants