Skip to content

Commit

Permalink
chore: Audit crypto folder (#11932)
Browse files Browse the repository at this point in the history
## Description

ref: #11362 

I did **NOT** review the following folders, as they contain cryptography which I don't think I'm competent enough to give a useful review:
- [ ] `crypto/xsalsa20symmetric` (new in v046, ported from TM i think)
- [ ] `crypto/keys/secp256k1` (some new stuff in v046 too)

Also performed some manual tests as part of #11939:

  - [x] Create keys on v0.45, make sure they still work in v0.46 #11939 (comment)
  - [x] Create new keys in v0.46 #11939 (comment)
  - [x] `--multisig` flag works with an address that's not in the keyring (see [repro](#9553))



---

### 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...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### 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...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
  • Loading branch information
amaury1093 authored May 15, 2022
1 parent 4f31162 commit b2b29d4
Show file tree
Hide file tree
Showing 13 changed files with 161 additions and 303 deletions.
5 changes: 3 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

### API Breaking Changes

* (types ) [#11881](https://github.com/cosmos/cosmos-sdk/issues/11881) Rename `AccAddressFromHex` to `AccAddressFromHexUnsafe`.
* (crypto/keyring) [#11932](https://github.com/cosmos/cosmos-sdk/pull/11932) Remove `Unsafe*` interfaces from keyring package. Please use interface casting if you wish to access those unsafe functions.
* (types) [#11881](https://github.com/cosmos/cosmos-sdk/issues/11881) Rename `AccAddressFromHex` to `AccAddressFromHexUnsafe`.
* (types) [#11788](https://github.com/cosmos/cosmos-sdk/pull/11788) The `Int` and `Uint` types have been moved to their own dedicated module, `math`. Aliases are kept in the SDK's root `types` package, however, it is encouraged to utilize the new `math` module. As a result, the `Int#ToDec` API has been removed.
* (grpc) [\#11642](https://github.com/cosmos/cosmos-sdk/pull/11642) The `RegisterTendermintService` method in the `tmservice` package now requires a `abciQueryFn` query function parameter.
* [\#11496](https://github.com/cosmos/cosmos-sdk/pull/11496) Refactor abstractions for snapshot and pruning; snapshot intervals eventually pruned; unit tests.
Expand All @@ -98,7 +99,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [\#10950](https://github.com/cosmos/cosmos-sdk/pull/10950) Add `envPrefix` parameter to `cmd.Execute`.
* (x/mint) [\#10441](https://github.com/cosmos/cosmos-sdk/pull/10441) The `NewAppModule` function now accepts an inflation calculation function as an argument.
* [\#10295](https://github.com/cosmos/cosmos-sdk/pull/10295) Remove store type aliases from /types
* [\#9695](https://github.com/cosmos/cosmos-sdk/pull/9695) Migrate keys from `Info` -> `Record`
* [\#9695](https://github.com/cosmos/cosmos-sdk/pull/9695) Migrate keys from `Info` (serialized as amino) -> `Record` (serialized as proto)
* Add new `codec.Codec` argument in:
* `keyring.NewInMemory`
* `keyring.New`
Expand Down
135 changes: 30 additions & 105 deletions api/cosmos/crypto/keyring/v1/record.pulsar.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 20 additions & 1 deletion client/keys/export.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ package keys

import (
"bufio"
"encoding/hex"
"fmt"

"github.com/spf13/cobra"

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/input"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
"github.com/cosmos/cosmos-sdk/crypto/types"
)

const (
Expand Down Expand Up @@ -75,7 +77,7 @@ func exportUnsafeUnarmored(cmd *cobra.Command, uid string, buf *bufio.Reader, kr
return nil
}

hexPrivKey, err := keyring.NewUnsafe(kr).UnsafeExportPrivKeyHex(uid)
hexPrivKey, err := unsafeExportPrivKeyHex(kr.(unsafeExporter), uid)
if err != nil {
return err
}
Expand All @@ -84,3 +86,20 @@ func exportUnsafeUnarmored(cmd *cobra.Command, uid string, buf *bufio.Reader, kr

return nil
}

// unsafeExporter is implemented by key stores that support unsafe export
// of private keys' material.
type unsafeExporter interface {
// ExportPrivateKeyObject returns a private key in unarmored format.
ExportPrivateKeyObject(uid string) (types.PrivKey, error)
}

// unsafeExportPrivKeyHex exports private keys in unarmored hexadecimal format.
func unsafeExportPrivKeyHex(ks unsafeExporter, uid string) (privkey string, err error) {
priv, err := ks.ExportPrivateKeyObject(uid)
if err != nil {
return "", err
}

return hex.EncodeToString(priv.Bytes()), nil
}
4 changes: 2 additions & 2 deletions client/keys/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ func runMigrateCmd(cmd *cobra.Command, _ []string) error {
return err
}

if _, err = clientCtx.Keyring.MigrateAll(); err != nil {
if err = clientCtx.Keyring.MigrateAll(); err != nil {
return err
}

cmd.Println("Keys migration has been successfully executed")
cmd.Println("Keys migration has been successfully executed.")
return nil
}
4 changes: 2 additions & 2 deletions client/keys/migrate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ func (s *MigrateTestSuite) Test_runListAndShowCmd() {
serializedLegacyMultiInfo := keyring.MarshalInfo(legacyMultiInfo)

item := design99keyring.Item{
Key: s.appName,
Key: s.appName + ".info",
Data: serializedLegacyMultiInfo,
Description: "SDK kerying version",
Description: "SDK keyring version",
}

//run test simd keys list - to see that the migrated key is there
Expand Down
10 changes: 2 additions & 8 deletions crypto/keyring/doc.go
Original file line number Diff line number Diff line change
@@ -1,17 +1,11 @@
// Package keys provides common key management API.
//
//
// The Keybase interface
// The Keyring interface
//
// The Keybase interface defines the methods that a type needs to implement to be used
// The Keyring interface defines the methods that a type needs to implement to be used
// as key storage backend. This package provides few implementations out-of-the-box.
//
// NewLegacy
//
// The NewLegacy constructor returns an on-disk implementation backed by LevelDB storage that has been
// the default implementation used by the SDK until v0.38.0. Due to security concerns, it is
// recommended to drop it in favor of the NewKeyring constructor as it will be removed in future releases.
//
// NewInMemory
//
// The NewInMemory constructor returns an implementation backed by an in-memory, goroutine-safe
Expand Down
Loading

0 comments on commit b2b29d4

Please sign in to comment.