noise: add an option to allow unknown peer ID in SecureOutbound #1823
Conversation
|
Can we make this a separate function, that has a very clear warning of the risks. Dialing nodes without a peer ID makes you susceptible to MITM attacks, see discussion in libp2p/specs#458. |
|
@marten-seemann Could you take another look at this? |
p2p/security/noise/handshake.go
Outdated
| // if (s.initiator && s.remoteID != id) || (!s.initiator && s.remoteID != "" && s.remoteID != id) { | ||
| // // use Pretty() as it produces the full b58-encoded string, rather than abbreviated forms. | ||
| // return nil, fmt.Errorf("peer id mismatch: expected %s, but remote key matches %s", s.remoteID.Pretty(), id.Pretty()) | ||
| // } |
There was a problem hiding this comment.
No commented code please.
@marten-seemann I considered it but wanted to adhere strictly to the |
ckousik
force-pushed
the
ckousik/initiator-noise-handshake
branch
from
039bd70
to
a2913e6
Compare
|
@marten-seemann I want to keep the interface consistent between |
|
Sorry for the long response time here, and for changing my mind. I think we should indeed go with an option, implementing this method on both the |
marten-seemann
changed the title
p2p/security/noise/transport.go
Outdated
| func (t *Transport) WithSessionOptions(opts ...SessionOption) (sec.SecureTransport, error) { | ||
| st := &SessionTransport{t: t} | ||
| func (t *Transport) WithSessionOptions(opts ...SessionOption) (*SessionTransport, error) { | ||
| st := &SessionTransport{t: t, disablePeerIdCheck: false} |
There was a problem hiding this comment.
| st := &SessionTransport{t: t, disablePeerIdCheck: false} | |
| st := &SessionTransport{t: t} |
|
|
||
| initiatorEarlyDataHandler, responderEarlyDataHandler EarlyDataHandler | ||
| } | ||
|
|
||
| // SecureInbound runs the Noise handshake as the responder. | ||
| // If p is empty, connections from any peer are accepted. | ||
| func (i *SessionTransport) SecureInbound(ctx context.Context, insecure net.Conn, p peer.ID) (sec.SecureConn, error) { | ||
| c, err := newSecureSession(i.t, ctx, insecure, p, i.prologue, i.initiatorEarlyDataHandler, i.responderEarlyDataHandler, false) | ||
| c, err := newSecureSession(i.t, ctx, insecure, p, i.prologue, i.initiatorEarlyDataHandler, i.responderEarlyDataHandler, false, p != "" || i.disablePeerIdCheck) |
There was a problem hiding this comment.
@ckousik Why are you setting checkPeerID = true when i.disalbePeerIdCheck?
There was a problem hiding this comment.
This is to keep behaviour consistent with setting an empty string as peer id.
There was a problem hiding this comment.
Why not p != "" || !i.disablePeerIDCheck?
There was a problem hiding this comment.
I added a quick test and the correct conditional here was p != "" && !i.disablePeerIDCheck, since on the inbound call we will only check the peer ID if the ID provided in the argument is not empty, and the ID check is not disabled.
| var _ sec.SecureTransport = &SessionTransport{} | ||
|
|
||
| // SessionTransport can be used | ||
| // to provide per-connection options | ||
| type SessionTransport struct { | ||
| t *Transport | ||
| // options | ||
| prologue []byte | ||
| prologue []byte | ||
| disablePeerIdCheck bool |
There was a problem hiding this comment.
| disablePeerIdCheck bool | |
| disablePeerIDCheck bool |
Allows the
SecureOutboundcall to be called with an unknown peer ID (empty string). This is required for libp2p/specs#412 (comment)