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

MYST-114 / Message signing and verification #74

Merged
merged 30 commits into from Jan 3, 2018

Conversation

Projects
None yet
5 participants
@ignasbernotas
Copy link
Member

commented Dec 28, 2017

No description provided.

@@ -5,4 +5,7 @@ import "github.com/ethereum/go-ethereum/accounts"
type keystoreInterface interface {
Accounts() []accounts.Account
NewAccount(passphrase string) (accounts.Account, error)
Find(a accounts.Account) (accounts.Account, error)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

Could You start using keystore.Find() in manager.GetIdentity() as-well?

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 28, 2017

Author Member

Yeah, good idea, cheers!

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 28, 2017

Author Member

should fakeKeystore.Find implement the actual find through fake []Accounts? Otherwise the tests fail, but seems redundant to kind of do it.

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 29, 2017

Author Member

Fixed.

keystoreManager := idm.keystoreManager
accountExisting := identityToAccount(identity)

account, err := keystoreManager.Find(accountExisting)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

What if I already have "found" identity which I retrieved with manager.GetIdentity()?

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 28, 2017

Author Member

What if you didn't? :D

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

Seems like we have 2 structures:

  • IdentityAddress or IdentityId - just unique string 0x4e878562376372CCa10397245CBc2950d42246e1, could point to another node
  • Identity or IdentityWallet- which is my controlled identity from my owned keystore. With Lock/Unlock functions
}

func (idm *identityManager) IsUnlocked(identity Identity) bool {
_, err := idm.keystoreManager.SignHash(identityToAccount(identity), messageHash([]byte("1")))

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member
  • Strange that readonly function does write action
  • I would skip implementing IsUnlocked flag without real usecase

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 28, 2017

Author Member

I agree, it just seemed like we will need something like this and this will be more friendly than err != keystore.ErrLocked. Plus we won't need to use keystore package externally for these checks:)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

And if finally we do have isLocked flag, we should read it from Ethereum library, see keystoreWallet.Status()

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 28, 2017

Member

If this IsUnlocked functionality already in wallet then we are good to go.
Use case:

  1. Consumer gets list of identities from tequila endoint
  2. Consumer needs to decide some how if he needs to unlock identity he wants to use for connection. Calling unlock everytime when he wants to use it - sound a bit overhead? lock-status subresource with locked : true| false would be a nice indication in tequila endpoint

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 29, 2017

Author Member

keystoreWallet is private to the keystore package as far as I can see. Have you seen it exposed through some function?

manager := NewIdentityManager(ks)
manager.Unlock(identity, "test_passphrase")

signer := manager.GetSigner(identity)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

You dont need full manager to test only signer.Sign()

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 28, 2017

Author Member

Accounts still need to be unlocked. How would you approach this?

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

Usualy tests skip regular factory New...
And loads just dependencies which are tested:

keystore := getIdentityTestKeystore()
keystore.Unlock("0x1e35193c8cadAA15b43B05ae3D882C91F49BB0Aa")

signer := keystoreSigner{keystore: keystore}

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 29, 2017

Author Member

Fixed

}

type keystoreSigner struct {
keystoreManager keystoreInterface

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

Just noticed. Signer reminds very much structure keystoreWallet from go-ethereum/accounts/accounts

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 29, 2017

Author Member

Yep, but keystoreWallet is private.

"github.com/ethereum/go-ethereum/accounts"
"github.com/stretchr/testify/assert"
"github.com/ethereum/go-ethereum/accounts/keystore"

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

This unit was using mocked keystore, we should not need Ethereum library and real keystore with filesystem.

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

If do, would be nice to separate such tests to integration_test.go

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 28, 2017

Author Member

We talked about this with @tadovas. It seemed logical to have some sort of checking whether the underlying ethereum keystore works. What if we update the eth keystore package and something breaks? We won't find out about it with mocks.

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

Yes, agree on such integration test:

  • 1 integration with happy path
  • 10 units with rare edge cases
}{
"0x53a835143c0eF3bBCBFa796D7EB738CA7dd28f68",
"123",
}

This comment has been minimized.

Copy link
@donce

donce Dec 28, 2017

Contributor

Is it a common practice to define test data struct even in a single case test?

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 28, 2017

Author Member

Probably unnecessary:)

}

type keystoreSigner struct {
keystoreManager keystoreInterface

This comment has been minimized.

Copy link
@donce

donce Dec 28, 2017

Contributor

If it satisfies keystoreInterface interface (and not keystoreManagerInterface), maybe Manager in keystoreManager variable name is redundant?

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

Yeah keystore is enough

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 29, 2017

Author Member

Fixed now.

return false
}
recoveredAddress := crypto.PubkeyToAddress(*crypto.ToECDSAPub(pk)).Hex()
return FromAddress(recoveredAddress) == ev.peerIdentity

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

Just notice. Would be possible to drop attribute peerIdentity,
by changing interface:
Verify(message []byte, signature []byte) bool
->
GetSigner(message []byte, signature []byte) Identity

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 28, 2017

Member

I thought about this approach, but then verification from caller side would be a bit clumsy. Also comparing extracted identity with given identity would require a) have a reference to expected identity b) compare it everywhere verification is needed. I think bool return value is cleaner indication that verification succeeded or not

// messageHash is a helper function that calculates a hash for the given message that can be
// safely used to calculate a signature from.
//
// The hash is calulcated as

This comment has been minimized.

Copy link
@donce

donce Dec 28, 2017

Contributor

calculated

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 29, 2017

Author Member

Fixed.

t,
hex.EncodeToString(sig),
"51e8c02f544c20a5b5b92894ffdd4dad90a71d994cad608cb3157b9ed7757de2758b9c0fc51bfaf079a4d60e81cc83c14cf6900c9bc031ba3f44cb119b82a5f000",
)

This comment has been minimized.

Copy link
@donce

donce Dec 28, 2017

Contributor

Second argument should be expected, and the third actual, not in reverse (https://godoc.org/github.com/stretchr/testify/assert#Equal):

func Equal(t TestingT, expected, actual interface{}, msgAndArgs ...interface{}) bool

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 29, 2017

Author Member

Fixed

privateKey, err := crypto.GenerateKey()
assert.NoError(t, err)

signature, err := crypto.Sign(crypto.Keccak256(message), privateKey)

This comment has been minimized.

Copy link
@donce

donce Dec 28, 2017

Contributor

If I'm not mistaken, we could re-use messageHash here instead of crypto.Keccak256, as in TestVerifyFunctionReturnsTrueWhenSignatureIsCorrect. That it would be easier to see the difference between these two tests - only different parts will be different.

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 28, 2017

Member

Missed that. Good point.

}

func (ksSigner *keystoreSigner) Sign(message []byte) ([]byte, error) {
signature, err := ksSigner.keystoreManager.SignHash(identityToAccount(ksSigner.identity), messageHash(message))

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 28, 2017

Member

If I am signing 100 messages, same identityToAccount is called several times for variable self.identity is is always same

return err
}

return nil

This comment has been minimized.

Copy link
@donce

donce Dec 29, 2017

Contributor

Instead of these 5 lines of code, we could just return result of Unlock:

return keystoreManager.Unlock(account, passphrase)

Unless you think it's good to keep err != nil check here for consistency?

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 29, 2017

Author Member

If we decide to get rid of IsUnlocked method then maybe we could abstract some of the errors with our own? So that packages using this manager wouldn't have to include ethereum's keystore package just for error constants.

ignasbernotas and others added some commits Dec 29, 2017

Important! Hash message as it is - do not put it into string with spe…
…cific data, it's very confusing and not very portable
Merge branch 'master' of github.com:MysteriumNetwork/node into featur…
…e/MYST-114-sign-com-channel-messages

* 'master' of github.com:MysteriumNetwork/node:
  review fixes, naming mainly
  fixing tests naming
  readability in client_fake.FindProposals
  proposalsTequilaDto{Id, ProviderId, ServiceDefinition{LocationOriginate{ASN}}}
  Improvement - print port number when started serving api requests
  Rename stats method to SendSessionStats
  tequila id.List to return object instead of array
  Use new stats endpoint
  proposals filter by nodeid
  tequila list proposals endpoint
Merge branch 'master' of github.com:MysteriumNetwork/node into featur…
…e/MYST-114-sign-com-channel-messages

* 'master' of github.com:MysteriumNetwork/node:
  Improve naming
  Drop SessionStatsDeprecated DTO
  Stop hardcoding slashes in paths
  Node password as constant
  Improve naming
  Vpn config as string
  Remove common session utils from openvpn package
  Inject all command dependencies thru factory
  Inject VPN session manager selector in command factory
  Openvpn session manager introduced
  Session manager factory
  Session manager implementation moved to openvpn
  Separate session DTO from internal session structure
  Overide command variables thru factory
  Remove output variables from commands
  Inject VPN config in command factory
  Inject identity selector in command factory
  Stats sending fails with autoloaded identity
  Node creates identity on each run
  Clarify Identity manager naming

@Waldz Waldz requested review from donce and zolia Jan 3, 2018

@tadovas

tadovas approved these changes Jan 3, 2018

Copy link
Member

left a comment

LGTM

@Waldz Waldz merged commit c984d31 into master Jan 3, 2018

@Waldz Waldz deleted the feature/MYST-114-sign-com-channel-messages branch Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.