Skip to content

Support for Pushed Authorization Requests in OidcHandler #51686

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.

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

Closed
josephdecock opened this issue Oct 26, 2023 · 16 comments · Fixed by #55069
Closed

Support for Pushed Authorization Requests in OidcHandler #51686

josephdecock opened this issue Oct 26, 2023 · 16 comments · Fixed by #55069
Labels
api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer enhancement This issue represents an ask for new feature or an enhancement to an existing one
Milestone

Comments

@josephdecock
Copy link
Contributor

josephdecock commented Oct 26, 2023

Background and Motivation

Pushed Authorization Requests (PAR) is a relatively new OAuth standard that improves the security of OAuth and OIDC flows by moving authorization parameters from the front channel to the back channel (that is, from redirect URLs in the browser to direct machine to machine http calls on the back end).

This prevents an attacker in the browser from

  • seeing authorization parameters (which could leak PII) and from
  • tampering with those parameters (e.g., the attacker could change the scope of access being requested).

Pushing the authorization parameters also keeps request URLs short. Authorize parameters might get very long when using more complex OAuth and OIDC features such as Rich Authorization Requests, and URLs that are long cause issues in many browsers and networking infrastructure.

The use of PAR is encouraged by the FAPI working group within the OpenID Foundation. For example, the FAPI2.0 Security Profile requires the use of PAR. This security profile is used by many of the groups working on open banking (primarily in Europe), in health care, and in other industries with high security requirements.

PAR is supported by a number of identity providers, including

  • Duende IdentityServer
  • Curity
  • Keycloak
  • Authlete

This proposal aims to add support for PAR to the OidcHandler.

Proposed API

We need a new flag to enable the behavior:

namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;

public class OpenIdConnectOptions
{
+    /// <summary>
+    /// Flag to set whether the handler should push authorization parameters on the backchannel before 
+    /// redirecting to the identity provider.
+    /// The default is 'false'.
+    /// </summary>
+    public bool UsePushedAuthorization { get; set; } 
}

And we need new properties on the OpenIdConnectConfiguration to describe the PAR-specific information we get from discovery (and use for manual configuration).

namespace Microsoft.IdentityModel.Protocols.OpenIdConnect;

public class OpenIdConnectConfiguration
{
+    //
+    // Summary:
+    //     Gets or sets the 'pushed_authorization_request_endpoint'.
+    [JsonProperty(DefaultValueHandling = DefaultValueHandling.Ignore, NullValueHandling = NullValueHandling.Ignore, PropertyName = "pushed_authorization_request_endpoint", Required = Required.Default)]
+    public string PushedAuthorizationRequestEndpoint { get; set; }
+
+    //
+    // Summary:
+    //     Boolean parameter indicating whether the authorization server accepts authorization request 
+    //     data only via PAR. Dafault Value is false.
+    [JsonProperty(DefaultValueHandling = DefaultValueHandling.Ignore, NullValueHandling = NullValueHandling.Ignore, PropertyName = "require_pushed_authorization_requests", Required = Required.Default)]
+    public bool RequirePushedAuthorizationRequests { get; set; }

}

When the UsePushedAuthorization flag is enabled in configuration or the require_pushed_authorization_requests flag is enabled in the discovery document or the explicit configuration, the parameters that the handler would normally set in the URL when it redirects to the authorize endpoint will instead be sent on a backchannel call to the PAR endpoint, and the resulting request_uri will instead be attached to the redirect, along with the client_id, as per the specification.

It is an error to attempt to enable UsePushedAuthorization with an identity provider that publishes a discovery document without a pushed_authorization_request_endpoint (which indicates that the server does not support PAR) or with explicit configuration that does not set the PushedAuthorizationRequestEndpoint.

It is also an error to attempt to disable UsePushedAuthorization with an identity provider that publishes a discovery document that enables require_pushed_authorization_requests or with explicit configuration that enables the RequirePushedAuthorizationRequests flag.

Usage Examples

            services.AddAuthentication(options =>
            {
                options.DefaultScheme = "cookie";
                options.DefaultChallengeScheme = "oidc";
            })
                .AddCookie("cookie")
                .AddOpenIdConnect("oidc", options =>
                {
                    options.Authority = "https://localhost:5001";

                    options.ClientId = "par";
                    options.ClientSecret = "secret";

                    options.ResponseType = "code";
                    options.UsePkce = true;

                    options.UsePushedAuthorization = true; // <------ New flag on
                })

Alternative Designs

Instead of adding support for PAR directly to the handler, we could instead allow users of the handler to implement PAR themselves within events. The challenge with the events is that the best currently existing event to use for that purpose is the OnRedirectToIdentityProvider event, and it is raised before the state parameter is computed. There are a couple of ways that I see to work around this:

  • Users could make an event that does the following:
  1. Copy the state parameter computation from the handler
  2. Push the authorization parameters (including state)
  3. Change the redirect url to use the client id and request uri
  4. Copy the rest of the logic of the event handler (without the state parameter computation)
  • Another alternative is to add a new event that fires later in the process - immediately after the state parameter is computed. Naming this event is a little tricky, since we already have OnRedirectToIdentityProvider, and this would also happen when redirecting to the identity provider, just later in the process. If such an even existed, then users of the handler could add use that event to push the authorization parameters and change the redirect url, without needing to copy large amounts of code from the handler into their event.

These two approaches are demonstrated here.

Risks

This is a low risk change. Users would have to opt in to get the new behavior. Existing Oidc scheme configurations are unaffected.

We are making a new outgoing HTTP request to implement this specification, but we're making the request to a url that was supplied by the OIDC authority through the discovery document or directly by the developer using the explicit configuration in the handler.

@josephdecock josephdecock added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 26, 2023
@Tratcher Tratcher added area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer and removed area-security labels Oct 26, 2023
@Tratcher
Copy link
Member

It is also an error to attempt to disable UsePushedAuthorization with an identity provider that publishes a discovery document that enables require_pushed_authorization_requests or with explicit configuration that enables the RequirePushedAuthorizationRequests flag.

That implies UsePushedAuthorization must be bool? and default to null to distinguish between the unset state and the disabled state. Alternatively require_pushed_authorization_requests would just override UsePushedAuthorization.

@josephdecock
Copy link
Contributor Author

Alternatively require_pushed_authorization_requests would just override UsePushedAuthorization.

I like this alternative, because it keeps the configuration simpler for the consumer of the API - it avoids the complexity of nullable booleans/ternary logic, and if you're working with a provider that declares its requirement for PAR in the metadata document, the handler just does the right thing.

@Tratcher
Copy link
Member

As part of the infrastructure we'll want some additional fields for this on OpenIdConnectMessage to indicate Pushed auth is being used (or a variant of CreateAuthenticationRequestUrl), as well as something like BuildFormPost for generating the push request.

if (Options.AuthenticationMethod == OpenIdConnectRedirectBehavior.RedirectGet)
{
var redirectUri = message.CreateAuthenticationRequestUrl();
if (!Uri.IsWellFormedUriString(redirectUri, UriKind.Absolute))
{
Logger.InvalidAuthenticationRequestUrl(redirectUri);
}
Response.Redirect(redirectUri);
return;
}
else if (Options.AuthenticationMethod == OpenIdConnectRedirectBehavior.FormPost)
{
var content = message.BuildFormPost();

@halter73 halter73 added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 30, 2024
@ghost
Copy link

ghost commented Jan 30, 2024

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Feb 6, 2024
@mkArtakMSFT mkArtakMSFT added this to the .NET 9 Planning milestone Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@josephdecock
Copy link
Contributor Author

What would be a good next step for this? Do we need more review/discussion of the API? Would it be helpful if I started a prototype/draft PR?

@josephdecock
Copy link
Contributor Author

@Tratcher I really appreciated your previous feedback on this. Can you give some guidance on what next steps should be please?

@Tratcher
Copy link
Member

Tratcher commented Apr 5, 2024

You'll need to work with @halter73.

@josephdecock
Copy link
Contributor Author

josephdecock commented Apr 11, 2024

I went ahead and did a bit of a proof-of-concept implementation, which I'll share in a moment as a draft PR. The things I've come across so far:

  • We don't absolutely need to make changes to System.IdentityModel. The main benefit to adding PAR support there would be that when we do discovery, we would get nice properties for the PAR metadata. However, the OIDC Configuration model gives us a dictionary of AdditionalData that contains everything not explicitly modeled in the configuration type, so it's easy enough to get at PAR metadata within the handler. So my thinking is that at least initially, we could implement PAR entirely within this repo. Then if/when wilson adds PAR support, we could potentially take advantage of that later on.

  • I think we want to add an event that occurs before we make the pushed authorization request, to give users the ability to customize that request. You might want to run your own logic to decide if you were going to push the authentication or do the whole push yourself, and you might also want to use a different form of client authentication (e.g., a private key jwt client assertion). So in my event, I'm proposing that we add two flags - one to indicate that the event handled client authentication, and another to indicate that the event handled the actual push.

  • Speaking of client authentication, for the pushed authorization request, I am authenticating using client id and secret in the basic auth header. Other requests that we make in this handler use the post body for client secrets, so this is a new behavior. However, all the specs advise against using the post body. E.g., RFC 6749:

    client credentials in the request-body using the two parameters is NOT RECOMMENDED and SHOULD be limited to clients unable to directly utilize the HTTP Basic authentication scheme

    My understanding is that auth headers are generally considered safer than a request body, e.g., it would be more obviously wrong to log an auth header. I think we might want to consider using basic auth headers by default in general (separately from this work).

@josephdecock
Copy link
Contributor Author

@halter73 can you help guide me towards next steps please?

@josephdecock
Copy link
Contributor Author

@halter73, just pinging this issue again to see what the next step is.

@josephdecock
Copy link
Contributor Author

I really don't want to be annoying about this issue, I just want to make sure that it isn't waiting on anything from me.

Would it be helpful to consolidate the original API proposal and subsequent discussion into a single doc for API review?

Alternatively, the draft PR (#55069) represents my thinking pretty well, and I created it as a draft because I have not written automated tests. I held off on that thinking that there might be more discussion on this thread, but I can write tests and get the PR ready for review if that is a better way forward.

@halter73
Copy link
Member

Sorry for the delay. I really do want to get this in for .NET 9, and the PR looks good! Feel free to start on the tests. I think those will be easy to update even if we adjust the API.

Would it be helpful to consolidate the original API proposal and subsequent discussion into a single doc for API review?

Yes please. Make sure to include details about the OnPushAuthorization event and PushedAuthorizationContext which I think are good additions.

I think it might be a good idea to give a way for developers to say use pushed authorization if it's supported by the OIDC provider. Arguably, that should be the default behavior, so it'd be nice if the updated API proposal accounted for that.

Please continue using the API proposal template you used in the initial proposal but copy it into a new comment on this thread. Don't worry about repeating stuff. It's nice to see the progress of the API design. If you could include a super simple example of someone using OnPushAuthorization to customize the PAR auth header via the HttpRequestMessage under the "Usage Examples" section, that'd be great.

I see you already submitted AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2499 to make the proposed OpenIdConnectConfiguration changes. You can leave those out of the updated API proposal in this issue. I'll give a nudge and see if we can't get that merged so we can depend on the new API rather than rely on AdditionalData in .NET 9.

We plan to review this next Thursday if that works for you. Thanks again!

@josephdecock
Copy link
Contributor Author

josephdecock commented Jun 7, 2024

Updated API Proposal:

Background and Motivation

Pushed Authorization Requests (PAR) is a relatively new OAuth standard that improves the security of OAuth and OIDC flows by moving authorization parameters from the front channel to the back channel (that is, from redirect URLs in the browser to direct machine to machine http calls on the back end).

This prevents an attacker in the browser from

  • seeing authorization parameters (which could leak PII) and from
  • tampering with those parameters (e.g., the attacker could change the scope of access being requested).

Pushing the authorization parameters also keeps request URLs short. Authorize parameters might get very long when using more complex OAuth and OIDC features such as Rich Authorization Requests, and URLs that are long cause issues in many browsers and networking infrastructure.

The use of PAR is encouraged by the FAPI working group within the OpenID Foundation. For example, the FAPI2.0 Security Profile requires the use of PAR. This security profile is used by many of the groups working on open banking (primarily in Europe), in health care, and in other industries with high security requirements.

PAR is supported by a number of identity providers, including

  • Duende IdentityServer
  • Curity
  • Keycloak
  • Authlete

This proposal aims to add support for PAR to the OidcHandler.

Proposed API

Config Flag

Add a flag to the options to enable the behavior:

namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;

public class OpenIdConnectOptions
{
+    /// <summary>
+    /// Flag to set whether the handler should push authorization parameters on the backchannel before
+    /// redirecting to the identity provider.
+    /// The default is 'true'.
+    /// </summary>
+    public bool UsePushedAuthorization { get; set; } = true;
}

When enabled, the parameters that the handler would normally set in the URL when it redirects to the authorize endpoint will instead be sent on a backchannel call to the PAR endpoint, and the resulting request_uri will instead be attached to the redirect, along with the client_id, as per the specification.

Note that there is ongoing discussion about exactly how we ought to go about enabling this feature, especially given the way that this interacts with what discovery tells us.

My current proposal is to enable the config flag by default, and then enable the feature if either of these conditions holds:

  • the UsePushedAuthorization flag is enabled AND the pushed_authorization_endpoint is available in discovery
  • the require_pushed_authorization_requests flag is enabled

Event For Extensibility

Add an event that will be invoked before authorization parameters are pushed to facilitate extensibility. Event implementors can

  • Tweak parameters before they are pushed
  • Control the client authentication process at the PAR endpoint (for example, set a client assertion)
  • Take over the entire push to the PAR endpoint, and pass the resulting parameter back to the handler so that it can use it at the authorize endpoint
  • Indicate to the handler that PAR should be skipped entirely
namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;

public class OpenIdConnectEvents
{
+    /// <summary>
+    /// Invoked before authorization parameters are pushed using PAR.
+    /// </summary>
+    public Func<PushedAuthorizationContext, Task> OnPushAuthorization { get; set; } = context => Task.CompletedTask;

+    /// <summary>
+    /// Invoked before authorization parameters are pushed during PAR.
+    /// </summary>
+    /// <param name="context"></param>
+    /// <returns></returns>
+    public virtual Task PushAuthorization(PushedAuthorizationContext context) => OnPushAuthorization(context);
}

The PushedAuthorizationContext passed to the event is similar to the existing context classes used in the other OIDC events. The OpenIdConnectMessage that it contains represents the request that the handler will send to the PAR endpoint. The event can manipulate this message to change the pushed parameters.

If the event wants to control the behavior of the handler more, there are methods on the context to indicate this intent, similar to many of the other contexts (e.g., AuthorizationCodeReceivedContext.HandleCodeRedemption).

To control client authentication, there is HandleClientAuthentication. This causes the handler to not set the client secret.

To skip PAR entirely, there is SkipPush. This indicates that no backchannel call was made in the event, and that the handler should not push either. Instead, the handler will send authorization parameters in the front channel (as if PAR was disabled).

To control pushing to the PAR endpoint, there is HandlePush(string requestUri). This indicates that the event made the backchannel call and received the request_uri as the result. The handler will then use that request_uri when it makes the authorize request.

+ /// <summary>
+ /// A context for <see cref="OpenIdConnectEvents.PushAuthorization(PushedAuthorizationContext)"/>.
+ /// </summary>
+ public class PushedAuthorizationContext : PropertiesContext<OpenIdConnectOptions>
+ {
+     /// <summary>
+     /// Initializes a new instance of <see cref="PushedAuthorizationContext"/>.
+     /// </summary>
+     /// <inheritdoc />
+     public PushedAuthorizationContext(HttpContext context, AuthenticationScheme scheme, OpenIdConnectOptions options, OpenIdConnectMessage parRequest, AuthenticationProperties properties)
+         : base(context, scheme, options, properties)
+     {
+         ProtocolMessage = parRequest;
+     }
+
+     /// <summary>
+     /// Gets or sets the <see cref="OpenIdConnectMessage"/> that will be sent to the PAR endpoint.
+     /// </summary>
+     public OpenIdConnectMessage ProtocolMessage { get; }
+
+     /// <summary>
+     /// Indicates if the OnPushAuthorization event chose to handle pushing the
+     /// authorization request. If true, the handler will not attempt to push the
+     /// authorization request, and will instead use the RequestUri from this
+     /// event in the subsequent authorize request.
+     /// </summary>
+     public bool HandledPush { [MemberNotNull("RequestUri")] get; private set; }
+
+     /// <summary>
+     /// Tells the handler that the OnPushAuthorization event has handled the process of pushing
+     /// authorization, and that the handler should use the provided request_uri
+     /// on the subsequent authorize call.
+     /// </summary>
+     public void HandlePush(string requestUri)
+     {
+         HandledPush = true;
+         RequestUri = requestUri;
+     }
+
+     /// <summary>
+     /// Indicates if the OnPushAuthorization event chose to skip pushing the
+     /// authorization request. If true, the handler will not attempt to push the
+     /// authorization request, and will not use pushed authorization in the
+     /// subsequent authorize request.
+     /// </summary>
+     public bool SkippedPush { get; private set; }
+
+     /// <summary>
+     /// The request_uri parameter to use in the subsequent authorize call, if
+     /// the OnPushAuthorization event chose to handle pushing the authorization
+     /// request, and null otherwise.
+     /// </summary>
+     public string? RequestUri { get; private set; }
+
+     /// <summary>
+     /// Tells the handler to skip pushing authorization entirely. If this is
+     /// called, the handler will not use pushed authorization on the subsequent
+     /// authorize call.
+     /// </summary>
+     public void SkipPush()
+     {
+         SkippedPush = true;
+     }
+
+     /// <summary>
+     /// Indicates if the OnPushAuthorization event chose to handle client
+     /// authentication for the pushed authorization request. If true, the
+     /// handler will not attempt to set authentication parameters for the pushed
+     /// authorization request.
+     /// </summary>
+     public bool HandledClientAuthentication { get; private set; }
+
+     /// <summary>
+     /// Tells the handler to skip setting client authentication properties for
+     /// pushed authorization. The handler uses the client_secret_basic
+     /// authentication mode by default, but the OnPushAuthorization event may
+     /// replace that with an alternative authentication mode, such as
+     /// private_key_jwt.
+     /// </summary>
+     public void HandleClientAuthentication() => HandledClientAuthentication = true;
+ }

Usage Examples

Basic Usage

            services.AddAuthentication(options =>
            {
                options.DefaultScheme = "cookie";
                options.DefaultChallengeScheme = "oidc";
            })
                .AddCookie("cookie")
                .AddOpenIdConnect("oidc", options =>
                {
                    options.Authority = "https://demo.duendesoftware.com";

                    options.ClientId = "interactive.confidential";
                    options.ClientSecret = "secret";

                    options.ResponseType = "code";
                    options.UsePkce = true;

                    options.UsePushedAuthorization = true; // <------ New flag on
                })

Customized Authentication

            services.AddAuthentication(options =>
            {
                options.DefaultScheme = "cookie";
                options.DefaultChallengeScheme = "oidc";
            })
                .AddCookie("cookie")
                .AddOpenIdConnect("oidc", options =>
                {
                    options.Authority = "https://demo.duendesoftware.com";

                    o.ClientId = "interactive.confidential.jwt";
                    // Using a private_key_jwt instead of a client secret
                    o.EventsType = typeof(PrivateKeyJwtParEvents);

                    options.ResponseType = "code";
                    options.UsePkce = true;

                    options.UsePushedAuthorization = true; // <------ New flag on
                })
public class PrivateKeyJwtParEvents(AssertionService assertionService) 
{
    public override Task AuthorizationCodeReceived(AuthorizationCodeReceivedContext context)
    {
        context.TokenEndpointRequest.ClientAssertionType = "urn:ietf:params:oauth:client-assertion-type:jwt-bearer";
        context.TokenEndpointRequest.ClientAssertion = assertionService.CreateClientToken();
        return Task.CompletedTask;
    }

    public override Task PushAuthorization(PushedAuthorizationContext context)
    {
        context.HandleClientAuthentication();
        context.ProtocolMessage.ClientAssertionType = "urn:ietf:params:oauth:client-assertion-type:jwt-bearer";
        context.ProtocolMessage.ClientAssertion = assertionService.CreateClientToken(); //
        return Task.CompletedTask;
    }
}
public class AssertionService
{
    private static string rsaKey =
        """
        {
            "d":"GmiaucNIzdvsEzGjZjd43SDToy1pz-Ph-shsOUXXh-dsYNGftITGerp8bO1iryXh_zUEo8oDK3r1y4klTonQ6bLsWw4ogjLPmL3yiqsoSjJa1G2Ymh_RY_sFZLLXAcrmpbzdWIAkgkHSZTaliL6g57vA7gxvd8L4s82wgGer_JmURI0ECbaCg98JVS0Srtf9GeTRHoX4foLWKc1Vq6NHthzqRMLZe-aRBNU9IMvXNd7kCcIbHCM3GTD_8cFj135nBPP2HOgC_ZXI1txsEf-djqJj8W5vaM7ViKU28IDv1gZGH3CatoysYx6jv1XJVvb2PH8RbFKbJmeyUm3Wvo-rgQ",
            "dp":"YNjVBTCIwZD65WCht5ve06vnBLP_Po1NtL_4lkholmPzJ5jbLYBU8f5foNp8DVJBdFQW7wcLmx85-NC5Pl1ZeyA-Ecbw4fDraa5Z4wUKlF0LT6VV79rfOF19y8kwf6MigyrDqMLcH_CRnRGg5NfDsijlZXffINGuxg6wWzhiqqE",
            "dq":"LfMDQbvTFNngkZjKkN2CBh5_MBG6Yrmfy4kWA8IC2HQqID5FtreiY2MTAwoDcoINfh3S5CItpuq94tlB2t-VUv8wunhbngHiB5xUprwGAAnwJ3DL39D2m43i_3YP-UO1TgZQUAOh7Jrd4foatpatTvBtY3F1DrCrUKE5Kkn770M",
            "e":"AQAB",
            "kid":"ZzAjSnraU3bkWGnnAqLapYGpTyNfLbjbzgAPbbW2GEA",
            "kty":"RSA",
            "n":"wWwQFtSzeRjjerpEM5Rmqz_DsNaZ9S1Bw6UbZkDLowuuTCjBWUax0vBMMxdy6XjEEK4Oq9lKMvx9JzjmeJf1knoqSNrox3Ka0rnxXpNAz6sATvme8p9mTXyp0cX4lF4U2J54xa2_S9NF5QWvpXvBeC4GAJx7QaSw4zrUkrc6XyaAiFnLhQEwKJCwUw4NOqIuYvYp_IXhw-5Ti_icDlZS-282PcccnBeOcX7vc21pozibIdmZJKqXNsL1Ibx5Nkx1F1jLnekJAmdaACDjYRLL_6n3W4wUp19UvzB1lGtXcJKLLkqB6YDiZNu16OSiSprfmrRXvYmvD8m6Fnl5aetgKw",
            "p":"7enorp9Pm9XSHaCvQyENcvdU99WCPbnp8vc0KnY_0g9UdX4ZDH07JwKu6DQEwfmUA1qspC-e_KFWTl3x0-I2eJRnHjLOoLrTjrVSBRhBMGEH5PvtZTTThnIY2LReH-6EhceGvcsJ_MhNDUEZLykiH1OnKhmRuvSdhi8oiETqtPE",
            "q":"0CBLGi_kRPLqI8yfVkpBbA9zkCAshgrWWn9hsq6a7Zl2LcLaLBRUxH0q1jWnXgeJh9o5v8sYGXwhbrmuypw7kJ0uA3OgEzSsNvX5Ay3R9sNel-3Mqm8Me5OfWWvmTEBOci8RwHstdR-7b9ZT13jk-dsZI7OlV_uBja1ny9Nz9ts",
            "qi":"pG6J4dcUDrDndMxa-ee1yG4KjZqqyCQcmPAfqklI2LmnpRIjcK78scclvpboI3JQyg6RCEKVMwAhVtQM6cBcIO3JrHgqeYDblp5wXHjto70HVW6Z8kBruNx1AH9E8LzNvSRL-JVTFzBkJuNgzKQfD0G77tQRgJ-Ri7qu3_9o1M4"
        }
        """;

    public string CreateClientToken()
    {
        var now = DateTime.UtcNow;
        var epochNow = DateTimeOffset.UtcNow.ToUnixTimeSeconds();
        var token = new JwtSecurityToken(
            "interactive.confidential.jwt",
            "https://demo.duendesoftware.com/connect/token",
            new List<Claim>()
            {
                new Claim(JwtRegisteredClaimNames.Jti, Guid.NewGuid().ToString()),
                new Claim(JwtRegisteredClaimNames.Sub, "interactive.confidential.jwt"),
                new Claim(JwtRegisteredClaimNames.Iat, epochNow.ToString(CultureInfo.InvariantCulture), ClaimValueTypes.Integer64)
            },
            now,
            now.AddMinutes(1),
            new SigningCredentials(new JsonWebKey(rsaKey), "RS256")
        );

        var tokenHandler = new JwtSecurityTokenHandler();
        tokenHandler.OutboundClaimTypeMap.Clear();

        return tokenHandler.WriteToken(token);
    }
}

Alternative Designs

Instead of adding support for PAR directly to the handler, we could instead allow users of the handler to implement PAR themselves within events. The challenge with the events is that the best currently existing event to use for that purpose is the OnRedirectToIdentityProvider event, and it is raised before the state parameter is computed. There are a couple of ways that I see to work around this:

  • Users could make an event that does the following:
  1. Copy the state parameter computation from the handler
  2. Push the authorization parameters (including state)
  3. Change the redirect url to use the client id and request uri
  4. Copy the rest of the logic of the event handler (without the state parameter computation)
  • Another alternative is to add a new event that fires later in the process - immediately after the state parameter is computed. Naming this event is a little tricky, since we already have OnRedirectToIdentityProvider, and this would also happen when redirecting to the identity provider, just later in the process. If such an even existed, then users of the handler could add use that event to push the authorization parameters and change the redirect url, without needing to copy large amounts of code from the handler into their event.

These two approaches are demonstrated here.

Risks

This is a low risk change. Users would have to be using an AS that supports PAR to get the new behavior, and they have a flag to opt out. There are some authorization servers that support PAR, but not for all clients. We might want to consider falling back to passing over the front channel if PAR fails.

We are making a new outgoing HTTP request to implement this specification, but we're making the request to a url that was supplied by the OIDC authority through the discovery document or directly by the developer using the explicit configuration in the handler.

@amcasey
Copy link
Member

amcasey commented Jun 10, 2024

[API Review]

  • It's possible to implement PAR now, but it's inconvenient
    • There's a lot of boilerplate
    • There's a bunch of code copied directly from OidcHandler in AspNetCore
      • We're doing that because we need to inject something into the middle of the existing codepath and, lacking a hook, we need to replicate and then modify the entire codepath
  • What happens if the user wants (implicitly or explicitly) to use PAR and the identity server doesn't support it?
    • We don't want to silently downgrade
    • Should at least log
    • Do we give the user the option of failing?
      • An enum could accomplish that (off, if-available, required)
      • Log level could depend on enum setting
    • The OIDC server can require PAR
      • If the user explicitly disables it, that's an error and they should see logging and probably an (early) exception
  • Could we just give users a hook (see Alternative Designs)?
    • We could but this is more convenient and more secure
  • It helps the doc team if you put default values in <value> elements
  • Seal all the things

Proposed enum:

namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;

+ public enum PushedAuthorizationBehavior
+ {
+    Disable,
+    UseIfAvailable,
+    Require,
+ }

public class OpenIdConnectOptions
{
+    /// <summary>
+    /// Enum to set whether the handler should push authorization parameters on the backchannel before
+    /// redirecting to the identity provider.
+    /// The default is 'PushedAuthorizationBehavior.Enabled'.
+    /// </summary>
+    public PushedAuthorizationBehavior PushedAuthorizationBehavior { get; set; } = PushedAuthorizationBehavior.Enabled;
}

@amcasey
Copy link
Member

amcasey commented Jun 10, 2024

namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;

+ public enum PushedAuthorizationBehavior
+ {
+    UseIfAvailable, // Default
+    Disable,
+    Require,
+ }

public class OpenIdConnectOptions
{
+    /// <summary>
+    /// Flag to set whether the handler should push authorization parameters on the backchannel before
+    /// redirecting to the identity provider.
+    /// <value>
+    /// The default value is <see cref="PushedAuthorizationBehavior.UseIfAvailable"/>.
+    /// </value>
+    /// </summary>
+    public PushedAuthorizationBehavior PushedAuthorizationBehavior { get; set; } = PushedAuthorizationBehavior.UseIfAvailable;
}
namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;

public class OpenIdConnectEvents
{
+    /// <summary>
+    /// Invoked before authorization parameters are pushed using PAR.
+    /// </summary>
+    public Func<PushedAuthorizationContext, Task> OnPushAuthorization { get; set; } = context => Task.CompletedTask;

+    /// <summary>
+    /// Invoked before authorization parameters are pushed during PAR.
+    /// </summary>
+    /// <param name="context"></param>
+    /// <returns></returns>
+    public virtual Task PushAuthorization(PushedAuthorizationContext context) => OnPushAuthorization(context);
}
+ /// <summary>
+ /// A context for <see cref="OpenIdConnectEvents.PushAuthorization(PushedAuthorizationContext)"/>.
+ /// </summary>
+ public sealed class PushedAuthorizationContext : PropertiesContext<OpenIdConnectOptions>
+ {
+     /// <summary>
+     /// Initializes a new instance of <see cref="PushedAuthorizationContext"/>.
+     /// </summary>
+     /// <inheritdoc />
+     public PushedAuthorizationContext(HttpContext context, AuthenticationScheme scheme, OpenIdConnectOptions options, OpenIdConnectMessage parRequest, AuthenticationProperties properties)
+         : base(context, scheme, options, properties)
+     {
+         ProtocolMessage = parRequest;
+     }
+
+     /// <summary>
+     /// Gets or sets the <see cref="OpenIdConnectMessage"/> that will be sent to the PAR endpoint.
+     /// </summary>
+     public OpenIdConnectMessage ProtocolMessage { get; }
+
+     /// <summary>
+     /// Indicates if the OnPushAuthorization event chose to handle pushing the
+     /// authorization request. If true, the handler will not attempt to push the
+     /// authorization request, and will instead use the RequestUri from this
+     /// event in the subsequent authorize request.
+     /// </summary>
+     public bool HandledPush { [MemberNotNull("RequestUri")] get; private set; }
+
+     /// <summary>
+     /// Tells the handler that the OnPushAuthorization event has handled the process of pushing
+     /// authorization, and that the handler should use the provided request_uri
+     /// on the subsequent authorize call.
+     /// </summary>
+     public void HandlePush(string requestUri)
+     {
+         HandledPush = true;
+         RequestUri = requestUri;
+     }
+
+     /// <summary>
+     /// Indicates if the OnPushAuthorization event chose to skip pushing the
+     /// authorization request. If true, the handler will not attempt to push the
+     /// authorization request, and will not use pushed authorization in the
+     /// subsequent authorize request.
+     /// </summary>
+     public bool SkippedPush { get; private set; }
+
+     /// <summary>
+     /// The request_uri parameter to use in the subsequent authorize call, if
+     /// the OnPushAuthorization event chose to handle pushing the authorization
+     /// request, and null otherwise.
+     /// </summary>
+     public string? RequestUri { get; private set; }
+
+     /// <summary>
+     /// Tells the handler to skip pushing authorization entirely. If this is
+     /// called, the handler will not use pushed authorization on the subsequent
+     /// authorize call.
+     /// </summary>
+     public void SkipPush()
+     {
+         SkippedPush = true;
+     }
+
+     /// <summary>
+     /// Indicates if the OnPushAuthorization event chose to handle client
+     /// authentication for the pushed authorization request. If true, the
+     /// handler will not attempt to set authentication parameters for the pushed
+     /// authorization request.
+     /// </summary>
+     public bool HandledClientAuthentication { get; private set; }
+
+     /// <summary>
+     /// Tells the handler to skip setting client authentication properties for
+     /// pushed authorization. The handler uses the client_secret_basic
+     /// authentication mode by default, but the OnPushAuthorization event may
+     /// replace that with an alternative authentication mode, such as
+     /// private_key_jwt.
+     /// </summary>
+     public void HandleClientAuthentication() => HandledClientAuthentication = true;
+ }

@amcasey
Copy link
Member

amcasey commented Jun 10, 2024

API approved!

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 10, 2024
@halter73 halter73 modified the milestones: .NET 9 Planning, 9.0-preview7 Aug 7, 2024
@halter73 halter73 linked a pull request Aug 7, 2024 that will close this issue
@halter73 halter73 closed this as completed Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants