Skip to content

Throw when UseAuthorization is incorrectly configured #14401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 1, 2019
Merged

Throw when UseAuthorization is incorrectly configured #14401

merged 1 commit into from
Oct 1, 2019

Conversation

pranavkm
Copy link
Contributor

  • Update AuthZ & Cors middlewares to only set endpoint routing metadata when
    executing in the context of endpoint routing
  • Add analyzers for incorrect UseAuth use

Fixes #14049

@Eilon Eilon added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Sep 24, 2019
@pranavkm pranavkm requested review from JamesNK and rynowak September 24, 2019 22:41
JamesNK
JamesNK reviewed Sep 24, 2019
View reviewed changes
src/Middleware/CORS/src/Infrastructure/CorsMiddleware.cs
var endpoint = context.GetEndpoint();

if (endpoint != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment about why the flag is only set if an endpoint is present

Tratcher
Tratcher reviewed Sep 24, 2019
View reviewed changes
src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs Outdated
// Flag to indicate to the system that the middleware was run in the context of endpoint routing.
// Setting this flag allows a check in EndpointRoutingMiddleware that verifies if the middleware
// pipeline is wired correctly to succeed.
context.Items[AuthorizationMiddlewareInvokedKey] = AuthorizationMiddlewareInvokedValue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename key/value to indicate this is endpoint specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EndpointAuthInvoked?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AuthorizationInvokedWithEndpointsKey?

@analogrelay analogrelay added this to the 3.1.0-preview1 milestone Sep 25, 2019
@pranavkm
Copy link
Contributor Author

🆙 📅

@pranavkm
Copy link
Contributor Author

@aspnet/build what's up with this error:

Running tests:   'tar' is not recognized as an internal or external command, operable program or batch file.
F:\workspace\_work\1\s\src\Framework\ref\Microsoft.AspNetCore.App.Ref.csproj(179,5): error MSB3073: The command "tar -czf F:\workspace\_work\1\s\artifacts\installers\Release\aspnetcore-targeting-pack-3.1.0-ci.tar.gz ." exited with code 9009.
##[error]src\Framework\ref\Microsoft.AspNetCore.App.Ref.csproj(179,5): error MSB3073: The command "tar -czf F:\workspace\_work\1\s\artifacts\installers\Release\aspnetcore-targeting-pack-3.1.0-ci.tar.gz ." exited with code 9009.

@dougbu
Copy link
Contributor

dougbu commented Sep 26, 2019

@pranavkm we've asked @MattGal why the Windows version on the Windows agents no longer includes tar. We haven't heard back on this regression.

rynowak
rynowak reviewed Sep 30, 2019
View reviewed changes
src/Analyzers/Analyzers/src/UseAuthorizationAnalyzer.cs

var type = (INamedTypeSymbol)context.Symbol;

foreach (var middlewareAnalysis in _context.GetRelatedAnalyses<MiddlewareAnalysis>(type))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YSK: this doesn't do control-flow analysis - ie, it's trivial to cause false positives or false negatives when .Map is involved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. We can always recommend turning the analyzer for users where this becomes problematic. Doing flow analysis for this sounds like a lot more effort

rynowak
rynowak reviewed Sep 30, 2019
View reviewed changes
src/Analyzers/Analyzers/test/TestFiles/StartupAnalyzerTest/UseAuthFallbackPolicy.cs
{
// This sort of setup would be useful if the user wants to use Auth for non-endpoint content to be handled using the Fallback policy, while
// using the second instance for regular endpoint routing based auth. We do not want to produce a warning in this case.
app.UseAuthorization();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't super get this - it only makes sense if there's a terminal middleware between here and routing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll toss in a Static files to appear after this.

rynowak
rynowak reviewed Sep 30, 2019
View reviewed changes
src/Analyzers/Analyzers/test/TestFiles/StartupAnalyzerTest/UseAuthMultipleTimes.cs
public void Configure(IApplicationBuilder app)
{
app.UseRouting();
app.UseAuthorization();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File is called UseAuthMultipleTimes - yet it uses auth a single time. How can this be possible?

rynowak
rynowak reviewed Sep 30, 2019
View reviewed changes
src/Http/Routing/test/FunctionalTests/EndpointRoutingIntegrationTest.cs Outdated
{
app.UseRouting();
app.UseEndpoints(b => b.Map("/", TestDelegate).RequireAuthorization());

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JamesNK

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@pranavkm pranavkm force-pushed the prkrishn/auth branch 2 times, most recently from 3a587df to 219827c Compare October 1, 2019 19:28
@pranavkm pranavkm changed the base branch from release/3.1 to release/3.1-preview1 October 1, 2019 20:43
@pranavkm
Throw when UseAuthorization is incorrectly configured
5256b76 
* Update AuthZ & Cors middlewares to only set endpoint routing metadata when
  executing in the context of endpoint routing
* Add analyzers for incorrect UseAuth use

Fixes #14049
@mkArtakMSFT mkArtakMSFT merged commit 29a1d50 into release/3.1-preview1 Oct 1, 2019
@mkArtakMSFT mkArtakMSFT deleted the prkrishn/auth branch October 1, 2019 23:08
@pranavkm pranavkm mentioned this pull request Oct 10, 2019
Merged
@centreboard centreboard mentioned this pull request Oct 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl davidfowl left review comments

@JamesNK JamesNK JamesNK left review comments

@rynowak rynowak rynowak approved these changes

@Tratcher Tratcher Tratcher left review comments

@HaoK HaoK HaoK left review comments

@analogrelay analogrelay Awaiting requested review from analogrelay

@jkotalik jkotalik Awaiting requested review from jkotalik

@SteveSandersonMS SteveSandersonMS Awaiting requested review from SteveSandersonMS

Assignees
No one assigned
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Milestone
3.1.0-preview1
Development

Successfully merging this pull request may close these issues.
10 participants
@pranavkm @dougbu @davidfowl @JamesNK @rynowak @Tratcher @HaoK @analogrelay @Eilon @mkArtakMSFT