Support for ECDSA KeyGeneration/Sign/Verify/SignatureConversion #1275
Conversation
|
Updated to current sources, added some more tests and now support P-521 as it has been added to jsrsasign |
|
Why is this not in ? |
|
Rebased to the current sources. Looks like there is something wrong with the UI tests: |
also an Operation for ECDSA signature conversion, as there could be multiple formats of the signature
| case "ASN.1 HEX": | ||
| result = signatureASN1Hex; | ||
| break; | ||
| case "Concat HEX": |
There was a problem hiding this comment.
This seems to error out for me, reproduction example.
There was a problem hiding this comment.
You don't convert keys with this but an ECDSA signature. An ECDSA signature consists of two parts an "R" and a "S" point (both are BigInteger). There are different formats how these two values are stored. OpenSSL uses an ASN1 SEQUENCE of two INTEGERs. Windows and JSONWebSigning use a format defined in IEEE-P1363 (just two equally long [zero padded] integer values). I will rename "Concat HEX" to "P1363 HEX" to hopefully make this more clear. I will also add a fourth option "JSONWebSignature", that is the base64url encoded P1363 format used for JWT signatures.
There was a problem hiding this comment.
Yeah, this is my mistake. I got very confused between signatures and keys and it made a lot of the comments in this thread inaccurate on my part. I like the changes you suggest above, and don't believe an error is necessarily incorrect given I've provided entirely the wrong thing!
| case "ASN.1 HEX": | ||
| signatureASN1Hex = input; | ||
| break; | ||
| case "Concat HEX": |
There was a problem hiding this comment.
This seems to be overly permissive, and could lead to misleading answers. E.g. this is an invalid entry, but gives me a sane-looking key back example reproduction
There was a problem hiding this comment.
I will add a check if the input is really a hex value
| case "Concat HEX": | ||
| signatureASN1Hex = r.KJUR.crypto.ECDSA.concatSigToASN1Sig(input); | ||
| break; | ||
| case "JSON": { |
There was a problem hiding this comment.
This doesn't seem like it represents the JWK format. What format is this? I would have expected something like this to work but it doesn't seem to.
I'd expect this format to prefer existing specifications, but either way there should be some sanity checks to ensure that both 'r' and 's' exist in the JSON.
There was a problem hiding this comment.
"r" and "s" are the two parts of an ECDSA signature. I'm not sure if the JSON format of the signature is used somewhere directly but some libraries want the signature as two parameters!
There was a problem hiding this comment.
My mistake, I believe this is caused by my confusion above between 'signature' and 'key' conversion. This seems like a valid format for inputting these values. Could we still add a check to make sure that the JSON is (a) valid and (b) Has both 'r' and 's' attributes.
src/core/operations/ECDSAVerify.mjs
Outdated
| if (inputFormat === "Auto") { | ||
| if (input.substr(0, 2) === "30" && r.ASN1HEX.isASN1HEX(input)) { | ||
| inputFormat = "ASN.1 HEX"; | ||
| } else if (input.indexOf("{") !== -1) { |
There was a problem hiding this comment.
It's probably better to write a function that just calls JSON.parse(), then wrap it in try {} catch{} to verify whether it's valid JSON or not.
| case "JWK": | ||
| pubKey = JSON.stringify(r.KEYUTIL.getJWKFromKey(keyPair.pubKeyObj)); | ||
| privKey = JSON.stringify(r.KEYUTIL.getJWKFromKey(keyPair.prvKeyObj)); | ||
| result = pubKey + "\n\n" + privKey; |
There was a problem hiding this comment.
Generally we prefer to use \n throughout the application.
There was a problem hiding this comment.
I output them now as JSONWebKeySet, as we have two keys here
this format name is more specific and easier to search for on the internet
also rename the format to "Raw JSON" as I will later introduce "JSON Web Signature"
used e.g. by JWT
|
Improved the error checking and added a new signature type "JSON Web Signature" that is used by e.g. JWT |
|
This looks way better, thanks so much for making the changes! |
|
thanks for the merge |
No description provided.