feat: allow library users to specify how PKCE challenge codes are generated

[Osipion]

First off, thanks for your work on this library - much appreciated and great to see it is certified.

I'm raising this PR to give consumers more control over how PKCE challenge codes are generated. Currently PKCE challenge codes in the latest release are UUIDs (in master they are Base64 URL encoded UUIDs) and the library user has no way to change this. However, a consumer may need to for the following reasons:

1. An OIDC provider only accepts challenge codes of certain lengths or using certain alphabets. Okta, for instance, rejects PKCE challenge codes shorter than 43 characters.
2. A library consumer may wish to fine tune the security of the challenge code - for instance, by using a specific source of randomness or by choosing longer or shorter challenge codes depending on.

This PR demonstrates a possible approach to allowing consumers to control how PKCE challenge codes are generated. I don't necessarily expect you to accept it as is - although I've tried to match the coding style used in the rest of this repo, this is a bit of a drive-by PR to try to solve a problem I'm facing right now and potentially breaks any custom implementations consumers have of the RelyingParty interface by adding a new method to it. However hopefully it helps get to a place where consumers have more control over PKCE challenge code generation.

[livio-a]

Hi @Osipion

Thanks for your feedback and this PR. I was thinking about exchanging the UUIDs and take random bytes using the crypto package. But I like your approach even more. I'll take a closer look tomorrow and give you some feedback.

[Osipion]

@livio-a - thanks for your response. There's probably a couple of things that should be tweaked (not sure it was a great idea to make the default code generator a public variable for instance) - feel free to suggest any changes you think are neccessary. As a bit of extra context, I was just reviewing RFC-7636 and it turns out that code verifiers are supposed to be a minimum of 43 characters (maximum of 128) - so the comment I put about 36, and the original UUID implementation, are both contrary to the standard I think.

[fforootd]

Just linking this to #126 😁

[livio-a]

Hey @Osipion

Finally found some time to think about your proposal, especially the problem of breaking current implementations of the RelyingParty interface.

How about creating a second interface RelyingPartyCodeChallenge which extends the RelyingParty interface with the PKCECodeGenerator method:

type RelyingPartyCodeChallenge interface {
	RelyingParty
	PKCECodeGenerator() PKCECodeGenerator
}

The default implementation could implement the new interface:

//NewRelyingPartyOIDC creates an (OIDC) RelyingPartyCodeChallenge with the given
//issuer, clientID, clientSecret, redirectURI, scopes and possible configOptions
//it will run discovery on the provided issuer and use the found endpoints
func NewRelyingPartyOIDC(issuer, clientID, clientSecret, redirectURI string, scopes []string, options ...Option) (RelyingPartyCodeChallenge, error) {
...

But doesn't even have to, because the GenerateAndStoreCodeChallenge would just have to check whether the provided RP also implements the RelyingPartyCodeChallenge interface:

func GenerateAndStoreCodeChallenge(w http.ResponseWriter, rp RelyingParty) (string, error) {
	codeGen := defaultPKCECodeGenerator
	c, ok := rp.(RelyingPartyCodeChallenge)
	if ok {
		codeGen = c.PKCECodeGenerator()
	}
	codeVerifier, err := codeGen()
	if err != nil {
		return "", err
	}
	if err := rp.CookieHandler().SetCookie(w, pkceCode, codeVerifier); err != nil {
		return "", err
	}
	return oidc.NewSHACodeChallenge(codeVerifier), nil
}

And as you mentioned, I would not have the defaultPKCECodeGenerator variable public. I can't remember or think of a reason I did this with the DefaultErrorHandler.

What do you think of that?

[Osipion]

Hey @livio-a apologies for taking a couple of days to get back to you. If you're happy adding a bit more complexity to the API to retain backwards compatibility then I am too 🙂 . I've made the changes. I've also changed how default codes are generated (concat 2 UUID strings) to make sure the default is spec compliant WRT length. In my own custom implementation I use:

var (
	ErrInvalidPKCEVerifierLength = errors.New("PKCE verifiers must be between 43 and 128 characters")
	PKCEVerifierAlphabet = []rune("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~")
)

func String(l int, alphabet []rune) (string, error) {
	chars := make([]rune, l)
	ln := big.NewInt(int64(len(alphabet)))
	for i := 0; i < l; i++ {
		idx, err := rand.Int(rand.Reader, ln)
		if err != nil {
			return "", err
		}
		chars[i] = alphabet[idx.Int64()]
	}
	return string(chars), nil
}

func PKCEChallenge(l int) (string, error) {
	if l < 43 || l > 128 {
		return "", ErrInvalidPKCEVerifierLength
	}
	return String(l, PKCEVerifierAlphabet)
}

Not sure if that's a good fit here?

[livio-a]

@Osipion looks good.

Quick question to the default implementation change: Wouldn't the base64 representation of a single uuid be enough. The uuid is always 36 chars long, so the base64 representation will always be 48 long. Which would suit the specs.

But as already mentioned, I thought about switching to a similar approach to generate the challenge as you did with generating a random string. So we could as well take this into the library as default.

[fforootd]

hi @Osipion thanks for your help so far, do you have further feedback for @livio-a

Wouldn't the base64 representation of a single uuid be enough. The uuid is always 36 chars long, so the base64 representation will always be 48 long. Which would suit the specs.

[fforootd]

@Osipion @thomas-welch are you still interested in this improvement? It looks like a question is still open 😁

[muhlemmer]

@Osipion @thomas-welch friendly ping if you still need this merged?

[muhlemmer]

RE: @livio-a

Quick question to the default implementation change: Wouldn't the base64 representation of a single uuid be enough. The uuid is always 36 chars long, so the base64 representation will always be 48 long. Which would suit the specs.

But as already mentioned, I thought about switching to a similar approach to generate the challenge as you did with generating a random string. So we could as well take this into the library as default.

If you still need to switch the random string generation, I can pick that up separately. Perhaps create a issue for it first? Btw, the above PR uses a slice of []rune but using a constant string is way faster.

[muhlemmer]

Closing this due to inactivity. However, feel free to ping us if you want to proceed to merge this.