Complain if auth hasn't been set up correctly #9181

pranavkm · 2019-04-08T22:33:23Z

pranavkm · 2019-04-08T22:34:26Z

Plain text benchmarks:

Description	RPS	CPU (%)	Memory (MB)	Avg. Latency (ms)	Startup (ms)	First Request (ms)	Latency (ms)
Baseline	1,042,838	95	161	2.73	318	114.95	0.47
Check HttpContext.Items first	939,324	94	158	2.69	329	109.38	0.34
Check endpoint first	1,022,092	95	158	3.57	331	108.98	0.5
Also check Cors	1,008,837	95	158	2.75	321	115.12	0.72
Do auth	114,312	100	173	20.01	418	183.73	0.93

Auth one is a likely worse than normal since I was manually signing in every request.

pranavkm · 2019-04-08T22:34:48Z

cc @davidfowl

src/Http/Routing/src/EndpointMiddleware.cs

src/Security/Authorization/Core/src/Properties/AssemblyInfo.cs

rynowak · 2019-04-09T03:44:37Z

src/Http/Http.Abstractions/src/IAllowAnonymous.cs

@@ -4,7 +4,7 @@
 namespace Microsoft.AspNetCore.Authorization
 {
    /// <summary>
-    /// Marker interface to enable the <see cref="AllowAnonymousAttribute"/>.
+    /// Marker interface to allow anonymous.


This doc comment makes me sad.

Do you have suggestions on what we could do to make this better?

Suggested change

/// Marker interface to allow anonymous.

/// Marker interface to allow access to anonymous users.

maybe?

src/Http/Routing/src/EndpointMiddleware.cs

src/SignalR/server/SignalR/test/Startup.cs

src/Http/Routing/src/EndpointMiddleware.cs

davidfowl · 2019-04-09T06:05:56Z

src/Http/Routing/src/EndpointMiddleware.cs

@@ -11,6 +14,9 @@ namespace Microsoft.AspNetCore.Routing
 {
    internal sealed class EndpointMiddleware
    {
+        internal const string AuthorizationMiddlewareInvokedKey = "__AuthorizationMiddlewareInvoked";


I wonder if these should be features... Then we could back them by fields in Kestrel and avoid the dictionary look up altogether

Is it possible to use mvc without ever initilizing Items? Most middleware use features instead. Note adding a feature would reset the version cache.

Is it possible to use mvc without ever initilizing Items?

MVC uses it in a couple of places (https://github.com/aspnet/AspNetCore/blob/c565386a3ed135560bc2e9017aa54a950b4e35dd/src/Mvc/Mvc.Core/src/Routing/UrlHelperFactory.cs, https://github.com/aspnet/AspNetCore/blob/c565386a3ed135560bc2e9017aa54a950b4e35dd/src/Mvc/Mvc.ViewFeatures/src/Filters/SaveTempDataFilter.cs#L34) so generally you'd see it initialized if MVC is involved.

JamesNK · 2019-04-09T20:30:15Z

src/Http/Routing/src/EndpointMiddleware.cs

+        private static void ThrowMissingAuthMiddlewareException(Endpoint endpoint)
+        {
+            throw new InvalidOperationException($"Endpoint {endpoint.DisplayName} contains authorization metadata, " +
+                "but a middleware was not found that supports authorization.");


We say "a middleware", but we really require middleware that sets AuthorizationMiddlewareInvokedKey.

Is this a problem for people who think that they can implement their own middleware to fix this problem? I think this exception could explicitly say .UseAuthorization() is required. It is the only middleware that knows to set AuthorizationMiddlewareInvokedKey

The alternative is providing a more first class way of saying that auth has run for the request.

The error message is a bit more explicit about what to do resolve it and includes a line saying you could add auth.

The alternative is providing a more first class way of saying that auth has run for the request.

We could add a gesture for this or turn it in to a feature with settable properties. Either of these seem viable to me

src/Http/Routing/src/EndpointMiddleware.cs

pranavkm · 2019-04-09T21:35:41Z

src/SignalR/clients/ts/FunctionalTests/Startup.cs

@@ -73,17 +77,33 @@ public void ConfigureServices(IServiceCollection services)
                    {
                        OnMessageReceived = context =>
                        {
-                            var signalRTokenHeader = context.Request.Query["access_token"];
+                            var endpoint = context.HttpContext.Features.Get<IEndpointFeature>()?.Endpoint;


Auth currently runs twice in SignalR (#9216). Perhaps it would be safer to leave that be in preview4. This particular change was the result of the WebSocketMiddleware not having run prior to endpoint routing - consequently the check context.HttpContext.WebSockets.IsWebSocketRequest failed.

pranavkm · 2019-04-09T22:10:42Z

🆙 📅

pranavkm · 2019-04-09T22:14:30Z

src/SignalR/clients/ts/FunctionalTests/Startup.cs

@@ -73,17 +77,33 @@ public void ConfigureServices(IServiceCollection services)
                    {
                        OnMessageReceived = context =>
                        {
-                            var signalRTokenHeader = context.Request.Query["access_token"];
+                            var endpoint = context.HttpContext.Features.Get<IEndpointFeature>()?.Endpoint;
+                            if (endpoint != null && endpoint.Metadata.GetMetadata<HubMetadata>() != null)


@davidfowl this particular change was suggested by @BrennanConroy. In lieu of checking if a particular request is a WebSocket request, this always checks if the query string is specified. However it needs to read the Auth header if available.

I think this is reasonable.

The slight concern I have is that this could encourage people to put an access_token in the query string when they could have set an Authorization header. We'd like to encourage using the header over the query string since the query string is more likely to be logged.

I don't think that concern is big enough to warrant changing this logic though.

Eilon · 2019-04-09T23:39:15Z

QB approved for Preview 4. Please check in and watch the build stay green.

Please also resolve https://devdiv.visualstudio.com/DevDiv/_workitems/edit/843164 when done.

pranavkm · 2019-04-10T13:02:22Z

@halter73 \ @BrennanConroy how do I proceed with the SignalR functional test failure? The Windows build doesn't say what test failed and I'm not sure how to get to the logs to diagnose the Ubuntu failure.

natemcmaster · 2019-04-10T16:00:05Z

👀 @aspnet/build - this will probably be the last Preview 4 change.

BrennanConroy · 2019-04-10T17:04:31Z

The windows test is the same failure as the ubuntu one.

You can look in the <os>_Test_Logs artifacts folder for "browserlogs.console.log" and "ts.functionaltests.dotnet.log" for the server and client side logs. I looked at them last night and couldn't see any obvious issue. I would just skip the test for now to get this PR in.

pranavkm · 2019-04-10T17:59:23Z

I'm giving it one more go putting back the custom CORS code that was in there. In the absence of it, I'm going to consider skipping the test.

pranavkm · 2019-04-10T19:00:37Z

CORS seems to have done the trick.

BrennanConroy · 2019-04-10T19:02:06Z

That is odd... any idea why?

pranavkm · 2019-04-10T19:10:40Z

Not entirely sure. Perhaps the way the CI runs tests requires CORS to have run prior to redirect?

src/Http/Routing/src/RouteOptions.cs

Fixes #9041

natemcmaster · 2019-04-10T23:03:39Z

/azp run AspNetCore-ci

azure-pipelines · 2019-04-10T23:03:47Z

Azure Pipelines failed to run 1 pipeline(s).

natemcmaster · 2019-04-11T17:33:28Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=150893&view=logs

natemcmaster · 2019-04-11T17:35:56Z

Previous test run failed on one flaky test. This has QB and review approvals, so I'm bringing it in with admin permissions.

pranavkm requested review from analogrelay, jkotalik and Tratcher as code owners April 8, 2019 22:33

pranavkm requested review from JamesNK and rynowak April 8, 2019 22:34

Eilon added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer labels Apr 8, 2019

pranavkm changed the base branch from master to release/3.0-preview4 April 8, 2019 22:53

pranavkm requested review from BrennanConroy, halter73 and mikaelm12 as code owners April 9, 2019 00:43

davidfowl reviewed Apr 9, 2019

View reviewed changes

src/Http/Routing/src/EndpointMiddleware.cs Outdated Show resolved Hide resolved

Tratcher reviewed Apr 9, 2019

View reviewed changes

src/Security/Authorization/Core/src/Properties/AssemblyInfo.cs Show resolved Hide resolved

rynowak reviewed Apr 9, 2019

View reviewed changes

src/Http/Routing/src/EndpointMiddleware.cs Show resolved Hide resolved

rynowak reviewed Apr 9, 2019

View reviewed changes

src/SignalR/server/SignalR/test/Startup.cs Show resolved Hide resolved

rynowak reviewed Apr 9, 2019

View reviewed changes

src/Http/Routing/src/EndpointMiddleware.cs Outdated Show resolved Hide resolved

davidfowl reviewed Apr 9, 2019

View reviewed changes

JamesNK reviewed Apr 9, 2019

View reviewed changes

pranavkm force-pushed the prkrishn/auth-check branch from 8ce2ba7 to 6e1bf58 Compare April 9, 2019 21:31

pranavkm commented Apr 9, 2019

View reviewed changes

rynowak reviewed Apr 10, 2019

View reviewed changes

src/Http/Routing/src/RouteOptions.cs Outdated Show resolved Hide resolved

rynowak approved these changes Apr 10, 2019

View reviewed changes

pranavkm added 8 commits April 10, 2019 15:08

Complain if auth hasn't been set up correctly

b3175fd

Fixes #9041

Update ref assemblies

32ae857

Update test app to do Auth

d298d1f

Fixup tests

9c70ea1

update ref

706ab65

Bring back CORS

d183469

Changes per PR

1787e2c

Update refs

6cd4599

pranavkm force-pushed the prkrishn/auth-check branch from 4ccde15 to 6cd4599 Compare April 10, 2019 22:09

pranavkm closed this Apr 10, 2019

pranavkm reopened this Apr 10, 2019

natemcmaster merged commit ae34697 into release/3.0-preview4 Apr 11, 2019

natemcmaster deleted the prkrishn/auth-check branch April 11, 2019 17:36

martincostello mentioned this pull request Apr 13, 2019

HTTP 500 if request to a CORS-enabled endpoint is requested without an origin header #9348

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complain if auth hasn't been set up correctly #9181

Complain if auth hasn't been set up correctly #9181

pranavkm commented Apr 8, 2019

pranavkm commented Apr 8, 2019

pranavkm commented Apr 8, 2019

rynowak Apr 9, 2019

pranavkm Apr 9, 2019

halter73 Apr 9, 2019

davidfowl Apr 9, 2019

Tratcher Apr 9, 2019

pranavkm Apr 9, 2019

JamesNK Apr 9, 2019

pranavkm Apr 9, 2019

pranavkm Apr 9, 2019

pranavkm commented Apr 9, 2019

pranavkm Apr 9, 2019

halter73 Apr 9, 2019

Eilon commented Apr 9, 2019 •

edited by analogrelay

Loading

pranavkm commented Apr 10, 2019

natemcmaster commented Apr 10, 2019

BrennanConroy commented Apr 10, 2019

pranavkm commented Apr 10, 2019

pranavkm commented Apr 10, 2019

BrennanConroy commented Apr 10, 2019

pranavkm commented Apr 10, 2019

natemcmaster commented Apr 10, 2019

azure-pipelines bot commented Apr 10, 2019

natemcmaster commented Apr 11, 2019

natemcmaster commented Apr 11, 2019

	/// Marker interface to allow anonymous.
	/// Marker interface to allow access to anonymous users.

Complain if auth hasn't been set up correctly #9181

Complain if auth hasn't been set up correctly #9181

Conversation

pranavkm commented Apr 8, 2019

pranavkm commented Apr 8, 2019

pranavkm commented Apr 8, 2019

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

pranavkm commented Apr 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eilon commented Apr 9, 2019 • edited by analogrelay Loading

pranavkm commented Apr 10, 2019

natemcmaster commented Apr 10, 2019

BrennanConroy commented Apr 10, 2019

pranavkm commented Apr 10, 2019

pranavkm commented Apr 10, 2019

BrennanConroy commented Apr 10, 2019

pranavkm commented Apr 10, 2019

natemcmaster commented Apr 10, 2019

azure-pipelines bot commented Apr 10, 2019

natemcmaster commented Apr 11, 2019

natemcmaster commented Apr 11, 2019

Eilon commented Apr 9, 2019 •

edited by analogrelay

Loading