Auth 2.0 + IISListener #338

HaoK · 2017-03-20T20:37:38Z

Given that ForwardWindows is on by default, not sure making auth optional is going to fly. All of the tests blew up with Auth missing until I added them all...

dnfclas · 2017-03-20T20:37:42Z

@HaoK,
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-03-20T20:55:23Z

I really wish we had a way of detecting at startup if/which WindowsAuth schemes were enabled in IIS. @shirhatti, @pan-wang? It's hard to implement a correct IIS auth handler without knowing, and we don't want people to have to configure it in two places.

Tratcher · 2017-03-20T20:59:30Z

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

-                {
-                    context.NotAuthenticated();
-                }
+                return Task.FromResult(AuthenticateResult.Success(new AuthenticationTicket(user, Scheme.Name)));


Note we must not call Merge on this principal, we need to keep the original for IsInRole to work. Same for WebListener.

Yuck, ok so that basically means you can't really combine the windows auth with the other auth types or the merging will break things

Yes, but those scenarios don't usually overlap. E.g. you don't do NTLM + Cookies at the same time. So long as we can make Merge smart enough not to merge when there's really just WindowsPrincipal and Anonymous it should be fine.

You made sure this identity never gets merged right?

No we'd have to add something to the security helper (or the authorize filter) to special case this somehow.

Okay so by default we don't have to do anything, merge is only called if the effective policy ends up with multiple schemes requested, which requires a "config error" to have been introduced

Main scenario = DefaultAuthenticate/ChallengeScheme = "Windows" correct?

You're registering both Negotiate and NTLM separately right now. The experience would be better if you combine them.

Then you wouldn't have to specify a default

Are we calling this Windows too like HttpSys?

Tratcher · 2017-03-20T21:01:14Z

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

-                return PriorHandler.SignInAsync(context);
+                case ChallengeBehavior.Automatic:
+                    // If there is a principal already, invoke the forbidden code path
+                    if (GetUser(context.HttpContext) == null)


GetUser needs to cache if this is going to work, it disposes the handle so it can't run more than once.

Updated with GetUser caching

It only caches on success though, not sure if we expect this to be called repeatedly on failures or if we care...

Should be fine, the failure scenarios are primarily for a missing header which will be fast.

HaoK · 2017-04-24T22:26:57Z

Updated with latests auth changes and packages, tests are all passing now

Tratcher · 2017-04-24T23:01:43Z

src/Microsoft.AspNetCore.Server.IISIntegration/IISMiddleware.cs

+                var auth = authentication.FirstOrDefault();
+                if (auth == null)
+                {
+                    throw new InvalidOperationException("IServiceCollection.AddAuthentication() is required to use Authentication.");


Remove this exception for now, ForwardWindowsAuthentication is enabled by default.

Tratcher · 2017-04-24T23:02:35Z

src/Microsoft.AspNetCore.Server.IISIntegration/IISMiddleware.cs

+                    throw new InvalidOperationException("IServiceCollection.AddAuthentication() is required to use Authentication.");
+                }
+
+                auth.AddScheme(new AuthenticationScheme("NTLM", /*displayName*/ null,typeof(AuthenticationHandler)));


named params instead of inline comments?

Tratcher · 2017-04-24T23:03:45Z

...Microsoft.AspNetCore.Server.IISIntegration/Microsoft.AspNetCore.Server.IISIntegration.csproj

    <PackageReference Include="Microsoft.AspNetCore.Hosting.Abstractions" Version="$(AspNetCoreVersion)" />
    <PackageReference Include="Microsoft.AspNetCore.Http" Version="$(AspNetCoreVersion)" />
    <PackageReference Include="Microsoft.AspNetCore.Http.Extensions" Version="$(AspNetCoreVersion)" />
    <PackageReference Include="Microsoft.AspNetCore.HttpOverrides" Version="$(AspNetCoreVersion)" />
    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="$(AspNetCoreVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="$(AspNetCoreVersion)" />


revert duplicate

Tratcher · 2017-04-24T23:07:12Z

test/Microsoft.AspNetCore.Server.IISIntegration.Tests/IISMiddlewareTests.cs

@@ -191,7 +201,7 @@ public void PathBaseHiddenFromServer()
                {
                    app.Run(context =>
                    {
-                        var auth = context.Features.Get<IHttpAuthenticationFeature>();
+                        var auth = context.RequestServices.GetService<IAuthenticationSchemeProvider>();


not equivalent. You should still add the auth service in this case, it's testing the ForwardWindowsAuthentication flag. You may clone this test to make sure everything also works without the auth service.

Updated test to verify handler only added when forward = true, also added a variant that ensures things don't blow up without auth services.

Tratcher · 2017-04-24T23:07:31Z

...pNetCore.Server.IISIntegration.Tests/Microsoft.AspNetCore.Server.IISIntegration.Tests.csproj

@@ -11,6 +11,8 @@

  <ItemGroup>
    <ProjectReference Include="..\..\src\Microsoft.AspNetCore.Server.IISIntegration\Microsoft.AspNetCore.Server.IISIntegration.csproj" />
+    <ProjectReference Include="..\..\..\Security\src\Microsoft.AspNetCore.Authentication\Microsoft.AspNetCore.Authentication.csproj" />


Tratcher · 2017-04-24T23:08:34Z

test/TestSites/StartupHttpsHelloWorld.cs

 using Microsoft.Extensions.Logging;

 namespace TestSites
 {
    public class StartupHttpsHelloWorld
    {
+        public void ConfigureServices(IServiceCollection services)
+        {
+            services.AddAuthenticationCore();


Auth shouldn't be needed in this scenario

It is because of the functional tests calling Challenge

Ah yes only needed in the Ntlm one.

Tratcher · 2017-04-24T23:08:46Z

test/TestSites/StartupHelloWorld.cs

 using Microsoft.Extensions.Logging;

 namespace TestSites
 {
    public class StartupHelloWorld
    {
+        public void ConfigureServices(IServiceCollection services)
+        {
+            services.AddAuthenticationCore();


Auth shouldn't be needed in this scenario

Tratcher · 2017-04-25T21:15:01Z

test/Microsoft.AspNetCore.Server.IISIntegration.Tests/IISMiddlewareTests.cs

-                        var negotiate = await auth.GetSchemeAsync("Negotiate");
-                        Assert.NotNull(negotiate);
-                        Assert.Equal("Microsoft.AspNetCore.Server.IISIntegration.AuthenticationHandler", negotiate.HandlerType.FullName);
+                        var windows = await auth.GetSchemeAsync("Windows");


File a followup issue to figure out how we can optionally handle granular schemes. This will be more relevant for HttpSys, but the solution will likely apply to both.

aspnet/Security#1189

Tratcher · 2017-04-25T21:56:15Z

test/Microsoft.AspNetCore.Server.IISIntegration.Tests/IISMiddlewareTests.cs

@@ -24,6 +25,7 @@ public class IISMiddlewareTests
                .UseSetting("PORT", "12345")
                .UseSetting("APPL_PATH", "/")
                .UseIISIntegration()
+                .ConfigureServices(services => services.AddAuthentication())


Only add this to tests that need it?

Tratcher · 2017-04-25T21:58:37Z

test/Microsoft.AspNetCore.Server.IISIntegration.Tests/IISMiddlewareTests.cs

                        Assert.NotNull(auth);
-                        Assert.Equal("Microsoft.AspNetCore.Server.IISIntegration.AuthenticationHandler", auth.Handler.GetType().FullName);
+                        var windowsAuth = await auth.GetSchemeAsync("Windows");
+                        if (forward)


else assert null? or would GetSchemeAsync throw?

Yup I forgot to add the else check null, thanks :)

Tratcher · 2017-04-25T22:00:11Z

test/Microsoft.AspNetCore.Server.IISIntegration.Tests/IISMiddlewareTests.cs

@@ -150,15 +156,62 @@ public void PathBaseHiddenFromServer()
                .UseSetting("PORT", "12345")
                .UseSetting("APPL_PATH", "/")
                .UseIISIntegration()
+                .ConfigureServices(services => services.AddAuthentication())


make a copy of this test without AddAuthentication and test auth.GetSchemeAsync("Windows"); returns null.

Well we can't even call GetScheme without AddAuthentication as the SchemeProvider will be null, we already have tests that verify that the server can start without AddAuthentication.

Tratcher · 2017-04-25T22:03:30Z

test/TestSites/StartupNtlmAuthentication.cs

                }

                if (context.Request.Path.Equals("/AutoForbid"))
                {
-                    return context.Authentication.ChallengeAsync();
+                    return context.ChallengeAsync();
                }

                if (context.Request.Path.Equals("/RestrictedNegotiate"))


You can remove RestrictedNegotiate, it's not used in the tests.

IISIntegration/test/IISIntegration.FunctionalTests/NtlmAuthentationTest.cs

Lines 97 to 102 in e2537e1

response = await httpClient.GetAsync("/RestrictedNTLM");

responseText = await response.Content.ReadAsStringAsync();

Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode);

Assert.Contains("NTLM", response.Headers.WwwAuthenticate.ToString());

// Note we can't restrict a challenge to a specific auth type, the native auth modules always add themselves.

Assert.Contains("Negotiate", response.Headers.WwwAuthenticate.ToString());

Tratcher · 2017-05-10T21:09:45Z

samples/IISSample/Startup.cs

@@ -12,6 +13,7 @@ public class Startup
    {
        public void ConfigureServices(IServiceCollection services)
        {
+            services.AddAuthentication();


You also need to add the auth middleware if you want HttpContext.User to get set without calling Authenticate, right? E.g. this sample doesn't work right now does it? Oh, the auth middelware is in the security repo, you can't reference it here can you?

HttpSysServer doesn't have this problem because it sets User before the app is invoked. What if the IISMiddleware always set the User? That code in GetUser should be run on every request regardless to clean up handles. The IISMiddleware could resolve the auth service, resolve the handler, and invoke it directly.

Didn't try running the samples, perhaps mistakenly I assumed functional tests were a superset, urgh yeah forgot we can't depend on security in this repo :/

Windows auth doesn't really need the AuthN middleware, it could do it itself / resolve and invoke the service directly.

Functional tests are failing, but other tests are passing

HaoK · 2017-05-11T21:59:54Z

Updated as we discussed, if ForwardWindowsAuth, add Windows scheme, and always set it to httpContext.User if found.

Tratcher · 2017-05-11T22:11:18Z

src/Microsoft.AspNetCore.Server.IISIntegration/IISMiddleware.cs

-                    await _next(httpContext);
-                }
-                finally
+                var result = await httpContext.AuthenticateAsync("Windows");


Constant for Windows. Public static readonly?

IISMiddleware.AuthenticationScheme

Tratcher · 2017-05-11T22:39:53Z

test/IISIntegration.FunctionalTests/NtlmAuthentationTest.cs

@@ -76,6 +76,7 @@ public Task NtlmAuthentication(RuntimeArchitecture architecture, ApplicationType
                        Assert.Equal(HttpStatusCode.OK, response.StatusCode);
                        Assert.Equal("Anonymous?True", responseText);

+                        /* Disabled for now 


Move the skip url here

Tratcher · 2017-05-11T23:00:32Z

samples/IISSample/Startup.cs

@@ -12,6 +13,7 @@ public class Startup
    {
        public void ConfigureServices(IServiceCollection services)
        {
+            services.AddAuthenticationCore();


This can be removed from samples and tests now that it's added by the main component.

Tratcher · 2017-05-11T23:01:36Z

test/TestSites/TestSites.csproj

@@ -12,6 +12,7 @@

  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.AspNetCoreModule" Version="$(AspNetCoreModuleVersion)" />
+    <PackageReference Include="Microsoft.AspNetCore.Authentication.Core" Version="$(AspNetCoreVersion)" />


Tratcher · 2017-05-11T23:01:45Z

samples/IISSample/IISSample.csproj

@@ -11,6 +11,7 @@
  </ItemGroup>

  <ItemGroup>
+    <PackageReference Include="Microsoft.AspNetCore.Authentication.Core" Version="$(AspNetCoreVersion)" />


HaoK · 2017-05-12T22:08:51Z

Updated

Tratcher · 2017-05-12T22:29:56Z

src/Microsoft.AspNetCore.Server.IISIntegration/IISMiddleware.cs

@@ -18,6 +18,8 @@ namespace Microsoft.AspNetCore.Server.IISIntegration
 {
    public class IISMiddleware
    {
+        public static readonly string AuthenticationScheme = "Windows";


Do you want to follow the pattern from Security? E.g. IISDefaults.AuthenticationScheme?
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication.Google/GoogleDefaults.cs#L11
Note those are public const string so they can be used in attributes.

Sure IISDefaults.AuthenticationScheme, and HttpSysDefaults.AuthenticationScheme for the other repo?

Tratcher · 2017-05-12T22:33:30Z

test/Microsoft.AspNetCore.Server.IISIntegration.Tests/IISMiddlewareTests.cs

+                        {
+                            Assert.NotNull(windowsAuth);
+                            Assert.Null(windowsAuth.DisplayName);
+                            Assert.Equal("AuthenticationHandler", windowsAuth.HandlerType.Name);


Tratcher · 2017-05-12T22:35:33Z

test/Microsoft.AspNetCore.Server.IISIntegration.Tests/IISMiddlewareTests.cs

+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public async Task DoesNotBlowUpWithoutAuth(bool forward)


Not relevant anymore? Auth is always added.

Tratcher · 2017-05-13T00:22:03Z

You'll need to update ServerTests too.

HaoK · 2017-05-17T20:55:34Z

8ed21d5

dnfclas added the cla-already-signed label Mar 20, 2017

Tratcher reviewed Mar 20, 2017

View reviewed changes

HaoK mentioned this pull request Apr 13, 2017

The Grand Auth Redesign of 2017 aspnet/Security#1179

Closed

13 tasks

HaoK force-pushed the haok/auth branch from c227f53 to 262c73a Compare April 24, 2017 22:25

Tratcher reviewed Apr 24, 2017

View reviewed changes

Tratcher reviewed Apr 25, 2017

View reviewed changes

Tratcher approved these changes Apr 25, 2017

View reviewed changes

HaoK force-pushed the haok/auth branch from e2b7288 to 2c08f72 Compare April 26, 2017 20:24

This was referenced Apr 27, 2017

Automatic Cookies challenge overridden by Windows Authentication since 1.1.0 aspnet/HttpSysServer#284

Closed

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

Closed

Tratcher suggested changes May 10, 2017

View reviewed changes

HaoK added 11 commits May 11, 2017 13:12

Auth 2.0 + IISIntegration

d52868b

Functional tests are failing, but other tests are passing

Fix typo

86f930c

Cache User

34bf2fd

Target auth 2.0

bfd17e9

Fix tests rebase to real packages

35206a4

PR fixes

37d0646

Fix tests

c369526

PR fixes

c903738

Only add auth to tests that use it

ef1f858

Fix merge

c76881a

Authenticate always for IIS

54b87d2

HaoK force-pushed the haok/auth branch from 2c08f72 to 54b87d2 Compare May 11, 2017 21:59

Tratcher reviewed May 11, 2017

View reviewed changes

Remove AddAuthCore

141a67c

PR fixes

410a9da

Add constant for Windows

41f0b00

Tratcher approved these changes May 12, 2017

View reviewed changes

HaoK closed this May 17, 2017

natemcmaster deleted the haok/auth branch November 1, 2018 06:04

	response = await httpClient.GetAsync("/RestrictedNTLM");
	responseText = await response.Content.ReadAsStringAsync();
	Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode);
	Assert.Contains("NTLM", response.Headers.WwwAuthenticate.ToString());
	// Note we can't restrict a challenge to a specific auth type, the native auth modules always add themselves.
	Assert.Contains("Negotiate", response.Headers.WwwAuthenticate.ToString());

Auth 2.0 + IISListener #338

Auth 2.0 + IISListener #338

Uh oh!

Conversation

HaoK commented Mar 20, 2017

Uh oh!

dnfclas commented Mar 20, 2017

Uh oh!

Tratcher commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 commented Apr 24, 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!

HaoK Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Tratcher commented Mar 20, 2017 •

edited

Loading

HaoK Apr 26, 2017 •

edited

Loading

Tratcher May 10, 2017 •

edited

Loading