PKCE support according to RFC 7636

[ZMaradics]

This PR implements PKCE support according to RFC 7636 (https://tools.ietf.org/html/rfc7636) which is current best practice when using OAuth2.0 in the context of mobile apps (RFC 8252 https://tools.ietf.org/html/rfc8252). This has already been requested in #455 and #688.
The authorize endpoint accepts code_challenge and code_challenge_method parameters (supported methods: plain and S256).
If these parameters are present the token endpoint will require and check the code_verifier parameter.
As org.springframework.security.oauth2.provider.code.AuthorizationCodeTokenGranter does not support PKCE up to now we created an enhanced version: PkceEnhancedAuthorizationCodeGranter

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/163393172

The labels on this github issue will be updated when the story is started.

[cfdreddbot]

❌ Hey ZMaradics!

All pull request submitters and commit authors must have a Contributor License Agreement (CLA). Click here for details on the CLA process.

The following github user @ZMaradics is not covered by a CLA.

After the CLA process is complete, this pull request will need to be closed & reopened. DreddBot will then validate the CLA(s).

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/163416848

The labels on this github issue will be updated when the story is started.

[cfdreddbot]

✅ Hey ZMaradics! The commit authors and yourself have already signed the CLA.

[fhanik]

Thanks for the PR could you please

1. Squash your commits so it's less reviews per commit
2. Remove all copyright modifications

[ZMaradics]

Done.
Let me know is anything else required.

[sebastianGit]

Do you still consider accepting this merge request? In case, we can support the "integration" / "merge" of this pull request, please let us know.

[cherukumilli]

@ZMaradics
Can you please help resolve the conflicts?

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ ZMaradics (f696b9a, 1bd7dfc)

[ZMaradics]

Hi, Sorry for the late answer, Yes I can help.

[ZMaradics]

I fixed the conflicts and did unit and integration tests with no fails.

[cherukumilli]

@fhanik - Can you please review this PR and help merge it?

[cherukumilli]

@ZMaradics - can you please sign the EasyCLA?

[ZMaradics]

Done

[cherukumilli]

@fhanik
Can you pls review this PR and see if it can merged?

[sebastianGit]

@cherukumilli, @fhanik: is there any news on this topic?

[fhanik]

@sebastianGit I'm not longer on this project, and will have to defer to the current contributors directly

[bsekar]

Looking at the evolution of OAuth, the need to use PKCE is continuously increasing:

• OAuth 2.0 for Native Apps (RFC 8252): PKCE for mobile and native Apps with public clients
• OAuth 2.0 for Browser-Based Apps (draft-ietf-oauth-browser-based-apps-06): PKCE for SPAs without backend making use of public clients
• The OAuth 2.1 Authorization Framework (draft-ietf-oauth-v2-1-00): PKCE for all kind of OAuth clients (incl. confidential clients)

All three documents are asking for the OAuth authorization server to support PKCE.

Could you share any planned timeline when CF UAA is to provide PKCE suppport (independent from whether this pull request gets merged or not)

In fact, OAuth2.1 is going to outright omit Implicit (access_token) https://tools.ietf.org/html/draft-ietf-oauth-v2-1-00

[Richard-Boyewa]

@shamus @strehle what is the resolution on this pull request. We are currently planning to implement PKCE in our authentication layer, and since we already use UAA, I think it is pointless to implement another layer on UAA just to be PKCE compliant.

Do we have a possible green light (even if this would be refactored later) or it is going to be closed?

[MrWhiteABEX]

@strehle As a SAP customer I can only say please merge as soon as possible and bring it to SAP Cloud Platform. Not supporting PKCE is a no go. As SAP pushes native apps (SAP MDK, etc.) this is a must have.

[strehle]

@MrWhiteABEX
PKCE is already supported in SAP Cloud Platform, see https://accounts.sap.com/.well-known/openid-configuration , this is no uaa but SAP own OIDC server. Then there are other OIDC servers, like UAA.
This is the community UAA, SAP has a fork where we will support it if it does not reach community UAA.

My Hope is, that after the CF summit there is time for further discussions, because I know that other vendors also simply fork UAA and add features.

[MrWhiteABEX]

@strehle Glad to hear that at least SAP ID Service does. But frankly I am not building apps for managing my sub accounts and spaces ;). That is my biggest problem right know. I can host API´s in SCP Cloud Foundry using Java, NodeJS etc. and access on-Premise systems securely using Cloud Connector but I cannot securely consume them securely. The (XS)UAA does not support code challenges and I have to provide the client-secret all the time and store it in my apps.
Side note: Common authentication libraries like AppAuth can obtain authorization codes but fail to obtain tokens. That part I have to code myself to make it work. Don´t know whose fault that is.
There is also an artificial length limit in the (XS-)UAA redirect filter (You can use arbitrary lengths in the actual code grant) which breaks Universal Windows apps because their platform redirect links are too long. The workaround is to use only a wildcard e.g. "ms-app://**". That is something I already reported to SAP support.
Current state of UAA seems to be a complete mess when you want to do something different than hosting web apps.
I really think about migrating everything to Azure AD and Azure App Service because of this.

Sorry for the rant and for hijacking the pull request :(

[sebastianGit]

Can you please provide any hint, when/whether CF UAA will support PKCE? This pull request has been around for two years now.

[rwwilden]

At a VMware customer right now where we are blocked by missing PKCE support in UAA. Implicit flow isn't an options because they are on pretty modern browsers that limit third-party cookie support. As asked before, can anyone provide a hint/timeline for when this is available?

[strehle]

As asked before, can anyone provide a hint/timeline for when this is available?

The PR itself is mergeable, so no technical issues, but to me a task about who takes the responsibility to merge it.
if @aramprice @peterhaochen47 @staylor14 does not rise any veto, I can merge

[peterhaochen47]

@strehle I will bring this with my team early next week.

[strehle]

@ZMaradics after all this time we want merge your PR.
I only want ask you if you agree, that we use Default S256 in case code method is null (not set in reuqest) ?

[ZMaradics]

@strehle: As RFC7636 ch. 4.3 (https://datatracker.ietf.org/doc/html/rfc7636#section-4.3) and also the upcoming draft for OAuth2.1 ch 4.1.1.1 (https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-02#section-4.1.1.1) specify that “plain” is the default if the code_challenge_method parameter is missing, I think we should stick to the specifications as long as there is no really important reason to deviate.
If the reason is to avoid the “plain” method it would be better to not support it at all (the consequence would be, that code_challenge_method must be present if a code_challenge is present or else an error must be returned)

[strehle]

If the reason is to avoid the “plain” method it would be better to not support it at all (the consequence would be, that code_challenge_method must be present if a code_challenge is present or else an error must be returned)

If you agree to omit plain complety, then that would be ok for me and the following discussion could be avoided, but if you want stay with both, plain and S256, then I vote for simply switching the default.

The reasons for me are

1. only with code_challange_method = S256 you protected in token request because only the original requestor can created the HMAC. With plain it sounds like security but it isn´t security, more security, then nonce have. That sounds to me similar to the issue in JWT validation (at the beginning) with type "alg" of JWT header; standard allows none but this caused problems, e.g. https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/
2. I vote for secure by default and as elaborated in 1. only S256 is secure
3. In next step I am sure that we want use PKCE in combination with public clients (which we currently dont have in UAA) in order to omit client secrets but here S256 should be the default (to me) and therefore I vote to have this default now with introduction of PKCE

I dont want reject plain but only switch the default to S256.
Finally I dont want reject your PR because of my opionion so if most voters also want stay then we can do it, only added some document issues I see

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ ZMaradics (f696b9a, 1bd7dfc, 10d4fa1, 0ba5adb)
• ✅ Markus Strehle (207a238)

[sebastianGit]

@strehle : Here my thoughts on the discussion which default to use, if the code_challenge_method is not specified in the authorization request:

As the behavior of the default is explicitly defined in the standard (and in the current version of the OAuth 2.1 draft), I believe it is best, if CF UAA implements PKCE as described in the standard (RFC 7636) and not just "close to the standard" in order to ensure compatibility with 3rd Party libs.

If there is a need for having "PKCE-like behavior" using sha256 as default (if the code_challenge_method is missing in the authorization request), a way out could be to make the default configurable as part of uaa.yml.

[strehle]

@ZMaradics you need to accept CLA, https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/1797134/3382195/939/#/?version=2

[ZMaradics]

Done