Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
242 changes: 242 additions & 0 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@ package multistream

import (
"bytes"
"crypto/rand"
"encoding/base64"
"errors"
"io"
"math/big"
"strings"
)

// ErrNotSupported is the error returned when the muxer does not support
Expand Down Expand Up @@ -74,6 +78,244 @@ func SelectOneOf(protos []string, rwc io.ReadWriteCloser) (string, error) {
return "", ErrNotSupported
}

// Performs protocol negotiation with the simultaneous open extension; the returned boolean
// indicator will be true if we should act as a server.
func SelectWithSimopen(protos []string, rwc io.ReadWriteCloser) (string, bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: SelectProtoOrFailSimultanious, or something like that.

  • It's unclear that this is "select proto or fail".
  • We're not paying by the letter.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is done.

if len(protos) == 0 {
return "", false, ErrNoProtocols
}

werrCh := make(chan error, 1)
go func() {
var buf bytes.Buffer
delimWrite(&buf, []byte(ProtocolID))
delimWrite(&buf, []byte("iamclient"))
delimWrite(&buf, []byte(protos[0]))
_, err := io.Copy(rwc, &buf)
werrCh <- err
}()

err := readMultistreamHeader(rwc)
if err != nil {
return "", false, err
}

tok, err := ReadNextToken(rwc)
if err != nil {
return "", false, err
}

if err = <-werrCh; err != nil {
return "", false, err
}

switch tok {
case "iamclient":
Copy link
Member

Choose a reason for hiding this comment

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

nit: define a string constant.

// simultaneous open
return simOpen(protos, rwc)

case "na":
// client open
proto, err := clientOpen(protos, rwc)
if err != nil {
return "", false, err
}
Comment on lines +120 to +122
Copy link
Member

Choose a reason for hiding this comment

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

nit: do we need this if err? Or can we just return proto, false, err.


return proto, false, nil

default:
return "", false, errors.New("unexpected response: " + tok)
}
}

func clientOpen(protos []string, rwc io.ReadWriteCloser) (string, error) {
// check to see if we selected the pipelined protocol
tok, err := ReadNextToken(rwc)
if err != nil {
return "", err
}

switch tok {
case protos[0]:
return tok, nil
case "na":
// try the other protos
for _, p := range protos[1:] {
err = trySelect(p, rwc)
switch err {
case nil:
return p, nil
case ErrNotSupported:
default:
return "", err
}
}

return "", ErrNotSupported
default:
return "", errors.New("unexpected response: " + tok)
Copy link
Member

Choose a reason for hiding this comment

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

nit: fmt.Errorf("unexpected response: %s", tok) is technically more idiomatic (although it really doesn't matter).

}
}

func simOpen(protos []string, rwc io.ReadWriteCloser) (string, bool, error) {
retries := 3
Copy link
Member

Choose a reason for hiding this comment

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

There's really no point in re-trying, or even building that into the protocol. If we collide, our randomness sources are likely identical and we'll never finish.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, the likelihood is abysmal. Have removed this.


again:
mynonce := make([]byte, 32)
_, err := rand.Read(mynonce)
if err != nil {
return "", false, err
}

werrCh := make(chan error, 1)
go func() {
myselect := []byte("select:" + base64.StdEncoding.EncodeToString(mynonce))
err := delimWriteBuffered(rwc, myselect)
werrCh <- err
}()

var peerselect string
for {
tok, err := ReadNextToken(rwc)
if err != nil {
return "", false, err
}

// this skips pipelined protocol negoatiation
// keep reading until the token starts with select:
if strings.HasPrefix(tok, "select:") {
Copy link
Member

Choose a reason for hiding this comment

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

We should skip exactly one protocol. Otherwise, if someone names their protocol select:, we'll have problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Stebalien I am not sure what you mean here.

Are you talking about the exact scenario where a peer sends "mutlsitream"|"iamclient"|"select:"|"select:nonce"

where that first "select:" is a security protocol the user wants to negotiate ? I agree this scenario will create problems.

But, is this the ONLY scenario you have in mind ? Does this really warrant fixing ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's the scenario I have in mind. My concern isn't really "fixing a bug" but rather "simplifying the spec". If we go with the current approach, the multistream-select spec would need to say "protocols beginning with select: are reserved" and "iamclient" would become an integral part of the spec. If we simply skip exactly one protocol, we can define "iamclient" to be it's own protocol.

The "iamclient" (maybe /bootstrap/simultaneous?) protocol:

  1. Send exactly one preferred protocol.
  2. Send exactly one select:nonce message to pick the winner.
  3. The winner recursively uses multistream-select to pick the final protocol. NOTE: this would require sending a new multistream header as well.

The beauty of this approach is that there's nothing inherently special about the "iamclient" protocol from a spec standpoint. The implementation can take advantage of the multistream-select ambiguity and pipeline "iamclient" with the "preferred protocol" from step 1, but that's not required. A simpler implementation could just:

  1. Negotiate one of "iamclient" or a set of other security transports (one round-trip per step).
  2. If "iamclient" is selected, send the preferred protocol and a nonce.
  3. Once the winner is picked, the winner negotiates their chosen protocol, potentially using the "preferred" protocol.

This version takes an extra round-trip but can be implemented as two compossible protocols. Later, as the libp2p implementation matures, it can be re-implemented to remove the extra round-trip without changing the spec.

Copy link
Member

Choose a reason for hiding this comment

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

TL;DR: it allows us to write the spec as follows:

  1. Multistream protocol.
  2. "iamclient" protocol for selecting a initiator when a simultaneous connect happens.
  3. Optional: oh, by the way, you can combine these as follows ... to save a round-trip.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Stebalien This is done.

peerselect = tok
break
}
}

if err = <-werrCh; err != nil {
return "", false, err
}

peernonce, err := base64.StdEncoding.DecodeString(peerselect[7:])
if err != nil {
return "", false, err
}

var mybig, peerbig big.Int
var iamserver bool
Copy link
Member

Choose a reason for hiding this comment

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

nit: drop this line.

mybig.SetBytes(mynonce)
peerbig.SetBytes(peernonce)

switch mybig.Cmp(&peerbig) {
case -1:
// peer nonce bigger, he is client
iamserver = true

case 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

You actually don't need this case, iamserver is already initialized to false.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this helps readability of the code a bit, so keeping it around.

// my nonce bigger, i am client
iamserver = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably nicer:

if peerNonce == myNonce {
   return ...
}
iamserver := peerNonce > myNonce

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.


case 0:
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This is done.

// wtf, the world is ending! try again.
if retries > 0 {
retries--
goto again
}

return "", false, errors.New("failed client selection; identical nonces!")

default:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need a default when using bytes.Compare.

Copy link
Contributor

Choose a reason for hiding this comment

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

This has been removed.

return "", false, errors.New("wut? bigint.Cmp returned unexpected value")
}

var proto string
if iamserver {
proto, err = simOpenSelectServer(protos, rwc)
} else {
proto, err = simOpenSelectClient(protos, rwc)
}

return proto, iamserver, err
}

func simOpenSelectServer(protos []string, rwc io.ReadWriteCloser) (string, error) {
werrCh := make(chan error, 1)
go func() {
err := delimWriteBuffered(rwc, []byte("responder"))
werrCh <- err
}()

tok, err := ReadNextToken(rwc)
if err != nil {
return "", err
}
if tok != "initiator" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make this a const? I see you're using it a few times.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is done for both initiator and responder.

return "", errors.New("unexpected response: " + tok)
}
if err = <-werrCh; err != nil {
return "", err
}

for {
tok, err = ReadNextToken(rwc)

if err == io.EOF {
return "", ErrNotSupported
Copy link
Contributor

Choose a reason for hiding this comment

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

@vyzo When can this EOF happen ? I mean, why do we treat this error separately ?

Copy link
Member

Choose a reason for hiding this comment

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

EOF will happen when the remote side gives up because we have no protocols in common. In general, it's best to avoid returning EOF for actual error cases anyways (usually, you want to either return ErrUnexpectedEOF, or a better error like we're doing here).

}

if err != nil {
return "", err
}

for _, p := range protos {
if tok == p {
err = delimWriteBuffered(rwc, []byte(p))
if err != nil {
return "", err
}

return p, nil
}
}

err = delimWriteBuffered(rwc, []byte("na"))
if err != nil {
return "", err
}
}

}

func simOpenSelectClient(protos []string, rwc io.ReadWriteCloser) (string, error) {
werrCh := make(chan error, 1)
go func() {
err := delimWriteBuffered(rwc, []byte("initiator"))
werrCh <- err
}()

tok, err := ReadNextToken(rwc)
if err != nil {
return "", err
}
if tok != "responder" {
return "", errors.New("unexpected response: " + tok)
}
if err = <-werrCh; err != nil {
return "", err
}

for _, p := range protos {
err = trySelect(p, rwc)
switch err {
case nil:
return p, nil

case ErrNotSupported:
default:
return "", err
}
}

return "", ErrNotSupported
}

func handshake(rw io.ReadWriter) error {
errCh := make(chan error, 1)
go func() {
Expand Down
Loading