Closed cryptphil closed 1 year ago
Thanks for the contribution. Looks promising. A thing we had on our end that would be helpful is, that the signer interface could generate the base header by itself, instead of just returning algorithm and kid. This would allow for more flexibility when generating the SD-JWT. Some signature mechanisms might want to use kid but others will not do that. We for example need a way to set the x5c header and and omit the kid.
So I am looking for something like:
interface SdJwtSigner {
fun baseHeader(): JWSHeader.Builder
fun jwsSigner(): JWSSigner
}
We could discuss which role the SdJwtHeader plays in this. I would refrain from duplicating all the possible header entries there that might be set and setting it this way. Setting x5c, kid, sigT (this is a custom JAdES header) and others belongs to the signing implementation IMHO. SdJwtHeader only serves the purpose of setting SD-JWT specific header fields.
An additional remark is, that I am not that happy with the fact that it extends JWSSigner instead of returning one. Basically the thing only signs SD-JWTs created with the library but could be used in other places where a JWSSigner is required when it extends that. The additional header values would then be ignored though. Thus returning a JWSSigner would be better IMHO to avoid confusion.
When it no longer is a JWSSigner it should not be called *JWSSigner any more. SdJwtSigner would work, as well as SigningConfiguration or SignerProvider or SignatureProvider. Just some thoughts, maybe you can come up with a better name.
I dislike, that we now have two methods buildJWT one for a key and one for a SdJwtSigner. I would do it the following way:
Thanks for your quick and constructive feedback, @markuskreusch! I will address your points as soon as possible, in the meantime I have set this PR as a draft.
I addressed your feedback and introduced a SdJwtSigner
interface @markuskreusch.
The createCredential
method, which expects a private key, also still exists and calls the new one with a KeyBasedSdJwtSigner
but I added a comment that this is actually a legacy method now.
@cryptphil Exactly what I had in mind.
One small thing: The KeyBasedSdJwtSigner now always returns the same header builder instance. This will work if a new instance is created per credential creation, but will not work if the same instance is reused. The baseHeader method should return a new builder each time it is invoked.
You could:
Apart from that LGTM.
@markuskreusch Thanks, good catch! I fixed it using your second proposed suggestion such that the return type is still JWSHeader.Builder.
I have now merged the changes from PR #4 into mine and this one would be ready to review again @fabian-hk
From my point of view we can drop the old method without the signer interface. As far as I know you two are the only users of the library right now. I will look into the PR as soon as possible.
The PR looks good now. Thank you very much for your contribution!
This PR adds support for external signer implementations used to sign SD-JWTs by introducing an SD-JWT signer interface. Therefore, the
createCredential
method can now also be called by providing a signer instance instead of a private key.Closes #5.