Skip to content
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

proposal: x/oauth2: support private_key_jwt client authentication #57186

Open
rdileep13 opened this issue Dec 9, 2022 · 18 comments
Open

proposal: x/oauth2: support private_key_jwt client authentication #57186

rdileep13 opened this issue Dec 9, 2022 · 18 comments
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@rdileep13
Copy link

This proposal is to support private_key_jwt client authentication method in x/oauth2 package. Details of this method are at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication

Details:
This is applicable for both two-legged (client_credentials grant) and three-legged (authorization_code grant) OAuth flows.
In the three-legged oauth flow, when the Relying Party (RP) tries to exchange the authorization code with the Authorization Server (AS) (aka IdP), it has a choice of various methods to authenticate itself. most common method is client_secret_post which sends the client_id and client_secret of the RP in the token API request as post body parameters along with the code to be exchanged and other details.

private_key_jwt is a more secure way of making the token API call where in instead of sending client_secret in the tokenAPI, the RP would compute a digitally-signed-token (aka jwt) to prove its identity at the AS. Details of this token computation are at https://datatracker.ietf.org/doc/html/rfc7523

This proposal is to enhance current token exchange API in x/oauth2 to offer private_key_jwt as one of the client authentication methods.

@gopherbot gopherbot added this to the Proposal milestone Dec 9, 2022
@rdileep13 rdileep13 changed the title proposal: affected/package: proposal: affected/package: x/oauth2 Dec 9, 2022
@seankhliao seankhliao changed the title proposal: affected/package: x/oauth2 proposal: x/oauth2: support private_key_jwt client authentication Dec 9, 2022
@seankhliao
Copy link
Member

what are the proposed API additions to support this feature?

@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Dec 9, 2022
@ianlancetaylor
Copy link
Contributor

CC @golang/security

@ericchiang
Copy link
Contributor

Chiming in since I've maintained https://github.com/coreos/go-oidc for a while now, which implements OpenID Connect on top of golang.org/x/oauth2. OpenID Connect supports JWTs as an additional field returned as part of the OAuth 2.0 token response.

If I'm reading the spec right, you can do a lot of this today with an auth code option oauth2.OAuthCodeOption. go-oidc does something similar for nonce's (oidc.Nonce).

opts := []oauth2.AuthCodeOption{
    oauth2. SetAuthURLParam("client_assertion_type", "urn:ietf:params:oauth:client-assertion-type:jwt-bearer"),
    oauth2. SetAuthURLParam("client_assertion", myJWT),
}
config. Exchange(ctx, code, opts...)

I believe AuthCodeOption even lets you override the grant_type (don't know if that's intended).

@rdileep13
Copy link
Author

@ericchiang thanks for the response! While AuthcodeOption lets you add custom name-value pairs in the API, here are some issues with that.

  1. Users still need to compute/generate the 'value' to be inserted in the API. Since this jwt computation is defined as a standard in the spec, we can make it easier for the community by providing the API.
  2. A bigger issue is semantic. privateKeyJwt is a 'client authentication type'. There is already another struct that defines clientAuthentication. So, combining this with AuthCodeOptions is probably not the best way.
  3. If we do 2 above, then we would be conflicting with the AuthStyle directive. Currently, they are AuthStyleInHeader or AuthStyleInParams (or the seldom used 'Auto'). All these assume inserting client_secret somewhere (basic header or post body). This (insertion of client_secret) should not be done for private_key_jwt.

Please let me know your thoughts. Excuse me if I'm misinterpreting anything or missing something.

@neild
Copy link
Contributor

neild commented Dec 12, 2022

What providers support private_key_jwt? Do any require it?

@rdileep13
Copy link
Author

@neild I can confirm that AzureAD, Okta, PingId, Kong support it. I'm sure many others that I'm not aware of support it.

@seankhliao Only one internal API is planned to support this. That API computes the jwt (https://datatracker.ietf.org/doc/html/rfc7523) to be sent in TokenAPI

@ericchiang
Copy link
Contributor

@rdileep13 proposals that introduce new APIs generally include the new API.

You may want to look at https://golang.org/x/oauth2/jwt, which is documented to support an older draft of RFC 7523.

@neild
Copy link
Contributor

neild commented Jan 26, 2023

Supporting private_key_jwt seems reasonable, but the proposal needs to include the proposed new API.

How does this proposal relate to golang.org/x/oauth2/jwt? Does it supersede that package? Should it be a feature of that package rather than of golang.org/x/oauth2?

@madaster97
Copy link

@rdileep13 , hey do you want any help moving this along? I'd also like to see this feature, and can help draft up the actual API changes for this team to review.

@neild , for your question on the golang.org/x/oauth2/jwt package, I don't think that's the right place to make the change. To illustrate, here's a table of the grant types and client authentication methods we support across a few packages:

Grant Type Supported Client Authentication Package/Relevant Config
authorization_code none?, client_secret_basic, client_secret_post golang.org/x/oauth2
client_credentials none?, client_secret_basic, client_secret_post golang.org/x/oauth2/clientcredentials
urn:ietf:params:oauth:grant-type:jwt-bearer none (technically, "assertion" JWT doesn't count) golang.org/x/oauth2/jwt

What rdileep13 is proposing above is to add private_key_jwt to the supported client authn methods of the authorization_code grant type. Since that grant type is implemented in golang.org/x/oauth2, I think it makes sense to add it there.

If I do contribute anything to this, I'd also request adding private_key_jwt to the client_credentials grant, implemented in golang.org/x/oauth2/clientcredentials.

To give an example of a spec that requires private_key_jwt, it's the preferred client authn protocol for the SMART App Launch, which is a healthcare specific superset of OAuth 2.0 and OIDC. It is a strict requirement for the client_credentials grant in that workflow.

To reiterate, I understand someone needs to propose an API for this team to approve, just adding some context.

@rdileep13
Copy link
Author

@madaster97 Please go ahead with the API proposal. That would be a great help!

@madaster97
Copy link

Here's my proposal for the API changes, in line with the work rdileep13 has already done.

With respect to client auth, both these packages have the same API (with slight differences in where the values go):
golang.org/x/oauth2
golang.org/x/oauth2/clientcredentials

The fields that matter for this change are:

My proposed are:

  • Extend AuthStyle with a new value for "private_key_jwt" (3)
  • Add new fields of PrivateKey []byte and KeyID string as a new struct called PrivateKeyConfig

Having a dedicated struct will make it easier to make edits later on specific to the JWT signing, and is in line with some of the changes @neild has recommended.

@giraffesyo
Copy link

giraffesyo commented Jul 12, 2023

What providers support private_key_jwt? Do any require it?

The login.gov OIDC provider requires use of private_key_jwt or PKCE, rather than client_secret. See this site for details: https://developers.login.gov/oidc/ where they state why they don't allow client_secret:

Other implementations of OpenID Connect use the “implicit flow” or the client_secret param, but Login.gov does not support those methods. The implicit flow is not recommended by the OAuth group for security reasons. Similarly client_secret is not supported by Login.gov because of security reasons. It requires managing a shared secret in two places: the client and the server, where private_key_jwt flow involves only sharing public keys with the server and PKCE only has a one-time secret.

@ericchiang
Copy link
Contributor

If you don't provide the client_secret today, then the AuthStyle setting shouldn't matter:

https://github.com/golang/oauth2/blob/ac6658e9cb5802cebf9b8fd5f5d58f22bedb527f/internal/token.go#L169

If this is exclusively to be used by OpenID Connect, then it might be reasonable for go-oidc (or other third-party packages) to provide this API (https://go.dev/doc/faq#x_in_std)? E.g. something like

package oidc

type JWTAuthConfig struct {
    PrivateKey crypto.PrivateKey
    KeyID      string
    ClientID   string
    Endpoint   oauth2.Endpoint
    Now        func() time.Time
    Validity   time.Duration
}

func NewJWTAuth(rand io.Reader, config JWTChallengeConfig) ([]oauth2.AuthCodeOption, error) {
    // ...
}

Anecdotally, it seems like the Go team has been hesitant to provide a full fledged JWT package (and I don't blame them).

Am I missing any blockers here? Or can this be done outside of golang.org/x/oauth2?

@madaster97
Copy link

Hi @ericchiang , JWT client authentication is independent of OIDC. The OIDC group initially proposed it, but it's been registered as an OAuth parameter rather than as OIDC specific

I think it would be best to implement private_key_jwt here, given that client authentication is a core part of the OAuth 2.0 protocol.

@neild
Copy link
Contributor

neild commented Jul 20, 2023

The current proposal supplies a PrivateKey as a []byte:

// PrivateKey contains the contents of an RSA private key or the
// contents of a PEM file that contains a private key. The provided
// private key is used to sign the Token request (https://openid.net/specs/openid-connect-core-1_0.html#TokenEndpoint).
// PEM containers with a passphrase are not supported.
PrivateKey []byte

This matches jwt.Config.PrivateKey, but it doesn't seem right.

Providing a serialized key means the oauth package needs to parse it. Either the key is reparsed on each request, or the package needs to cache it. Caching introduces questions of cache size and lifetime. In addition, if the caller already has a deserialized key, they're going to need to serialize it again to provide it to the oauth package.

I think the oauth.Config should contain the serialized key, such as an *rsa.PrivateKey. That avoids all these questions.

Is an *rsa.PrivateKey right, though? RFC 7515 describes a number of signature algorithms, and RFC 7518 Section 8.1 specifically suggests that implementations use modular algorithms to allow for evolution over time. Should this be a crypto.Signer instead? (Note that *rsa.PrivateKey implements crypto.Signer.)

If so, the API might look something like:

type Config struct {
  Signer crypto.Signer
  KeyID string
}

And perhaps the crypto.SignerOpts needs to be in there as well?

@rdileep13
Copy link
Author

rdileep13 commented Dec 21, 2023

@neild Spec allows various signature algorithms as mentioned at https://www.rfc-editor.org/rfc/rfc7518#section-3

As you mentioned, it does not make sense to pass *rsa.PrivateKey as some ECDSA algorithms are allowed as well. crypto.Signer might be a good option since *ecdsa.PrivateKey also implements the crypto.Signer interface.

@rdileep13
Copy link
Author

@neild Looks like the issue is more involved.
Underlying jws library only seems to support rsa keys - https://github.com/golang/oauth2/blob/master/jws/jws.go#L156

func Encode(header *Header, c *ClaimSet, key *rsa.PrivateKey) (string, error) {

@neild
Copy link
Contributor

neild commented Jan 5, 2024

Underlying jws library only seems to support rsa keys

There's also jws.EncodeWithSigner, which looks like it should be straightforward to use with a crypto.Signer:

type Signer func(data []byte) (sig []byte, err error)
func EncodeWithSigner(header *Header, c *ClaimSet, sg Signer) (string, error)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Incoming
Development

No branches or pull requests

8 participants