Support optional audience on clientCredentials call

South-Paw commented 1 year ago

Lib is great 👍, thanks for your hard work!

The Kinde management API requires an audience be sent on client credentials grant

// client.ts L144
async clientCredentials(params?: { audience?: string, scope?: string[] }): Promise<OAuth2Token> {
    const body:ClientCredentialsRequest = {
      grant_type: 'client_credentials',
      audience: params?.audience,
      scope: params?.scope?.join(' '),
    };

   // ...
}

South-Paw commented 1 year ago

Created #91 for this feature 👍 - getting this merged would unblock me on another project 🙏

evert commented 1 year ago

Do you know anything about where this parameter is defined? It's not part of the base OAuth2 spec. It doesn't feel right to blindly add non-standard parameters per service.

South-Paw commented 1 year ago

Do you know anything about where this parameter is defined

Can't find it in a spec - some googling turned up a few results where it's implemented by other providers in their client credential flows as well

When setting up OAuth2 client credential grants in both Postman and Insomnia they have advanced options that allow for an audience to be passed with the OAuth2 client credentials flow

evert commented 1 year ago

One of the issues with not having a central authority explaining what it is, I also don't know if I'm implementing something that's potentially insecure, or something that should be verified.

Furthermore, I looked around a bit and the format isn't even consistent between vendors. It looks like some at least support multiple values.

Blindly implementing a security feature without following a definition feels like the wrong move for a security product.

If you're willing to put in the legwork, it would go a long way to ask on the OAuth2 mailing list for a recommendation.

South-Paw commented 1 year ago

I really don't want to wade into that sort of stuff - mailing lists of people discussing stuff is not my thing nor would people be happy I'm spending time on that - I'm just trying to get something implemented so I can carry on with my work.

OAuth 2.1 spec mentions issued bearer tokens SHOULD contain an audience restriction in 7.1.3.6, 7.1.5 and 7.3.2 - but without any notes for how the client can define what that audience is, despite that some providers still have added it in their flows.

Looks like oauth4webapi supports custom parameters on a client credentials flow - I'll either use that or just re-publish this package for myself with a 2 line change made 😭

evert commented 1 year ago

I really don't want to wade into that sort of stuff - mailing lists of people discussing stuff is not my thing nor would people be happy I'm spending time on that - I'm just trying to get something implemented so I can carry on with my work.

Don't forget that the reason any of this exists at all is because people do take the time to do this ;)

OAuth 2.1 spec mentions issued bearer tokens SHOULD contain an audience restriction in 7.1.3.6, 7.1.5 and 7.3.2 - but without any notes for how the client can define what that audience is, despite that some providers still have added it in their flows.

My take from those paragraphs is mainly: don't share access tokens across contexts, but yeah I'm also not sure. Honestly any source that seems like a good suggestion on what to do would suffice for me. Whether that's about the audience parameter specifically or a good example to follow for other arbitrary/proprietary parameters.

South-Paw commented 1 year ago

I really don't want to wade into that sort of stuff - mailing lists of people discussing stuff is not my thing nor would people be happy I'm spending time on that - I'm just trying to get something implemented so I can carry on with my work.

Don't forget that the reason any of this exists at all is because people do take the time to do this ;)

Yes, point taken - I'm just really out of my depth dealing with those mailing list and don't really know where to begin 😄

Honestly any source that seems like a good suggestion on what to do would suffice for me. Whether that's about the audience parameter specifically or a good example to follow for other arbitrary/proprietary parameters.

So I went looking at some other industry auth providers to see what others are doing on client_credentials parameters.

Auth0 Client Credentials Flow

grant_type required
client_id required
client_secret required
audience required

I believe this means that this library would not support Auth0's oauth endpoint because this library always(?) sends the client id and secret via the Authorization header - where-as Auth0 expects them in the request body and does not support the Authorization header

OAuth 2.1 RFC Section 2.4.1 does state that

"In addition to [the Authorization header], the authorization server MAY support including the client credentials in the request content using the following parameters: client_id, client_secret"

though it also states

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

Okta Client Credentials

grant_type required
scope required

Okta seems to be strictly adhering to the RFC with only 2 parameterss implemented and has a note that

"The client ID and secret aren't included in the POST body, but rather are placed in the HTTP Authorization header following the rules of HTTP Basic Auth."

OneLogin Client Credentials

grant_type required
client_id required if token endpoint method is set to POST
client_secret required if token endpoint method is set to POST
resource optional
scope optional

resource is mentioned in the OAuth 2.1 RFCs a bit as an optional extension - but this library doesn't support that parameter - however I agree that given it's discussed and suggested in the linked OAuth 2.1 RFCs it makes more sense that these implementations should be using resource rather than audience

My personal take away's are

there isn't really a clear cut answer on if additional parameters are valid or not. Some vendors have them, others don't.
Auth0 isn't even technically following the RFC spec here either 😆
the only additional parameters that are missing from the library are the client_id, client_secret and resource ones but that's not necessarily why I opened this issue

I'd encourage you to consider the addition of other parameters to a client_credentials request though - despite it not being explicitly defined in the RFC - as it would mean the library is more flexible with other providers and I think that fits in with the philosophy of the endpoint discovery too.

As well as that other oauth libraries like the one I linked earlier support additional client_credentials params (though the developer experience of this lib is preferable to that other one 😄)

But I doubt there is a clear source of truth here to work from though so in my opinion it's just whether the library should support additional parameters in the request or not

brockallen commented 1 year ago

The resource indicator spec is the one that you might want to look into for audience constraining the access tokens: https://datatracker.ietf.org/doc/html/rfc8707

evert commented 1 year ago

Hi @South-Paw ,

Appreciate all the research.

the only additional parameters that are missing from the library are the client_id, client_secret and resource ones but that's not necessarily why I opened this issue

Note that client_id and client_secret are supported, but they are passed when creating the client. As for resource I agree with @brockallen , the resource parameter is well-defined so it's easier to justify because we have some source to figure out the implications.

I believe this means that this library would not support Auth0's oauth endpoint because this library always(?) sends the client id and secret via the Authorization header - where-as Auth0 expects them in the request body and does not support the Authorization header

This still needs to be merged and released, but is planned:

https://github.com/badgateway/oauth2-client/pull/84

Long story short, I think there's basically 2 paths here:

Figure out where audience comes from. If it's some draft spec I can see this library support this, but adding vendor-specific parameters is definitely a code-smell, so that's a hard no.
Support arbitrary parameters so that users can use to provide any additional data. I think this is somewhat reasonable, but then instead of a new audience option, I would want a 'extraParameters' option typed as Record<string, string>.

Given that there's 2 vendors that use audience, and they used such a generic name that also appears in OpenID Connect claims, I suspect that there is some draft out there, so I'd like to get some advice from the list to see what the recommendation for implementors is. (I understand you're a bit nervous asking there, so ill do this)

As well as that other oauth libraries like the one I linked earlier support additional client_credentials params (though the developer experience of this lib is preferable to that other one smile)

I think keeping a pretty firm grip on what and how we're implementing the protocol will ensure that it will continue to be the better developer experience. I don't think there's harm in treading carefully and being very deliberate about adding features ;)

In the meantime I would suggest you keep your fork a little while longer until I figure out the correct way of handling this!

evert commented 1 year ago

Sent: https://mailarchive.ietf.org/arch/msg/oauth/CgWt6uA35UN2VTbOTqZqWHDqjCA/

South-Paw commented 1 year ago

The resource indicator spec is the one that you might want to look into for audience constraining the access tokens: https://datatracker.ietf.org/doc/html/rfc8707

Ah yeap, thats the one, thank you - I'd already spelunked through enough RFCs for one night and didn't want to try find the one that I'd read it in 😄

Support arbitrary parameters so that users can use to provide any additional data

This seems like a good approach and I'm more than happy to amend my PR to do this if/when required 👍

I think keeping a pretty firm grip on what and how we're implementing the protocol will ensure that it will continue to be the better developer experience.

Totally understand what you mean and I think you're right with that approach. With this in mind (and given what that mailing list replies with) adding support for additional parameters (in my view) aligns with that

Thanks again @evert, appreciate you taking the time to communicate on this

evert commented 1 year ago

The answer is in.

audience is non-standard
The resource parameter is the way to go, but at least auth0 implemented audience before resource was standardized.
So given that we can call these vendor-specific, lets add a generic way for users to add additional parameters instead.

If you're up for modifying your PR to use a general extraParameters property instead, I'd be happy to merge that. Just make sure that any known parameters are rejected with an error (client_id, client_secret, scope, resource)

South-Paw commented 1 year ago

I've updated #91, hopefully it's along the lines of what you were looking for - if not let me know 👍

South-Paw commented 1 year ago

Updated #91 again and re-requested review 👍

badgateway / oauth2-client