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

Terminate SignalR connections when the corresponding token expires #1159

Open
muratg opened this Issue Nov 27, 2017 · 42 comments

Comments

Projects
None yet
5 participants
@muratg
Member

muratg commented Nov 27, 2017

Originally asked in: #1155 (comment)

@anurse

This comment has been minimized.

Member

anurse commented Nov 27, 2017

Hmm, this is tricky since the token isn't "re-transmitted" on each message. I suppose we could have code that checks the expiration of the token each time a message is received.

@anurse

This comment has been minimized.

Member

anurse commented Nov 27, 2017

We should make sure this is relatively easy to add, even if we don't add it ourselves. Having "filter"-style logic for incoming/outgoing HubMessages would be good.

@muratg

This comment has been minimized.

Member

muratg commented Nov 30, 2017

One idea is to use KeepAlive heartbeat to check token expiry. cc @anurse

@anurse anurse self-assigned this Dec 1, 2017

@anurse anurse added this to the 2.1.0-preview2 milestone Dec 1, 2017

@anurse

This comment has been minimized.

Member

anurse commented Dec 1, 2017

This is something we'll look to make possible, but I don't think we'll actually disconnect the connection ourselves. The new IConnectionHeartbeatFeature should allow the implementer of a Hub to check the token at a regular interval and then terminate the connection if they need to. I'll look at making an example of this in the preview2 milestone.

@davidfowl

This comment has been minimized.

Member

davidfowl commented Feb 16, 2018

We're not making this first class in 2.1.0 moving to backlog.

@davidfowl davidfowl closed this Feb 16, 2018

@davidfowl davidfowl reopened this Feb 16, 2018

@davidfowl davidfowl modified the milestones: 2.1.0-preview2, Backlog Feb 16, 2018

@jpmnteiro

This comment has been minimized.

jpmnteiro commented Feb 23, 2018

Original question:

I understand that this has been moved to the backlog, but I've ran with @anurse's idea and added some logic to the IConnectionHeartbeatFeature to try and check the token.

The issue, however, is how to access the token from within the callback (or from inside a hub for that matter). AFAICT, the caller only has access to the identity, not the token (one could potentially include the expiration has a claim in the token, but that smells)

Any ideas are appreciated.

Update:

Actually it is possible, as for anyone that stumbles upon this thread with a similar question, there is a way to access the token via Context.Connection.GetHttpContext().GetTokenAsync(""). Make sure you configure your tokens to be saved to the AuthenticationProperties (kudos to this article for pointing me in the right direction).

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Feb 27, 2018

@muratg @anurse is this what you had in mind?

public override async Task OnConnectedAsync()
{
    var feature = Context.Connection.Features.Get<IConnectionHeartbeatFeature>();
    if (feature == null)
    {
        return;
    }

    var context = Context.Connection.GetHttpContext();
    if (context == null)
    {
        throw new InvalidOperationException("The HTTP context cannot be resolved.");
    }

    // Extract the authentication ticket from the access token.
    // Note: this operation should be cheap as the authentication result
    // was already computed when SignalR invoked the authentication handler
    // and automatically cached by AuthenticationHandler.AuthenticateAsync().
    var result = await context.AuthenticateAsync(OAuthValidationDefaults.AuthenticationScheme);
    if (result.Ticket == null)
    {
        Context.Connection.Abort();

        return;
    }

    feature.OnHeartbeat(state =>
    {
        var (ticket, connection) = ((AuthenticationTicket, HubConnectionContext)) state;

        // Ensure the access token token is still valid.
        // If it's not, abort the connection immediately.
        if (ticket.Properties.ExpiresUtc < DateTimeOffset.UtcNow)
        {
            connection.Abort();
        }
    }, (result.Ticket, Context.Connection));
}
@anurse

This comment has been minimized.

Member

anurse commented Feb 27, 2018

Yeah, that looks about right.

@davidfowl

This comment has been minimized.

Member

davidfowl commented Feb 28, 2018

Nice! That's a great sample for the docs.

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Feb 28, 2018

Feel free to copy it, but if you adapt it for the JWT handler, double check it works correctly 'cause I don't think the JWT handler populates the AuthenticationProperties like the aspnet-contrib validation handler does.

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Feb 28, 2018

Also, it might be cool to handle that more gracefully. Aborting the connection without telling the client why the server did that is a bit meh 😅

@anurse

This comment has been minimized.

Member

anurse commented Feb 28, 2018

Aborting the connection without telling the client why the server did that is a bit meh

Seems OK to me ;P

https://media1.tenor.com/images/dd302f6644d1571646143137ad5a2320/tenor.gif?itemid=4905604

@davidfowl

This comment has been minimized.

Member

davidfowl commented Feb 28, 2018

Logs....

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Feb 28, 2018

Logs....

Logs are for developers, not for the SignalR client.

When an access token expires, a client is expected to renew it and re-try the request using the new token (in the OIDC world, using a refresh token or by sending a prompt=none authorization request).

If the client isn't told the connection was aborted because the access token expired, it will have to blindly renew the token, even if the connection wasn't aborted for this reason.

@anurse

This comment has been minimized.

Member

anurse commented Feb 28, 2018

I do wonder if we should consider a "Close" frame in the protocol to allow the client to have a chance to see why the connection was closed (including an Exception message, such as "Authentication token expired" or even possibly a code). The weird thing is things like Long Polling where the connection does have to stay "open" long enough for the client to poll for the Close message.

@anurse

This comment has been minimized.

Member

anurse commented Feb 28, 2018

Of course this could be achieved in an app-specific way in the hub itself by just invoking some method on the client before aborting the connection.

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Feb 28, 2018

Of course this could be achieved in an app-specific way in the hub itself by just invoking some method on the client before aborting the connection.

It's worth noting the heartbeat callback is not async. If you do that, you'll end up with some nasty blocking IO 😅

@anurse

This comment has been minimized.

Member

anurse commented Feb 28, 2018

You could do a fire-and-forget task to send the message and Abort after an Ack of some kind (or a timeout).

@davidfowl

This comment has been minimized.

Member

davidfowl commented Mar 31, 2018

Unfortunately, this logic will now break with long polling because we did this crazy thing #1644. We should add a test case for this so that we can make it work.

This line:

var result = await context.AuthenticateAsync(OAuthValidationDefaults.AuthenticationScheme);

Will likely explode.

One thing you can do is look for the presence of IConnectionInherentKeepAliveFeature and disable this code if that feature exists. That means that the transport itself supports timing out (i.e. long polling) and doesn't need this heartbeat.

@jpmnteiro

This comment has been minimized.

jpmnteiro commented Mar 31, 2018

@davidfowl would you recommend checking for the feature or checking for the type of transport on the OnConnected(...) method?

@davidfowl

This comment has been minimized.

Member

davidfowl commented Mar 31, 2018

The feature.

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Mar 31, 2018

Unfortunately, this logic will now break with long polling because we did this crazy thing #1644.

Crazy is really the right word. It will make user code relying on request services quite hard to debug. If the real context is not available, I'm not sure injecting a fake one is really a sane idea.

That's super annoying in this specific case, because we only need the HTTP context very early in OnConnectedAsync(), which is supposed to be always available as this method is invoked for the first time when the first Long Polling request is still processed.

@davidfowl

This comment has been minimized.

Member

davidfowl commented Mar 31, 2018

Crazy is really the right word. It will make user code relying on request services quite hard to debug. If the real context is not available, I'm not sure injecting a fake one is really a sane idea.

We'll find out when people try out preview2.

That's super annoying in this specific case, because we only need the HTTP context very early in OnConnectedAsync(), which is supposed to be always available as this method is invoked for the first time when the first Long Polling request is still processed.

No, it's not available. Thats a bad assumption. Still I agree what we're doing is slightly crazy now (and I personally made the change) but we're waiting to get more feedback before changing it.

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Mar 31, 2018

No, it's not available. Thats a bad assumption.

Care to explain why?

@davidfowl

This comment has been minimized.

Member

davidfowl commented Mar 31, 2018

Care to explain why?

Your hub is not executed on a request thread, everything is asynchronous and nothing runs inline. The request comes in and bytes are written to a queue and the request ends. Later on that data is picked up and parsed into a hub message and that pipeline runs. The two things are completely decoupled

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Mar 31, 2018

And that's specific to Long Polling or applicable to all transports?

@davidfowl

This comment has been minimized.

Member

davidfowl commented Mar 31, 2018

And that's specific to Long Polling or applicable to all transports?

All transports. The main issue with long polling is that there is no persistent http request for the entire life of the connection. SSE and websockets both have that.

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Mar 31, 2018

Makes sense. I guess this problem would be solved if we had a way to execute code inline when the HTTP request is received... but you have no plan to do that, right?

@davidfowl

This comment has been minimized.

Member

davidfowl commented Mar 31, 2018

Makes sense. I guess this problem would be solved if we had a way to execute code inline when the HTTP request is received... but you have no plan to do that, right?

That's correct, we have no plans to do that. The decoupling is a feature.

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Mar 31, 2018

That's unfortunate, because code like that won't work as you'd expect if you don't register the cancellation logic in OnConnectedAsync():

public async Task Test()
{
    while (!Context.Connection.ConnectionAbortedToken.IsCancellationRequested)
    {
        await Clients.Client(Context.ConnectionId).SendAsync("Ping");
        await Task.Delay(TimeSpan.FromSeconds(10));
    }
}

(when I tested this snippet in February, the ConnectionAbortedToken token was never triggered... even after the token became invalid and the client app started receiving a 401 LP response).

@davidfowl

This comment has been minimized.

Member

davidfowl commented Mar 31, 2018

That's unfortunate, because code like that won't work as you'd expect if you don't register the cancellation logic in OnConnectedAsync():

Why wouldn't that work? You lost me there. The ConnectionAbortedToken doesn't map to the http request for long polling, it maps to the logical connection (which spans multiple requests).

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Mar 31, 2018

The ConnectionAbortedToken doesn't map to the http request for long polling, it maps to the logical connection (which spans multiple requests).

For reasons I ignore, the "logical" connection was never considered "dead" even after returning a 401 LP response (the connection was still listed in LifetimeManager._connections).

@davidfowl

This comment has been minimized.

Member

davidfowl commented Mar 31, 2018

Long polling connections currently timeout, though we should be explicitly killing the connection if there's a non 200 status code (which we don't do right now). It's semi related to #1281

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Mar 31, 2018

Long polling connections currently timeout

What's the default timeout? I'll give that another try, but I'm 99,99% sure I remember the token was never fired, even after waiting 4 or 5 minutes.

@davidfowl

This comment has been minimized.

Member

davidfowl commented Mar 31, 2018

What's the default timeout? I'll give that another try, but I'm 99,99% sure I remember the token was never fired, even after waiting 4 or 5 minutes.

This has been of the notoriously buggier areas of SignalR in the past. Usually after 5 seconds in inactivity things should tear down

if (!Debugger.IsAttached && status == HttpConnectionContext.ConnectionStatus.Inactive && (DateTimeOffset.UtcNow - lastSeenUtc).TotalSeconds > 5)
(+ the heartbeat interval).

Regardless, I'll make sure we have tests that cover that sorta scenario.

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Mar 31, 2018

OMG, this !Debugger.IsAttached check is so evil. I'm a F5 guy, so this would explain why the connection was never aborted.

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Mar 31, 2018

Yep, I can confirm this works as one would expect when running the server app without the debugger attached. FWIW, I'm not sure this kind of check is really a good idea.

@davidfowl

This comment has been minimized.

Member

davidfowl commented Mar 31, 2018

FWIW, I'm not sure this kind of check is really a good idea.

I don't disagree, we might want to only do it if it's a debug build. These exist because it's hard to debug tests with timeouts generally.

/cc @anurse

@anurse

This comment has been minimized.

Member

anurse commented Mar 31, 2018

Yeah, it's one of those "it sucks if it's there and it sucks that it's there" things. I think limiting it to DEBUG builds is an OK option. The reason I'm not 100% sold on that is that if you are debugging an application (using a Release build) it can be very annoying to have the connection time out, so having a way for users to turn it on this behavior would be good. We could maybe use a quirking flag for that?

@PinpointTownes

This comment has been minimized.

PinpointTownes commented Mar 31, 2018

Why not a configurable option, independent of the build configuration/debugging environment?

services.AddSignalR(options =>
{
    options.DisableAutomaticConnectionRemoval = true;
});

If you think it's a too dangerous option, an obscure AppContext switch might be a good option.

@anurse

This comment has been minimized.

Member

anurse commented Mar 31, 2018

I think it's weird to put something like this on a public options surface. It's really only something you want while actively debugging the app.

@davidfowl davidfowl removed the 1 - Ready label Jul 19, 2018

@anurse anurse changed the title from Terminate WebSockets connections when the corresponding token expires to Terminate SignalR connections when the corresponding token expires Aug 17, 2018

@anurse

This comment has been minimized.

Member

anurse commented Aug 17, 2018

We should also consider the impact on Server-Sent Events and Long Polling connections. See #2553 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment