Change jws tool #6
Conversation
|
|
||
| namespace ACMESharp.Crypto.JOSE | ||
| { | ||
| public class JWSAlgorithm |
There was a problem hiding this comment.
I'm trying to follow these standard naming conventions. I believe everywhere else you're golden, but this one slipped by, should be JwsAlgorithm.
| { | ||
| string JwsAlg | ||
| { get; } | ||
|
|
||
| void Init(); | ||
| string ExportPrivateJwk(); |
There was a problem hiding this comment.
You renamed ExportJwsk to ExportPublicJwk which is right on, since that exports only the public component of a key pair as per the various JWK specs, it is more accurate.
But here you also renamed the general-purpose Export method to ExportPrivateJwk which is not accurate. In the RSA and ECDSA implementations, this produces an XML export of the entire key pair. That's in keeping with the intention of the original method, to produce a serialized form of the key that can be used to later reconstitute it, but it is definitely not a JWK representation.
I would recommend restoring the original name. Right now there is no need for a JWK export of the private key component, so I wouldn't go down that route unless you have some other intentions?
There was a problem hiding this comment.
I agree. I wanted to pronounce the fact, that this will export the private key also, but currently it seems to export the private key only (which is neither correct nor intended).
| { | ||
| var export = new JwsAlgorithmExport | ||
| { | ||
| Algorithm = _jwsTool.JwsAlg, |
There was a problem hiding this comment.
I wonder if should derive the Algorithm value from the original constructor argument instead of what the IJwsSigner is giving you. In this case the JwsAlg value is what is needed for a JWS payload to understand the contents and associated key type, but it may not exactly translate to the specific IJwsSigner implementation upon reload.
And along those same lines, the JwsAlgorithm(string) constructor would actually be taking an implementation identifier of sorts, not actually a JWS algorithm ID.
For example, in the current core code base, there are the 2 default JWS signer implementations, RSA and ECDSA, but I recently had to add a third one, an alternative ECDSA version because the 2 default implementations would not work in my target environment because they relied on the BCL crypto support which was missing, and I needed to add an alternative variation that was based on BouncyCastle. Even though it's a distinct implementation of it's own, it still implements the ES256 (and ES384 and ES521) algorithms which is what it returns in its JwsAlg property.
There was a problem hiding this comment.
You are right - I reduced the extend-ability here, which should be there to swap out the actual JWS implementation. I was so fixated on making reuse of the key more "graspable" and straight forward, that I ignored that.
I have an idea, where you would get the possibility to define which JwsImplementation reacts on which name - something like a Dict<string, Func<string, IJwsSigner>> or something along those lines. Also instead of using the JwsAlg-Names i could use a more distinguished list like DefaultRSA374 or something like that.
|
Thanks for the PR! In general I like the direction you're taking here. I'm still trying to think through the I'm almost wondering if instead of a composition-like approach it should be more of a factory? |
|
Most importantly I think it must be super simple to export the JwsAlgorithm and import it again. Therefor I hid away the IJwsSigner (or IJwsTool), where the user of the library has to reselect the Implementation. |
|
The second variant is probably the better one |
Partial implementation of #5
Did not implement Factory, but hid algorithm behind basic class (could be a Decorator with a Factory-like-ctor)