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

feat: added simple PKCE and state checks utils, used PKCE and state checks in auth0 #12

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Azurency
Copy link
Contributor

I don't if that's something that something in the scope of a "Minimalist Authentication module" but I added simple PKCE and state checks in the auth0 provider for extra security.

The crypto methods and check logic could easily be extracted and reused in other oauth provider. I think that Google and Spotify accept the PKCE flow and Google and Twitch both accept a state check.

@justserdar
Copy link
Contributor

justserdar commented Nov 11, 2023

The other providers could definitly benefit from a universal util that allows state checks and/or a pkce check.
If we can get this into like a importable util, i'd happily add it to the Discord and LinkedIn provider too for extra security.

@sifferhans
Copy link
Contributor

You should probably use uncrypto instead of crypto here, as crypto might not be available in all server runtimes 😁

@atinux
Copy link
Owner

atinux commented Nov 12, 2023

@Azurency
Copy link
Contributor Author

Tanks for the feedback 👍, I updated the checks to use uncrypto.

I also moved the logic in a separate util file so that other providers can simply call

const authParam = await checks.create(event, config.checks)

to get extra query param to be passed in the authorization request and

const checkResult = await checks.use(event, config.checks)

to verify the checks and get back the code_verifier if applicable.

@Azurency Azurency changed the title feat: added simple pkce and state checks for auth0 feat: added simple PKCE and state checks utils, used PKCE and state checks in auth0 Nov 14, 2023
# Conflicts:
#	src/runtime/server/lib/oauth/auth0.ts
if (checks?.includes('pkce')) {
const pkceVerifier = generateCodeVerifier()
const pkceChallenge = await pkceCodeChallenge(pkceVerifier)
console.log('pkceVerifier', pkceVerifier)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.log leftover

console.log('pkceChallenge', pkceChallenge)
res['code_challenge'] = pkceChallenge
res['code_challenge_method'] = 'S256'
setCookie(event, 'nuxt-auth-util-verifier', pkceVerifier, { maxAge: 60 * 15, secure: true, httpOnly: true, sameSite: 'lax' })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the cookie settings should configurable or reuse the cookie settings from the module

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't reuse the cookie settings because they were under the session key, I don't know if it would be confusing to reuse that or not. But I agree that a shared cookie config somewhere would be nice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it would be a bit confusing. Maybe an optional config for pkce cookie?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a cookie setting (runtimeConfig), under nuxtAuthConfig.security.cookie

@maximilianmikus
Copy link
Contributor

I would like to build upon this PR and add a generic OIDC provider. I already started but these utils would be super helpful for supporting pkce flow.

if (!state || !stateInCookie) {
const error = createError({
statusCode: 401,
message: 'Auth0 login failed: state is missing'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message is still specific to Auth0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I updated this

if (state !== stateInCookie) {
const error = createError({
statusCode: 401,
message: 'Auth0 login failed: state does not match'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message is still specific to Auth0

@itpropro
Copy link

itpropro commented Dec 16, 2023

Hey @Azurency,
the current implementation in the security utils file is not RFC 7636 compliant. There are some issues with the generation of the verifier and the challenge, the verifier is saved server side, which should be the challenge (the other way around if used as confidential client) and the hash validation doesn't take place.
I would like to create a PR to adjust your implementation to the spec, but I have some questions:

  • What is the target of the current handler implementation for PKCE? There is currently no identity provider in this repo that would need a verification of the PKCE challenge or verifier on our Nuxt server side, as we are not issuing tokens to the user. The validation of the challenge takes place at the Idp, so auth0 in you example. The helper functions, as soon as adjusted to the spec are still helpful for usage in the providers though.
  • What is the target of the state check? The referenced auth0 documentation refers to SPA flow, but as we are using Nuxt SSR, we have a confidential client scenario. For simple CSRF protection, requiring a fixed CSRF header value would be enough, as this would force all client to make preflight requests. PKCE also helps mitigate some scenarios that can be abused due to a missing state, but I would assume that the state check would also just be used as a helper function inside of the respective providers, as it is also used for providing context to the application.

@maximilianmikus
Copy link
Contributor

@itpropro
There is currently no identity provider in this repo that would need a verification of the PKCE challenge

As @Azurency states in the PR description:
The crypto methods and check logic could easily be extracted and reused in other oauth provider. I think that Google and Spotify accept the PKCE flow and Google and Twitch both accept a state check.
Also #25 relies on this

@itpropro I think it would be great if you could improve the implementation.

@itpropro
Copy link

@itpropro There is currently no identity provider in this repo that would need a verification of the PKCE challenge

As @Azurency states in the PR description: The crypto methods and check logic could easily be extracted and reused in other oauth provider. I think that Google and Spotify accept the PKCE flow and Google and Twitch both accept a state check. Also #25 relies on this

@itpropro I think it would be great if you could improve the implementation.

Hey,
I think I wasn't clear with my statement. I didn't mean that there is no authentication provider that would use PKCE, but they would validate the verifier on their end, so that there is no need to verify at the Nuxt server side. I was just talking about the right place and level of PKCE in the implementation as it's about having a "PKCE client" no a PKCE server implementation.
I think it makes sense to have some kind of pipeline where the requests go through and the supported/configured features are applied (like PKCE, Nonce, State, etc.).

@amandesai01
Copy link

amandesai01 commented May 22, 2024

Thanks @itpropro I was exactly getting confused by the same issue all along while reading code changes and discussion.

Yes, PKCE is meant to be used by SPAs since they cannot have secret keys. All the security comes by providing correct redirect_uri to provider.

What I would suggest is adding those utils on client side, there by allowing this module to work with SPAs and nuxt generate. Then we can have a separate set of clients which work client side. I think it should be a separate discussion. Work done in this PR is extemely valuable but should be ported to client side.

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.

7 participants