cosmo icon indicating copy to clipboard operation
cosmo copied to clipboard

feat: handle websocket authentication via initial payload

Open alexandra-c opened this issue 1 year ago • 9 comments

Fixing #915

  • New websocketInitialPayloadAuthenticator:

Introduced a new authentication.Authenticator implementation called websocketInitialPayloadAuthenticator. This handles authentication specifically via the initial payload of a WebSocket connection.

  • Refactoring and Renaming:

The existing jwksAuthenticator has been refactored and renamed to httpHeaderAuthenticator to better reflect its functionality, which is to authenticate solely through HTTP headers.

  • JWT Handling Code Extraction:

The JWT handling logic has been extracted into a separate interface. This change allows the JWT processing code to be reused across both the websocketInitialPayloadAuthenticator and httpHeaderAuthenticator.

  • WebSocket Authentication Configuration:

A new configuration section for WebSocket authentication has been added. This section allows the configuration of authentication via the initial payload and includes an option to forward the token from the initial payload to the request header (export_token).

"WebSocket": {
   [...]
   "Authentication": {
     "FromInitialPayload": {
       "Enabled": false,
       "Key": "Authorization",
       "ExportToken": {
         "Enabled": true,
         "HeaderKey": "Authorization"
       }
     }
   }
}

Motivation and Context

The Cosmo router's built-in authentication previously only checked for JWT tokens in the request header. However, when making WebSocket requests from a browser, headers cannot be modified, meaning the token must be sent via URL Query, Cookie, or the initial payload.

This limitation caused issues when the router's configuration required authentication (require_authentication: true), resulting in WebSocket requests being unauthorized and leading to subscription failures.

To address this, the WebSocket authentication behavior of the Cosmo router has been updated. The JWT token can now be retrieved from the initial payload, in addition to the request header, thereby improving the router's flexibility and compatibility with WebSocket connections.

TODO

alexandra-c avatar Jul 04 '24 13:07 alexandra-c

@alexandra-c if a client authenticates via initial payload, is this then forwarded as initial payload to a subgraph subscription, via header, or both?

Subsequently, what about sub requests via http to other subgraphs, e.g. to join data to the subscription. In this case I would expect that we're able to map the initial payload field to a subgraph request header. This might be Authorization, but some services, e.g. on AWS cannot use this header name, so it needs to be configurable.

Have you thought about the two use cases?

jensneuse avatar Jul 04 '24 16:07 jensneuse

@alexandra-c if a client authenticates via initial payload, is this then forwarded as initial payload to a subgraph subscription, via header, or both?

The initial payload is being sent to the subgraphs by default, so it will only be on the initial payload since right now no one is copying it to the header.

Subsequently, what about sub requests via http to other subgraphs, e.g. to join data to the subscription. In this case I would expect that we're able to map the initial payload field to a subgraph request header. This might be Authorization, but some services, e.g. on AWS cannot use this header name, so it needs to be configurable.

Have you thought about the two use cases?

I haven't really, I tested it right now and indeed we might need to copy the token to the headers also, for the sub requests. Not sure how the configuration should look like or where to put it... I think the header where we'd copy the token should be the same we configured in the authentication section, but there we actually have an array.

How do you suggest I should implement this?

alexandra-c avatar Jul 05 '24 07:07 alexandra-c

Hey @jensneuse, I think I finished the implementation here, can you take a look? 😊

alexandra-c avatar Jul 09 '24 13:07 alexandra-c

Hi @alexandra-c, sry for the late response. This is not the way we like to treat community contributions, but we don't have the capacity yet to review and ship the PR in time. I ask you to be a little more patient with us.

StarpTech avatar Jul 26 '24 09:07 StarpTech

We're taking a look this week.

jensneuse avatar Aug 06 '24 09:08 jensneuse

@alexandra-c overall looks good to me, just some minor things

jensneuse avatar Aug 08 '24 06:08 jensneuse

@alexandra-c overall looks good to me, just some minor things

thanks! I implemented your suggestion, can we run the test workflows again?

alexandra-c avatar Aug 08 '24 09:08 alexandra-c

@alexandra-c overall looks good to me, just some minor things

thanks! I implemented your suggestion, can we run the test workflows again?

I found one more thing. Can you add this? Then I'll run it again.

jensneuse avatar Aug 08 '24 09:08 jensneuse

Great work and endurance 😄 Please write a few lines about the feature including edge cases. We need this to extend the docs. Alternatively, you can also contribute them here

Thank you! This was quite a journey 😁. It was actually my first time coding in Go—I even took a quick workshop just to tackle this PR. 😄 I'll follow up with another PR to update the documentation as well.

alexandra-c avatar Aug 12 '24 07:08 alexandra-c

Hey @jensneuse 👋, could you take a look again to see if any further adjustments are needed? Thanks!

alexandra-c avatar Aug 14 '24 12:08 alexandra-c

Hey @jensneuse 👋, could you take a look again to see if any further adjustments are needed? Thanks!

Hey @alexandra-c! Sorry that it took a bit of time. Well done, great work!

I've added 2 minor comments which you could quickly address while merging main. Give me a ping and we can run tests + merge.

jensneuse avatar Aug 15 '24 06:08 jensneuse

Hey @jensneuse 👋, could you take a look again to see if any further adjustments are needed? Thanks!

Hey @alexandra-c! Sorry that it took a bit of time. Well done, great work!

Thank you! 😊

I've added 2 minor comments which you could quickly address while merging main. Give me a ping and we can run tests + merge.

I updated the branch and included the small changes you suggested. 👍

alexandra-c avatar Aug 19 '24 08:08 alexandra-c