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

Handle websocket authentication via initial payload #915

Closed
alexandra-c opened this issue Jul 4, 2024 · 4 comments
Closed

Handle websocket authentication via initial payload #915

alexandra-c opened this issue Jul 4, 2024 · 4 comments
Labels
enhancement New feature or request internally-reviewed The issue has been reviewed internally.

Comments

@alexandra-c
Copy link
Contributor

Component(s)

router

Is your feature request related to a problem? Please describe.

The Cosmo router's built-in authentication currently looks for the JWT token only in the request header. For websocket requests made from a browser, headers cannot be altered, so the token can only be sent via URL Query, Cookie, or initial payload.

Currently, if the router configuration is set to authorization >> require_authentication: true, websocket requests will receive an Unauthorized response, causing Subscriptions to fail.

Describe the solution you'd like

The Cosmo router's websocket authentication behavior needs to be updated to allow the JWT token to be acquired from the initial payload as well as the header.

@jensneuse suggested the following implementation steps on Discord:

Add a configuration option to modify WebSocket authentication behavior to allow authentication via the initial payload.
Allow configuration of the initial payload field where the auth token is located.
Modify websocket.go to consider this configuration.

Describe alternatives you've considered

No response

Additional context

I have started working on a version of authentication that also looks for the token in the initial payload. I created this feature request to further discuss the implementation steps in more detail.

Thank you! :)

@alexandra-c alexandra-c added the enhancement New feature or request label Jul 4, 2024
Copy link

github-actions bot commented Jul 4, 2024

WunderGraph commits fully to Open Source and we want to make sure that we can help you as fast as possible.
The roadmap is driven by our customers and we have to prioritize issues that are important to them.
You can influence the priority by becoming a customer. Please contact us here.

@jensneuse
Copy link
Member

Hey, thanks for opening this issue.
Can you describe how you'd like to solve this issue?
Ideally, you could start outlining the solution and create a PR that updates the Router config so that we can discuss how to implement this.

@jensneuse jensneuse added the internally-reviewed The issue has been reviewed internally. label Jul 4, 2024
@alexandra-c
Copy link
Contributor Author

Here's a draft of my implementation #918
I added a new configuration section for websocket that handles authentication, I was thinking in the future maybe other authentication methods may be implemented...
It looks something like this:

websocket:
  ...
  authentication:
    from_initial_payload:
      enabled: true
      secret_key: "authorization"

Then I separated the initialPayload authentication from the header one, so I defined a new authenticator websocketInitialPayloadAuthenticator and renamed the existing one in httpHeaderAuthenticator since it was only handling authentication via header.
Also extracted the JWT decoding part in a separate interface called jwksTokenDecoder in order to reuse it in both authenticators.

Now the problem is that in handleUpgradeRequest method from websocket.go file, we are checking access control before upgrading the connection, which for the auth via initial payload that's too soon, we won't have the initial payload. For ws auth via initial payload, the checking must be done later on, after handler.Initialize() is called, hence the new WebSocketAuthenticationConfiguration section, and then set tehn initialPayload to the request context so we can use it the authenticator. This probably can be made in a better way...I'm open to suggestions.

Can you take a look at the PR draft and share your thoughts regarding my implementation idea?

Thanks!

@StarpTech
Copy link
Contributor

Fixed with #918

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request internally-reviewed The issue has been reviewed internally.
Projects
None yet
Development

No branches or pull requests

3 participants