Feature Request: Add PKCE Support

[ds-sebastian]

I'm currently using this plugin with an OIDC provider (Kanidm) that requires PKCE (Proof Key for Code Exchange).

Feature request: Please add support for PKCE in the OAuth2 authorization flow. This would involve:

1. Generating a code verifier and code challenge
2. Including the code challenge in the initial authorization request
3. Sending the code verifier when exchanging the authorization code for tokens

PKCE is becoming a standard security practice for OAuth2, especially for public clients, and adding this feature would greatly enhance the plugin's compatibility and security!

[sevensolutions]

Hi @ds-sebastian,

thx for creating this feature request. Yes PKCE is a standard today and the plugin should really support it. Just give me some time to implement that.

[sevensolutions]

Ok PKCE is not that easy, because i'am currently relying on the introspection response to validate tokens and this doesn't support PKCE. So i need to rework the token validation too, to validate them by myself. But this is something i already wanted to do.

[sevensolutions]

@ds-sebastian PKCE should be supported now. I need to so some more tests but feel free to checkout the feature/pkce-branch if you want to test it with your provider. Should be enough to follow the steps under "Local Development".

[ds-sebastian]

Finally got around to testing. Following the local dev steps with the pkce-branch and KanIDM provider, I don't get the PKCE error I was seeing before.

[WhySoBad]

Hi, How is testing coming along? I'm looking forward to use PKCE since it's not only a security improvement but also a big performance improvement. I've just found out that for every request to a route which is protected by the middleware a request is sent to the introspection endpoint which causes a lot of overhead which is noticeable on a lower end system like a raspberry pi. With the current PKCE implementation this problem should be resolved

[WhySoBad]

I've seen you check a jwt for the expiration date and the issuer here: https://github.com/sevensolutions/traefik-oidc-auth/blob/562330709302299b0dc96f53ad99c7595cb8309e/oidc.go#L228-L231

What do you think of adding a check for the aud claim with the WithAudience option where the audience is the Provider.ClientId?

I think this would be great for the security of this middleware since at the moment we only check for a non-expired token from the provided auth server but we don't check whether the token was really issued for our application.

[sevensolutions]

@WhySoBad yes makes sense. But i think we should make it configurable. I think it doesn't always neccesarily need to match the client id. I've quickly drafted together something like this:

options := []jwt.ParserOption{
    jwt.WithIssuer(oidcAuth.DiscoveryDocument.Issuer),
    jwt.WithExpirationRequired(),
}

if oidcAuth.Config.Provider.ValidAudience != "" {
    options = append(options, jwt.WithAudience(oidcAuth.Config.Provider.ValidAudience))
}

parser := jwt.NewParser(options...)

[WhySoBad]

@WhySoBad yes makes sense. But i think we should make it configurable. I think it doesn't always neccesarily need to match the client id. I've quickly drafted together something like this:

options := []jwt.ParserOption{
    jwt.WithIssuer(oidcAuth.DiscoveryDocument.Issuer),
    jwt.WithExpirationRequired(),
}

if oidcAuth.Config.Provider.ValidAudience != "" {
    options = append(options, jwt.WithAudience(oidcAuth.Config.Provider.ValidAudience))
}

parser := jwt.NewParser(options...)

I think having it configurable is a good idea. I also noticed the iss and aud claims are optional in the jwt spec. Maybe it should be possible to opt-out for validating those claims.

Additionally, I think the issuer should be configurable too. What do you think of the following config options:

• Config.Provider.Audience: Audience id which should be included in claim, default client id
• Config.Provider.Issuer: Issuer of the token, default the issuer of the discovery document
• Config.ValidateAudience: Boolean whether to validate the audience, default true
• Config.ValidateIssuer: Boolean whether to validate the issuer, default true

which would allow for any combination of those assertions whilst still having a reasonable default values