Add oidc pkce support

[idsulik]

In the pull request I've added support for the Proof Key for Code Exchange by OAuth Public Clients

Closes #182

[SuperSandro2000]

I approved by accident...

[kluevandrew]

+1 i'm waiting for it

[idsulik]

@SuperSandro2000 ping

[SuperSandro2000]

I can't say if the code is good or not.

[idsulik]

@SuperSandro2000 and what do you suggest?)

[cormacrelf]

You were right not to accept that code, well done @SuperSandro2000, good instinct. I got distracted by this and decided to do a review, firstly of the PKCE implementation and then of the broader attack mitigations PKCE is meant to solve.

PKCE implementation

The repo (likewise the copy of verifier.go embedded here) fails to use cryptographic random data for the code_verifier (random string), and it is extremely vulnerable to guessing based on the unix time seed value. With the raw code_verifier almost trivially guessable (the server literally sends a nearby time stamp around!), PKCE is useless against all of the attacks it is meant to prevent, as noted by section 7.1 of RFC7636. Someone noticed in December and submitted a PR to fix it..

Consider incorporating/reviewing that PR, generally using crypto/rand instead of math/rand. I would also wonder if there's a way to tell Go/GitHub to mark it as vulnerable when other people try to use it until that's fixed.

I would advise reading the RFC thoroughly as this person has done, it's quite short and has a number of useful suggestions that give you an idea for how this code should be reviewed.

• For example section 7.2: MUST NOT downgrade to "plain" after S256 has been attempted and generally "plain" SHOULD NOT be used or added to new implementations. I notice the code in this PR uses S256 only, so that's good to see. The actual verification (incl downgrade prevention) of code_challenge == t(code verifier) is not something a client has to worry about. So that's good.
• I would review the customised base64 against the RFC's allowed characters.

Authorisation Code Injection

I have some more suggestions regarding IETF OAuth 2.0 Security Current Best Practice (2018) section 2.1.1:

• First up, "Clients MUST prevent injection (replay) of authorization codes into the authorization response by attackers." As it stands, the codebase does not do any mitigation of authorisation code injection attacks.
• There are lots of ways to steal an authorisation code that cannot be prevented, and when the code is injected into an interaction with this proxy, the proxy will happily fetch an access token for it using its client secret.
• Basically you must use PKCE or verify nonces or both. This codebase does neither. (Nonces here are different from the CSRF tokens you do have. Edit: the CSRF token’s state nonce, the OIDC nonce and the PKCE code verifier must all be distinct cryptorandom nonces. Especially because PKCE relies on not disclosing the raw value through insecure callback params etc until the access token request. Store them all in the same cookie if you like, though.)
• I consider the fact that neither coreos/go-oidc nor golang's oauth2 package enforces or makes default the use of at least one of these to be kinda bad, but I'm really just doing a drive by review here and I'm not invested. You can upstream this if you like. go-oidc has a nonce mode but it is not mandatory, but at least the main example uses it. Nevertheless this is an example of code that is poorly served by the defaults in both libraries. The only mention of PKCE in golang's oauth2 package is a 3yo issue where they decided to leave it to downstream users by simply allowing arbitrary extra params, and then maybe one or two organisations worldwide decided to do the rest of the work required for authorisation code flows to be secure in their own codebase.
• PKCE and nonces provide similar security according to this, but PKCE is better as a general solution as it is checked server side and works without client secrets to completely replace and deprecate the implicit flow. I believe that also, in addition to what nonces provide, PKCE enables configuring clients for any purpose as public clients and not using a client secret, since the only purpose of client secret was to let the authors server rely on some security implemented in the client to ensure the session/person requesting an access token is the same person who requested the authorisation code. PKCE does that just as well as does client secret + checked nonces stored in a cookie does. I don't have a good citation for that, except that Authorisation Flow With PKCE is recommended for native apps everywhere and doesn't require a client secret. I think the guides continue to recommend confidential clients where possible simply because it is supported everywhere already and doesn't impose a browser modernity requirement to use SubtleCrypto/ other browser features.
• PKCE-only (no nonces) sounds great if you know your OIDC server supports it and will actually verify it rather than silently ignoring the params. 2.1.1 of the guide says you can often find this published in the OAuth2 metadata response.
• Given that this project is designed for use with arbitrary OIDC servers, you could start checking the metadata endpoint, or just implement both PKCE and nonces, such that even if PKCE is silently ignored by a particular OIDC server, the client is still doing replay mitigation.
• So I would lean towards only implementing both PKCE and nonces and making both mandatory. There are of course legacy non-compliant servers people might complain about. Deal with that when it comes up.
• To be clear, that requires storing two more things in a cookie, PKCE’s raw code verifier and an OIDC nonce, both ALSO cryptographically-secure-randomly generated. Again, coreos/go-OIDC does have example code for this. Implementing PKCE as well means people can probably quit doing the annoying job of securely storing client secrets if their OIDC provider supports the Authorisation Code + PKCE for public clients.

In summary

• Use a cryptographic random code_verifier.
• Always store/send PKCE, always store/send/verify nonces.
• Do not make either configurable for now.
• Consider upstreaming something, I'm sure there are countless OIDC and OAuth 2.0 consumers out there not aware of today's best practices and the fact that the go-to implementations in Go do not mitigate this by default, or even include a worthy PKCE implementation.

[cormacrelf]

Also I haven't read the rest of the codebase enough to know for sure, but this PR doesn't make it obvious that the PKCE code_verifier is actually stored anywhere. Sure it might pass a test, but how does the pkceVerifier have the same value across GetLoginUrl, (redirects...), ExchangeCode? I think it's just going to be nil in ExchangeCode. And hence panic. Right? Did you test this? I think it needs a cookie, just like nonce.

[Benoit485]

@thomseddon Hey! Why not add this PR in code ? it should be better to use pkce code

[idsulik]

@cormacrelf thank you for the details. pushed changes, it will use nonce always, and crypto package instead of the math

[cormacrelf]

Still needs to be using cookies for storing both the code verifier and the nonce. At the moment this will fail if more than one user tries to log in at the same time. Because it is storing these values on the OIDC struct in memory.

[ds-sebastian]

Some IDMs now require PKCE (KanIDM). Looking forward to this implementation

[idsulik]

@cormacrelf hi! Pushed changes, now it saves data into cookie

[andig]

Worth noting that Go has build-in PKCE support meanwhile

[idsulik]

Worth noting that Go has build-in PKCE support meanwhile

where? Couldn't find it

[idsulik]

Is there anything else I can do to push this PR?
@cormacrelf fyi

[andig]

Worth noting that Go has build-in PKCE support meanwhile

where? Couldn't find it

See https://cs.opensource.google/go/x/oauth2/+/refs/tags/v0.23.0:pkce.go;l=26

[idsulik]

@andig , thank you! unfortunately, it was added only for the v2 while this package uses v1 of the oauth library, as soon as we make a major update we can remove my code and start using the oauth's pkce

[andig]

I may get this wrong, but: Oauth2 is not a v2 of the package. If the PKCE function you need are compliant with the Oauth2 standard (which I can‘t evaluate), then you should be able from Go perspective to use the PKCE functions of the oauth2 package.

[idsulik]

@SuperSandro2000 hi! Pushed changes, could you please check? it's more secure now