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.

Sign up for GitHub

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

Jump to bottom

Federation: subscription protocol, websocket and authentication #268

Closed
PacoDu opened this issue Aug 28, 2020 · 5 comments
Closed

Federation: subscription protocol, websocket and authentication #268

PacoDu opened this issue Aug 28, 2020 · 5 comments

Comments

@PacoDu
Copy link
Contributor

PacoDu commented Aug 28, 2020

I'm experimenting with federation and subscriptions and I noticed a flaw in the current implementation.

Problem

When the gateway starts and services.wsUrl is defined, the gateway will open a ws connection with each services and send the connection_init payload.
Once a client creates a websocket connection and sends the connection_init payload, the gateway will instantiate a SubscriptionConnection and uses the gateway's subscription.onConnect handler. Now, if the Client wants to start a subscription with the gql_start payload (defined by a federated service), the gateway will use the existing connection of the federated service and forward the gql_start payload and then it will forward any message coming from this subscription to the original client. This means that the federated service uses the same SubscriptionConnection for every gateway clients.

The issue here is that the service that defined this subscription and holds the logic/validation/permissions etc. didn't received the connection_init payload. Thus services have no way to authenticate and authorize messages coming from the gateway (the issue is the same if we use websocket headers as the connection between service and gateway was establish before any client request).

Solution ?

I think that the solution would be to establish a connection from gateway to service per client:

The gateway will not open websocket connections at startup, but only if needed. It can then forward the connection_init payload, initially received during the client <-> gateway handshake, to the related service. The federated service will create a SubscriptionConnection that is properly isolated per user.
This solution will create more connections between gateway and services but it should allow services to properly handle authentication/authorization and isolation without modifying the subscription protocol.

Before trying any implementation I wanted to share this issue because I'm not sure if it's a good idea to store the connection_init payload in-memory to be able to establish new connections but I currently don't see any other way that wouldn't require modification on the service side or on the subscription protocol. This would also decrease the overall performance of the gateway as it should handle more connections.
@mcollina
Copy link
Collaborator

Good spot.

I think there are two cases:

  1. the server is aware that is talking to a gateway, and they negotiate the auth in some app-specific way - this is essentially how the gateway currently work. We might want to add some extension on this protocol to be able to trace the user details for each message. The connection from the gateway to the server is privileged.

  2. The server is not aware that is talking to a gateway, in this case a new connection for each client must be performed (currently not supported). This is where your issue is about.

Would you like to add an option for keeping one connection on each front? The problem would be scaling such a deployment as it significantly increase memory consumption of the gateway. I'm not opposed on adding an option to support this second mode of operation.

@PacoDu
Copy link
Contributor Author

PacoDu commented Aug 28, 2020

  1. the server is aware that is talking to a gateway, and they negotiate the auth in some app-specific way - this is essentially how the gateway currently work. We might want to add some extension on this protocol to be able to trace the user details for each message. The connection from the gateway to the server is privileged.

I also thought to this solution but I removed it before posting the issue because:

If the server is aware of the gateway this kinda breaks the 'pug-and-play' aspect of the federation, I mean that the service will need some custom logic in the resolvers to handle message coming from the gateway. It also doesn't work well with the current design of graphql subscriptions (inspired by apollo implementation): in the current implementation, authentication logic is usually handled by headers or in the connection_init payload (because frontend clients can't set headers) which means that a the subscription resolver assumes that the connection is already authenticated and/or the context has been populated with some data to authorize and filter events. In this case the schema and resolvers of a standalone GraphQL service (that do not reference any external type) would differ from the federated version.

@PacoDu
Copy link
Contributor Author

PacoDu commented Aug 28, 2020 •
edited
Loading

Hum I think I understand better your point 1 now.

We could extend the gql_start message with a field like initPayload, the gateway could automatically inject the connection_init payload and in the SubscriptionConnection.handleGQLStrat we could use the onConnect handler if initPayload is defined in the message and populate the subscribe context with onConnect result (like the connection_init would do), then reject or accept the subscription. This wouldn't change the current interface and would be transparent for the user. What do you think ?

@mcollina
Copy link
Collaborator

We could extend the gql_start message with a field like initPayload, the gateway could automatically inject the connection_init payload and in the SubscriptionConnection.handleGQLStrat we could use the onConnect handler if initPayload is defined in the message and populate the subscribe context with onConnect result (like the connection_init would do), then reject or accept the subscription. This wouldn't change the current interface and would be transparent for the user. What do you think ?

+1

@PacoDu PacoDu mentioned this issue Aug 31, 2020
Merged
@PacoDu
Copy link
Contributor Author

PacoDu commented Sep 29, 2020

Closed by #271

@PacoDu PacoDu closed this as completed Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mcollina @PacoDu