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 CommunicationChannel. Sign all payload messages #87

Merged
merged 15 commits into from Jan 10, 2018

Conversation

Projects
None yet
5 participants
@Waldz
Copy link
Member

commented Jan 8, 2018

  • Communication packer which packs with signature
  • Communication unpacker which unpacks with signature validation
  • 2 different verifiers: VerifyIsAuthorized() and NewVerifyIdentity("0x1234..")
@shroomist
Copy link
Contributor

left a comment

lgtm

}

func NewVerifier(peerIdentity Identity) *ethereumVerifier {
return &ethereumVerifier{peerIdentity}
type verifyIsAuthorized struct{}

This comment has been minimized.

Copy link
@shroomist

shroomist Jan 8, 2018

Contributor

confusing naming for struct with no fields.
VerifyIsAuthorized doesn't make much sense to me.

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Contributor

AuthorizationVerifier?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Author Member

Renamed

)

type Verifier interface {
Verify(message []byte, signature Signature) bool
}

type ethereumVerifier struct {
peerIdentity Identity
func NewVerifyIsAuthorized() *verifyIsAuthorized {

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 8, 2018

Member

"verify" english verb, verifyIsAuthorized - verify action is allowed ? This name is very confusing

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Author Member

Renamed to NewVerifierSigned and NewVerifierIdentity

}

func (ev *ethereumVerifier) Verify(message []byte, signature Signature) bool {
recoveredKey, err := crypto.Ecrecover(messageHash(message), signature.Bytes())
func (ev *verifyIdentity) Verify(message []byte, signature Signature) bool {

This comment has been minimized.

Copy link
@donce

donce Jan 8, 2018

Contributor

ev *verifyIdentity name doesn't make sense anymore - it should be vi or something longer.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Author Member

Fixed

@@ -2,26 +2,52 @@ package identity

import (
"github.com/ethereum/go-ethereum/crypto"
"github.com/syndtr/goleveldb/leveldb/errors"

This comment has been minimized.

Copy link
@donce

donce Jan 8, 2018

Contributor

Are you sure you want to use this package and not "errors"?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Author Member

Thanks, fixed

contactAddressFactory func(contact dto_discovery.Contact) (*nats_discovery.NatsAddress, error)
}

func (establisher *dialogEstablisher) CreateDialog(contact dto_discovery.Contact) (communication.Dialog, error) {
var dialog *dialog

This comment has been minimized.

Copy link
@donce

donce Jan 8, 2018

Contributor

Adding this variable at the beginning of method and using it before initializing does not improve readability IMO:

var dialog *dialog
...
return dialog, fmt.Errorf("Failed to connect to: %#v. %s", contact, err)

This way, you have to run through the whole method in order to understand that dialog is nil.
return nil, error is more readable than return dialog, error, when dialog is was not initialised before (as it was before change):

...
return nil, fmt.Errorf("Failed to connect to: %#v. %s", contact, err)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Author Member

Returning nil is unsafe and is kinda agains Golang idealogies.
It's such a practice that method ALWAYS return that he promised to return

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Author Member

This is exactly why higher layed can do tricks like:

dialogInstance, err := establisher.CreateDialog()
defer dialogInstance.Close()
}
}

type codecSigner struct {

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Contributor

This name hides that it not only signs outgoing messages, but also verifies incoming ones.
Maybe SignedCodec abstracts sign+verify better?

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 9, 2018

Member

What about SecuredCodec ?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Author Member

Renamed to codecSecured

@donce
Copy link
Contributor

left a comment

LGTM, only minor comments.

verifier identity.Verifier
}

func (codec *codecSigner) Pack(payloadPtr interface{}) ([]byte, error) {

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Contributor

payloadPtr refers that it's a pointer? Maybe we can come up with some better naming alternative for interface{}?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Author Member

We have special ticket for that MYST-226

err := codec.Unpack([]byte(tt.data), &payload)

assert.NoError(t, err)
assert.Exactly(t, tt.expectedPayload, payload)

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Contributor

These last 2 lines seem identical in each case - maybe we can move them just after all case statements?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Author Member

Tried, but cant fix that :/
because var payload string|boolean|customPayload needs to be defined earlier then I dont know type yet

`{
"payload": "hello \"name\""
}`,
"Invalid message signature: ",

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Contributor

Maybe it's worth to add "" around signature in error message? Then this message would look like this: Invalid message signature: "". Current message with empty signature is confusing.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Author Member

Improved

}

func NewVerifier(peerIdentity Identity) *ethereumVerifier {
return &ethereumVerifier{peerIdentity}
type verifyIsAuthorized struct{}

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Contributor

AuthorizationVerifier?

type VerifierFake struct{}

func (verifier *VerifierFake) Verify(message []byte, signature Signature) bool {
signatureExpected := "signed" + string(message)

This comment has been minimized.

Copy link
@donce

donce Jan 9, 2018

Contributor

This is coupled to fakeSigner, because signature generation is duplicated - changing one requires changing another. Maybe we can extract this somewhere?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Author Member

Moved to reusable hash function

assert.Exactly(t, Identity{}, signerId)
}

func TestVerifier_extractSignerWhenMessageIsChanged(t *testing.T) {

This comment has been minimized.

Copy link
@zolia

zolia Jan 9, 2018

Member

I think "IsChanged" in the name does not reflect the actual test. I would expect an error test here.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 9, 2018

Author Member

Improved naming.
That's exactly what I checkin g - if attacker changes a payload body

signature := SignatureHex("1f89542f406b2d638fe09cd9912d0b8c0b5ebb4aef67d52ab046973e34fb430a1953576cd19d140eddb099aea34b2985fbd99e716d3b2f96a964141fdb84b32000")

verifier := NewVerifyIsAuthorized()
assert.True(t, verifier.Verify(message, signature))

This comment has been minimized.

Copy link
@zolia

zolia Jan 9, 2018

Member

Should't here be assert.False? Since WrongSender test?

@Waldz Waldz force-pushed the feature/MYST-114-com-signer branch from e1cda84 to 2090509 Jan 9, 2018

@Waldz

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

All fixes have been done

@tadovas
Copy link
Member

left a comment

LGTM

@Waldz Waldz merged commit 757e18e into master Jan 10, 2018

@Waldz Waldz deleted the feature/MYST-114-com-signer branch Jan 11, 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.