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

Introduce EcdsaKeyPair::from_private_key_unchecked. #889

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

partim
Copy link

@partim partim commented Aug 20, 2019

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
Copy link
Author

partim commented Sep 13, 2019

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.

@briansmith briansmith changed the base branch from master to main June 19, 2020 17:00
@briansmith
Copy link
Owner

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.

@partim
Copy link
Author

partim commented Jun 22, 2020

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
Copy link
Author

partim commented Nov 20, 2020

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?

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.

EcdsaKeyPair from private key component only?
2 participants