markbates / goth

Package goth provides a simple, clean, and idiomatic way to write authentication packages for Go web applications.
https://blog.gobuffalo.io/goth-needs-a-new-maintainer-626cd47ca37b
MIT License
5.2k stars 566 forks source link

When should you generate the code challenge for a `goth`/`gothic` PKCE OAuth flow? #516

Open jchen42703 opened 11 months ago

jchen42703 commented 11 months ago

Normal PKCE authorization URL generation looks like:

// Creates the initial authorization url with the state and code challenge.
// The state and code verifier are passed back alongside the URL here,
// these would be stored in sessions or HTTP only cookies, in memory, etc.
// Something to make sure that they stick around to verify the state and are able to
// send the code verifier along with the auth code request.
func AuthorizationURL(config *oauth2.Config) (*AuthURL, error) {
    codeVerifier, verifierErr := RandomBytesInHex(32) // 64 character string here
    if verifierErr != nil {
        return nil, fmt.Errorf("could not create a code verifier: %v", verifierErr)
    }
    sha2 := sha256.New()
    io.WriteString(sha2, codeVerifier)
    codeChallenge := base64.RawURLEncoding.EncodeToString(sha2.Sum(nil))

    state, stateErr := RandomBytesInHex(24)
    if stateErr != nil {
        return nil, fmt.Errorf("could not generate random state: %v", stateErr)
    }

    authUrl := config.AuthCodeURL(
        state,
        oauth2.SetAuthURLParam("code_challenge_method", "S256"),
        oauth2.SetAuthURLParam("code_challenge", codeChallenge),
    )

    return &AuthURL{
        URL:          authUrl,
        State:        state,
        CodeVerifier: codeVerifier,
    }, nil
}

However, in goth.Provider only supports authorization url creation with a state (missing code_challenge_method, and code_challenge).

It's clear to me that certain goth providers (Fitbit, Gitea OIDC, and Zoom) support PKCE, so it seems possible.

So, in conclusion, I just don't understand how you're supposed to complete a PKCE OAuth flow with goth and gothic because of the issues I just described. I would love some pointers on how to do this (if possible)!

In the meanwhile, I'm going to be writing my own PKCE implementation with github.com/golang/oauth2 :(.

punmechanic commented 2 months ago

The PKCE challenge should be generated as part of BeginAuth() and stored in the session, and later should be retrieved from a session to be exchanged for a token in (*Session) Authorize. Unfortunately there is no way to implement this without modifying the library itself, because BeginAuth() as you mentioned does not accept non-state parameters.

Session.Authorize does seem to have measures in place to deal with code_verifier, but this is a bit of a red herring: That function retrieves code_verifier from the parameters:

codeVerifier := params.Get("code_verifier")
if codeVerifier != "" {
  ...
}

However, the parameters come directly from the request sent by the IdP as part of the callback; this will never contain the code_verifier, as the code_verifier is something that the client (whatever is using Goth) presents to the IdP as proof that it has the original challenge. The code as written seems to assume that the IdP would send us the code_verifier or that the user would be expected to modify the URL parameters of the *http.Request to include it before passing it on to Goth for processing.

As PKCE is recommended in all cases (and the parameters can be silently ignored by a server that does not implement it), my personal fork simply always generates a PKCE challenge: https://github.com/punmechanic/goth/commit/4944a61d216648738baa6e6535805829bd85ae10

This change does have an issue when using an IdP that generates large access, refresh or ID tokens; Goth attempts to store all three of these values within the session at all times, and additionally storing a 32b verifier in the session can cause it to exceed the 4kB limit for cookies and cause securecookie to error out. A potentially better implementation would store the verifier in a separate session intended only for short-lived values, rather than the session that is used for long-lived values like tokens.