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

crypto: add secp256r1 #8559

Merged
merged 79 commits into from
Mar 4, 2021
Merged

crypto: add secp256r1 #8559

merged 79 commits into from
Mar 4, 2021

Conversation

robert-zaremba
Copy link
Collaborator

@robert-zaremba robert-zaremba commented Feb 10, 2021

Description

closes: #7718

This design follows the ecdsa design in Go stdlib: we have one structure for all ecdsa keys

When serializing key, we are prepending one byte to store information about the key type. For any other ecdsa key, we don't need to create different structure.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

compressed[1] = byte(pk.Y.Bit(0)) | 2
pk.X.FillBytes(compressed[2:])
return compressed
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When serializing I'm adding additional byte to add curve information. Alternative design would be to use duplicate structures.

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

There are still a couple of tmcrypto.Address and XXX lying around, but apart from that I'm good with this PR 👍

}

// PrivKey defines a secp256r1 ECDSA private key.
message PrivKey {
Copy link
Member

Choose a reason for hiding this comment

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

Do we even need a PrivKey in protobuf? This should not be created in our keyring but just by hardware devices.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created it for completeness. Do you prefer to remove it?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... I don't have a strong preference, but generally prefer to not include things which won't/shouldn't be used. For tests we have an internal privkey to use already right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, tests and private key generation will work with the internal/ecdsa.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, so I will remove the PrivKey proto.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sdkcrypto.PrivKey interface requires to be proto.Message. While it doesn't need to implement Marsha and Unmarshal method, it's meant to be used in protobuf ecosystem as well as being marshaled (eg proto.Marshal(pb proto.Message). Hence I think it's better to keep the proto file and generated files. What do you think?

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

LGTM. There are a few minor improvements that could be made before merging. Otherwise it has enough approvals

@alessio alessio added A:automerge Automatically merge PR once all prerequisites pass. and removed A:automerge Automatically merge PR once all prerequisites pass. labels Mar 3, 2021
@alessio
Copy link
Contributor

alessio commented Mar 3, 2021

I've added and then removed automerge as it appears that @robert-zaremba is still doing some changes to this PR?

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

Merge when you're ready @robert-zaremba

@robert-zaremba robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 4, 2021
@mergify mergify bot merged commit c66f1f7 into master Mar 4, 2021
@mergify mergify bot deleted the robert/secp256r1 branch March 4, 2021 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add secp256r1 Signing Keys
6 participants