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

[feature] Implement PKCE for oauth2 #2225

Open
Tracked by #2232
daenney opened this issue Sep 25, 2023 · 4 comments
Open
Tracked by #2232

[feature] Implement PKCE for oauth2 #2225

daenney opened this issue Sep 25, 2023 · 4 comments
Labels
enhancement New feature or request security

Comments

@daenney
Copy link
Member

daenney commented Sep 25, 2023

Is your feature request related to a problem ?

We currently don't do PKCE for oauth2. I discovered this while setting up Kani since it requires PKCE unless explicitly disabled.

Because the frontend can't keep the client secret a secret (since you'd see it in the JS and in the requests your browser does), it's only aware of the client ID (I'm not even sure where it gets this from, as it's in the server's config YAML). PKCE is meant to make it safer to do this kind of thing.

Describe the solution you'd like.

Implement PKCE in the frontend, or do auth without the JS client (so entirely server side) so it's not a "public" client.

Also needs #2224 for completeness.

Describe alternatives you've considered.

Not implement it, but that's meh.

Additional context.

No response

@daenney daenney added enhancement New feature or request security frontend Frontend-related stuff labels Sep 25, 2023
@daenney
Copy link
Member Author

daenney commented Sep 25, 2023

Unless I'm strongly mistaken, it's here that we need to change something:

}).then((app) => {
let url = new URL(instance);
url.pathname = "/oauth/authorize";
url.searchParams.set("client_id", app.client_id);
url.searchParams.set("redirect_uri", SETTINGS_URL);
url.searchParams.set("response_type", "code");
url.searchParams.set("scope", app.scopes);
let redirectURL = url.toString();
window.location.assign(redirectURL);
return { data: null };

That needs to include both the challenge and the challenge method. Roughly:

  • Generate a code verifier: random string using the characters A-Z, a-z, 0-9, and the punctuation characters -._~ (hyphen, period, underscore, and tilde), between 43 and 128 characters long. SHA256 it, then base64 it
  • Store the code verifier in a cookie, because the client will need it
  • Set code_challenge to the verifier and code_challenge_method to S256 in the above code snippet
  • Eventually we get the token back, do the usual state validation etc.

Then, in:

return baseQuery({
method: "POST",
url: "/oauth/token",
body: {
client_id: app.client_id,
client_secret: app.client_secret,
redirect_uri: SETTINGS_URL,
grant_type: "authorization_code",
code: code
}
}).then(unwrapRes).then((token) => {

  • Add the verifier as code_verifier when we exchange it for an access token, so the IDP can check if that code matches the one used during the authorization request
  • We should also not be setting the client_secret there

@daenney
Copy link
Member Author

daenney commented Sep 25, 2023

Although, I'm a little confused overall as to how auth works in our frontend, so I might be looking in the wrong places entirely.

@daenney
Copy link
Member Author

daenney commented Sep 25, 2023

Argh, there's 2 things. I think the code I was just looking add is for the frontend "app" to authenticate, and maybe for app registration?

It seems what I want is the AuthCodeURL that gets generated from

AuthCodeURL(state string) string
The x/oauth2 package recently got an update for PKCE support: golang/go#59835

But since we're doing it fully on the backend, I'm a little surprised I run into PKCE issues. That shouldn't be necessary here. Although it's still a good thing to have.

@daenney daenney removed the frontend Frontend-related stuff label Sep 25, 2023
@daenney
Copy link
Member Author

daenney commented Sep 25, 2023

Alright, back to where this started: Kani has 2 types of Oauth2 clients, confidential and public ones. Confidential ones, which is what GtS is when it itself is doing the Oauth2 dance, have PKCE required by default but can have it disabled. This is fine, since a confidential client can keep the client secret, well, secret. This happens when you have an IDP configured.

Public clients, like a SPA, cannot keep the client secret a secret, and there PKCE should be mandatory. However, I'm not entirely sure what oauth2 things our frontend is doing. There's code that does it, hence my initial confusion about what was happening where. I think we ourselves act as an oauth2 authorization server when username/password auth is in use? We might still want to do something around supporting PKCE there, but idk if this is some Masto-specific flavour that we need to not break for other client apps?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request security
Projects
None yet
Development

No branches or pull requests

1 participant