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

Fix PKCE Oauth2 exchange #24858

Merged
merged 1 commit into from
May 5, 2020
Merged

Fix PKCE Oauth2 exchange #24858

merged 1 commit into from
May 5, 2020

Conversation

mmmorris1975
Copy link
Contributor

Providers like Okta and AWS Cognito expect that the PKCE challenge
uses base64 URL Encoding without any padding (base64.RawURLEncoding)

Additionally, Okta strictly adheres to section 4.2 of RFC 7636 and
requires that the unencoded key for the PKCE data is at least 43
characters in length.

Providers like Okta and AWS Cognito expect that the PKCE challenge
uses base64 URL Encoding without any padding (base64.RawURLEncoding)

Additionally, Okta strictly adheres to section 4.2 of RFC 7636 and
requires that the unencoded key for the PKCE data is at least 43
characters in length.
@hashicorp-cla
Copy link

hashicorp-cla commented May 5, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #24858 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
command/login.go 48.40% <100.00%> (+0.12%) ⬆️
terraform/evaluate.go 53.60% <0.00%> (+0.90%) ⬆️
dag/marshal.go 53.40% <0.00%> (+1.13%) ⬆️

@danieldreier
Copy link
Contributor

danieldreier commented May 5, 2020

Thanks @mmmorris1975! If you update your commit message to include "fixes #24858" that will close the issue automatically when this is merged.

@mmmorris1975
Copy link
Contributor Author

I gave it a quick run against TF Cloud, and all seemed to work as expected. Output show in this gist

@apparentlymart
Copy link
Contributor

Hi @mmmorris1975! Thanks for working on this.

We had previously seen this working but I expect that's because previously the challenge had a length that didn't require base64 padding and so it didn't encounter this problem.

With that said, this fix looks good to me and I was able to reproduce the testing with Terraform Cloud and with a minimal Amazon Cognito configuration, and so I'm going to merge this. I have not re-verified with Okta because I don't currently have a usable Okta account to test with, but I can see that this change does indeed make Terraform's implementation match the details of the RFC.

Thanks again!

@apparentlymart apparentlymart merged commit 9568de6 into hashicorp:master May 5, 2020
@ghost
Copy link

ghost commented Jun 5, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jun 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants