Aspnetcore: Terminate SignalR connections when the corresponding token expires

[muratg]

Originally asked in: https://github.com/aspnet/SignalR/issues/1155#issuecomment-347287019

[anurse]

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]

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]

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

[anurse]

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]

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

[jpmnteiro]

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).

[kevinchalet]

@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]

Yeah, that looks about right.

[davidfowl]

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

[kevinchalet]

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.

[kevinchalet]

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 :sweat_smile:

[anurse]

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

Seems OK to me ;P

[davidfowl]

Logs....

[kevinchalet]

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]

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]

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.

[kevinchalet]

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 :sweat_smile:

[anurse]

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

[davidfowl]

Unfortunately, this logic will now break with long polling because we did this crazy thing https://github.com/aspnet/SignalR/issues/1644. We should add a test case for this so that we can make it work.

This line:

C# 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]

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

[davidfowl]

The feature.

[kevinchalet]

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]

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.

[kevinchalet]

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

Care to explain why?

[davidfowl]

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

[kevinchalet]

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

[davidfowl]

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.

[kevinchalet]

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]

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.

[kevinchalet]

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]

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).

[kevinchalet]

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]

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 https://github.com/aspnet/SignalR/issues/1281

[kevinchalet]

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]

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 https://github.com/aspnet/SignalR/blob/90aa48c09f79fe7c0f3c0e8aeb420816ec15c7f0/src/Microsoft.AspNetCore.Http.Connections/HttpConnectionManager.cs#L171 (+ the heartbeat interval).

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

[kevinchalet]

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

[kevinchalet]

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]

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]

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?

[kevinchalet]

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]

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.

[anurse]

We should also consider the impact on Server-Sent Events and Long Polling connections. See https://github.com/aspnet/SignalR/issues/2553#issuecomment-413706330

[weedkiller]

How can we terminate a SignarlR2 Client Connection, and I dont mean a timeout.

[alexb5dh]

Any up-to-date workarounds at the moment?

I'm thinking about implementing custom TimedPrincipal that will throw on all methods after token expires and assigning it to HttpContext.User. Exception handler in that case will force a client to disconnect.

Global handling is limited until #5353 is done but that can be fixed by temporarily adding try/catch in all methods involved.

Would be great if anyone could propose any simpler idea.

[davidfowl]

The core idea here is solid https://github.com/dotnet/aspnetcore/issues/5283#issuecomment-368864944. The only change would be to stash the AuthenticationResult in HttpContext.Items after the authentication middleware runs then write the same logic in OnConnectedAsync but instead of calling AuthenticateAsync just grab the AuthenticationResult and use it to implement the same logic (aborts the connection after it times out).

[netbidev]

Is there any news about this feature?