Skip to content

Add signing algorithm selection #46

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

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

Add signing algorithm selection #46

wants to merge 2 commits into from

Conversation

stephank
Copy link
Member

@stephank stephank commented Feb 24, 2025

I'm somewhat into ditching RSA, and wanted a formal path towards doing that in Portier. I don't really have the literature to back this up or anything, it's mostly the feeling of what is 'good cryptography' that's been instilled on me from reading blogs.

We currently have an experimental path implemented in the broker (but none of our client libs) that is not really documented anywhere and is very much non-standard. The protocol described in this PR however is compatible with OIDC Dynamic Client Registration, basically what they describe as a 'stateless registration' where the client_id string itself contains all the registration details. (I've been working on an implementation of this, also to further our success in the OIDC self-certification test suite.)

Simply put, we use a query string in client_id to allow alg selection, and the client stores it along with the nonce. Then it can verify the parameters weren't modified in the callback. (Just like we already stipulate it must check the email wasn't modified from the original request.)

@onli
Copy link
Member

onli commented Feb 24, 2025

Two thoughts:

  1. alg-selection is or was one of the big attack vectors for JWTs, https://www.chosenplaintext.ca/2015/03/31/jwt-algorithm-confusion.html as an example. A sound alg selection and not accepting unknown parameters should be secure, but from this perspective this feels not ideal
  2. On the other hand, yes, it's a good idea to prepare alternatives to RSA. Though I'm not aware that RS256 is currently problematic.

@stephank
Copy link
Member Author

Thanks for taking a look!

I'm concerned about that as well, and hopefully wrote up something that doesn't allow for any gaps here. The client is supposed to use (nonce, client_id, email) as some sort of primary key, so that client id params must match from start to end. Then the client checks the jwt was signed according to client id params.

The only gap I could think of is that, before those checks, we do allow verification using any signing algorithm in this setup. The checks would still fail, but perhaps you could trick a client into first doing some super expensive signature verification a million times over.

It can be mitigated by having the client use state to find its record, which allows checking alg earlier, and that is suggested in the spec as a MAY. Or the client could just very carefully parse the JWT payload first. We could make a MUST out of one of those solutions, though I'm not sure what generic OIDC clients do.

Tbh I'm also itching to write an alternative OIDC-incompatible protocol that makes all of those parameters constant. Spec it as Ed25519, and if that ever needs to change, write a v2. 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants