Updated UID generation in DefaultClaimUidExtractor #52

ajaybhargavb · 2016-02-06T00:12:47Z

Issue - #30

rynowak · 2016-02-06T02:10:53Z

src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultClaimUidExtractor.cs

+                return new string[]
+                {
+                    "sub",
+                    subClaim.Value


This comment #30 (comment) mentions the claim and issuer... @blowdart want to explain exactly what to do?

You'd take the claim value, and the claim issuer (the Issuer property on the claim), and safely concatenate them (basically you do len(value)+value+len(issuer)+issuer), then either put that in the token, or a SHA256 hash of the concatenated value.

This basically means that if, for example, facebook and google issued the same sub claim value, you could still tell them apart, because the issuer is different.

Well these 2-string arrays are run through a SHA256 hash soon after this method returns. Questions:

Should they be 3-string arrays, adding the issuer?

Use Issuer or OriginalIssuer from the Claim?

rynowak · 2016-02-06T02:11:37Z

⌚

dougbu · 2016-02-06T06:01:50Z

test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultClaimUidExtractorTest.cs

+        {
+            // Arrange
+            var identity = new ClaimsIdentity();
+            identity.AddClaim(new Claim("fooClaim", "fooClaimValue"));


Nit: Please avoid "foo" and similar

Our tests use foo in thousands of places, what's wrong with it? It at least makes it clear that it's an arbitrary value...

ajaybhargavb · 2016-02-08T20:39:33Z

Updated.

ajaybhargavb · 2016-02-08T20:39:54Z

Need to add more tests. Will do it once the updated design is accepted.

rynowak · 2016-02-08T22:25:51Z

src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgeryTokenGenerator.cs

@@ -66,7 +66,12 @@ public AntiforgeryToken GenerateCookieToken()
            if (identity != null && identity.IsAuthenticated)


Should we still check this now that you're checking User?.Identities? Seems redundant and unnecessary

Also, why does caller pass in httpContext.User.Identity when this code turns around and uses httpContext.User?.Identities? Another reason to start w/ the ClaimsPrinciple as @rynowak suggested below.

Note that if Identity is equal to the first member of Identities where IsAuthenticated is true, existing code was 🆗. (I'm not sure whether "primary" in the Identity doc comment means IsAuthenticated or something else.)

I looked at the code. It does not check for IsAuthenticated. It simply selects first non-null identity. https://github.com/dotnet/corefx/blob/master/src/System.Security.Claims/src/System/Security/Claims/ClaimsPrincipal.cs

rynowak · 2016-02-08T22:35:13Z

⌚

dougbu · 2016-02-08T23:01:27Z

src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultClaimUidExtractor.cs

-            var nameIdentifierClaim = claimsIdentity.FindFirst(
-                claim => string.Equals(ClaimTypes.NameIdentifier, claim.Type, StringComparison.Ordinal));
-            if (nameIdentifierClaim != null && !string.IsNullOrEmpty(nameIdentifierClaim.Value))
+            if (claimsIdentities == null)


Can the IEnumerable<T> be null in real scenarios?

Not sure. But I guess returning null is better than just letting it blow up.

We don't add these kind of checks just because. It may mislead developers down the road if it's not a real scenario. Check.

Actually, this isn't a real scenario since the caller checks exactly this condition. Just Debug.Assert().

dougbu · 2016-02-08T23:05:45Z

⌚

ajaybhargavb · 2016-02-09T23:09:37Z

Updated.

dougbu · 2016-02-09T23:56:24Z

src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgeryTokenGenerator.cs

@@ -60,16 +61,17 @@ public AntiforgeryToken GenerateCookieToken()
            };

            var isIdentityAuthenticated = false;
-            var identity = httpContext.User?.Identity as ClaimsIdentity;
+            var authenticatedIdentity = httpContext.User?.Identities?.FirstOrDefault(c => c.IsAuthenticated == true);


We're trying hard to optimize Antiforgery. Don't introduce Linq here.

Also,

Can Identities really be null? Check the real code and don't check here unless it's a real scenario.

Lots of boilerplate around authenticatedIdentity repeated in two methods. Suggest a helper method.

Can Identities really be null?

You are right. It can never be null. An empty list is always allocated.

dougbu · 2016-02-10T00:06:59Z

⌚

ajaybhargavb · 2016-02-10T01:12:18Z

Updated.

dougbu · 2016-02-10T02:35:47Z

src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgeryTokenGenerator.cs

@@ -60,16 +61,17 @@ public AntiforgeryToken GenerateCookieToken()
            };

            var isIdentityAuthenticated = false;
-            var identity = httpContext.User?.Identity as ClaimsIdentity;
+            var authenticatedIdentity = httpContext.User != null ? GetAuthenticatedIdentity(httpContext.User) : null;


Remove the ternary and have GetAuthenticatedIdentity() return null when passed null.

dougbu · 2016-02-10T02:56:01Z

test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultClaimUidExtractorTest.cs

+                "LOCAL AUTHORITY",
+                ClaimTypes.Name,
+                "claimName",
+                "LOCAL AUTHORITY",


This test demonstrates the behaviour I complained about here. Should never mix claims from multiple identities in the same UID.

#52 (comment)

dougbu · 2016-02-10T02:58:02Z

⌚

ajaybhargavb · 2016-02-10T18:37:38Z

Updated.

rynowak · 2016-02-10T22:48:17Z

src/Microsoft.AspNetCore.Antiforgery/Internal/IClaimUidExtractor.cs

 using System.Security.Claims;

 namespace Microsoft.AspNetCore.Antiforgery.Internal
 {
    /// <summary>
-    /// This interface can extract unique identifers for a claims-based identity.
+    /// This interface can extract unique identifers for a list of claims-based identities.


For a ClaimsPrincipal

rynowak · 2016-02-10T22:50:30Z

from me. Get a signoff of @dougbu and @blowdart also

dougbu · 2016-02-10T23:21:09Z

src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultClaimUidExtractor.cs

+            Debug.Assert(claimsPrincipal != null);
+
+            var uniqueIdentifierParameters = GetUniqueIdentifierParameters(claimsPrincipal.Identities);
+            if (uniqueIdentifierParameters == null)
            {
                // Skip anonymous users


Fix this comment. Never reach here if the principal isn't logged in somehow.

dougbu · 2016-02-10T23:44:51Z

with a couple more tests. Suggest walking @blowdart though the tests and getting his agreement on the new behaviour.

ajaybhargavb · 2016-02-11T00:25:11Z

Updated and Rebased.

ajaybhargavb · 2016-02-12T00:25:38Z

Ping @blowdart

blowdart · 2016-02-12T18:30:20Z

Works for me. Might need more kittens though.

dnfclas added the cla-already-signed label Feb 6, 2016

rynowak reviewed Feb 6, 2016
View reviewed changes

dougbu reviewed Feb 6, 2016
View reviewed changes

ajaybhargavb force-pushed the gen-uid branch from 55c1f91 to 5c929f8 Compare February 8, 2016 20:39

ajaybhargavb force-pushed the gen-uid branch from 5c929f8 to 93b1d53 Compare February 8, 2016 20:41

rynowak reviewed Feb 8, 2016
View reviewed changes

dougbu reviewed Feb 8, 2016
View reviewed changes

dougbu reviewed Feb 9, 2016
View reviewed changes

dougbu reviewed Feb 10, 2016
View reviewed changes

rynowak reviewed Feb 10, 2016
View reviewed changes

dougbu reviewed Feb 10, 2016
View reviewed changes

ajaybhargavb force-pushed the gen-uid branch from 49f6321 to 8d6950e Compare February 11, 2016 00:23

[Fixes #30] Updated UID generation in DefaultClaimUidExtractor

220479c

ajaybhargavb force-pushed the gen-uid branch from 8d6950e to 220479c Compare February 16, 2016 18:15

ajaybhargavb merged commit 220479c into dev Feb 16, 2016

ajaybhargavb deleted the gen-uid branch February 16, 2016 18:18

		@@ -66,7 +66,12 @@ public AntiforgeryToken GenerateCookieToken()
		if (identity != null && identity.IsAuthenticated)

Updated UID generation in DefaultClaimUidExtractor #52

Updated UID generation in DefaultClaimUidExtractor #52

Conversation

ajaybhargavb commented Feb 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Feb 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajaybhargavb commented Feb 8, 2016

ajaybhargavb commented Feb 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Feb 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Feb 8, 2016

ajaybhargavb commented Feb 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Feb 10, 2016

ajaybhargavb commented Feb 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Feb 10, 2016

ajaybhargavb commented Feb 10, 2016

Choose a reason for hiding this comment

rynowak commented Feb 10, 2016

Choose a reason for hiding this comment

dougbu commented Feb 10, 2016

ajaybhargavb commented Feb 11, 2016

ajaybhargavb commented Feb 12, 2016

blowdart commented Feb 12, 2016