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

Create PKCE verifier according to RFC 7636 #13

Merged
merged 2 commits into from
Jan 17, 2024
Merged

Create PKCE verifier according to RFC 7636 #13

merged 2 commits into from
Jan 17, 2024

Conversation

decius7bc
Copy link
Contributor

No description provided.

@maennchen
Copy link
Member

Thanks for your PR @decius7bc ❤️

I’m not sure that I understand your change. The new codes reduces the amount of random
bytes, and then both base 32 and base 64 encodes it.

The current solution uses 96 bytes of entropy and creates a 128 bytes base 64 string from it.

Can you explain why the current solution is not confirming to the RFC and how the new solution is?

@maennchen
Copy link
Member

Oh, we didn’t use url_encode64 and used encode64 instead. Did you see invalid characters in the generated string?

In that case leaving the amount of bytes at 96 and using the url safe function will probably solve the issue.

@decius7bc
Copy link
Contributor Author

According to RFC 7636 section 4.1 the ABNF for the code verifier is as follows:

code-verifier = 43*128unreserved
unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"
ALPHA = %x41-5A / %x61-7A
DIGIT = %x30-39

The base 64 characters /, + and the padding character = are illegal within the context. I discovered this minor bug while testing with Keycloak 23.0.3, always returning the error PKCE invalid code verifier. 😕

I certainly misunderstood the recommendation note:

NOTE: The code verifier SHOULD have enough entropy to make it impractical to guess the value. It is RECOMMENDED that the output of a suitable random number generator be used to create a 32-octet sequence. The octet sequence is then base64url-encoded to produce a 43-octet URL safe string to use as the code verifier.

Using url_encode64/1 with padding: false will do more suitable.

Updated my PR with another fixup commit.

@maennchen maennchen enabled auto-merge (squash) January 17, 2024 08:44
@maennchen maennchen merged commit 709a8fd into erlef:main Jan 17, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants