-
Notifications
You must be signed in to change notification settings - Fork 74
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
Change jws tool #6
Conversation
|
||
namespace ACMESharp.Crypto.JOSE | ||
{ | ||
public class JWSAlgorithm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to follow these standard naming conventions. I believe everywhere else you're golden, but this one slipped by, should be JwsAlgorithm
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix that.
{ | ||
string JwsAlg | ||
{ get; } | ||
|
||
void Init(); | ||
string ExportPrivateJwk(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)