dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.46k stars 10.03k forks source link

Resource indicators are missing in access token request #41176

Open pago93 opened 2 years ago

pago93 commented 2 years ago

Is there an existing issue for this?

Describe the bug

The Resource-property set in the OpenIdConnectOptions does not flow through the whole process.

When configuring OpenIdConnect authentication with a resource indication, the resource value is only added to the initial authorization request but not to the subsequent access token request.

Expected Behavior

When setting the Resource-property the value should be set in subsequent requests and no additional configuration should be needed. As per RFC 8707 Section 2.2 the access token request in the 'authorization_code' grant type should also contain the resource indicator.

Steps To Reproduce

Example configuration:

builder.Services.AddAuthentication()
    .AddOpenIdConnect(options =>
    {
        options.Authority = "https://localhost:5001/";
        options.Resource = "urn:test";

        options.Scope.Add("profile");

        options.ClientId = "testclient";
        options.ClientSecret = "secret";
        options.ResponseType = "code";

        // workaround:
        options.Events = new OpenIdConnectEvents
        {
            OnAuthorizationCodeReceived = context =>
            {
                // the resource property here is null but should be set
                context.TokenEndpointRequest.Resource = "urn:test";

                return Task.FromResult(0);
            }
        }
    };

Exceptions (if any)

No response

.NET Version

6.0.201

Anything else?

A fix may be applied somewhere around this line.

Something simple like this may already be enough:

if (Options.Resource != null)
{
    tokenEndpointRequest.Resource = Options.Resource;
}
brockallen commented 2 years ago

Just for context, the Resource setting in the current OIDC plumbing was added pre-RFC8707 and was for an AAD-specific feature, IIRC.

Tratcher commented 2 years ago

@jennyf19 Is the Resource supposed to be used in the access token request?

jennyf19 commented 2 years ago

@Tratcher No, on the Azure AD v2 endpoint scopes are used, not resource. Resource was an AAD v1 concept which was not standard compliant.

brockallen commented 2 years ago

And in RFC8707 referenced above, multiple resource params are possible (and expected) on the authorize endpoint. And not a single param with multiple values mind you, but multiple instances of the same param each with one value. IIRC the current OIDC plumbing can't handle this as specified by the RFC.

adityamandaleeka commented 2 years ago

@blowdart Should we pursue this?

blowdart commented 2 years ago

Yes, but the question is do we reuse the existing parameter, or pull it out, and then replace it with a Resources one. Would that break existing users who never moved to Identity Web for aad?

Needs a dev to explore approaches.

blowdart commented 2 years ago

@jennyf19 @brentschmaltz Now that there's a real spec that clashes with AAD's custom use of resources IdentityModel needs correcting to match RFC8707. Brent have you looked into this? If not, can you?

brentschmaltz commented 2 years ago

@blowdart just read the spec, interesting that this is similar to SignedHttpRequest - draft, 'u' parameter.

This does map nicely to some work we are completing where inbound policy has an 'address' that could be used to identify policies that are applicable to a token.

What type of changes are you suggesting?

blowdart commented 2 years ago

Is the AAD feature still called Resource? It'd need to get changed first, and then bring Wilson in line with the spec's idea of resource

adityamandaleeka commented 2 years ago

@brentschmaltz Any further comment on this one?

blowdart commented 2 years ago

@jmprieur As Brent isn't answering, could you take a look here?

jmprieur commented 2 years ago

@blowdart : will discuss this with @brentschmaltz at the beginning of next week (beginning of August) cc: @jennyf19

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

jpda commented 6 months ago

Any movement on this? We are designing our oauth2 systems and determining if we follow 8707 or mimic AAD's resource/scope parsing. A lot of our customers are dotnet shops and requiring a workaround if we adopt 8707 wouldn't be ideal.