Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

change interfaces for the security-in-multiaddr change #215

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

change interfaces for the security-in-multiaddr change #215

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
4f3d531
return ma.Protocols from Transport.Protocols
marten-seemann Sep 19, 2021
f829c09
add a Protocol method to the SecureTransport interface
marten-seemann Sep 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Expand Up @@ -13,7 +13,7 @@ require (
github.com/libp2p/go-openssl v0.0.7
github.com/minio/sha256-simd v0.1.1
github.com/mr-tron/base58 v1.2.0
github.com/multiformats/go-multiaddr v0.2.2
github.com/multiformats/go-multiaddr v0.4.1
github.com/multiformats/go-multihash v0.0.14
github.com/multiformats/go-varint v0.0.6
github.com/stretchr/testify v1.7.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Expand Up @@ -46,8 +46,8 @@ github.com/multiformats/go-base32 v0.0.3 h1:tw5+NhuwaOjJCC5Pp82QuXbrmLzWg7uxlMFp
github.com/multiformats/go-base32 v0.0.3/go.mod h1:pLiuGC8y0QR3Ue4Zug5UzK9LjgbkL8NSQj0zQ5Nz/AA=
github.com/multiformats/go-base36 v0.1.0 h1:JR6TyF7JjGd3m6FbLU2cOxhC0Li8z8dLNGQ89tUg4F4=
github.com/multiformats/go-base36 v0.1.0/go.mod h1:kFGE83c6s80PklsHO9sRn2NCoffoRdUUOENyW/Vv6sM=
github.com/multiformats/go-multiaddr v0.2.2 h1:XZLDTszBIJe6m0zF6ITBrEcZR73OPUhCBBS9rYAuUzI=
github.com/multiformats/go-multiaddr v0.2.2/go.mod h1:NtfXiOtHvghW9KojvtySjH5y0u0xW5UouOmQQrn6a3Y=
github.com/multiformats/go-multiaddr v0.4.1 h1:Pq37uLx3hsyNlTDir7FZyU8+cFCTqd5y1KiM2IzOutI=
github.com/multiformats/go-multiaddr v0.4.1/go.mod h1:3afI9HfVW8csiF8UZqtpYRiDyew8pRX7qLIGHu9FLuM=
github.com/multiformats/go-multibase v0.0.3 h1:l/B6bJDQjvQ5G52jw4QGSYeOTZoAwIO77RblWplfIqk=
github.com/multiformats/go-multibase v0.0.3/go.mod h1:5+1R4eQrT3PkYZ24C3W2Ue2tPwIdYQD509ZjSb5y9Oc=
github.com/multiformats/go-multihash v0.0.13/go.mod h1:VdAWLKTwram9oKAatUcLxBNUjdtcVwxObEQBtRfuyjc=
Expand Down
40 changes: 24 additions & 16 deletions sec/insecure/insecure.go
View file
Open in desktop
Expand Up @@ -9,12 +9,13 @@ import (
"io"
"net"

"github.com/libp2p/go-libp2p-core/crypto"
"github.com/libp2p/go-libp2p-core/peer"
"github.com/libp2p/go-libp2p-core/sec"
"github.com/libp2p/go-msgio"

ci "github.com/libp2p/go-libp2p-core/crypto"
pb "github.com/libp2p/go-libp2p-core/sec/insecure/pb"

"github.com/libp2p/go-msgio"
ma "github.com/multiformats/go-multiaddr"
)

// ID is the multistream-select protocol ID that should be used when identifying
Expand All @@ -28,28 +29,36 @@ const ID = "/plaintext/2.0.0"
// No authentication of the remote identity is performed.
type Transport struct {
id peer.ID
key ci.PrivKey
key crypto.PrivKey
Copy link
Member

Choose a reason for hiding this comment

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

The reason we use a different import alias is to differentiate from the Go SDK crypto. I don't have strong opinions either way, just noting it.

}

var _ sec.SecureTransport = &Transport{}

// NewWithIdentity constructs a new insecure transport. The provided private key
// is stored and returned from LocalPrivateKey to satisfy the
// SecureTransport interface, and the public key is sent to
// remote peers. No security is provided.
func NewWithIdentity(id peer.ID, key ci.PrivKey) *Transport {
func NewWithIdentity(id peer.ID, key crypto.PrivKey) *Transport {
return &Transport{
id: id,
key: key,
}
}

var protoPlaintextV2 = ma.ProtocolWithCode(ma.P_PLAINTEXTV2)

func (t *Transport) Protocol() ma.Protocol {
return protoPlaintextV2
}

// LocalPeer returns the transport's local peer ID.
func (t *Transport) LocalPeer() peer.ID {
return t.id
}

// LocalPrivateKey returns the local private key.
// This key is used only for identity generation and provides no security.
func (t *Transport) LocalPrivateKey() ci.PrivKey {
func (t *Transport) LocalPrivateKey() crypto.PrivKey {
return t.key
}

Expand Down Expand Up @@ -114,12 +123,14 @@ type Conn struct {
local peer.ID
remote peer.ID

localPrivKey ci.PrivKey
remotePubKey ci.PubKey
localPrivKey crypto.PrivKey
remotePubKey crypto.PubKey
}

func makeExchangeMessage(pubkey ci.PubKey) (*pb.Exchange, error) {
keyMsg, err := ci.PublicKeyToProto(pubkey)
var _ sec.SecureConn = &Conn{}

func makeExchangeMessage(pubkey crypto.PubKey) (*pb.Exchange, error) {
keyMsg, err := crypto.PublicKeyToProto(pubkey)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -153,7 +164,7 @@ func (ic *Conn) runHandshakeSync() error {
}

// Pull remote ID and public key from message
remotePubkey, err := ci.PublicKeyFromProto(remoteMsg.Pubkey)
remotePubkey, err := crypto.PublicKeyFromProto(remoteMsg.Pubkey)
if err != nil {
return err
}
Expand Down Expand Up @@ -221,14 +232,11 @@ func (ic *Conn) RemotePeer() peer.ID {

// RemotePublicKey returns whatever public key was given by the remote peer.
// Note that no verification of ownership is done, as this connection is not secure.
func (ic *Conn) RemotePublicKey() ci.PubKey {
func (ic *Conn) RemotePublicKey() crypto.PubKey {
return ic.remotePubKey
}

// LocalPrivateKey returns the private key for the local peer.
func (ic *Conn) LocalPrivateKey() ci.PrivKey {
func (ic *Conn) LocalPrivateKey() crypto.PrivKey {
return ic.localPrivKey
}

var _ sec.SecureTransport = (*Transport)(nil)
var _ sec.SecureConn = (*Conn)(nil)
5 changes: 5 additions & 0 deletions sec/security.go
View file
Open in desktop
Expand Up @@ -7,6 +7,8 @@ import (

"github.com/libp2p/go-libp2p-core/network"
"github.com/libp2p/go-libp2p-core/peer"

ma "github.com/multiformats/go-multiaddr"
)

// SecureConn is an authenticated, encrypted connection.
Expand All @@ -24,6 +26,9 @@ type SecureTransport interface {

// SecureOutbound secures an outbound connection.
SecureOutbound(ctx context.Context, insecure net.Conn, p peer.ID) (SecureConn, error)

// Protocol returns the handshake protocol used by this SecureTransport.
Protocol() ma.Protocol
}

// A SecureMuxer is a wrapper around SecureTransport which can select security protocols
Expand Down
6 changes: 3 additions & 3 deletions transport/transport.go
View file
Open in desktop
Expand Up @@ -75,10 +75,10 @@ type Transport interface {
// Listen listens on the passed multiaddr.
Listen(laddr ma.Multiaddr) (Listener, error)

// Protocol returns the set of protocols handled by this transport.
//
// Protocols returns the set of protocols handled by this transport.
// If protocols A and B are returned, this means that this transport supports running B on top of A.
Copy link
Member

Choose a reason for hiding this comment

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

Is this a semantic change, or was the previous interpretation supposed to be stacked too?

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, I just read the PR description. I think we will find this interface limiting, and suggest something more sophisticated. [][]protocol.ID could be an option, but we might want to go with a struct for more developer friendliness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s correct, it is a semantic change (as I’ve noted in the PR description). None of our transports ever returned more than one value here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just read your 2nd comment after posting my reply. We could define a ProtocolStack as []Protocol, and then return a []ProtocolStack here.

One example where returning multiple protocols might be useful is when we mint a new code point for a new QUIC version. On the other hand, one might argue that if two code points are warranted, the differences between the two transports are probably large enough to just initializing two objects.

Copy link
Member

Choose a reason for hiding this comment

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

That would be more self-descriptive, yes. Not sure if ProtocolStack is the right term -- since in the past we've talked about libp2p being a factory of stacks with different characteristics and API shapes/semantics (e.g. message orientation). So that might get overloaded in the future. Perhaps ProtocolSeq?

Copy link
Member

@raulk raulk Sep 23, 2021 •
edited

Choose a reason for hiding this comment

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

Also, the term "stack" to me denotes some kind of behaviour associated with that type, versus choosing a keyword for a type with the very limited purpose of enabling AND and OR combinations of protocols IDs.

// See the Network interface for an explanation of how this is used.
Protocols() []int
Protocols() []ma.Protocol

// Proxy returns true if this is a proxy transport.
//
Expand Down