aspnet / Security

[Archived] Middleware for security and authorization of web apps. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
1.27k stars 599 forks source link

State parameter generated by middleware is too large for Azure AD #978

Closed mclark1129 closed 7 years ago

mclark1129 commented 8 years ago

I'm trying to implement a login flow that requires more than one redirect in order to authenticate the user. I'm using Identity Server 4 to manage my application users, with Azure AD as an external identity provider. Both the Application and IdSrv4 are using the OpenIdConnectMiddleware in order to manage authentication. The problem that I have is the value for the state parameter appears to be generated with the RedirectUrl, and in my case this includes the original state parameter that was sent from the Application. During the redirect from Identity Server to Azure AD, the value for state is so large that Azure AD cannot accept it.

Here is what the URLs look like as they progress through the login flow:

Redirect from Application to Identity Server http://localhost:5000/connect/authorize?client_id=mvc.hybrid&redirect_uri=http%3A%2F%2Flocalhost%3A3308%2Fsignin-oidc&response_type=code%20id_token&scope=openid%20profile%20api1&response_mode=form_post&nonce=636099103137269285.NjdjNzIzN2QtZjViMC00ODk2LWJhYjEtZTFmMWFmYzlmYTYzZjZjNjI0NzItMmQxNS00Y2Y4LTlmMGYtZTk2MTRkNDc5YzE5&**state=CfDJ8CzeOmnYfHZGhtEWipLMU_XmVmfISkhgsuCtOWHlMVDRTYFSl-7k1ucHounv3I2HFyqo4AqNGp8UsvWaB75eOxHmJkshz6svbOy6GWT5V6VfTQuZvHNOnVUw6cJMoXzz5fBwm8_BIOXdwc85jfReDH3AhiB8lOfxiMMdl7ZJgOD9EV3Y5xmbuaZDWidkRxFAWp0bSYMPqd_iETkh8pGUGN5RIDSCFlSJSdNp5N9QCuOcP9hHLd1c2YgqwUqJ6kXkgWb4aqkJniZSg6tOQkmA7MoVRkuZ2aaa5xyqTGP0s2bDminivKXjpWSvlqBt-iNljoU8OKqRdtAh5ECcDacwWHbPbUO4NaxRtOftp2I2-Gll9EWODsFtrOhAhbSbg3iZ2w**

Redirect From Identity Server to Azure AD https://login.microsoftonline.com/TENANT_ID/oauth2/authorize?client_id=CLIENT_ID&redirect_uri=http%3A%2F%2Flocalhost%3A5000%2Fsignin-oidc&response_type=id_token&scope=openid%20profile&response_mode=form_post&nonce=636099103228378395.MjliODZlYjEtOWE5OC00MmM0LTg2M2MtZjMzZmQzYTAwNjg4OThiZTgzZWMtMWM2Mi00NDk5LWIwNTgtM2I2NmI2OGE3OGIw**&state=CfDJ8CzeOmnYfHZGhtEWipLMU_VfFN3hx-zqNh3iBffthWff3MWiE3HGC_eb06CdBeKysRqr12NzDpO7li0x4GePsJN8dKc0PSLcuA77BDM3B9K1n9ZO2l16Yg3e932B1P33nXSxMCT9-V4eJJHY4tT1bIdczmvFnOxNQ7TziZl0q-Z6N4E4nsuZC-txWNYkTob2TMdH0JyIVXHRHK2CJ-4zsenHDqN2KDpDtQQYZi0rpbOVfJ013yYKnO6ZCJjS9eKlFWixSIjhCF4343yhH0zQwLUCeT94UiiXpArDYoojHr7WRxlRUsNCcTdjCES8JvECqe1aio--rjMjP1Q5A1dW4HCFaqBo4USMA8u0mn1hnSbbDMe6ulZNvyuTIktybvYrDS_9_Eg_6V33nE94875rqk8khW4BShJDLuqx3nZ0MjWBCcvl1DJ_jxFBM_7RxNOfRUL0KxMeTrkytKGQKw1YrV8d4fH45E74aozu9kvnnvNrfK-nKLw8FTzmySH3Io8gUVVYbfeTBPT2wP4q5Yr_98Fd02yLV7qQsocQSfar2UfMkWBCiWVIAAnf75w9bRaUz0F5lOA3bYQz707Ls1dBYXJTW1qimrT4rMtYN_P8LBVmu4CIWe74bSoRmr52wPsEcQTXhgyz3xPtAPSOYSVIp6jwHLUO2DvJoL6DtvTxNWRVoKZjwHjgE2_kfj6V4bqHCj8lsnrOTc2Cg5ZKe1n151MbqbjO6vonEznAVNVvSQGk-CBe2HgfmKKhe1eYlPEFt6CT15hZOx_zsxMQBwOu_xNQp4mZ2XsGchPYCWAqo2UaagrhD87Wqnj12OUmYGTFOtzOXGOmTp75ALkXOYhN76QRhYKHuQi2Nq26jimnSqiS9lfyYwOUOvGbsNEwiDJ03uXaXzPuEfU0PNdJPjTLLhQCfjm2ekG0hb3XMongMBaDP3tdQld4rVkAytGNv4-QaJlEbJ3EZPTWsOAUT36rBagYvELqSOaD1kXi80vicZN34s557zlx3l7JNXfvoaC-EITZvhISxrNN6VDCw3gmB04HtRN_SKDocJ_k4Q4FlKAFkwGzAsa2hifiTURoIdOlauhfm9LEbyHgtazLXhljqFh7Gl8MkFR0-XFUApxss9ifvi06tvMEFqsdtPcsguBycy5Znmf0VBgfRE4Gfs0EbozLtipmzm4iQvNX4knr6Kd1vSKv5ce7lsR8FSN3aNhEXczP_uIzbzGbDLBp-BhB42zNTChn51EcDH2U-nf25XwvDJdkz9Wvi1bBda8Mu_y5Y6UDm43sIe2uVxzapAlkQd_1h0ECMVfhg6Cslvb0nWjZsplyjHQVeNE80gW9TXsNf6a7bMdswrV_M1qlKyXzFpD3tctlm4PngmUaDiCJve86dHvZ5lUiP-jMY9hUZ-SDhA**

As you can see, the the length of the state value jumps considerably. In the options I've tried to substitute my own StateDataFormat into the options that will generate the State without the .redirect key in the AuthorizationProperties. While this cuts down on the query size, the middleware seems to use the state value to validate the redirect value sent to the callback, and without using that value to generate the state, the redirect fails.

Is it possible to somehow customize how state is generated so that the size doesn't grow exponentially large, and also is able to be validated by the middleware during the callback?

Tratcher commented 8 years ago

We have similar issues with cookies when there are too many claims to serialize. One workaround we use is the CookieAuthenticationOptions.SessionStore ITicketStore. This stores the real value on the local server in a cache and only sends a GUID to the client. The next time we see that GUID we retrieve the claims from the cache. https://github.com/aspnet/Security/blob/dev/samples/CookieSessionSample/MemoryCacheTicketStore.cs

You could try something similar on the Identity Server with StateDataFormat. Instead of being a serializer you could store the state in a cache by GUID and return the GUID as the result. Then when asked to deserialize you take the GUID and look it up again in the cache. I would still recommend applying data protection to the GUID to make sure you were the one that issued it.

mclark1129 commented 8 years ago

@Tratcher Thanks! I will look into this over the next few days and let you know how it shakes out. The only trick will be caching in such a way that it works in a web farm. We're hosting this in Azure and want to be able to scale out easily.

mclark1129 commented 8 years ago

@Tratcher I was able to work out a solution pretty quickly like you suggested, by implementing my own StateDataFormat using in memory caching. I will upgrade this to use distributed caching going forward at some point, so we can scale out Identity Server, but other than that it worked like a charm.

Thanks again!

mclark1129 commented 8 years ago

I uploaded my implementation of a custom StateDataFormat that uses caching to resolve this issue. If anyone's interested you can find it here:

https://github.com/mclark1129/IdentityServer4.Samples/tree/master/AzureAd_CustomStateDataFormat

brockallen commented 8 years ago

@Tratcher have you spoken with the AAD team about this? that ~1800 char limit is quite ... well, limiting. this workaround proposed above is really not ideal, especially since this is an AAD specific problem. // @vibronet

Tratcher commented 8 years ago

@brockallen no, this is the only report I've seen of this issue. @brentschmaltz, is this something you've seen?

brockallen commented 8 years ago

We've seen it more than once from our users of IdentityServer.

leastprivilege commented 8 years ago

It basically happens when you chain two middlewares together (e.g. MVC app to IdSrv to AAD). The state parameter gets too big (according to AAD).

Solving that at the application level requires quite a bit of extra plumbing when you want to be load balancing compatible. I think AAD should lift that length restriction. @vibronet

Tratcher commented 8 years ago

@brentschmaltz says this is due to some browsers having a 2kb limit and AAD reserving a little room for itself.

brockallen commented 8 years ago

And by "some browsers" we're talking about IE which is out of support. So it needs to be expanded somewhat on the azure side IMO, or optimize on the OIDC MW side to reduce the state size.

Tratcher commented 8 years ago

Re-opened for triage.

Eilon commented 7 years ago

I've contacted the AAD team about this. I'll report back when I have an update.

HadeelElbitar-zz commented 7 years ago

Correct, the chains of middlewares cause the state parameter size to increase. The 2k limit is due to a set of limitations, but we are reconsidering it. Will update when we (AAD) finalize a better experience :)

ehvattum commented 7 years ago

The problem is also experienced when using OIDC with Auth0 as a "wrapper" provider. If the state parameter is too large, a step in the redirection chain from Auth0 sets a cookie that sometimes is too large for some browsers.

brockallen commented 7 years ago

It would be great to have an ASP.NET Core provided solution for this, even if it's optional.

leastprivilege commented 7 years ago

oh yes - that would be great!

HadeelElbitar-zz commented 7 years ago

The size limit has been updated from 2K to 8K, this change has been released for couple of weeks now, please let us know if you are still seeing the issue :)

brockallen commented 7 years ago

wow, very cool. thanks @HadeelElbitar

brockallen commented 7 years ago

@HadeelElbitar Turns out I am still seeing this for accounts that are live/Microsoft accounts. AAD accounts seem to handle it ok. This means you probably changed it for one of the STSs in the azure offering, but not the other(s)?

deckerbl commented 6 years ago

@HadeelElbitar. I know this is a closed issue but I am still having this same problem with live accounts as @brockallen mentioned in his prior post. How can we get this addressed with live?

leastprivilege commented 6 years ago

We fixed this problem when you use IdentityServer as the federation gateway.

http://docs.identityserver.io/en/release/topics/signin_external_providers.html?highlight=StateFormatter#state-url-length-and-isecuredataformat

deckerbl commented 6 years ago

Thanks @leastprivilege ! I didn't catch that in the documentation.

AKlaus commented 6 years ago

Does services.AddOidcStateDataFormatterCache(); work only for registrations added via AddOpenIdConnect() method or also for AddMicrosoftAccount()?

In my case, the code below keeps returning a too long query string on successful MS authentication with state parameter over 1.5KB.

services.AddOidcStateDataFormatterCache();

services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
    .AddMicrosoftAccount(options =>
    {
        options.ClientId = Configuration["Authentication:Microsoft:ClientId"];
        options.ClientSecret = Configuration["Authentication:Microsoft:Password"];
        options.Scope.AddRange(new[] {"openid", "profile", "email"});
        options.SaveTokens = true;
    })
    // Add IdentityServer authentication
    .AddIdentityServerAuthentication(options => { options.RequireHttpsMetadata = true; });

Seems that adding AddOidcStateDataFormatterCache() didn't make any difference. Being more prescriptive with services.AddOidcStateDataFormatterCache(MicrosoftAccountDefaults.AuthenticationScheme); didn't help either.

Am using the latest IdentityServer4.

leastprivilege commented 6 years ago

We only fixed it for OIDC.

leastprivilege commented 6 years ago

But the same approach could be applies to any of the authentication handlers...