Support for Pushed Authorization (PAR) in OIDC Handler #55069

josephdecock · 2024-04-11T03:06:47Z

Support for Pushed Authorization Requests

Adds support for Pushed Authorization Requests (RFC 9126) to the OIDC authentication handler.

Adds support for pushed authorization requests (RFC 9126) to the OpenIdConnect authentication handler.

josephdecock · 2024-04-11T13:30:43Z

@dotnet-policy-service agree company="Duende Software"

halter73

Thanks for the PR. This helps a lot with the API review. I don't think I would have noticed the interaction between UsePushedAuthorization and "require_pushed_authorization_requests" without looking at the code for example.

Sorry for the delay reviewing the corresponding API proposal. Feel free to start on the tests. I think those will be easy to update even if we adjust the API.

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

halter73 · 2024-05-31T01:14:34Z

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs


+        var parRequired = ConfigFlagEnabled("require_pushed_authoriation_requests");
+
+        if (Options.UsePushedAuthorization || parRequired)


Do we need a way for the developer to say use pushed authorization if possible, but don't fail if there's no "pushed_authorization_request_endpoint" available? Maybe this should be the default behavior.

Do we expect this support to be variable? E.g. an auth server will support it or not, you should be able to determine that during development. If that support changes later I don't know that we should react dynamically in prod to it as there may be side-effects such as your extension callbacks no longer being called.

The change in behavior seems small. It doesn't even seem to prevent any callbacks from being called. It does change the parameters used for the user agent's ultimate authorization request, but I don't see how that's likely to cause issues. I'd be more worried about the OIDC provider requiring a custom authentication scheme to use the PAR endpoint, but we could fall back to not using PAR if given a non-200 response if PAR wasn't explicitly opted into. We could even stop trying PAR if the first attempt fails.

I think it's very possible that an OIDC provider that didn't support PAR during initial development might add support later. I understand not wanting to risk any potential breaks caused by an OIDC provider lighting up a feature that's not actually required for us to use, but it feels like we should default to the more secure option if it's available and it's unlikely to break anyone.

@blowdart do you have an opinion on this?

An apps behavior should be in control of the developer, not a 3rd party, and shouldn't change without the developer initiating and testing that change.

The OpenIdConnectHandler lighting up features only if the .well-known/openid-configuration indicates the OIDC provider supports it is not breaking new ground. If you set GetClaimsFromUserInfoEndpoint to true, we will ignore it (aside from logging a debug-level message) if there is no "userinfo_endpoint".

aspnetcore/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

Lines 934 to 940 in 53db4d9

var userInfoEndpoint = _configuration?.UserInfoEndpoint;

if (string.IsNullOrEmpty(userInfoEndpoint))

{

Logger.UserInfoEndpointNotSet();

return HandleRequestResult.Success(new AuthenticationTicket(principal, properties, Scheme.Name));

}

We can debate what the default should be, but I think it's reasonable to want to have a way to say if the OIDC provider supports a feature that should enhance security, use it. I could see somebody wanting to do this in a library or code analyzer that ensures PAR is used when available.

One thing that I've encountered is that some providers actually support PAR, but not for all clients. We ran into this when we added similar logic to IdentityModel.OidcClient (DuendeArchive/IdentityModel.OidcClient#428).

halter73 · 2024-05-31T01:27:25Z

src/Security/Authentication/OpenIdConnect/src/Events/PushedAuthorizationContext.cs

+    /// Initializes a new instance of <see cref="PushedAuthorizationContext"/>.
+    /// </summary>
+    /// <inheritdoc />
+    public PushedAuthorizationContext(HttpContext context, AuthenticationScheme scheme, OpenIdConnectOptions options, OpenIdConnectMessage message, HttpRequestMessage parRequest, AuthenticationProperties properties)


Suggested change

public PushedAuthorizationContext(HttpContext context, AuthenticationScheme scheme, OpenIdConnectOptions options, OpenIdConnectMessage message, HttpRequestMessage parRequest, AuthenticationProperties properties)

public PushedAuthorizationContext(HttpContext context, AuthenticationScheme scheme, OpenIdConnectOptions options, OpenIdConnectMessage message, AuthenticationProperties properties)

If we think the PushedAuthorizationContext needs the HttpRequestMessage for some scenario, we should add a property for it instead.

I've rearranged this, so we don't need a separate parameter at all. It turned out to be quite inconvenient to deal with the request message, because both the handler and the event might want to make changes to the parameters that will be sent.

halter73 · 2024-05-31T01:36:24Z

@Tratcher Do you mind taking a look at this too? Thanks.

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectOptions.cs

halter73 · 2024-05-31T19:00:48Z

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

+        var credential = $"{escapedClientId}:{escapedClientSecret}";
+        var encodedCredential = Convert.ToBase64String(Encoding.UTF8.GetBytes(credential));
+
+        parRequest.Headers.Authorization = new AuthenticationHeaderValue("basic", encodedCredential);


Why was this header chosen for authentication by default? Looking at RFC 9126, it allows authenticating the same way you would with the token endpoint as defined in RFC 6749.

I don't see anything where the client ID and client secret are put into a basic auth header. The "introductory example" in RFC 9126 uses "JWT client assertion-based authentication" defined in RFC 7523, but that doesn't seem to provide a mechanism to acquire the JWT.

The common thing for the token endpoint seems to be to put the client ID and client secret in the request body of the token endpoint even though it's "NOT RECOMMENDED and SHOULD be limited to clients unable to directly utilize the HTTP Basic authentication scheme."

Why not just include the client secret in the parRequest body via authorizeRequest.Parameters since it's never seen by the client? It already includes the client ID, the spec seems to allow it, and it's effectively what we do by default for the token endpoint, right?

The request also includes, as appropriate for the given client, any additional parameters necessary for client authentication (e.g., client_secret or client_assertion and client_assertion_type)

https://datatracker.ietf.org/doc/html/rfc9126#section-2.1

I think you're right - I'll switch to the request body style.

I was trying to follow the "NOT RECOMMENDED" language, despite the inconsistency with the way we currently call the token endpoint. I had some thought that we might want to change that as well, but the latest draft of OAuth 2.1 actually drops this language:

Change client credentials to be required to be supported in the request body to avoid HTTP Basic authentication encoding interop issues

So that really makes it a moot point, and we should definitely just use the request body.

Having basic auth of any description would be rather problematic from a security POV :)

halter73 · 2024-05-31T19:06:41Z

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

+                Content = new FormUrlEncodedContent(authorizeRequest.Parameters),
+            };
+            var context = new PushedAuthorizationContext(Context, Scheme, Options, authorizeRequest, parRequest, properties);
+            await Events.PushAuthorization(context);


I just noticed that the authorizeRequest.Parameters are already serialized into the FormUrlEncodedContent by the time this event is called. I know that's probably because you want to allow the event to modify the HttpRequestMessage, but I think we should set HttpRequestMessage.Content after this event if it's still null.

It's probably also a good idea to give the event a chance to set parRequest.RequestUri before failing due to missing "pushed_authorization_request_endpoint" from the configuration. I'm not sure how likely it is that anyone will need it, but it probably doesn't hurt to allow for that flexibility.

Instead of allowing the event to modify the request message, I'm just passing an OpenIdConnectMessage that the handler later pushes. With no http request message, we don't have to worry about its content being null.

The context now can affect the flow of control in the handler in 3 ways:

If it handles authentication with HandleClientAuthentication, we skip setting the client secret

If it decides that we don't need to push with SkipPush, we completely skip pushing, and redirect as normal

If it decides to completely control the call to the par endpoint with HandlePush, the event won't make the par request and instead will use the request_uri passed to HandlePush

The event can also tweak parameters before they are pushed, but not call any of the context's methods to get otherwise default behavior.

The downside to this is that it is possible to use the API in an ambiguous way: what should the handler do if the event calls both HandlePush and SkipPush? Should that be an error? A logged warning? Should the last call take precedence?

The downside to this is that it is possible to use the API in an ambiguous way: what should the handler do if the event calls both HandlePush and SkipPush? Should that be an error? A logged warning? Should the last call take precedence?

I think throwing an InvalidOperationException is a good choice if we detect this. I don't think it should depend on order.

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

halter73 · 2024-05-31T19:21:47Z

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs


+        var parRequired = ConfigFlagEnabled("require_pushed_authoriation_requests");
+
+        if (Options.UsePushedAuthorization || parRequired)


The change in behavior seems small. It doesn't even seem to prevent any callbacks from being called. It does change the parameters used for the user agent's ultimate authorization request, but I don't see how that's likely to cause issues. I'd be more worried about the OIDC provider requiring a custom authentication scheme to use the PAR endpoint, but we could fall back to not using PAR if given a non-200 response if PAR wasn't explicitly opted into. We could even stop trying PAR if the first attempt fails.

I think it's very possible that an OIDC provider that didn't support PAR during initial development might add support later. I understand not wanting to risk any potential breaks caused by an OIDC provider lighting up a feature that's not actually required for us to use, but it feels like we should default to the more secure option if it's available and it's unlikely to break anyone.

@blowdart do you have an opinion on this?

blowdart · 2024-06-06T18:50:40Z

I think it's very possible that an OIDC provider that didn't support PAR during initial development might add support later. I understand not wanting to risk any potential breaks caused by an OIDC provider lighting up a feature that's not actually required for us to use, but it feels like we should default to the more secure option if it's available and it's unlikely to break anyone.

We should be more secure by default, and if that breaks people give them an opt-out

halter73 · 2024-06-18T00:29:51Z

Just to give you a heads up, code complete for preview 6 is this Wednesday. I know that's very soon, so I'm fine with waiting for preview 7 which is scheduled to be code complete July 19th. I think that's the latest we'd take this change.

halter73 · 2024-07-15T18:26:36Z

Remember, code complete for the last preview is this Friday, July 19th.

You can indicate if the event handled authentication, handled the push entirely, or opted out of pushing. Doing more than 1 of those is at best redundant but often ambiguous, so it is always disallowed.

josephdecock · 2024-07-18T14:04:26Z

@halter73 I marked this as ready for review last night, then woke up and realized I wanted to do a bit more cleanup and handle the error case of the event calling multiple context methods (e.g., you shouldn't be able to both say that pushing is skipped, but also you handled the authentication part of the push). That's all in now, and it is really, truly, for-real-this-time ready for review.

Thanks!

halter73 · 2024-07-18T19:52:22Z

...Security/Authentication/OpenIdConnect/samples/OpenIdConnectSample/OpenIdConnectSample.csproj

  <PropertyGroup>
    <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
-    <UserSecretsId>aspnet5-OpenIdConnectSample-20151210110318</UserSecretsId>
+     <UserSecretsId>aspnet5-OpenIdConnectSample-20151210110318</UserSecretsId> 


Suggested change

<UserSecretsId>aspnet5-OpenIdConnectSample-20151210110318</UserSecretsId>

<UserSecretsId>aspnet5-OpenIdConnectSample-20151210110318</UserSecretsId>

src/Security/Authentication/OpenIdConnect/src/Events/PushedAuthorizationContext.cs

halter73 · 2024-07-18T20:22:09Z

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

    }

+    // TODO GetConfigString and ConfigFlagEnabled are only used in PAR, and won't be necessary after
+    // Microsoft.IdentityModel.Protocols.OpenIdConnect is updated to v7.6.1 or later.


We've updated to 8.0.0, so we should be able to use PushedAuthorizationRequestEndpoint and the like now.

Awesome, I switched to it!

Out of curiosity, how are the dependencies managed? The build system is ... well it's a lot for the uninitiated 😄

I found eng/BaselineDesigner.props, but it has this:

<ItemGroup Condition=" '$(PackageId)' == 'Microsoft.AspNetCore.Authentication.JwtBearer' AND ('$(TargetFramework)' == '$(DefaultNetCoreTargetFramework)' OR '$(TargetFramework)' == 'net8.0') "> <BaselinePackageReference Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" Version="[7.0.3, )" /> </ItemGroup>

That looks like 7.0.3 is being referenced, which is clearly not the case.

The dependency versions we use come from Version.props. BaselineDesigner.props is to catch unintentional breaking changes in servicing builds. This should provide some more context.

aspnetcore/eng/targets/ResolveReferences.targets

Lines 11 to 17 in dfc5189

Items used by the resolution strategy:

* BaselinePackageReference = a list of packages that were referenced in the last release of the project currently building

- mainly used to ensure references do not change in servicing builds unless $(UseLatestPackageReferences) is not true.

* LatestPackageReference = a list of the latest versions of packages

* Reference = a list of the references which are needed for compilation or runtime

* ProjectReferenceProvider = a list which maps of assembly names to the project file that produces it

src/Security/Authentication/OpenIdConnect/src/PushedAuthorizationBehavior.cs

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

…dler.cs Co-authored-by: Stephen Halter <[email protected]>

…ionBehavior.cs Co-authored-by: Stephen Halter <[email protected]>

…dler.cs Co-authored-by: Stephen Halter <[email protected]>

…pnetcore into josephdecock/par

josephdecock · 2024-07-19T16:38:26Z

@javiercn could you please retry the pipeline failures?

halter73

Thank you!

halter73 · 2024-07-19T17:30:46Z

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

+            var requestMessage = new HttpRequestMessage(HttpMethod.Post, parEndpoint);
+            requestMessage.Content = new FormUrlEncodedContent(parRequest.Parameters);
+            requestMessage.Version = Backchannel.DefaultRequestVersion;
+            var parResponseMessage = await Backchannel.SendAsync(requestMessage, Context.RequestAborted);


One thing that I've encountered is that some providers actually support PAR, but not for all clients. We ran into this when we added similar logic to IdentityModel.OidcClient (DuendeArchive/IdentityModel.OidcClient#428).

I just noticed this from the previous thread about making UseIfAvailable the default. I was also concerned that there may be providers that authenticate the PAR request with something other than the client_secret parameter. Would it make sense to log a warning and fallback to a normal non-pushed authorization attempt if we cannot get a PAR request URI in the UseIfAvailable case?

I'm still fine with merging this in a preview to see how it goes, but not many people use previews compared to major releases. It might be a good idea for us to update this behavior in RC1 even if no one complains to be extra safe, but I'm curious what everyone else thinks about this.

On the one hand, in the issue I linked to in IdentityModel.OidcClient, the issue was for public clients specifically and most users of the OidcHandler will be confidential clients.

On the other hand, if you have customized the behavior of the handler to call the token endpoint using a different form of authentication, presumably you need to do the same for the PAR requests that UseIfAvailable will start making for you.

But, users who are customizing the handler's authentication already are advanced users and personally I'm okay with expecting them to sort this out.

My feeling is to keep the UseIfAvailable default. For RC1, we could take a look at exactly what the error message is if authentication to the PAR endpoint fails and log a hint about the new APIs.

Initial implementation of PAR in OIDC Handler

853fa9c

Adds support for pushed authorization requests (RFC 9126) to the OpenIdConnect authentication handler.

ghost added the area-security label Apr 11, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 11, 2024

Fix API declaration

a75529e

mkArtakMSFT assigned halter73 Apr 15, 2024

josephdecock mentioned this pull request Apr 30, 2024

Support for Pushed Authorization Requests in OidcHandler #51686

Closed

halter73 reviewed May 31, 2024

View reviewed changes

Tratcher reviewed May 31, 2024

View reviewed changes

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectOptions.cs Outdated Show resolved Hide resolved

halter73 reviewed May 31, 2024

View reviewed changes

josephdecock added 5 commits June 6, 2024 14:03

PAR updates from review

8837fa9

Simplify PAR event context

d343623

Clarify a few PAR related comments

61813a1

Enable PAR by default, if available in disco

cdc0796

Very minor changes for clarity

5d6eff3

josephdecock added 5 commits July 17, 2024 15:35

Add enum to control PAR behavior

ea017ff

Added basic tests for pushed authorization

635b666

Seal PushedAuthorizationContext

fddc18f

Add more PAR unit tests

6f7987c

Clean up compiler warnings

b62ea10

josephdecock marked this pull request as ready for review July 18, 2024 04:20

josephdecock added 4 commits July 18, 2024 08:22

Revert commented out config in sample

a202e8c

Revert work in progress test

9ed07b3

Clean up test

3261d7c

Disallow multiple PAR context actions

388de5b

You can indicate if the event handled authentication, handled the push entirely, or opted out of pushing. Doing more than 1 of those is at best redundant but often ambiguous, so it is always disallowed.

josephdecock changed the title ~~Initial implementation of PAR in OIDC Handler~~ Support for Pushed Authorization (PAR) in OIDC Handler Jul 18, 2024

halter73 reviewed Jul 18, 2024

View reviewed changes

josephdecock and others added 7 commits July 18, 2024 15:45

Update src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHan…

d599a81

…dler.cs Co-authored-by: Stephen Halter <[email protected]>

Update src/Security/Authentication/OpenIdConnect/src/PushedAuthorizat…

6371165

…ionBehavior.cs Co-authored-by: Stephen Halter <[email protected]>

Update src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHan…

9645f05

…dler.cs Co-authored-by: Stephen Halter <[email protected]>

Fix exception messages when multiple PAR event methods are called

5adc108

Merge branch 'josephdecock/par' of https://github.com/josephdecock/as…

080e8f3

…pnetcore into josephdecock/par

Merge branch 'main' into josephdecock/par

120b45c

Use PAR support from System.IdentityModel to simplify discovery

20bcf33

halter73 approved these changes Jul 19, 2024

View reviewed changes

halter73 reviewed Jul 19, 2024

View reviewed changes

halter73 enabled auto-merge (squash) July 19, 2024 17:31

halter73 merged commit bef4dae into dotnet:main Jul 19, 2024

dotnet-policy-service bot added this to the 9.0-preview7 milestone Jul 19, 2024

halter73 linked an issue Aug 7, 2024 that may be closed by this pull request

Support for Pushed Authorization Requests in OidcHandler #51686

Closed


		var parRequired = ConfigFlagEnabled("require_pushed_authoriation_requests");

		if (Options.UsePushedAuthorization \|\| parRequired)

	var userInfoEndpoint = _configuration?.UserInfoEndpoint;

	if (string.IsNullOrEmpty(userInfoEndpoint))
	{
	Logger.UserInfoEndpointNotSet();
	return HandleRequestResult.Success(new AuthenticationTicket(principal, properties, Scheme.Name));
	}

	public PushedAuthorizationContext(HttpContext context, AuthenticationScheme scheme, OpenIdConnectOptions options, OpenIdConnectMessage message, HttpRequestMessage parRequest, AuthenticationProperties properties)
	public PushedAuthorizationContext(HttpContext context, AuthenticationScheme scheme, OpenIdConnectOptions options, OpenIdConnectMessage message, AuthenticationProperties properties)

	<UserSecretsId>aspnet5-OpenIdConnectSample-20151210110318</UserSecretsId>
	<UserSecretsId>aspnet5-OpenIdConnectSample-20151210110318</UserSecretsId>

	Items used by the resolution strategy:

	* BaselinePackageReference = a list of packages that were referenced in the last release of the project currently building
	- mainly used to ensure references do not change in servicing builds unless $(UseLatestPackageReferences) is not true.
	* LatestPackageReference = a list of the latest versions of packages
	* Reference = a list of the references which are needed for compilation or runtime
	* ProjectReferenceProvider = a list which maps of assembly names to the project file that produces it

Support for Pushed Authorization (PAR) in OIDC Handler #55069

Support for Pushed Authorization (PAR) in OIDC Handler #55069

Uh oh!

Conversation

josephdecock commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Support for Pushed Authorization Requests

Uh oh!

josephdecock commented Apr 11, 2024

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

halter73 May 31, 2024 • 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!

Tratcher May 31, 2024 • 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

Uh oh!

Tratcher May 31, 2024 • 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

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!

halter73 commented May 31, 2024

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!

halter73 May 31, 2024 • 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!

josephdecock Jun 7, 2024 • 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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blowdart commented Jun 6, 2024

Uh oh!

halter73 commented Jun 18, 2024

Uh oh!

halter73 commented Jul 15, 2024

Uh oh!

josephdecock commented Jul 18, 2024

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

josephdecock commented Apr 11, 2024 •

edited

Loading

halter73 May 31, 2024 •

edited

Loading

Tratcher May 31, 2024 •

edited

Loading

Tratcher May 31, 2024 •

edited

Loading

halter73 May 31, 2024 •

edited

Loading

josephdecock Jun 7, 2024 •

edited

Loading