x/oauth2: support PKCE

[hickford]

feature request golang/oauth2#603

Motivation: support PKCE (RFC 7636), also known as code_challenge and code_verifier. PKCE is an OAuth best practice that protects against authorization code injection attacks and CSRF. Draft spec OAuth 2.1 makes PKCE required by default:

Clients MUST use code_challenge and code_verifier and authorization servers MUST enforce their use except under the conditions described in Section 7.6. In this case, using and enforcing code_challenge and code_verifier as described in the following is still RECOMMENDED.

Currently to use PKCE with x/oauth2, you have to generate the verifier and challenge yourself, and use SetAuthURLParam to send the appropriate subsets of parameters with AuthCodeURL and Exchange. GitHub code search shows few users doing this.

Adding user-friendly PKCE support to x/oauth2 would make it easier to write secure OAuth clients.

Proposed x/oauth2 API:

// GenerateVerifier generates a code verifier with 32 octets of randomness.
// This follows recommendations in RFC 7636 (PKCE).
//
// A fresh verifier should be generated for each authorization.
// S256ChallengeOption(verifier) should then be passed to Config.AuthCodeURL and
// VerifierOption(verifier) to Config.Exchange.
func GenerateVerifier() string

// S256ChallengeOption derives an S256 code challenge from verifier following
// RFC 7636 (PKCE). It should be passed to Config.AuthCodeURL only.
func S256ChallengeOption(verifier string) AuthCodeOption 

// VerifierOption describes a RFC 7636 (PKCE) code verifier. It should be
// passed to Config.Exchange only.
func VerifierOption(verifier string) AuthCodeOption

And also #59835 (comment)

// S256ChallengeFromVerifier returns the PKCE code challenge from verifier with method S256
func S256ChallengeFromVerifier(verifier string) string 

Prototype implementation https://go-review.googlesource.com/c/oauth2/+/463979

[ianlancetaylor]

CC @golang/security

[eikemeier]

A remark: A client needs to store the verifier for the exchange, possibly for a long time, since user interaction is required. Also, since you may run the service distributed, serialization and a database (or a session cookie) might be involved. The challenge is only send with the initial request and not used afterwards.

So using PKCEParams in the exchange seems wasteful to me, it would be better if both just take the verifier and AuthCodeURLWithPKCE calculates the challenge on the fly.

[andig]

@ianlancetaylor Could this be added to the proposal discussion to give it more visibility?

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[andig]

So using PKCEParams in the exchange seems wasteful to me, it would be better if both just take the verifier and AuthCodeURLWithPKCE calculates the challenge on the fly.

I'm wondering if that ship has sailed if you want to reuse the existing PKCEParams?

[hickford]

it would be better if both just take the verifier and AuthCodeURLWithPKCE calculates the challenge on the fly

I like this idea, it's simpler. I'll update the proposal.

@andig the authhandler package has very few users and it seems skipped the proposal process, so I'm indifferent to reusing authhandler.PKCEParams

[rsc]

Do we need a new method, or should the PKCE verifier just be an AuthCodeOption?
What would the API look like if we did that?
I see there have been edits to the proposal although I can't quite puzzle through them in the GitHub UI.
The linked CL's example_test.go seems to make the verifier an option, but the code in the main package does not.
The option form seems cleaner and simpler.
Is there a reason not to do that?

[hickford]

Do we need a new method, or should the PKCE verifier just be an AuthCodeOption?

@rsc For PKCE you need to send one set of query parameters with AuthCodeURL (code_challenge and code_challenge_method) and a different set with Exchange (code_verifier). Passing the same AuthCodeOption to both AuthCodeURL and Exchange thus won't work. A solution could be to introduce intermediate functions func S256ChallengeOption(string verifier) AuthCodeOption and func VerifierOption(string verifier) AuthCodeOption.

That's very appealing to me, because the API and code changes are both smaller. I'll prepare to update the proposal and CL.

[hickford]

Updated proposal and https://go-review.googlesource.com/c/oauth2/+/463979 including example

[andig]

Logical next step if this gets accepted would be deprecating the authhandler package.

[andig]

With regards to #61401 there are a number of oauth2 applications that today cannot use the oauth2.Config mechanism which is required for this PR. This can be partly addressed by #61417. There are probably other cases, too like passing additional headers during code exchange like golang/oauth2#533.
If #61417 doesn't get adopted it would be nice to expose the calculation of the challenge, too. Otherwise users would still need to rely on duplicating this logic or using libraries like https://github.com/nirasan/go-oauth-pkce-code-verifier. Doing so could be a separate proposal.

[rsc]

It sounds like the current proposal is:

// GenerateVerifier returns a new random verifier for use with [S256ChallengeOption] and [VerifierOption].
func GenerateVerifier() string 

// S256ChallengeOption derives a PKCE code challenge from verifier with method
// S256. It should be passed to Config.AuthCodeURL only.
func S256ChallengeOption(verifier string) AuthCodeOption 

// VerifierOption describes a PKCE code verifier. It should be
// passed to Config.Exchange only.
func VerifierOption(verifier string) AuthCodeOption

Do I have that right? Does that look right to people?

[andig]

Having re-checked my code I would additionally suggest

// S256ChallengeFromVerifier returns the PKCE code challenge from verifier with method S256
func S256ChallengeFromVerifier(verifier string) string 

Otherwise this API is strictly tied to being able to use oauth2.Config

[rsc]

With the three funcs in my comment and the one func in @andig's followup comment, have all remaining concerns been addressed?

[eikemeier]

When you need code for the above: https://github.com/fillmore-labs/login-sample/blob/main/web/app/pkce/pkce.go

I could adapt the signatures to the one suggested by @rsc . Otherwise, take what you like and thanks for the work.

Just an additional note: This needs documentation, sample code and should probably be the default. It might be useful to discuss at least the last aspect, since the current approach is only an extension to the existing library, which has “state“ directly in the API, but not PKCE.

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

[hickford]

@rsc @ianlancetaylor @andig Brilliant! https://go-review.googlesource.com/c/oauth2/+/463979 is ready for review. Please add more reviewers. The oauth2 code owners are inactive.

[gopherbot]

Change https://go.dev/cl/463979 mentions this issue: oauth2: support PKCE

[andig]

Noticed one thing after the merge. AuthCodeURL says:

To protect against CSRF attacks, opts should include a PKCE challenge (S256ChallengeOption). Not all servers support PKCE. An alternative is to generate a random state parameter and verify it after exchange.

Yet, state is always a mandatory parameter- that doesn't sound like an alternative in terms of generating it?