-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat: secp256k1 public key constant time #18026
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (45)
- client/v2/go.mod
- client/v2/go.sum
- go.mod
- go.sum
- simapp/go.mod
- simapp/go.sum
- simapp/gomod2nix.toml
- tests/go.mod
- tests/go.sum
- tests/starship/tests/go.mod
- tests/starship/tests/go.sum
- x/accounts/go.mod
- x/accounts/go.sum
- x/auth/go.mod
- x/auth/go.sum
- x/authz/go.mod
- x/authz/go.sum
- x/bank/go.mod
- x/bank/go.sum
- x/circuit/go.mod
- x/circuit/go.sum
- x/distribution/go.mod
- x/distribution/go.sum
- x/evidence/go.mod
- x/evidence/go.sum
- x/feegrant/go.mod
- x/feegrant/go.sum
- x/gov/go.mod
- x/gov/go.sum
- x/group/go.mod
- x/group/go.sum
- x/mint/go.mod
- x/mint/go.sum
- x/nft/go.mod
- x/nft/go.sum
- x/params/go.mod
- x/params/go.sum
- x/protocolpool/go.mod
- x/protocolpool/go.sum
- x/slashing/go.mod
- x/slashing/go.sum
- x/staking/go.mod
- x/staking/go.sum
- x/upgrade/go.mod
- x/upgrade/go.sum
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (45)
- client/v2/go.mod
- client/v2/go.sum
- go.mod
- go.sum
- simapp/go.mod
- simapp/go.sum
- simapp/gomod2nix.toml
- tests/go.mod
- tests/go.sum
- tests/starship/tests/go.mod
- tests/starship/tests/go.sum
- x/accounts/go.mod
- x/accounts/go.sum
- x/auth/go.mod
- x/auth/go.sum
- x/authz/go.mod
- x/authz/go.sum
- x/bank/go.mod
- x/bank/go.sum
- x/circuit/go.mod
- x/circuit/go.sum
- x/distribution/go.mod
- x/distribution/go.sum
- x/evidence/go.mod
- x/evidence/go.sum
- x/feegrant/go.mod
- x/feegrant/go.sum
- x/gov/go.mod
- x/gov/go.sum
- x/group/go.mod
- x/group/go.sum
- x/mint/go.mod
- x/mint/go.sum
- x/nft/go.mod
- x/nft/go.sum
- x/params/go.mod
- x/params/go.sum
- x/protocolpool/go.mod
- x/protocolpool/go.sum
- x/slashing/go.mod
- x/slashing/go.sum
- x/staking/go.mod
- x/staking/go.sum
- x/upgrade/go.mod
- x/upgrade/go.sum
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 67-69: The issues with the changelog entry for pull request feat: extra checks on signatures/pubkeys + check the signature first in antehandle #18194 have already been addressed in previous comments.
Signed-off-by: bizk <santiago.yanzon1999@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CHANGELOG.md
Outdated
* (crypto | x/auth) [#14372](https://github.com/cosmos/cosmos-sdk/pull/18194) Key checks on signatures antehandle. | ||
* [#18204](https://github.com/cosmos/cosmos-sdk/pull/18204) Use streaming json parser to parse chain-id from genesis file. | ||
* (crypto/keys) [#7051](https://github.com/cosmos/cosmos-sdk/issues/7051) Made public key generation constant time on `secp256k1` | ||
* (crypto | x/auth) [#14372](https://github.com/cosmos/cosmos-sdk/pull/18194) Key checks on signatures antehandle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changelog entry for pull request #14372 contains a typo or incomplete sentence: "Key checks on signatures antehandle." This should be corrected for clarity and completeness.
@@ -434,9 +427,8 @@ func TestAnteHandlerChecks(t *testing.T) { | |||
// Secp256r1 keys that are not on curve will fail before even doing any operation i.e when trying to get the pubkey | |||
testCases := []testCase{ | |||
{"secp256k1_onCurve", []cryptotypes.PrivKey{priv1}, msgs[0], []uint64{accs[0].GetAccountNumber()}, []uint64{0}, false, true}, | |||
{"secp256k1_NotOnCurve", []cryptotypes.PrivKey{priv12}, msgs[1], []uint64{accs[1].GetAccountNumber()}, []uint64{1}, true, true}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean the key is always on the curve now or why remove the test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new library does not allow the gneration of keys that are not in the curve, it triggers an error before generating the private key. Even bypassing it and generating a priv key that is not in the curve and assigning it as the key will fail if you try to get the pubkey/address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utack
@bizk could you resolve conflicts so the bot can merge this pr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (45)
- client/v2/go.mod
- client/v2/go.sum
- go.mod
- go.sum
- simapp/go.mod
- simapp/go.sum
- simapp/gomod2nix.toml
- tests/go.mod
- tests/go.sum
- tests/starship/tests/go.mod
- tests/starship/tests/go.sum
- x/accounts/go.mod
- x/accounts/go.sum
- x/auth/go.mod
- x/auth/go.sum
- x/authz/go.mod
- x/authz/go.sum
- x/bank/go.mod
- x/bank/go.sum
- x/circuit/go.mod
- x/circuit/go.sum
- x/distribution/go.mod
- x/distribution/go.sum
- x/evidence/go.mod
- x/evidence/go.sum
- x/feegrant/go.mod
- x/feegrant/go.sum
- x/gov/go.mod
- x/gov/go.sum
- x/group/go.mod
- x/group/go.sum
- x/mint/go.mod
- x/mint/go.sum
- x/nft/go.mod
- x/nft/go.sum
- x/params/go.mod
- x/params/go.sum
- x/protocolpool/go.mod
- x/protocolpool/go.sum
- x/slashing/go.mod
- x/slashing/go.sum
- x/staking/go.mod
- x/staking/go.sum
- x/upgrade/go.mod
- x/upgrade/go.sum
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- client/keys/add.go (2 hunks)
- tests/integration/runtime/query_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/integration/runtime/query_test.go
Additional comments: 4
CHANGELOG.md (2)
69-69: The entry for issue secp256k1: Not constant time #7051 correctly reflects the PR objective of enhancing the
secp256k1
public key generation process for security reasons.68-68: The entry for pull request perf: parse chain-id from big genesis file could be slow #18204 aligns with the summary mentioning the addition of a streaming JSON parser for parsing chain-id from the genesis file.
client/keys/add.go (2)
20-26: The addition of the
secp256k1
import aligns with the PR objectives to enhance thesecp256k1
public key generation process.218-234: The changes to create an empty
secp256k1
public key and marshal it into JSON align with the PR objectives to use the newsecp256k1
implementation.
tempAny, err := codectypes.NewAnyWithValue(algo.Generate()([]byte{}).PubKey()) | ||
|
||
// create an empty seckp256k1 pubkey since it is the key returned by algo Generate function. | ||
enotySecpPubKey, err := codectypes.NewAnyWithValue(&secp256k1.PubKey{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The secp256k1.PubKey{}
struct should be initialized with a key of the correct size to prevent potential issues.
- enotySecpPubKey, err := codectypes.NewAnyWithValue(&secp256k1.PubKey{})
+ enotySecpPubKey, err := codectypes.NewAnyWithValue(&secp256k1.PubKey{Key: make([]byte, secp256k1.PubKeySize)})
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
enotySecpPubKey, err := codectypes.NewAnyWithValue(&secp256k1.PubKey{}) | |
enotySecpPubKey, err := codectypes.NewAnyWithValue(&secp256k1.PubKey{Key: make([]byte, secp256k1.PubKeySize)}) |
@@ -65,6 +65,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
* (types) [#17670](https://github.com/cosmos/cosmos-sdk/pull/17670) Use `ctx.CometInfo` in place of `ctx.VoteInfos` | |||
* [#17733](https://github.com/cosmos/cosmos-sdk/pull/17733) Ensure `buf export` exports all proto dependencies | |||
* (version) [#18063](https://github.com/cosmos/cosmos-sdk/pull/18063) Include additional information in the Info struct. This change enhances the Info struct by adding support for additional information through the ExtraInfo field | |||
* [#18204](https://github.com/cosmos/cosmos-sdk/pull/18204) Use streaming json parser to parse chain-id from genesis file. | |||
* (crypto/keys) [#7051](https://github.com/cosmos/cosmos-sdk/issues/7051) Made public key generation constant time on `secp256k1` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entry for pull request #14372 contains a potential typo: "antehandle" may be intended to be "ante handle" or "antehandler". Please verify the correct terminology and update accordingly.
* feat: secp256k1 public key constant time (cosmos#18026) Signed-off-by: bizk <santiago.yanzon1999@gmail.com> * chore: Fixed changelog duplicated items (cosmos#18628) * adr: Un-Ordered Transaction Inclusion (cosmos#18553) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * docs: lint ADR-070 (cosmos#18634) * fix(baseapp)!: postHandler should run regardless of result (cosmos#18627) * docs: fix typos in adr-007-specialization-groups.md (cosmos#18635) * chore: alphabetize labels (cosmos#18640) * docs(x/circuit): add note on ante handler (cosmos#18637) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * fix: telemetry metric label variable (cosmos#18643) * chore: typos fix (cosmos#18642) * refactor(store/v2): updates from integration (cosmos#18633) * build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(store/v2): snapshot manager (cosmos#18458) * chore(client/v2): fix typos in the README.md (cosmos#18657) * fix(baseapp): protocompat.go gogoproto.Merge does not work with custom types (cosmos#18654) Co-authored-by: unknown unknown <unknown@unknown> * chore: fix several minor typos (cosmos#18660) * chore(tools/confix/cmd): fix typo in view.go (cosmos#18659) * refactor(x/staking): check duplicate addresses in StakeAuthorization's params (cosmos#18655) * feat(accounts): use gogoproto API instead of protov2. (cosmos#18653) Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(store/commitment/iavl): honor tree.Remove error firstly (cosmos#18651) * build(deps): Bump actions/stale from 8 to 9 (cosmos#18656) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(docs): fix typos & wording in docs (cosmos#18667) * chore: fix several typos. (cosmos#18666) * feat(telemetry): enable `statsd` and `dogstatsd` telemetry sinks (cosmos#18646) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com> Co-authored-by: Marko <marko@baricevic.me> * feat(store/v2): add SetInitialVersion in SC (cosmos#18665) * feat(client/keys): support display discreetly for `keys add` (cosmos#18663) Co-authored-by: Julien Robert <julien@rbrt.fr> * ci: add misspell action (cosmos#18671) * chore: typos fix by misspell-fixer (cosmos#18683) Co-authored-by: github-merge-queue <github-merge-queue@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> * chore: add v0.50.2 changelog to main (cosmos#18682) * build(deps): Bump github.com/jhump/protoreflect from 1.15.3 to 1.15.4 in /tests (cosmos#18678) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * refactor(bank): remove .String() calls (cosmos#18175) Co-authored-by: Facundo <facundomedica@gmail.com> * ci: use codespell instead of misspell-fixer (cosmos#18686) Co-authored-by: Marko <marbar3778@yahoo.com> * feat(gov): add proposal types and spam votes (cosmos#18532) * feat(accounts): use account number as state prefix for account state (cosmos#18664) Co-authored-by: unknown unknown <unknown@unknown> * chore: typos fixes by cosmos-sdk bot (cosmos#18689) Co-authored-by: github-merge-queue <github-merge-queue@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: marbar3778 <marbar3778@yahoo.com> * feat(client/keys): support display discreetly for keys mnemonic (cosmos#18688) * refactor: remove panic usage in keeper methods (cosmos#18636) * ci: rename pr name in misspell job (cosmos#18693) Co-authored-by: Marko <marko@baricevic.me> * build(deps): Bump github.com/pelletier/go-toml/v2 from 2.1.0 to 2.1.1 in /tools/confix (cosmos#18702) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(client/keys): support display discreetly for keys export (cosmos#18684) * feat(x/gov): better gov genesis validation (cosmos#18707) --------- Signed-off-by: bizk <santiago.yanzon1999@gmail.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Carlos Santiago Yanzon <27785807+bizk@users.noreply.github.com> Co-authored-by: yihuang <huang@crypto.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Co-authored-by: Akaonetwo <107335783+Akare123@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: dreamweaverxyz <153101746+dreamweaverxyz@users.noreply.github.com> Co-authored-by: Pioua <136521243+dzizazda@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: cool-developer <51834436+cool-develope@users.noreply.github.com> Co-authored-by: leonarddt05 <139609434+leonarddt05@users.noreply.github.com> Co-authored-by: testinginprod <98415576+testinginprod@users.noreply.github.com> Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: Sukey <35202440+sukey2008@users.noreply.github.com> Co-authored-by: axie <152680487+azukiboy@users.noreply.github.com> Co-authored-by: Luke Ma <867273263@qq.com> Co-authored-by: Emmanuel T Odeke <emmanuel@orijtech.com> Co-authored-by: 0xn4de <109149873+0xn4de@users.noreply.github.com> Co-authored-by: hattizai <150505746+hattizai@users.noreply.github.com> Co-authored-by: Devon Bear <itsdevbear@berachain.com> Co-authored-by: Marko <marko@baricevic.me> Co-authored-by: Halimao <1065621723@qq.com> Co-authored-by: Cosmos SDK <113218068+github-prbot@users.noreply.github.com> Co-authored-by: github-merge-queue <github-merge-queue@users.noreply.github.com> Co-authored-by: Facundo <facundomedica@gmail.com> Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com>
Description
Closes: #7051
This PR replaces public key generation on secp256k1 keys from dcrd library which is non constant to another implementation that it is.
It tests legacy code and the newest key generation, also adds tests and benchmarks
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
secp256k1
, enhancing security.Bug Fixes
Documentation
Refactor
Info
struct with additionalExtraInfo
field.Tests
require
assertions for better readability.Chores
buf export
command.Please note that these release notes are a high-level summary and do not include internal code-level details to maintain confidentiality.