Accept RSA Signing Algorithms

[vincentkelleher]

Hi 👋

In our goal of supporting OIDC4VCI/OIDC4VP protocols in the Gaia-X ecosystem we based ourselves on your OID4VCI lib which is very easy to get up to speed with thanks to the documentation and examples.

Our Use Case

Gaia-X Clearing Houses currently need an EV SSL certificate in order to be able to issue compliance verifiable credentials. Those certificates are mostly delivered with RSA signing algorithms and are costly so we wanted to try using those with your library to avoid having to ask members to buy new EV SSL certificates with EdDSA, ES256 or ES256K signatures.

The Issue

We noticed that you implemented a closed list of algorithms with the Alg enum and that a check is done to make sure the credential request proof uses a supported algorithm. The following error is raised :

I have tried adding the PS256 algorithm we use to the Alg enum and everything works fine 👍

Thoughts

As jwtVerifyCallback is the method that's depending on the signing algorithm and that this callback is defined by the developer using the library, hence its responsibility, wouldn't it be possible to remove this check and just leave it to the implementer ?

If not, would it be possible to add widely used algorithms to the Alg enum or even leave the implementing developer define it in the VcIssuer metadata ?

It would be my pleasure to exchange with you about this 😊

[nklomp]

Hi @vincentkelleher Thanks for the nice words. Actually the OID4VC libs need an awful lot of work on the doc side, but given everything is changing below our feet, we only expect to do that over the next few months. Good to hear you can find your bearing though.

Yeah you bring up some valid points. The primary reason is to protect the developer a bit from making mistakes by implementing an identifier that is not officially recognized, whilst also providing strong type support. At the same time I agree that we should not really be a barrier for new algorithms being implemented, since we do not make many assumptions about signing to begin with. Probably we should just extend the mechanism to also accept strings next to the enum. Or better yet move to a union type of string literal values as well as the string type. Then we have the best of both worlds. An IDE doing autocompletion/suggestions, but also allows having new Algos.

BTW happy to have short introduction call to see if and how we can help to begin with, given we also are working in certain Gaia-X related projects

[nklomp]

In the mean time we will add PS256 to the Alg enum, but that would be followed by refactoring the Alg code.

[vincentkelleher]

Hi @nklomp,

Thanks for your answer 👍

If you want I can produce the merge request with the changes, the only thing I'm not confortable with is that I didn't find any tests covering the parts I mentioned like the statement that throws the algorithm error. I don't know what the guidelines are in this project about testing.

Furthermore, I like to base myself on the Jwt.io Libraries page to list all the algorithms to be supported. It lists all the main algorithms on the market to date.

I would be delighted to have an introduction call with you 👍 You can contact me at vincent.kelleher@gaia-x.eu to set up that call.

[nklomp]

Sorry for the late reply.

If you would be happy to provide the PR that would be most welcome. Regarding the test coverage; we need to up that for this library indeed. We will be doing some refactoring and documentation work soon, now the specs are stabilizing a bit more; that is also the moment in which we will up coverage significantly. So ideally you would provide some increased test coverage for it, but we are okay at this point if that isn't the case.

Let me reach out after next week to setup a call to see if and where we can help out eachother.

[vincentkelleher]

I've raised a Pull Request with a change to allow any algorithm as its the responsibility of the implementer to manage the signature part. It also increases the coverage of the part I've modified 😊

Thank you for the quick commit to support other algorithms 👍

[nklomp]

Forgot to close this issue while merging the PR. Thanks again for the PR