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

Do not try to refresh token when no getToken function is given #216

Closed
nfode opened this issue Feb 22, 2023 · 8 comments
Closed

Do not try to refresh token when no getToken function is given #216

nfode opened this issue Feb 22, 2023 · 8 comments

Comments

@nfode
Copy link

nfode commented Feb 22, 2023

Hello, firstly I would like to thank you for the work on centrifuge.

Is your feature request related to a problem? Please describe.
We are using a centrifuge with the connect proxy. This means instead of giving the token on constructing the Centrifuge-Client we pass the token in the data field like this:

new Centrifuge('wss://test.de/connection/websocket', {
                    data: {
                        token: ...
                    },
                });

In our backend, we then validate the token and set the expire_at like described in the documentation here. This is necessary since the build in token mechanism does not work with our internal systems.

Connecting then works like a charm, but it gets problematic when the tokens are expired and the client wants to trigger a refresh. We then get the following error in the browser console:

provide a function to get connection token

The error is thrown here on token refresh because the lib tries to call the getToken function we did not define, since we have our own token handling.

Describe the solution you'd like
On refresh, when no getToken function is defined, the client triggers an error event and does not try to reconnect. When the error event is triggered, the implementor then can reinitialize the client using its own logic.

Describe alternatives you've considered
The code wrapping the client remembers when the token expires and reinitialize the client when it is time for a refresh. It then also ignores that an error is thrown because of the missing getToken function.

If there are any questions or further clarification needed I am happy to help.

@FZambia
Copy link
Member

FZambia commented Feb 23, 2023

Hello @nfode

Did you consider configuring refresh proxy to periodically validate connection fully on server-side? This way you can fully avoid disconnects and re-init I think.

@nfode
Copy link
Author

nfode commented Feb 24, 2023

We still use JWT Tokens. So we want the server to disconnect the client when the token expires, to then force a reconnect with a new token. This as of now works already very good: the token expires and the client errors. The only problem is that the error comes from the missing getToken function and not an “expired authentication” error.

Another way to improve this behavior could be to upgrade the getToken to a generic refresh function that either updates the token or the data. From my understanding of the code, this should be possible since the connection is just reinitialized on refresh with the new token from getToken.

Changing the behavior of the refresh handling in the client would improve our and other uses of the connection proxy with custom auth by a lot.

@FZambia
Copy link
Member

FZambia commented Feb 24, 2023

We still use JWT Tokens. So we want the server to disconnect the client when the token expires, to then force a reconnect with a new token. This as of now works already very good: the token expires and the client errors. The only problem is that the error comes from the missing getToken function and not an “expired authentication” error.

I am trying to think on a better way to handle this. To avoid disconnects at all if the connection is still valid. Like it happens with native Centrifugo JWTs.

Let's consider this scenario:

  1. Now you are passing your custom token to the backend over data field. Let's imagine we will be able to set it as token. Currently connect proxy endpoint won't be called in this case (since token set), but we can introduce an option of Centrifugo server (sth like "proxy_connection_token": true) to tweak that behaviour and pass token to the backend in connect proxy request.
  2. On the backend you can validate token and set connection expiration. At some point in the future getToken function on the client side will be called.
  3. You are providing new token as a response for getToken and SDK sends it to Centrifugo.
  4. Centrifugo understands that this is a token which should be proxied to the backend (because proxy_connection_token on) and sends refresh HTTP proxy request to the backend with new token. Your backend validates it and returns new expiration time to Centrifugo.
  5. Centrifugo sends refresh response to the client like it does with Centrifugo native JWT case.

Can this fit your case? This will be very similar to our native JWT handling – but with delegating token handling to the backend.

BTW, one more question. Since you are using JWT – why not you just generated Centrifugo native JWT for the connection? What is the reason you want to use custom token instead? The answer may help me to understand the use case better.

@nfode
Copy link
Author

nfode commented Feb 26, 2023

Firstly, thank you for taking time thinking about possible solutions!

I love the idea. That would fit our use case perfectly.

BTW, one more question. Since you are using JWT – why not you just generated Centrifugo native JWT for the connection? What is the reason you want to use custom token instead? The answer may help me to understand the use case better.

We use Keycloak with multiple realms and therefore have multiple JWKs endpoints. This was already discussed for the v4 roadmap here: centrifugal/centrifugo#500 (comment) But from my understanding there was nothing done in that direction.

@FZambia
Copy link
Member

FZambia commented Feb 26, 2023

We use Keycloak with multiple realms and therefore have multiple JWKs endpoints. This was already discussed for the v4 roadmap here: centrifugal/centrifugo#500 (comment) But from my understanding there was nothing done in that direction.

I did some prototyping with what I described above in centrifugal/centrifugo#612. While it works – the feeling is that all the benefits of JWTs are lost in that case. You will get more requests to the backend than required - on connection (from Centrifugo to app), on time to refresh (from SDK to app), and on refreshing (from Centrifugo to app).

And it feels a bit odd actually since Centrifuge SDK first asks your backend for the token, then Centrifugo almost immediately sends it over refresh proxy for validation. In this perspective current Centrifugo refresh proxy behaviour which does not involve client-side request to the app for token at all have much more sense since user is already known.

Let's talk more about that JWKs and multiple realms. I suppose selecting JWKs endpoint based on iss field of token is not that hard. I had no experience with it though so not sure I see the full picture and all the requirements. Maybe you can describe what's the desired Centrifugo configuration regarding JWKs should be and its behavior – to let Centrifugo process JWTs natively without using connect/refresh proxies at all?

@nfode
Copy link
Author

nfode commented Feb 27, 2023

Your proposal feels exactly what I would have expected in the first place when checking out Centrifugo for us. When using Centrifugo with "custom authn" you automatically accept that you have more requests. That is a risk we are willing to take. Before using Centrifugo we had a custom NATS patch based on an outdated NATS fork, the Centrifugo proxy therefore is a major improvement for us. But I also understand that it feels odd.

Regarding the idea with the iss claim of the token:

Selecting the JWKs endpoint only by the iss claim inside the token is in my understanding unsafe. An attacker could send a token with an iss claim that points to the attacker's (JWKs) endpoint. A verification then would be successful, since the attacker signed the token.

To avoid this, Centrifugo could check if the iss is in a list of allowed issuers. This works only if the different realms/tenants have the same base url. For us, that would be a problem since one tenant/realm can have multiple base domains. So having an allow list is not an option.

What can work is configuring a template url to the jwks endpoint, and then also configuring where to get the tenant/realm from the token.

The config then would look like this for Keycloak:

jwks_endpoint: "https://example.com/REALM/protocol/openid-connect/certs", // Centrifugo then replaces REALM with a value from the token
token_realm_claim: "realm"

Centrifugo then would parse the token, get the realm, and then get the JWKs with the configured url template. If the token was forged by an attacker, the check would fail even if the realm exists, since we verify with our own JWKs.

This would work for Keycloak besides that the sub claim provided by Keycloak contains : characters. With that sub from my understanding, user channels do not work since it clashes with the channel naming conventions using : as namespace boundary. To avoid this, another configuration would be necessary to select the user ID from another claim in the token. Something like this:

token_user_id_claim: "centrifugo_user"

Instead of adding another configuration, we also could ditch the user channel boundaries and use the channels claim and build a user-channel syntax for ourselves.

@FZambia
Copy link
Member

FZambia commented Feb 27, 2023

Could you reach me out in our Telegram or Discord community groups to discuss this? There are some things I can suggest regarding this and some things for which I still need more clarification. Think in a chat this may be more effective. Hopefully we can come to the effective solution here – but it's hard to implement without understanding all the details.

@FZambia
Copy link
Member

FZambia commented Apr 22, 2023

Eventually fixed over centrifugal/centrifugo#638, docs

@FZambia FZambia closed this as completed Apr 22, 2023
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

No branches or pull requests

2 participants