IdentityModel / oidc-client-js

OpenID Connect (OIDC) and OAuth2 protocol support for browser-based JavaScript applications
Apache License 2.0
2.43k stars 841 forks source link

OIDC silent renew only asking for id_token #612

Closed cdwaddell closed 6 years ago

cdwaddell commented 6 years ago

We just updated our identityserver4 application to the latest dotnet core 2.0 version. After updating we started getting hammered with requests from oidc-client-js silent renew asking for only an id_token (10 million per day):

https://[idserverurl]/connect/authorize?client_id=[clientid]&redirect_uri=[clienturl]%2Fsilent.html&**response_type=id_token**&scope=openid&state=2b62fc7780a2410ea0e17574b01b139b&nonce=a8d4e95210f947448060e4df03b985b4&prompt=none

Fortunately our application handled the load after some scaling.

The identity server is only responding with the id_token as requested. So the client immediately fires off another request as I think the client is expecting the access token too, even though it didn't request it. After looking through our logs, we see that this was happening at a low frequency before updating from the identityserver4 dotnet core 1.1 version (50k per day).

Most of our requests are coming through as expected (400k per day):

https://[idserverurl]/connect/authorize?client_id=[clientid]&redirect_uri=[clienturl]%2Fsilent.html&**response_type=id_token token**&scope=openid profile [apiscope]&state=0989757c0c8b4ae2bedff14b303f125d&nonce=4bfc94c6c38c4f6b8a25d9a9abac86a7&prompt=none&id_token_hint=[current id_token]

The earlier url also didn't include the id_token_hint, which makes me think their oidc-client-js bits might be outdated. Are these users possibly running an outdated cached version of the client? Was there an issue in an older version that would have caused this behavior, or am I barking up the wrong tree. Could my old identityserver have been responding with a token that met their expectations?

Here is our oidc configuration:

{
     authority: '[idserverurl]',
     client_id: '[clientid]',
     redirect_uri: '[clienturl]/auth.html',
     post_logout_redirect_uri: '[logouturl]',
     response_type: 'id_token token',
     scope: 'openid profile [apiscope]',
     silent_redirect_uri: '[clienturl]/silent.html',
     automaticSilentRenew: true,
     filterProtocolClaims: true,
     loadUserInfo: true
 }
cdwaddell commented 6 years ago

I was able to trace down the origin of the calls within the oidc client. It appears that when I log out of our IdentityServer, the oidc client immediately makes a call to the silent refresh endpoint to ask for a refreshed id_token. This all appears to be related to the check session logic.

cdwaddell commented 6 years ago

The question still remains, what could cause the oidc client to start spamming our IdentityServer with silent identity token requests. An increase in frequency by 2 orders of magnitude when compared to frequencies before we updated to dotnet core 2.0.

Count of id_token only refresh requests by day. image

brockallen commented 6 years ago

Silent renew frequency is controlled by access token lifetime.

cdwaddell commented 6 years ago

In this case normal silent renew requests (response_type=id_token token) are coming in at expected frequencies and those frequencies did not change.

image

The only requests to come in at abnormal frequencies are these "response_type=id_token" requests that appear to be coming from the check session logic.

Access token lifetime settings are currently set to expire after 24 hours:

image

cdwaddell commented 6 years ago

Users affected by this problem are submitting around 20-50 identity only (response_type=id_token) silent token requests per minute from the oidc client. We are unable to recreate the conditions necessary to manually cause these errors. Was there a historical version that had a bug that would exhibit these behaviors?

Maybe similar to this issue: Issue #593

I would be glad to give you debug data if I could recreate the issue.

brockallen commented 6 years ago

The issue in the other thread is that the OP was creating many instances of the UserManager, and each one was doing its own silent renew. That's where that traffic came from.

In your situation, the client-side code did not change, right? You said you only updated IdentityServer?

cdwaddell commented 6 years ago

Ok, I think we are experiencing this issue: Issue #196

We had a caching problem in the past, and we may have some users holding onto a version of the oidc client that is susceptible. When we updated IdentityServer4 our token endpoint appears to be slightly slower than it was before, causing this issue to blow up. I have removed the silent renew mechanisms and converted to a fully request based renew process.

cdwaddell commented 6 years ago

That is correct, we have a singleton UserManager.

brockallen commented 6 years ago

I have removed the silent renew mechanisms and converted to a fully request based renew process.

I wish I had never added the automatic silent renew into this library. After having worked on many other projects, I concluded that it's best to handle the access token expiring event yourself in your app and manage silent renews manually -- of course using the silent renew API on the user manager, but not its automatic one.

sanmscse commented 6 years ago

Hi @cdwaddell

The issue occurs due to multi object creation, make sure in your project, you created this only once this.userManager = new UserManager(getClientSettings());

and avoid calling the component class unnecessarily.

acvila-de-munte commented 6 years ago

@brockallen , hi ! Have the same issue as @cdwaddell . Use angular 6, the latest version of asp.net core (2.1) and the latest version of identity server 4. As example I've used this article https://www.scottbrady91.com/Angular/SPA-Authentiction-using-OpenID-Connect-Angular-CLI-and-oidc-client . I wrap user manager in service (which is singleton). So in my client side app I have only one instance of user manager.

Settings oidc settings As you can see automaticSilentRenew property is set to false. And I handle renew access token manually (when manager addAccessTokenExpiring event rises I detect access token expiration and refresh it using signinSilent method). And it works. But between this oidc client spams identity server endpoint.

I tried oidc client js v1.5.1 and v1.5.2 and result - the same. I tried v1.5.1 with asp net core 2.0 + identity server 4 app. And didn't have this issue (everything for identity server was added in memory).

Don't understand what to do to solve this. Could you please give some advice ?

Thanks a lot for help. Kind regards

brockallen commented 6 years ago

Ok, I'll see if I can repro this.

acvila-de-munte commented 6 years ago

thanks ! @brockallen I've tried dummy project with the latest oidc client js + angular 6 app and asp.net core (2.1). All clients, users, etc. were added in memory and there is no this issue. But in my project everything is in data base and I have implementation of IProfileService. In implementation of IProfileService I get some info about user from data base (first name, last name). At back end part I use Identity server 4 + ASP.NET Core Identity. Client app and back end are under https. Also I use locale data base. Don't know can it help or not. Left it to have more info.

brockallen commented 6 years ago

Ok, my apologies to @cdwaddell -- I'm a crap reader, and it wasn't until now that I finally digested your scenario. Yes, your assessment was spot on -- when the user's login status changes at the token server, the check session logic will detect the status has changed and then send an id_token only authorization request to query the user's status and current sub at the token server and compare that to what the client app thinks the user's sub should be. That's why you'd see that special id_token only request, and yes, it's 100% unrelated to access token expiration. My apologies for the unhelpful and unrelated comments.

As for why the number of requests is different, I don't know. I wonder if it's some code change in IdentityServer about when it gets a prompt=none request. You don't have any recent changes in the authorization interaction response generator (not many people customize this piece, so I'd not expect much luck here). Do you see any error logs from IdentityServer on those requests? Or do they respond as they should to the client with an error of login_required?

When the check session iframe code sees that something has changed at the token server, then it stops its polling, so this doesn't seem like it should be the culprit. But, the consuming app is still expected to decide what to do with its own user session when the user has signed out of the OP. As such, if you do nothing, then I could possibly see the automatic access token renewal automatically triggering a new request to obtain a new access token, but then it should also stop trying because it too should get back a login_required error.

So yea, not sure.

brockallen commented 6 years ago

@AndriiAlbu I tried to repro this on my local IdentityServer and the Clients sample. I can't repo it.

If you can repro this with our samples (perhaps branch and change the code and config as needed), then I can look into it. Until I can repro it, or you investigate more, there's not much else I can do.

acvila-de-munte commented 6 years ago

@brockallen hi ! Thanks a lot. I'll try to investigate and if find something I'll write here.

Kind regards

acvila-de-munte commented 6 years ago

@brockallen hi one more time ! Probably I've found what is the source of my issue. According to microsoft docs about asp net core migration from 2.0 to 2.1 I've added this in Startup.cs, ConfigureServices method (in identity server 4 project)

public void ConfigureServices(IServiceCollection services)
{
            services.Configure<CookiePolicyOptions>(options =>
            {
                // This lambda determines whether user consent for non-essential cookies is needed for a given request.
                options.CheckConsentNeeded = context => true;
                options.MinimumSameSitePolicy = SameSiteMode.None;
            });

            services.AddMvc()
                .SetCompatibilityVersion(CompatibilityVersion.Version_2_1);
           // add asp net core identity
           // add services
          // add identity server 4
}

When I change options.CheckConsentNeeded = context => true; to options.CheckConsentNeeded = context => false;

oidc client doesn't spam identity server endpoint anymore.

Kind regards

brockallen commented 6 years ago

Ok, closing since it's sorted and I think @cdwaddell's original issue is addressed. @cdwaddell if not, please let me know.

WellspringCS commented 6 years ago

As someone with no working baseline, it's hard to express my gratitude for your conversations. Lost a day? Yes. But thanks to back-and-forth between @brockallen @AndriiAlbu to name only two... I now find that what's been driving me INSANE all day is not my doing, and there is a solution.

I implemented @AndriiAlbu 's solution and it worked on my system. Before that, I was watching, helplessly, a literal stream of network traffic on my Chrome browser, as @AndriiAlbu described so well.

For me, I have no working copy prior to the upgrade. No way to know if it's just my problem. I have just me, this quiet room, and a seemingly endless stream of mysteries (perhaps a one-to-one match with the stream of network calls on my browser... a nod to Cantor's countable infinities, I suppose).

Thanks for solving one of them, guys.

brockallen commented 6 years ago

Yea, security's hard.

cdwaddell commented 6 years ago

Indeed, security is hard. Thanks again for the libraries that make this hard topic accessible.

WellspringCS commented 6 years ago

Question has to be asked...

The solution (turning CheckConsentNeeded to false) sure sounds like a decision to not comply with GDPR. (I do confess ignorance here...)

https://docs.microsoft.com/en-us/aspnet/core/security/gdpr?view=aspnetcore-2.1

Thankfully, my customers won't be concerned about that. But for a website facing a wider audience... (a) Are my suspicions regarding GDPR compliance on the mark? And (b) If so, what would be the better solution to the problem @AndriiAlbu and I and presumably others have encountered?

Wondering if/which cookies used by OAuth2 flow process can be marked as essential while still complying with GDPR etc.

To paraphrase @brockallen, "Yea, security's hard. And GDPR makes it exciting."

cdwaddell commented 6 years ago

This does not constitute legal advice and I am not a lawyer nor an expert on GDPR.

This cookie, however, is an essential cookie so a better solution may be to flag it as such:

new CookieOptions() { IsEssential = true });

Then this other setting will not need to be set to false.

brockallen commented 6 years ago

GDPR consent is really unrelated to OAuth2/OIDC consent.

cdwaddell commented 6 years ago

It does look like the cookies for Session in IdentityServer4 are not considered Essential cookies.

Here is a commit I can PR: https://github.com/cdwaddell/IdentityServer4/commit/cb667c4414e196fe0bfb951808968e96e3ab8c20

The consent mentioned is for GDPR to check if cookie tracking consent has been given:

services.Configure<CookiePolicyOptions>(options =>
{
    // This lambda determines whether user consent for non-essential cookies is needed for a given request.
    options.CheckConsentNeeded = context => true;
    options.MinimumSameSitePolicy = SameSiteMode.None;
});

I included Message cookies, I assume these are also essential. If they are not marked essential. The server will not give them to browser:

https://docs.microsoft.com/en-us/aspnet/core/security/gdpr?view=aspnetcore-2.1#essential-cookies

acvila-de-munte commented 6 years ago

In my project I've returned options.CheckConsentNeeded = context => true; Now client doesn't spam server. The only difference - instead of logging to file I log to sql server.

cdwaddell commented 6 years ago

Yes, we solved this problem by speeding up our "UserClaimsPrincipalFactory" and updating our oidc-client-js to latest. So I consider this one closed.

WellspringCS commented 6 years ago

Interesting, @AndriiAlbu, @cdwaddell -- after I have a smooth as glass system running as my happy baseline, I'll come back and play with those settings. My OAuth server is in Azure, but I'm not paying for lightning response times, so perhaps that's my version of the "slow" problem? No idea what the mystical connection is between response time and cookie consent.

cdwaddell commented 6 years ago

@brockallen, the default value of CheckConsentNeeded is false, so this will only affect users that turn this feature on for compliance with GDPR.

WellspringCS commented 6 years ago

This does not constitute legal advice and I am not a lawyer nor an expert on GDPR. ;-)

@brockallen, I haven't a clue about anything on GDPR, but at a gut level I was just assuming that any website that tracks who you are and keeps your email address (and likely a link between you and Google/FB/MS) in a database... would fall within the purview of GDPR.

No?

brockallen commented 6 years ago

GDPR requires the consuming app to disclose what the data is used for and get consent form the user. OIDC/OAuth2 consent is the user consenting to the token server that it can release that info about the user to the consuming app. the token server has no idea what the consuming app does with the data, thus it couldn't properly ask for consent (or disclose).

cdwaddell commented 6 years ago

@WellspringCS , this is true! But some cookies are required to make a website work. According to GDPR those can be considered "Essential" cookies, they cannot be used for tracking purposes, only for operational purposes. In this case, it would be up to the developers using IdentityServer4 to communicate to the end user that essential cookies and non-essential cookies are being used. You can see that Microsoft Identity uses these flags to require authentication cookies.

https://github.com/aspnet/Security/blob/master/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationOptions.cs

WellspringCS commented 6 years ago

All I knew was the acronym and (in the vaguest terms) what "the hubbub" was about... so thanks to you both for the edumacation! (mutter, mutter, not legal advice, mutter, mutter)

Anyway, that being the case certainly takes a burden off of IdentityServer4's shoulders, and off the shoulders of those who use it. (mutter, mutter)