🐛 fix: use OpenID token signature algo as discovered from the server.

[ragavpr]

Summary

Currently if an Authentication server uses a different Token Signing Algorithm other than the default 'RS256', it will fail.

This change sets the OpenID Token Algorithm to the one advertised by the Authentication server.

id_token_signed_response_alg has to be set at the time of the Client object creation, it doesn't take account of that property even if set before or after the creation of the Client object.

All authentication servers advertise their token signing algorithm in id_token_signing_alg_values_supported as an array with one element (as far as I can check)

I tried to see if updating the openid-client package will resolve the issue without this patch fix, but it seems that package has fundamentally changed a lot and requires a lot of changes to use that.

This patch fix will last until the next re-write of openid-client strategy.

Fixes #5358.

Change Type

Please delete any irrelevant options.

• Bug fix (non-breaking change which fixes an issue)

Testing

• Use any OIDC authentication server that signs the response Tokens with EllipticCrypto (EC) Keys instead of the RSA keys.
• Only RS256 (signed with RSA keys) will work before the patch.
• After this patch, any algorithm should be accepted as advertised by the authentication server.

Test Configuration:

• Authentik with two keys with different algorithms (EC and RSA).
• Token signing with different keys, both worked as expected without any user intervention.

Checklist

Please delete any irrelevant options.

• My code adheres to this project's style guidelines
• I have performed a self-review of my own code
• I have commented in any complex areas of my code
• My changes do not introduce new warnings
• I have written tests demonstrating that my changes are effective or that my feature works
• Local unit tests pass with my changes
• Any changes dependent on mine have been merged and published in downstream modules.

[danny-avila]

Can we make this an optional assignment? Would require an update to the docs too: https://github.com/LibreChat-AI/librechat.ai

[ragavpr]

The existing documentation does cover everything properly, this is something the user might configure in the Authentication server, for which the default is RS256 algorithm.

This change only covers other algorithms, that are discovered seamlessly without user interaction, I hope there should be no need to include this in docs, removing documentation action items.

If this commit is merged, no need to update Docs.
If not merged, might need to add a warning block to use RS256 in Docs.

[ragavpr]

I will try to see why the checks fails and prefer merging this, will update docs as a last resort.

[danny-avila]

My main concern is that the object being passed to new issuer.Client( currently works for a lot of folks, so I don't want to break anything by introducing another property that may or may not be needed.

There might be a default value in place but this would be the best way to avoid potential side effects

[ragavpr]

Checked the upstream package openid-client's code, found that v5 does set the default algorithm 'RS256' and doesn't support any other algorithms advertised by the Auth server.

https://github.com/panva/openid-client/blob/45c96f67ce0644bd829f61e82fba3dd8c051c89e/lib/client.js#L190

But v6 does fixes this and uses the one from the server, but keeps 'ES256' as default (this commit still defaults 'RS256')
https://github.com/panva/openid-client/blob/f7fcd7c21eee78ba1e1d848d14acfbfe52686d1c/tap/helper.ts#L25
https://github.com/panva/openid-client/blob/f7fcd7c21eee78ba1e1d848d14acfbfe52686d1c/tap/helper.ts#

I'll agree with you, it is just one of the five other properties this algorithm is required for, I'll close this PR, best way to support other algorithms is to update the openid-client package to v6.

Updating the docs can be optional, since most Auth servers still default to RS256 and the error messages are now displayed after #5337, I'll see if I could update that package.

[ragavpr]

Just keeping this as a draft in case if required, the error was just happening due to the missing property.

Since v6 of openid-client seems ESM only (which is not possible to use here), Also, v6 seems to default only ES256 and I wasn't able to find it properly assigning to the one sent from the server (maybe it does, but requires testing).

I think assigning this property in new issuer.Client( probably doesn't affect existing users using RS256 (I was able to test both algorithms fine), but I'll leave the decision to you.

If more users try to use ES256, consider using a different package to handle openid-strategy if this patch feels uneasy, this can be used.