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: Changes OIDC to use static URL in Redirect_URI #144

Merged
merged 3 commits into from
Jul 30, 2024

Conversation

EthanHeilman
Copy link
Contributor

@EthanHeilman EthanHeilman commented May 27, 2024

This PR addresses issue #140 where the randomized redirect URI in the OIDC breaks some OpenID Providers redirect URI allow lists.

Tested with Google with the SSH3 linux running on OSX connecting to SSH3 running on Ubuntu.

@francoismichel
Copy link
Owner

Would you be okay to keep the previous mechanism in the case where PKCE is not enabled ?

I think one good thing would be to force PKCE at some point but I'm currently unaware of what providers support it.

@EthanHeilman
Copy link
Contributor Author

EthanHeilman commented Jul 22, 2024

Would you be okay to keep the previous mechanism in the case where PKCE is not enabled ?

PKCE was standardized in 2015 and is very widely deployed. The only times I've encountered OPs that don't support PKCE, it is OPs where the attack that PKCE addresses is solved in a different way. For instance the github actions OIDC flow does not use PKCE but the github actions OIDC doesn't use redirect URIs at all:

  1. Github Action knows an API key and API secret bound to a specific identity and metadata
  2. Github Action makes an API request to github OP which is authenticated with key and secret,
  3. Github OP responses to API request with ID Token

I don't see much harm in keeping the redirect URI other than cost of one more configuration option and hitting potential bugs in OPs by having slightly different configuration requirements than vanilla OIDC.

@francoismichel Have you found any cases or OPs where PKCE is both not supported, this randomized redirect URI could be configured and doing so would increase security?

@francoismichel
Copy link
Owner

I am currently struggling making it work with my Google account without PKCE.
Either there's something wrong with Google's identity provider or here.

Here's the log I receive when PKCE is activated, I'm trying to investigate that:

error when parsing oauth token: oauth2: "invalid_grant" "Bad Request"

@francoismichel
Copy link
Owner

I found the error, once I merge the bugfix I enable PKCE by default and I can then merge this PR.

@francoismichel francoismichel merged commit 64b140f into francoismichel:main Jul 30, 2024
29 checks passed
@francoismichel
Copy link
Owner

I just reverted your changes to go.mod and go.sum to make the commit clearer.

Thanks for the PR ! It helps a lot!

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