Replace INonceCache by IDistributedCache #293

kevinchalet · 2015-06-22T15:57:15Z

Fixes #212.

For the first attempt, I opted for the "hybrid" approach (#212 (comment)): the distributed cache is retrieved from the options when available, and from the DI container when the user didn't provide his own cache.

Storing nonces in the distributed cache is not enabled by default but can easily be using StoreNoncesInCache. I'm not super satisfied with the name, so don't hesitate to suggest another one 😄

FYI, I haven't been able to use the OpenIdConnectSample app because it delegates authentication to AAD, that doesn't allow me to log in using my live.com account. I worked around this issue using AspNet.Security.OpenIdConnect.Server (https://github.com/aspnet-contrib/AspNet.Security.OpenIdConnect.Server).

Of course, I'll revert these changes before merging (though depending on AAD is not really a good idea IMHO).

/cc @Eilon @HaoK @Tratcher

Tratcher · 2015-06-23T17:13:47Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs

                    {
                        Logger.LogError(Resources.OIDCH_0033_TryAddNonceFailed, message.Nonce);
                        throw new OpenIdConnectProtocolException(string.Format(CultureInfo.InvariantCulture, Resources.OIDCH_0033_TryAddNonceFailed, message.Nonce));
                    }
+
+                    // Review: do we need to define an expiration date?
+                    await Options.NonceCache.SetAsync(message.Nonce, new byte[0], new DistributedCacheEntryOptions());


Yes, specify an expiration. The Nonce's are only good for an hour(?).

Is it worth making it configurable?
If yes, I guess we would have to modify cookies' lifetime to match this new parameter.

Use the existing config.
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/KDev/src/Microsoft.IdentityModel.Protocol.Extensions/OpenIdConnectProtocolValidator.cs#L108
Options.ProtocolValidator.NonceLifetime

The cookie doesn't set a timeout because we want it to expire as soon as you close the tab. Setting a timeout may extend it beyond the tab lifetime.

Well, IIRC, most browsers actually clear sessions cookies when their process is shut down, not just when the tab is closed. And it's not rare to see people who never close their browser: in this case, session cookies can have a "virtually unlimited lifetime" 😄

Anyway, that's now fixed.

HaoK · 2015-06-23T20:56:01Z

In regards to name, would CacheNonces be correct?

kevinchalet · 2015-06-23T21:36:27Z

@HaoK name updated, thanks 😄

Tratcher · 2015-06-24T17:28:53Z

src/Microsoft.AspNet.Authentication.OpenIdConnect/OpenIdConnectAuthenticationHandler.cs

                {
-                    if (!Options.NonceCache.TryAddNonce(message.Nonce))
+                    if (await Options.NonceCache.GetAsync(message.Nonce) != null)
                    {
                        Logger.LogError(Resources.OIDCH_0033_TryAddNonceFailed, message.Nonce);


This resource name is out of date. It was probably too specific.

Updating it is on my todo list, but it's a bit painful as I have to edit the .resx file and the designer class manually. I'll do that when the rest is done 😄

You shouldn't have to update the designer manually. build resx should update the designer.

Except that there are currently many designers that are not located in the Properties directory: build resx doesn't detect them and creates new designers under Properties, which results in duplicate files. I could remove the existing designers, but that's not something I want to do as part of this PR.

Ah, file a separate bug for that please.

Yeah build resx is busted for security repo

Tratcher · 2015-06-25T18:31:27Z

Looks good. Remove aspnet-contrib and squash.

kevinchalet · 2015-06-26T22:40:56Z

@Tratcher is there a public identity provider supporting OIDC that we could use to replace AAD?
Because not being able to run the samples is a bit annoying, specially since I'd like to start working on the POST authorization requests thing, that will require much testing.

kevinchalet · 2015-06-26T22:41:44Z

.resx item renamed. And squashed 😄

Tratcher · 2015-06-26T22:44:10Z

None that I know of. That has been one of our big problems verifying the middleware at all.

Tratcher · 2015-06-26T22:45:28Z

NuGet.Config

@@ -4,5 +4,6 @@
    <add key="AspNetVNext" value="https://www.myget.org/F/aspnetvnext/api/v2" />
    <add key="NuGet" value="https://nuget.org/api/v2/" />
    <add key="AzureAD" value="http://www.myget.org/F/azureadwebstacknightly"/>
+    <add key="aspnet-contrib" value="https://www.myget.org/F/aspnet-contrib"/>


I thought you were removing this?

I'll remove everything related to the embedded OIDC server once we're done with all the changes... testing an OIDC middleware without a server is a bit hard 🎉

Push it into a separate branch, I don't want to merge this into dev.

Meh, removing it from this PR will be easier. Just tell me we're done with this PR, and I'll revert these changes.

That's what I meant. I think we're done and ready to merge.

Ah, that was not clear 😄

Changes reverted.

@HaoK can you merge this? I'm going offline.

From: Kévin Chaletmailto:notifications@github.com
Sent: ‎6/‎26/‎2015 4:10 PM
To: aspnet/Securitymailto:Security@noreply.github.com
Cc: Chris Rmailto:Tratcher@Outlook.com
Subject: Re: [Security] Replace INonceCache by IDistributedCache (#293)

@@ -4,5 +4,6 @@

Ah, that was not clear 😄

Changes reverted.

Reply to this email directly or view it on GitHub:
https://github.com/aspnet/Security/pull/293/files#r33403824

Monday. I don't do Friday afternoon merges without good reason :)

dnfclas added the cla-already-signed label Jun 22, 2015

Tratcher reviewed Jun 23, 2015
View reviewed changes

Tratcher reviewed Jun 24, 2015
View reviewed changes

Tratcher reviewed Jun 26, 2015
View reviewed changes

Replace INonceCache by IDistributedCache

102f113

Tratcher merged commit 102f113 into aspnet:dev Jun 29, 2015

kevinchalet deleted the nonce branch July 1, 2015 21:01

kevinchalet mentioned this pull request Jul 1, 2015

OIDC expiration time when adding nonce may not accuratly reflect the validity of the nonce #318

Closed

Replace INonceCache by IDistributedCache #293

Replace INonceCache by IDistributedCache #293

Uh oh!

Conversation

kevinchalet commented Jun 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK commented Jun 23, 2015

Uh oh!

kevinchalet commented Jun 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK Jun 24, 2015 via email

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Jun 25, 2015

Uh oh!

kevinchalet commented Jun 26, 2015

Uh oh!

kevinchalet commented Jun 26, 2015

Uh oh!

Tratcher commented Jun 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK Jun 26, 2015 via email

Choose a reason for hiding this comment

Uh oh!

Uh oh!