ring icon indicating copy to clipboard operation
ring copied to clipboard

Introduce EcdsaKeyPair::from_private_key_unchecked.

Open partim opened this issue 6 years ago • 4 comments

This PR adds EcdsaKeyPair::from_private_key_unchecked which creates an ECDSA key pair from the private key only.

Note: The two private keys in the test case are random specimen from the NIST CAVP 186-4 ECDSA2VS Test Vectors. I think testing for short, long, and correctly sized keys should about cover it?

If merged, fixes #882.

partim avatar Aug 20 '19 13:08 partim

From from_private_key_and_public_key I only added test cases for P-256. I think that’s enough, but we can easily add more to the txt file.

partim avatar Sep 13 '19 09:09 partim

Sorry I lost track of this. I'm updating the base branch to be main so I can remove master today. The main issue I see now is with the tests: We should use the same test files (*.txt) to verify all the variants of this API, instead of having separate files just for this API. I.e. every place we test from_private_key_unchecked we should also be verifying the checked variant.

briansmith avatar Jun 19 '20 17:06 briansmith

No worries, I got caught up in all sorts of other work as well.

I’m pretty sure there was a reason why I added two test files, but I can’t remember now. I’ll look into merging the two and provide an update soon.

partim avatar Jun 22 '20 09:06 partim

Now it is my turn to apologize for the long silence.

I have pushed a commit to bring the branch in line with current main.

I have also looked into merging the two test files, but I’m not sure it makes sense. For one, the meaning of the PublicKey field in the two cases differs: In one case it contains input, in the other it contains a value the result needs to be checked against. In addition, there are test cases in ecdsa_from_private_key_and_public_key_test that test that the function fails for invalid public key input which don’t make sense in ecdsa_from_private_key_unchecked_test.

This could be fixed by adding additional fields to distinguish the semantics, but I am not sure if this improves clarity?

partim avatar Nov 20 '20 09:11 partim