Prototype Auth 2.0 changes #325

HaoK · 2017-03-13T19:59:30Z

@Tratcher thoughts? Also we need to update/replace the auth tests since MessagePump is being new'ed up and we need RequestServices for auth to work...

HaoK · 2017-03-13T20:00:31Z

src/Microsoft.AspNetCore.Server.HttpSys/AuthenticationHandler.cs

            foreach (var scheme in ListEnabledAuthSchemes())
            {
                var authScheme = scheme.ToString();
                // Not including any auth types means it's a blanket challenge for any auth type.
-                if (automaticChallenge || string.Equals(context.AuthenticationScheme, authScheme, StringComparison.Ordinal))
+                if (string.Equals(context.AuthenticationScheme, authScheme, StringComparison.Ordinal))


I actually don't think we need this check at all anymore, since we are registering a handler per scheme

We'd just need to check if the scheme is enabled to determine if we return anything or not

HaoK · 2017-03-13T20:02:41Z

Tests are broken due to RequestServices being null so this is just an early look at how things stand.

Tratcher · 2017-03-15T22:31:10Z

src/Microsoft.AspNetCore.Server.HttpSys/WebHostBuilderHttpSysExtensions.cs

+            options.AddScheme("Negotiate", builder => builder.HandlerType = typeof(AuthenticationHandler));
+            options.AddScheme("NTLM", builder => builder.HandlerType = typeof(AuthenticationHandler));
+            options.AddScheme("Basic", builder => builder.HandlerType = typeof(AuthenticationHandler));
+            options.AddScheme("None", builder => builder.HandlerType = typeof(AuthenticationHandler));


Remove None

Lets move this to server Start. See MessagePump

Tratcher · 2017-03-15T22:33:09Z

src/Microsoft.AspNetCore.Server.HttpSys/AuthenticationHandler.cs

@@ -91,13 +81,23 @@ public Task ChallengeAsync(ChallengeContext context)
            return TaskCache.CompletedTask;
        }

-        public void GetDescriptions(DescribeSchemesContext context)
+        public Task<bool> HandleRequestAsync()


Didn't you move this to a separate interface?

Yep this PR was created against Iteration 2 (prior to named changes and the split of HandleRequest)

I'll update this PR now against latest

Tratcher · 2017-03-15T22:40:22Z

src/Microsoft.AspNetCore.Server.HttpSys/AuthenticationHandler.cs

@@ -112,14 +112,14 @@ public Task SignOutAsync(SignOutContext context)
            return TaskCache.CompletedTask;


These comments about !Accepted aren't true anymore. SignIn should throw NotSupportedException. SignOut is something we're discussing elsewhere.

Tratcher · 2017-03-15T22:41:58Z

src/Microsoft.AspNetCore.Server.HttpSys/AuthenticationHandler.cs

@@ -16,14 +17,7 @@ internal class AuthenticationHandler : IAuthenticationHandler
        private AuthenticationSchemes _authSchemes;
        private AuthenticationSchemes _customChallenges;

-        internal AuthenticationHandler(RequestContext requestContext)
-        {
-            _requestContext = requestContext;


So how do we get access to this now?

HaoK · 2017-03-16T22:21:09Z

Updated with fixes, register the handlers in MessagePump Start instead which is much cleaner, luckily it doesn't take any options... there's no story yet for how to configure handler options via dynamic AddScheme....

Tratcher · 2017-03-16T22:42:58Z

src/Microsoft.AspNetCore.Server.HttpSys/MessagePump.cs

@@ -32,7 +33,7 @@ internal class MessagePump : IServer

        private readonly ServerAddressesFeature _serverAddresses;

-        public MessagePump(IOptions<HttpSysOptions> options, ILoggerFactory loggerFactory)


For this to work UseHttpSysServer has to register the service?

Well maybe AddAuthentication needs to be added as part of hosting similar to options, its kinda required...

It's required for HttpSysServer, but only if auth is enabled. Same for IIS Integration. There are plenty of apps that don't need it though.

Probably need to factor the new auth stuff into the 'core stuff' (and the security base class stuff). Hosting should add the core stuff so everyone can add schemes...

Resolve it as optional?

this mimics how anyone could hook their own IAuthenticationHandler on httpContext today, its always there today

Well do we really want context.AuthenticateAsync to throw due to no service found if AddAuthentication isn't called?

Sounds fine. Why Authenticate without Authentication?

Ok so we are leaving it as an exercise for the user to ensure something is calling AddAuthentication, MVC/identity will do this, as well as all of our security projects

Updated to resolve IAuthenticationSchemeProvider as enumerable and only throw if AuthentionSchemes for the server options is not none.

Tratcher · 2017-03-20T19:38:21Z

src/Microsoft.AspNetCore.Server.HttpSys/Microsoft.AspNetCore.Server.HttpSys.csproj

@@ -9,7 +9,9 @@
    <PackageTags>aspnetcore;weblistener;httpsys</PackageTags>
  </PropertyGroup>
  <ItemGroup>
-    <PackageReference Include="Microsoft.AspNetCore.Hosting" Version="1.2.0-*" />
+    <ProjectReference Include="..\..\..\Hosting\src\Microsoft.AspNetCore.Hosting\Microsoft.AspNetCore.Hosting.csproj" />
+    <ProjectReference Include="..\..\..\Security\src\Microsoft.AspNetCore.Authentication\Microsoft.AspNetCore.Authentication.csproj" />


Authentication or Authentication.Abstractions? These are getting move to HttpAbstractions right? Otherwise this is going to cause a layering issue.

Should be abstractions yes

Tratcher · 2017-03-20T19:39:32Z

Looks pretty good. Still need help writing the tests?

HaoK · 2017-03-20T19:48:57Z

Yeah tests don't work at all here, (request services are null). I'm working on the PR for IISIntegration now.

Tratcher

Try adding this to Utilities:



        internal static IWebHost CreateDynamicHost(string basePath, AuthenticationSchemes authType, bool allowAnonymous, out string root, out string baseAddress, RequestDelegate app)
        {
            lock (PortLock)
            {
                while (NextPort < MaxPort)
                {
                    var port = NextPort++;
                    var prefix = UrlPrefix.Create("http", "localhost", port, basePath);
                    root = prefix.Scheme + "://" + prefix.Host + ":" + prefix.Port;
                    baseAddress = prefix.ToString();

                    var builder = new WebHostBuilder()
                        .UseHttpSys(options =>
                        {
                            options.UrlPrefixes.Add(prefix);
                            options.Authentication.Schemes = authType;
                            options.Authentication.AllowAnonymous = allowAnonymous;
                        })
                        .Configure(appBuilder => appBuilder.Run(app));

                    var host = builder.Build();


                    try
                    {
                        host.Start();
                        return host;
                    }
                    catch (HttpSysException)
                    {
                    }

                }
                NextPort = BasePort;
            }
            throw new Exception("Failed to locate a free port.");
        }

(Untested, but at least a starting point for you)

Tratcher · 2017-03-20T23:10:36Z

test/Microsoft.AspNetCore.Server.HttpSys.FunctionalTests/AuthenticationTests.cs

+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+        }
+
+        private static TestServer CreateServer(Action<HttpSysOptions> configureHttpSys = null, Func<HttpContext, Task> testpath = null)


This doesn't belong in the FunctionalTest project. Move to unit tests.

HaoK · 2017-03-27T23:57:09Z

Updated after switching to your new dynamic server and things are almost working. Challenge is the last test that is broken, I'll comment on the test to explain the issue

HaoK · 2017-03-27T23:58:36Z

src/Microsoft.AspNetCore.Server.HttpSys/AuthenticationHandler.cs

-                            throw new NotSupportedException(context.Behavior.ToString());
-                    }
-                }
-            }
            // A challenge was issued, it overrides any pre-set auth types.
            _requestContext.Response.AuthenticationChallenges = _customChallenges;


This set for the _customChallenges no longers works because we have 3 different handlers all trying to challenge at the same time, before they were all the same handler instance, so _customChallenges would have all 3. While now they are different handlers, so we end up only setting the last auth scheme.

HaoK · 2017-03-27T23:59:53Z

test/Microsoft.AspNetCore.Server.HttpSys.FunctionalTests/AuthenticationTests.cs

            {
                Assert.NotNull(httpContext.User);
                Assert.NotNull(httpContext.User.Identity);
                Assert.False(httpContext.User.Identity.IsAuthenticated);
-                return httpContext.Authentication.ChallengeAsync();
+                return httpContext.ChallengeAsync(authType.ToString());


This test ends up failing since it expects to see all 3 challenges but we only send 1. Not sure how best to fix this behavior.

HTTP auth is one of the few places it makes sense for there to be multiple automatic challenge types. Bearer, Basic, NTLM, Negotiate, Kerberos, and Digest can all be challenged on the same response. That implies DefaultChallengeScheme and DefaultAuthenticationScheme would need to be a list in this scenario.

Meh one of our main goals was to make it impossible to say challenge Bearer + Cookies via DefaultChallengeScheme

Since HTTP auth is special, can't it do the multiple challenges itself? Why does it need the ability to be baked in.

How can it do multiple itself? Make one handler for 'Windows' and have it issue challenges for all enabled schemes?

Yeah what's the usage scenario, would that work? Can it be considered a "Host" scheme, and translate that into the AuthenticationSchemes inside of its options? Or do you expect ppl to actually put Authorize(NTLM/Basic/Negotitate)?

Most people would do all or none, but it's an awkward limitation not to be able to invoke them individually. What if you had two sets of handlers? One each for the individuals, and one that combines all of them?

Note IIS would just have one, it can't be more specific

Yeah that's not too bad, so we can have something like "Windows" which would challenge everything that was enabled on the host, and leave the individual ones as is?

HaoK · 2017-04-26T20:39:25Z

Replaced by #354

HaoK commented Mar 13, 2017

View reviewed changes

Tratcher reviewed Mar 15, 2017

View reviewed changes

Tratcher reviewed Mar 16, 2017

View reviewed changes

Tratcher reviewed Mar 20, 2017

View reviewed changes

HaoK force-pushed the haok/auth branch from e687381 to 4bd7ab9 Compare March 27, 2017 23:56

HaoK commented Mar 27, 2017

View reviewed changes

Tratcher mentioned this pull request Mar 30, 2017

Automatic Cookies challenge overridden by Windows Authentication since 1.1.0 #284

Closed

HaoK mentioned this pull request Apr 13, 2017

The Grand Auth Redesign of 2017 aspnet/Security#1179

Closed

13 tasks

HaoK added 10 commits April 24, 2017 15:38

Prototype Auth 2.0 changes

de340e4

PR fixes

9516e86

Cleanup

23f3a3e

Make auth optional

bc56c3a

Try to get tests working

2370336

Fix tests

bbc4675

Fixed tests

0122711

PR fixes

f99755f

Update project

17418f6

Fix tests

b3f6351

HaoK force-pushed the haok/auth branch from 4bd7ab9 to b3f6351 Compare April 25, 2017 20:21

HaoK mentioned this pull request Apr 26, 2017

HttpSys => Auth 2.0 #354

Closed

HaoK closed this Apr 26, 2017

HaoK mentioned this pull request Apr 27, 2017

Auth 2.0 Part II: Revenge of AuthZ aspnet/Security#1190

Closed

15 tasks

		@@ -112,14 +112,14 @@ public Task SignOutAsync(SignOutContext context)
		return TaskCache.CompletedTask;

		@@ -32,7 +33,7 @@ internal class MessagePump : IServer

		private readonly ServerAddressesFeature _serverAddresses;

		public MessagePump(IOptions<HttpSysOptions> options, ILoggerFactory loggerFactory)

Prototype Auth 2.0 changes #325

Prototype Auth 2.0 changes #325

Conversation

HaoK commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Mar 13, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Mar 16, 2017

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

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

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

Tratcher commented Mar 20, 2017

HaoK commented Mar 20, 2017

Tratcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK Mar 28, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Apr 26, 2017

HaoK Mar 28, 2017 •

edited

Loading