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.

Sign up for GitHub

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

Problem: no exposed db keyring used in the keystore #1419

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

Problem: no exposed db keyring used in the keystore #1419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1bcc36f
Problem: no exposed db keyring used in the keystore
mmsqe May 2, 2024
3cf11a2
keep prefix
mmsqe Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
* (e2ee)[#1421](https://github.com/crypto-org-chain/cronos/pull/1421) Validate e2ee key when register.
* (store) [#1448](https://github.com/crypto-org-chain/cronos/pull/1448) Upgrade rocksdb to `v9.1.1`.
* [#1431](https://github.com/crypto-org-chain/cronos/pull/1431) Integrate testground to run benchmark on cluster.
* (e2ee)[#](https://github.com/crypto-org-chain/cronos/pull/) Simpily new keyring for e2ee module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure the PR number is specified for the e2ee module keyring simplification.

It appears that the PR number is missing in the changelog entry for the e2ee module keyring simplification. This should be corrected to maintain accurate documentation and traceability. Here's a suggested change:

- * (e2ee)[#](https://github.com/crypto-org-chain/cronos/pull/) Simplify new keyring for e2ee module.
+ * (e2ee)[#1419](https://github.com/crypto-org-chain/cronos/pull/1419) Simplify new keyring for e2ee module.
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.

Suggested change
* (e2ee)[#](https://github.com/crypto-org-chain/cronos/pull/) Simpily new keyring for e2ee module.
* (e2ee)[#1419](https://github.com/crypto-org-chain/cronos/pull/1419) Simplify new keyring for e2ee module.
Tools
LanguageTool

[uncategorized] ~24-~24: You might be missing the article “the” here.
Context: ...n/cronos/pull/) Simpily new keyring for e2ee module. ### Bug Fixes * (rpc) [#1397]...


### Bug Fixes

Expand Down
11 changes: 5 additions & 6 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ require (
github.com/spf13/pflag v1.0.5
github.com/spf13/viper v1.18.2
github.com/stretchr/testify v1.9.0
github.com/test-go/testify v1.1.4
golang.org/x/crypto v0.21.0
google.golang.org/genproto/googleapis/api v0.0.0-20240227224415-6ceb2ff114de
google.golang.org/grpc v1.63.2
google.golang.org/protobuf v1.33.0
Expand Down Expand Up @@ -228,6 +226,7 @@ require (
go.opentelemetry.io/otel/metric v1.22.0 // indirect
go.opentelemetry.io/otel/trace v1.22.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
golang.org/x/crypto v0.21.0 // indirect
golang.org/x/exp v0.0.0-20240222234643-814bf88cf225 // indirect
golang.org/x/mod v0.15.0 // indirect
golang.org/x/net v0.23.0 // indirect
Expand All @@ -253,10 +252,10 @@ require (

// release/v0.50.x
replace (
cosmossdk.io/client/v2 => github.com/crypto-org-chain/cosmos-sdk/client/v2 v2.0.0-20240415105151-0108877a3201
cosmossdk.io/store => github.com/crypto-org-chain/cosmos-sdk/store v0.0.0-20240415105151-0108877a3201
cosmossdk.io/x/tx => github.com/crypto-org-chain/cosmos-sdk/x/tx v0.0.0-20240415105151-0108877a3201
github.com/cosmos/cosmos-sdk => github.com/crypto-org-chain/cosmos-sdk v0.0.0-20240415105151-0108877a3201
cosmossdk.io/client/v2 => github.com/crypto-org-chain/cosmos-sdk/client/v2 v2.0.0-20240603035522-8d30134159e0
cosmossdk.io/store => github.com/crypto-org-chain/cosmos-sdk/store v0.0.0-20240603035522-8d30134159e0
cosmossdk.io/x/tx => github.com/crypto-org-chain/cosmos-sdk/x/tx v0.0.0-20240603035522-8d30134159e0
github.com/cosmos/cosmos-sdk => github.com/crypto-org-chain/cosmos-sdk v0.0.0-20240603035522-8d30134159e0
)

replace (
Expand Down
16 changes: 8 additions & 8 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,14 +422,14 @@ github.com/crypto-org-chain/btree v0.0.0-20240406140148-2687063b042c h1:MOgfS4+F
github.com/crypto-org-chain/btree v0.0.0-20240406140148-2687063b042c/go.mod h1:twD9XRA5jj9VUQGELzDO4HPQTNJsoWWfYEL+EUQ2cKY=
github.com/crypto-org-chain/cometbft-db v0.0.0-20231011055109-57922ac52a63 h1:R1QJ9a3XdYMSKo+1RdFifxb/g3lNypC52L/rpYrWoKo=
github.com/crypto-org-chain/cometbft-db v0.0.0-20231011055109-57922ac52a63/go.mod h1:rocwIfnS+kA060x64gkSIRvWB9StSppIkJuo5MWzL24=
github.com/crypto-org-chain/cosmos-sdk v0.0.0-20240415105151-0108877a3201 h1:3R54xSBI4geLX3H5Ljk0FSVDMPRpywq5L5K//aHwo8s=
github.com/crypto-org-chain/cosmos-sdk v0.0.0-20240415105151-0108877a3201/go.mod h1:DkCxCPi3veciROiq36PbpXDhboMjqHAS0Xyv2dEEW04=
github.com/crypto-org-chain/cosmos-sdk/client/v2 v2.0.0-20240415105151-0108877a3201 h1:O1qN/jmh/BZK6JiEADgwDK7jwSQNKqvs5qVYjo0UFa4=
github.com/crypto-org-chain/cosmos-sdk/client/v2 v2.0.0-20240415105151-0108877a3201/go.mod h1:GjpaaxatOEnkSlNKjYxSbzVVtHHAG0tWm26AoPA1g9Q=
github.com/crypto-org-chain/cosmos-sdk/store v0.0.0-20240415105151-0108877a3201 h1:0T8U5tgQLfD8k8kxisez5ks9s7yxU2JSRhi5MUQ0Cp0=
github.com/crypto-org-chain/cosmos-sdk/store v0.0.0-20240415105151-0108877a3201/go.mod h1:lfuLI1f4o+0SGtlHQS4x5qsjRcZZfYqG8bp3k8hM0M8=
github.com/crypto-org-chain/cosmos-sdk/x/tx v0.0.0-20240415105151-0108877a3201 h1:DbCOM19ywdL5K+bOy4h+0MppzcPgI2guHnYCfDNnAcM=
github.com/crypto-org-chain/cosmos-sdk/x/tx v0.0.0-20240415105151-0108877a3201/go.mod h1:CBCU6fsRVz23QGFIQBb1DNX2DztJCf3jWyEkHY2nJQ0=
github.com/crypto-org-chain/cosmos-sdk v0.0.0-20240603035522-8d30134159e0 h1:nnpQA65+0qbg9fBhnzJvvBvco3+9hNxp3gKrOFhK7lQ=
github.com/crypto-org-chain/cosmos-sdk v0.0.0-20240603035522-8d30134159e0/go.mod h1:DkCxCPi3veciROiq36PbpXDhboMjqHAS0Xyv2dEEW04=
github.com/crypto-org-chain/cosmos-sdk/client/v2 v2.0.0-20240603035522-8d30134159e0 h1:mZoahlBJjDnf9nMoDgq4m4mzGZLolfdWhmUh48Vf1XM=
github.com/crypto-org-chain/cosmos-sdk/client/v2 v2.0.0-20240603035522-8d30134159e0/go.mod h1:GjpaaxatOEnkSlNKjYxSbzVVtHHAG0tWm26AoPA1g9Q=
github.com/crypto-org-chain/cosmos-sdk/store v0.0.0-20240603035522-8d30134159e0 h1:VBEgrOHB6XHInt8lDxCVZNRh76qFkK69PgHr0MjawvE=
github.com/crypto-org-chain/cosmos-sdk/store v0.0.0-20240603035522-8d30134159e0/go.mod h1:lfuLI1f4o+0SGtlHQS4x5qsjRcZZfYqG8bp3k8hM0M8=
github.com/crypto-org-chain/cosmos-sdk/x/tx v0.0.0-20240603035522-8d30134159e0 h1:j0AYThWObuRsdrMgww7/jHmpVNRZEKjchwlDe1vOy6s=
github.com/crypto-org-chain/cosmos-sdk/x/tx v0.0.0-20240603035522-8d30134159e0/go.mod h1:CBCU6fsRVz23QGFIQBb1DNX2DztJCf3jWyEkHY2nJQ0=
github.com/crypto-org-chain/ethermint v0.6.1-0.20240502052908-179e436703b3 h1:YYmMJowZyiyioNHYnps5hw3XkV1zcXSC3jy/xzqK2Rg=
github.com/crypto-org-chain/ethermint v0.6.1-0.20240502052908-179e436703b3/go.mod h1:9MVSajfKloRP8h2chP78LhCKx5u9O2pCMBvxrmx6+0s=
github.com/crypto-org-chain/go-block-stm v0.0.0-20240408011717-9f11af197bde h1:sQIHTJfVt5VTrF7po9eZiFkZiPjlHbFvnXtGCOoBjNM=
Expand Down
13 changes: 5 additions & 8 deletions gomod2nix.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ schema = 3
version = "v0.7.3"
hash = "sha256-G72m5tUKLrpl89YKM/S7J/e8rYkqctuDOqHbqxyzTBI="
[mod."cosmossdk.io/client/v2"]
version = "v2.0.0-20240415105151-0108877a3201"
version = "v2.0.0-20240603035522-8d30134159e0"
hash = "sha256-4RYdjGJDIKLHfRE3LHwKmAE1C/JZPHtLMl3Sne7i//g="
replaced = "github.com/crypto-org-chain/cosmos-sdk/client/v2"
[mod."cosmossdk.io/collections"]
Expand All @@ -45,7 +45,7 @@ schema = 3
version = "v0.0.0-20230608160436-666c345ad23d"
hash = "sha256-6BMBA98BpK3jG6++ZE4LdPQwwpS+lZ0GLMRF1fO4UfM="
[mod."cosmossdk.io/store"]
version = "v0.0.0-20240415105151-0108877a3201"
version = "v0.0.0-20240603035522-8d30134159e0"
hash = "sha256-5fFhveNdA4rEwtTVNE4MIzK7udgNF80q9fkiDcw7T/Q="
replaced = "github.com/crypto-org-chain/cosmos-sdk/store"
[mod."cosmossdk.io/tools/confix"]
Expand All @@ -58,7 +58,7 @@ schema = 3
version = "v0.1.0"
hash = "sha256-/gWvrqvy6bW90+NU66T+9QysYgvG1VbwfYJZ8tkqpeA="
[mod."cosmossdk.io/x/tx"]
version = "v0.0.0-20240415105151-0108877a3201"
version = "v0.0.0-20240603035522-8d30134159e0"
hash = "sha256-jhCBzToU7KglOA4CuY16ogOA0dZ4ESX2HWshhgYUjvE="
replaced = "github.com/crypto-org-chain/cosmos-sdk/x/tx"
[mod."cosmossdk.io/x/upgrade"]
Expand Down Expand Up @@ -170,8 +170,8 @@ schema = 3
version = "v1.0.0-beta.4"
hash = "sha256-5Kn82nsZfiEtuwhhLZqmMxdAY1tX/Fi3HJ0/MEaRohw="
[mod."github.com/cosmos/cosmos-sdk"]
version = "v0.0.0-20240415105151-0108877a3201"
hash = "sha256-qgfVUWIH2nX4l339+e1SVQ1iVQkv8Ew4Aj6zcHH2tPg="
version = "v0.0.0-20240603035522-8d30134159e0"
hash = "sha256-Mdw7biEUIzKFD6WQr607ZOoklWJkvR/ZLBN3jexhppA="
replaced = "github.com/crypto-org-chain/cosmos-sdk"
[mod."github.com/cosmos/go-bip39"]
version = "v1.0.0"
Expand Down Expand Up @@ -578,9 +578,6 @@ schema = 3
[mod."github.com/tendermint/go-amino"]
version = "v0.16.0"
hash = "sha256-JW4zO/0vMzf1dXLePOqaMtiLUZgNbuIseh9GV+jQlf0="
[mod."github.com/test-go/testify"]
version = "v1.1.4"
hash = "sha256-8xygO1Rd4eTrmRe/g7zaifpNkeb6EmjNfUvTWbjDtPg="
[mod."github.com/tidwall/btree"]
version = "v0.0.0-20240406140148-2687063b042c"
hash = "sha256-8eDLGHhw4qXG6MEa7w5Q9KLwOobXr8Vn5qqyQhuipQw="
Expand Down
172 changes: 26 additions & 146 deletions x/e2ee/keyring/keyring.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,18 @@
package keyring

import (
"bufio"
"fmt"
"io"
"os"
"path/filepath"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import Note

Certain system packages contain functions which may be a possible source of non-determinism
"unsafe"

Check notice

Code scanning / CodeQL

Sensitive package import Note

Certain system packages contain functions which may be a possible source of non-determinism

"github.com/99designs/keyring"
"golang.org/x/crypto/bcrypt"

errorsmod "cosmossdk.io/errors"
"github.com/cosmos/cosmos-sdk/client/input"
sdkkeyring "github.com/cosmos/cosmos-sdk/crypto/keyring"
)

const (
keyringFileDirName = "e2ee-keyring-file"
keyringTestDirName = "e2ee-keyring-test"
passKeyringPrefix = "e2ee-keyring-%s" //nolint: gosec
maxPassphraseEntryAttempts = 3
)
const keyringDirPrefix = "e2ee-keyring-%s"

type Keyring interface {
Get(string) ([]byte, error)
Expand All @@ -30,60 +22,31 @@
func New(
appName, backend, rootDir string, userInput io.Reader,
) (Keyring, error) {
var (
db keyring.Keyring
err error
)
serviceName := appName + "-e2ee"
switch backend {
case sdkkeyring.BackendMemory:
return newKeystore(keyring.NewArrayKeyring(nil), sdkkeyring.BackendMemory), nil
case sdkkeyring.BackendTest:
db, err = keyring.Open(keyring.Config{
AllowedBackends: []keyring.BackendType{keyring.FileBackend},
ServiceName: serviceName,
FileDir: filepath.Join(rootDir, keyringTestDirName),
FilePasswordFunc: func(_ string) (string, error) {
return "test", nil
},
})
case sdkkeyring.BackendFile:
fileDir := filepath.Join(rootDir, keyringFileDirName)
db, err = keyring.Open(keyring.Config{
AllowedBackends: []keyring.BackendType{keyring.FileBackend},
ServiceName: serviceName,
FileDir: fileDir,
FilePasswordFunc: newRealPrompt(fileDir, userInput),
})
case sdkkeyring.BackendOS:
db, err = keyring.Open(keyring.Config{
ServiceName: serviceName,
FileDir: rootDir,
KeychainTrustApplication: true,
FilePasswordFunc: newRealPrompt(rootDir, userInput),
})
case sdkkeyring.BackendKWallet:
db, err = keyring.Open(keyring.Config{
AllowedBackends: []keyring.BackendType{keyring.KWalletBackend},
ServiceName: "kdewallet",
KWalletAppID: serviceName,
KWalletFolder: "",
})
case sdkkeyring.BackendPass:
prefix := fmt.Sprintf(passKeyringPrefix, serviceName)
db, err = keyring.Open(keyring.Config{
AllowedBackends: []keyring.BackendType{keyring.PassBackend},
ServiceName: serviceName,
PassPrefix: prefix,
})
default:
return nil, errorsmod.Wrap(sdkkeyring.ErrUnknownBacked, backend)
}

if err != nil {
return nil, err
var db keyring.Keyring
if backend == sdkkeyring.BackendMemory {
db = keyring.NewArrayKeyring(nil)

Check warning on line 28 in x/e2ee/keyring/keyring.go

View check run for this annotation

Codecov / codecov/patch

x/e2ee/keyring/keyring.go#L28 

Added line #L28 was not covered by tests
} else {
kr, err := sdkkeyring.New(serviceName, backend, rootDir, userInput, nil)
if err != nil {
return nil, err

Check warning on line 32 in x/e2ee/keyring/keyring.go

View check run for this annotation

Codecov / codecov/patch

x/e2ee/keyring/keyring.go#L32 

Added line #L32 was not covered by tests
}
db = kr.DB()
switch backend {
case sdkkeyring.BackendTest, sdkkeyring.BackendFile:
fileDir := filepath.Join(rootDir, fmt.Sprintf(keyringDirPrefix, backend))
el := reflect.ValueOf(db).Elem()
if f := el.FieldByName("dir"); f.IsValid() {
reflect.NewAt(f.Type(), unsafe.Pointer(f.UnsafeAddr())).Elem().SetString(fileDir)
}
case sdkkeyring.BackendPass:
prefix := fmt.Sprintf(keyringDirPrefix, serviceName)
el := reflect.ValueOf(db).Elem()
if f := el.FieldByName("prefix"); f.IsValid() {
reflect.NewAt(f.Type(), unsafe.Pointer(f.UnsafeAddr())).Elem().SetString(prefix)

Check warning on line 46 in x/e2ee/keyring/keyring.go

View check run for this annotation

Codecov / codecov/patch

x/e2ee/keyring/keyring.go#L42-L46 

Added lines #L42 - L46 were not covered by tests
}
}
Comment on lines +26 to +48
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of reflection and unsafe operations in the New function increases flexibility but also adds complexity and potential risks. Consider alternative approaches that avoid reflection for better maintainability and safety.

}

return newKeystore(db, backend), nil
}

Expand Down Expand Up @@ -117,86 +80,3 @@
Label: name,
})
}

func newRealPrompt(dir string, buf io.Reader) func(string) (string, error) {
return func(prompt string) (string, error) {
keyhashStored := false
keyhashFilePath := filepath.Join(dir, "keyhash")

var keyhash []byte

_, err := os.Stat(keyhashFilePath)

switch {
case err == nil:
keyhash, err = os.ReadFile(keyhashFilePath)
if err != nil {
return "", errorsmod.Wrap(err, fmt.Sprintf("failed to read %s", keyhashFilePath))
}

keyhashStored = true

case os.IsNotExist(err):
keyhashStored = false

default:
return "", errorsmod.Wrap(err, fmt.Sprintf("failed to open %s", keyhashFilePath))
}

failureCounter := 0

for {
failureCounter++
if failureCounter > maxPassphraseEntryAttempts {
return "", sdkkeyring.ErrMaxPassPhraseAttempts
}

buf := bufio.NewReader(buf)
pass, err := input.GetPassword(fmt.Sprintf("Enter keyring passphrase (attempt %d/%d):", failureCounter, maxPassphraseEntryAttempts), buf)
if err != nil {
// NOTE: LGTM.io reports a false positive alert that states we are printing the password,
// but we only log the error.
//
// lgtm [go/clear-text-logging]
fmt.Fprintln(os.Stderr, err)
continue
}

if keyhashStored {
if err := bcrypt.CompareHashAndPassword(keyhash, []byte(pass)); err != nil {
fmt.Fprintln(os.Stderr, "incorrect passphrase")
continue
}

return pass, nil
}

reEnteredPass, err := input.GetPassword("Re-enter keyring passphrase:", buf)
if err != nil {
// NOTE: LGTM.io reports a false positive alert that states we are printing the password,
// but we only log the error.
//
// lgtm [go/clear-text-logging]
fmt.Fprintln(os.Stderr, err)
continue
}

if pass != reEnteredPass {
fmt.Fprintln(os.Stderr, "passphrase do not match")
continue
}

passwordHash, err := bcrypt.GenerateFromPassword([]byte(pass), 2)
if err != nil {
fmt.Fprintln(os.Stderr, err)
continue
}

if err := os.WriteFile(keyhashFilePath, passwordHash, 0o600); err != nil {
return "", err
}

return pass, nil
}
}
}
10 changes: 5 additions & 5 deletions x/e2ee/keyring/keyring_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
package keyring
package keyring_test

import (
"bytes"
"io"
"testing"

"filippo.io/age"
"github.com/test-go/testify/require"

"github.com/cosmos/cosmos-sdk/crypto/keyring"
sdkkeyring "github.com/cosmos/cosmos-sdk/crypto/keyring"
"github.com/crypto-org-chain/cronos/v2/x/e2ee/keyring"
"github.com/stretchr/testify/require"
)

func TestKeyring(t *testing.T) {
kr, err := New("cronosd", keyring.BackendTest, t.TempDir(), nil)
kr, err := keyring.New("cronosd", sdkkeyring.BackendTest, t.TempDir(), nil)
require.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

The function call keyring.New should be updated to sdkkeyring.New to reflect the new import alias and ensure consistency.


identity, err := age.GenerateX25519Identity()
Expand Down
Loading