WsFederation review #1441

Tratcher · 2017-09-19T19:14:10Z

This PR is for review purposes only, we still need to work out the final branching structure.

This is a port of Microsoft.Owin.Security.WsFederation. It targets Core 2.0 dependencies so that we can release it sooner than 2.1. As such some of the project files are unusual. We're also using the IdentityModel 5.2.0-preview1 dependencies from nuget.org. The version for this package will start as 2.0.0-alpha1.

The behavior is approximately the same as the Katana version, with minor updates to mirror those we made to OIDC when it was ported over. Changes:

User state in the wctx is nested rather than appended to.
CallbackPath is required and defaults to "/signin-wsfed". However it can be set to "/" along with options.SkipUnrecognizedRequests = true; to get some of the stranger legacy behavior apps have asked for in the past. OIDC uses the same mechanic.
Better resolving of relative to absolute redirect urls.

dnfclas · 2017-09-19T19:14:14Z

@Tratcher,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

Tratcher · 2017-09-19T19:28:05Z

@brentschmaltz @leastprivilege @brockallen

Tratcher · 2017-09-19T19:28:43Z

#43

Tratcher · 2017-09-19T21:33:19Z

We have a build plan, @JunTaoLuo will be in charge of that. We'll start as soon as we get code sign-off from @HaoK.

HaoK · 2017-09-19T21:58:34Z

samples/WsFedSample/Startup.cs

+            services.AddAuthentication(sharedOptions =>
+            {
+                sharedOptions.DefaultScheme = CookieAuthenticationDefaults.AuthenticationScheme;
+                sharedOptions.DefaultSignInScheme = CookieAuthenticationDefaults.AuthenticationScheme;


Nit: doesn't DefaultSignInScheme fallback to DefaultScheme?

Not correctly in the 2.0.0 branch we're targeting. That was just fixed in 2.1 and 2.0.x.

Ugh, yeah that's too bad

HaoK · 2017-09-19T21:59:26Z

samples/WsFedSample/Startup.cs

+
+                if (context.Request.Path.Equals("/signout"))
+                {
+                    await context.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);


Should this just use the empty overload since you are setting the defaults?

We could. This just mirrors the template code.

HaoK · 2017-09-19T22:00:05Z

samples/WsFedSample/Startup.cs

+
+                if (context.Request.Path.Equals("/Account/AccessDenied"))
+                {
+                    await context.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);


Why does AccessDenied sign out?

HaoK · 2017-09-19T22:01:54Z

src/Microsoft.AspNetCore.Authentication.WsFederation/Events/AuthenticationFailedContext.cs

+    /// <summary>
+    /// The context object used in for <see cref="WsFederationEvents.AuthenticationFailed"/>.
+    /// </summary>
+    public class AuthenticationFailedContext : RemoteAuthenticationContext<WsFederationOptions>


Nit: should this have a more specific name, i.e. WsFedFailedContext?

shrug I don't see why it needs one. This type will rarely be referenced by name, and if you really need to and have a conflict you can do WsFederation.AuthenticationFailedContext.

HaoK · 2017-09-19T22:05:56Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationHandler.cs

+            try
+            {
+                // Retrieve our cached redirect uri
+                string state = wsFederationMessage.Wctx;


HaoK · 2017-09-19T22:06:47Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationHandler.cs

+                else
+                {
+                    // Extract the user state from properties and reset.
+                    properties.Items.TryGetValue(WsFederationDefaults.UserstatePropertiesKey, out var userState);


Should this do something if TryGetvalue fails? Or is that ok to just ignore?

No, it's optional.

HaoK · 2017-09-19T22:07:44Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationHandler.cs

+                if (wsFederationMessage.Wresult == null)
+                {
+                    Logger.LogWarning("Received a sign-in message without a WResult.");
+                    return (HandleRequestResult)HandleRequestResult.NoResult();


Do we need the cast?

Yes, NoResult returns the base type AuthenticateResult.

Why using NoResult here? A WS-Fed sign-in response without a wresult parameter is illegal so this should return an errored result.

HaoK · 2017-09-19T22:08:42Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationHandler.cs

+
+                // Copy and augment to avoid cross request race conditions for updated configurations.
+                var tvp = Options.TokenValidationParameters.Clone();
+                IEnumerable<string> issuers = new[] { _configuration.Issuer };


HaoK · 2017-09-19T22:09:50Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationHandler.cs

+                        break;
+                    }
+                }
+


Is the validator contract that if principal != null, parsedToken will be non null? Should this check parsedToken as well?

Yes, ValidateToken always returns both a ClaimsPrincipal and a parsedToken.

Still might be safer to just check principal != null && parsedToken != null

We only reference the token for the expiration values, so I'll check there.

HaoK · 2017-09-19T22:12:51Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationOptions.cs

+        /// <summary>
+        /// Gets or sets the collection of <see cref="ISecurityTokenValidator"/> used to read and validate the <see cref="SecurityToken"/>s.
+        /// </summary>
+        [SuppressMessage("Microsoft.Usage", "CA2208:InstantiateArgumentExceptionsCorrectly", Justification = "By design")]


Can we remove this supression and just throw the argument exception with parameter name correctly?

HaoK · 2017-09-19T22:14:55Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationOptions.cs

+        /// <summary>
+        /// Gets or sets the AuthenticationType used when creating the <see cref="System.Security.Claims.ClaimsIdentity"/>.
+        /// </summary>
+        public string SignInAsAuthenticationType


Nit: SignInAuthenticationType? Does the As add anything? This is what we use along with SignInScheme right?

Obsolete, removed.

HaoK · 2017-09-19T22:15:50Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationOptions.cs

+        /// Gets or sets the <see cref="TokenValidationParameters"/>
+        /// </summary>
+        /// <exception cref="ArgumentNullException"> if 'TokenValidationParameters' is null.</exception>
+        [SuppressMessage("Microsoft.Usage", "CA2208:InstantiateArgumentExceptionsCorrectly", Justification = "Name of the property.")]


Remove? use nameof for exceptions everywhere as well instead of constants.

HaoK · 2017-09-19T22:17:21Z

test/Microsoft.AspNetCore.Authentication.WsFederation.Test/CustomStateDataFormat.cs

+        public string Protect(AuthenticationProperties data)
+        {
+            lastSavedAuthenticationProperties = Serialize(data);
+            return "ValidStateData";


Nit: use a const?

HaoK · 2017-09-19T22:20:01Z

test/Microsoft.AspNetCore.Authentication.WsFederation.Test/WsFederationTest.cs

+            var server = new TestServer(builder);
+
+            var httpClient = server.CreateClient();
+


Can we move all the builder/server/client creation into a CreateClient(), the tests all seem to only use httpClient right?

HaoK

Looks fine in general, just some minor stuff

Tratcher · 2017-09-19T22:22:56Z

test/Microsoft.AspNetCore.Authentication.WsFederation.Test/WsFederationTest.cs

+            services.AddAuthentication(sharedOptions =>
+            {
+                sharedOptions.DefaultScheme = CookieAuthenticationDefaults.AuthenticationScheme;
+                sharedOptions.DefaultSignInScheme = CookieAuthenticationDefaults.AuthenticationScheme;


We do today due to the 2.0.x patch bug.

JunTaoLuo

Regarding build-related items (*.csproj, etc)

kevinchalet · 2017-09-20T14:37:33Z

src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationHandler.cs

+                if (string.IsNullOrEmpty(token))
+                {
+                    Logger.LogWarning("Received a sign-in message without a token.");
+                    return (HandleRequestResult)HandleRequestResult.NoResult();


Same remark. Plus it's inconsistent with the OIDC handler.

I don't see a direct equivalent in OIDC, it detects correct messages by looking for State and CorrelationId, both of which are optional for WsFed. That said, we should probably be using the SkipUnrecognizedRequests pattern here.

That said, we should probably be using the SkipUnrecognizedRequests pattern here.

It's something you're already doing at line 129 by making an IsSignInMessage check, which ensures that a wa parameter is present and corresponds to a sign-in operation. Once this check is made, you can consider the request as a valid WS-Fed message. Every missing parameter that is marked as required by the spec should result in an error.

kevinchalet · 2017-09-20T14:41:28Z

@Tratcher it's 2017 and CSRF/session fixation attacks are no longer cool so please consider adding built-in antiforgery support like what we have in the OIDC handler. To support unsolicited assertions, an opt-in option AcceptUnsolicitedAssertions could be be added.

Tratcher · 2017-09-20T16:20:38Z

The anti-xsrf with an opt-out is an interesting suggestion. I'll open a new issue for that.

…d tests.

natemcmaster · 2017-09-20T18:04:29Z

samples/WsFedSample/WsFedSample.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <PackageReference Include="Microsoft.AspNetCore.Authentication.Cookies" Version="2.0.0" />


This is going to cause issues. You should always use a ProjectReference to anything that is built from the same repo.

cc @JunTaoLuo

Ah I see why this fails universe. We will build this for this release without using Universe as I'll build locally instead. The reason we wanted to take this approach is because we wanted to use the release 2.0.0 packages as dependencies.

Tratcher added this to the 2.0.0-wsfed milestone Sep 19, 2017

Tratcher self-assigned this Sep 19, 2017

Tratcher requested a review from HaoK September 19, 2017 19:14

dnfclas added the cla-already-signed label Sep 19, 2017

Tratcher requested a review from JunTaoLuo September 19, 2017 21:31

HaoK reviewed Sep 19, 2017

View reviewed changes

HaoK suggested changes Sep 19, 2017

View reviewed changes

Tratcher commented Sep 19, 2017

View reviewed changes

HaoK approved these changes Sep 19, 2017

View reviewed changes

JunTaoLuo approved these changes Sep 19, 2017

View reviewed changes

Tratcher force-pushed the tratcher/wsfed branch from b3b8950 to 7fc888f Compare September 19, 2017 22:41

kevinchalet reviewed Sep 20, 2017

View reviewed changes

Tratcher mentioned this pull request Sep 20, 2017

Anti-xsrf plus opt-out for WsFed #1443

Closed

#43 Add Microsoft.AspNetCore.Authentication.WsFederation, samples, an…

674145d

…d tests.

Tratcher force-pushed the tratcher/wsfed branch from 7fc888f to 674145d Compare September 20, 2017 17:22

natemcmaster suggested changes Sep 20, 2017

View reviewed changes

Update preview tag in version number

38f726d

Tratcher closed this Sep 22, 2017

		var server = new TestServer(builder);

		var httpClient = server.CreateClient();

WsFederation review #1441

WsFederation review #1441

Uh oh!

Conversation

Tratcher commented Sep 19, 2017

Uh oh!

dnfclas commented Sep 19, 2017

Uh oh!

Tratcher commented Sep 19, 2017

Uh oh!

Tratcher commented Sep 19, 2017

Uh oh!

Tratcher commented Sep 19, 2017

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!

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!

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo left a comment

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!

JunTaoLuo Sep 20, 2017 •

edited

Loading