Skip to content
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

Support for ECDSA KeyGeneration/Sign/Verify/SignatureConversion #1275

Merged
merged 6 commits into from
Apr 15, 2024

Conversation

cplussharp
Copy link
Contributor

No description provided.

@cplussharp
Copy link
Contributor Author

Updated to current sources, added some more tests and now support P-521 as it has been added to jsrsasign

@RobLinux
Copy link

Why is this not in ?

@cplussharp
Copy link
Contributor Author

Rebased to the current sources. Looks like there is something wrong with the UI tests:

An error occurred while creating a new ChromeDriver session: [SessionNotCreatedError] session not created: This version of ChromeDriver only supports Chrome version 114
Current browser version is 117.0.5938.132 with binary path /usr/bin/google-chrome

also an Operation for ECDSA signature conversion,
as there could be multiple formats of the signature
@cplussharp
Copy link
Contributor Author

Updated to current master sources. This fixes #1791 and #1792

case "ASN.1 HEX":
result = signatureASN1Hex;
break;
case "Concat HEX":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to error out for me, reproduction example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (inputFormat === "Auto") {
if (input.substr(0, 2) === "30" && r.ASN1HEX.isASN1HEX(input)) {
inputFormat = "ASN.1 HEX";
} else if (input.indexOf("{") !== -1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

case "JWK":
pubKey = JSON.stringify(r.KEYUTIL.getJWKFromKey(keyPair.pubKeyObj));
privKey = JSON.stringify(r.KEYUTIL.getJWKFromKey(keyPair.prvKeyObj));
result = pubKey + "\n\n" + privKey;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally we prefer to use \n throughout the application.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I output them now as JSONWebKeySet, as we have two keys here

Copy link
Member

@a3957273 a3957273 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Included feedback in other comments.

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"
@cplussharp
Copy link
Contributor Author

Improved the error checking and added a new signature type "JSON Web Signature" that is used by e.g. JWT

@a3957273
Copy link
Member

This looks way better, thanks so much for making the changes!

@a3957273 a3957273 merged commit 7538be6 into gchq:master Apr 15, 2024
4 checks passed
@cplussharp
Copy link
Contributor Author

thanks for the merge

@cplussharp cplussharp deleted the ec-asn1 branch April 15, 2024 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants