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

ASP.NET sliding cookie expiration logic breaks with server-side sessions and a persistent cookie #1417

Closed
RandScullard opened this issue Sep 20, 2024 · 7 comments
Assignees

Comments

@RandScullard
Copy link

Which version of Duende IdentityServer are you using?
7.0.5

Which version of .NET are you using?
8.0.8

Describe the bug

We have server-side sessions enabled in Identity Server. We have a client-side app that uses OIDC to get tokens from Identity Server. The Client is configured with:

RefreshTokenExpiration = TokenExpiration.Sliding

So every time the client-side app refreshes the access token, the expiration of the refresh token and the user's server-side session in Identity Server are both pushed ahead in time. This worked great until we tried using a persistent cookie for the user's Identity Server login session (the "remember me" option on the login page). Then we noticed that the expiration of the .AspNetCore.Identity.Application cookie (the cookie itself, not the expiration of the ticket inside the cookie) never advances, and the cookie eventually expires, even though the refresh token periodically expires and the user's browser is redirected to Identity Server to get a new one.

We took a look at the code in ASP.NET for refreshing the auth cookie and we see why this behavior is happening. Please refer to https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs, specifically the CheckForRefreshAsync function. The logic in this function looks at the issue and expiration timestamps in the ticket and decides to refresh the cookie if the current time is closer to the expiration time than to the issue time (more than halfway through the lifetime). The problem here is that Identity Server's server-side sessions feature is advancing the issue and expiration timestamps in the ticket every time our client-side app uses the refresh token. As long as this keeps happening, CheckForRefreshAsync will never find that it is more than halfway through the ticket lifetime, and therefore will never decide to send a new auth cookie to the user's browser. Ultimately, the cookie will expire when it should have been kept alive.

It looks as if CheckForRefreshAsync assumes that the issue and expiration timestamps reflect the actual times the cookie was issued and will expire, but the behavior of Identity Server server-side sessions breaks this assumption - in this case the timestamps reflect the ticket lifetime, which is out of sync with the cookie lifetime. (This is only a problem when the cookie is persistent, since session cookies have no expiration date.)

We hope you can provide some guidance on how to deal with this!

@AndersAbel
Copy link
Member

Thank you for your detailed report. It looks like you might have found a combination of settings that we didn't consider when designing the solution.

If all of these features are used on the same time, I don't think we handle this correctly.

  • Server side session
  • Session lifetime coordination (enabling session renewal through refresh token usage)
  • Persistent cookies (the remember me option).

We will investigate and consider if there's something we need to fix in our implementation.

@AndersAbel AndersAbel self-assigned this Oct 2, 2024
@RandScullard
Copy link
Author

Thanks @AndersAbel, we appreciate you taking the time to think through this issue.

In the meantime, we have come up with a workaround that seems to be effective. We register the following middleware in our Identity Server implementation, early in the pipeline so that its Response.OnStarting runs after the authentication middleware. This middleware simply checks if there is an auth cookie in the request cookies but not in the response cookies. If this is the case, it copies the auth cookie from the request to the response with an expiration equal to (now + expire time span). (Note that it doesn't do anything with the ticket inside the cookie.)

We are planning to set our Identity Server sliding session lifetime to 30 days, and our sliding refresh token lifetime to 8 hours. With these settings, we expect users to be redirected to Identity Server's authorization endpoint to get a new refresh token about once per day, and each time this happens, their auth cookie will be set to expire 30 more days in the future - it should never expire prematurely. The minor downside is that every (authenticated) response from Identity Server will include a set-cookie header for the auth cookie, adding a bit of extra overhead.

It would be great if you could take a look at this and let us know if you think this approach makes sense, and if there are any improvements we could make.

public class AlwaysSetResponseAuthCookieMiddleware
{
    private readonly RequestDelegate _next;
    private readonly CookieAuthenticationOptions _cookieAuthenticationOptions;

    public AlwaysSetResponseAuthCookieMiddleware(
        RequestDelegate next,
        IOptionsSnapshot<CookieAuthenticationOptions> cookieAuthenticationOptions)
    {
        _next = next;
        _cookieAuthenticationOptions = cookieAuthenticationOptions.Get(IdentityConstants.ApplicationScheme);
    }

    public async Task InvokeAsync(
        HttpContext httpContext)
    {
        httpContext.Response.OnStarting(() =>
        {
            string cookieName = _cookieAuthenticationOptions.Cookie.Name!;

            string? requestCookie = httpContext.Request.Cookies[cookieName];

            if (string.IsNullOrEmpty(requestCookie))
                return Task.CompletedTask;

            IList<SetCookieHeaderValue> setCookies = SetCookieHeaderValue.ParseList(httpContext.Response.Headers.SetCookie);

            if (setCookies.Any(c => c.Name == cookieName))
                return Task.CompletedTask;

            CookieOptions cookieOptions = _cookieAuthenticationOptions.Cookie.Build(httpContext);

            cookieOptions.Expires = DateTime.UtcNow.Add(_cookieAuthenticationOptions.ExpireTimeSpan);

            _cookieAuthenticationOptions.CookieManager.AppendResponseCookie(
                httpContext,
                cookieName,
                requestCookie,
                cookieOptions);

            return Task.CompletedTask;
        });

        await _next(httpContext);
    }
}

@AndersAbel
Copy link
Member

I would not bother with keeping the cookie sliding - I'd just give it a long (10 years?) lifetime right away. That could be done using a custom CookieManager.

I've created an issue to fix this in IdentityServer's server side session handling.

@RandScullard
Copy link
Author

I liked your suggestion, so I put together a simple CookieManager and immediately ran into the following limitation: https://developer.chrome.com/blog/cookie-max-age-expires

In Chrome it is no longer possible to set an expiration date more than 400 days in the future. A quick test in Firefox showed that it implements the same limit.

In light of this, I think my proposed workaround is still the best solution, but I am open to other suggestions!

@AndersAbel
Copy link
Member

I think that renewing the cookie on every request is too aggressive. But there certainly needs to be a mechanism to renew the cookie, but I'd prefer a less aggressive algorithm.

@RandScullard
Copy link
Author

Given the limited information available in the cookie and ticket, I was not able to find a less aggressive algorithm that still keeps the cookie from expiring. If you know of one, please let me know.

@AndersAbel
Copy link
Member

You are right, the expiry date of the cookie is not made available by the browser to the server. We would have to somehow store that expiry date (separate from the session expiry date). We will investigate how this can be done when we work on the implementation.

I'm closing this issue now and we'll work on the implementation in DuendeSoftware/products#1622

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

No branches or pull requests

2 participants