Merge pull request #17 from libp2p/peer-verification

improve peer verification

p2p/security/tls/conn.go

@@ -4,18 +4,18 @@ import (
 	"crypto/tls"
 
 	cs "github.com/libp2p/go-conn-security"
-	ic "github.com/libp2p/go-libp2p-crypto"
+	ci "github.com/libp2p/go-libp2p-crypto"
 	peer "github.com/libp2p/go-libp2p-peer"
 )
 
 type conn struct {
 	*tls.Conn
 
 	localPeer peer.ID
-	privKey   ic.PrivKey
+	privKey   ci.PrivKey
 
 	remotePeer   peer.ID
-	remotePubKey ic.PubKey
+	remotePubKey ci.PubKey
 }
 
 var _ cs.Conn = &conn{}
@@ -24,14 +24,14 @@ func (c *conn) LocalPeer() peer.ID {
 	return c.localPeer
 }
 
-func (c *conn) LocalPrivateKey() ic.PrivKey {
+func (c *conn) LocalPrivateKey() ci.PrivKey {
 	return c.privKey
 }
 
 func (c *conn) RemotePeer() peer.ID {
 	return c.remotePeer
 }
 
-func (c *conn) RemotePublicKey() ic.PubKey {
+func (c *conn) RemotePublicKey() ci.PubKey {
 	return c.remotePubKey
 }

p2p/security/tls/crypto.go

@@ -15,30 +15,59 @@ import (
 	peer "github.com/libp2p/go-libp2p-peer"
 )
 
+const certValidityPeriod = 180 * 24 * time.Hour
+
 // Identity is used to secure connections
 type Identity struct {
-	*tls.Config
+	config tls.Config
 }
 
 // NewIdentity creates a new identity
 func NewIdentity(privKey ic.PrivKey) (*Identity, error) {
-	conf, err := generateConfig(privKey)
+	key, cert, err := keyToCertificate(privKey)
 	if err != nil {
 		return nil, err
 	}
-	return &Identity{conf}, nil
+	return &Identity{
+		config: tls.Config{
+			MinVersion:         tls.VersionTLS13,
+			InsecureSkipVerify: true, // This is not insecure here. We will verify the cert chain ourselves.
+			ClientAuth:         tls.RequireAnyClientCert,
+			Certificates: []tls.Certificate{{
+				Certificate: [][]byte{cert.Raw},
+				PrivateKey:  key,
+			}},
+			VerifyPeerCertificate: func(_ [][]byte, _ [][]*x509.Certificate) error {
+				panic("tls config not specialized for peer")
+			},
+		},
+	}, nil
+}
+
+// ConfigForAny is a short-hand for ConfigForPeer("").
+func (i *Identity) ConfigForAny() (*tls.Config, <-chan ic.PubKey) {
+	return i.ConfigForPeer("")
 }
 
-// ConfigForPeer creates a new tls.Config that verifies the peers certificate chain.
-// It should be used to create a new tls.Config before dialing.
-func (i *Identity) ConfigForPeer(remote peer.ID) *tls.Config {
+// ConfigForPeer creates a new single-use tls.Config that verifies the peer's
+// certificate chain and returns the peer's public key via the channel. If the
+// peer ID is empty, the returned config will accept any peer.
+//
+// It should be used to create a new tls.Config before securing either an
+// incoming or outgoing connection.
+func (i *Identity) ConfigForPeer(
+	remote peer.ID,
+) (*tls.Config, <-chan ic.PubKey) {
+	keyCh := make(chan ic.PubKey, 1)
 	// We need to check the peer ID in the VerifyPeerCertificate callback.
 	// The tls.Config it is also used for listening, and we might also have concurrent dials.
 	// Clone it so we can check for the specific peer ID we're dialing here.
-	conf := i.Config.Clone()
+	conf := i.config.Clone()
 	// We're using InsecureSkipVerify, so the verifiedChains parameter will always be empty.
 	// We need to parse the certificates ourselves from the raw certs.
 	conf.VerifyPeerCertificate = func(rawCerts [][]byte, _ [][]*x509.Certificate) error {
+		defer close(keyCh)
+
 		chain := make([]*x509.Certificate, len(rawCerts))
 		for i := 0; i < len(rawCerts); i++ {
 			cert, err := x509.ParseCertificate(rawCerts[i])
@@ -47,49 +76,31 @@ func (i *Identity) ConfigForPeer(remote peer.ID) *tls.Config {
 			}
 			chain[i] = cert
 		}
+
 		pubKey, err := getRemotePubKey(chain)
 		if err != nil {
 			return err
 		}
-		if !remote.MatchesPublicKey(pubKey) {
+		if remote != "" && !remote.MatchesPublicKey(pubKey) {
 			return errors.New("peer IDs don't match")
 		}
+		keyCh <- pubKey
 		return nil
 	}
-	return conf
-}
-
-// KeyFromChain takes a chain of x509.Certificates and returns the peer's public key.
-func KeyFromChain(chain []*x509.Certificate) (ic.PubKey, error) {
-	return getRemotePubKey(chain)
-}
-
-const certValidityPeriod = 180 * 24 * time.Hour
-
-func generateConfig(privKey ic.PrivKey) (*tls.Config, error) {
-	key, cert, err := keyToCertificate(privKey)
-	if err != nil {
-		return nil, err
-	}
-	return &tls.Config{
-		MinVersion:         tls.VersionTLS13,
-		InsecureSkipVerify: true, // This is not insecure here. We will verify the cert chain ourselves.
-		ClientAuth:         tls.RequireAnyClientCert,
-		Certificates: []tls.Certificate{{
-			Certificate: [][]byte{cert.Raw},
-			PrivateKey:  key,
-		}},
-	}, nil
+	return conf, keyCh
 }
 
+// getRemotePubKey derives the remote's public key from the certificate chain.
 func getRemotePubKey(chain []*x509.Certificate) (ic.PubKey, error) {
 	if len(chain) != 1 {
 		return nil, errors.New("expected one certificates in the chain")
 	}
 	pool := x509.NewCertPool()
 	pool.AddCert(chain[0])
 	if _, err := chain[0].Verify(x509.VerifyOptions{Roots: pool}); err != nil {
-		return nil, err
+		// If we return an x509 error here, it will be sent on the wire.
+		// Wrap the error to avoid that.
+		return nil, fmt.Errorf("certificate verification failed: %s", err)
 	}
 	remotePubKey, err := x509.MarshalPKIXPublicKey(chain[0].PublicKey)
 	if err != nil {

p2p/security/tls/transport.go

@@ -3,6 +3,7 @@ package libp2ptls
 import (
 	"context"
 	"crypto/tls"
+	"errors"
 	"net"
 	"os"
 
@@ -34,23 +35,25 @@ func New(key ci.PrivKey) (*Transport, error) {
 	if err != nil {
 		return nil, err
 	}
+	t := &Transport{
+		localPeer: id,
+		privKey:   key,
+	}
+
 	identity, err := NewIdentity(key)
 	if err != nil {
 		return nil, err
 	}
-	return &Transport{
-		identity:  identity,
-		localPeer: id,
-		privKey:   key,
-	}, nil
+	t.identity = identity
+	return t, nil
 }
 
 var _ cs.Transport = &Transport{}
 
 // SecureInbound runs the TLS handshake as a server.
 func (t *Transport) SecureInbound(ctx context.Context, insecure net.Conn) (cs.Conn, error) {
-	serv := tls.Server(insecure, t.identity.Config)
-	return t.handshake(ctx, serv)
+	config, keyCh := t.identity.ConfigForAny()
+	return t.handshake(ctx, tls.Server(insecure, config), keyCh)
 }
 
 // SecureOutbound runs the TLS handshake as a client.
@@ -61,11 +64,14 @@ func (t *Transport) SecureInbound(ctx context.Context, insecure net.Conn) (cs.Co
 // If the handshake fails, the server will close the connection. The client will
 // notice this after 1 RTT when calling Read.
 func (t *Transport) SecureOutbound(ctx context.Context, insecure net.Conn, p peer.ID) (cs.Conn, error) {
-	cl := tls.Client(insecure, t.identity.ConfigForPeer(p))
-	return t.handshake(ctx, cl)
+	config, keyCh := t.identity.ConfigForPeer(p)
+	return t.handshake(ctx, tls.Client(insecure, config), keyCh)
 }
 
-func (t *Transport) handshake(ctx context.Context, tlsConn *tls.Conn,
+func (t *Transport) handshake(
+	ctx context.Context,
+	tlsConn *tls.Conn,
+	keyCh <-chan ci.PubKey,
 ) (cs.Conn, error) {
 	// There's no way to pass a context to tls.Conn.Handshake().
 	// See https://github.com/golang/go/issues/18482.
@@ -92,7 +98,15 @@ func (t *Transport) handshake(ctx context.Context, tlsConn *tls.Conn,
 		}
 		return nil, err
 	}
-	conn, err := t.setupConn(tlsConn)
+
+	// Should be ready by this point, don't block.
+	var remotePubKey ci.PubKey
+	select {
+	case remotePubKey = <-keyCh:
+	default:
+	}
+
+	conn, err := t.setupConn(tlsConn, remotePubKey)
 	if err != nil {
 		// if the context was canceled, return the context error
 		if ctxErr := ctx.Err(); ctxErr != nil {
@@ -103,11 +117,11 @@ func (t *Transport) handshake(ctx context.Context, tlsConn *tls.Conn,
 	return conn, nil
 }
 
-func (t *Transport) setupConn(tlsConn *tls.Conn) (cs.Conn, error) {
-	remotePubKey, err := KeyFromChain(tlsConn.ConnectionState().PeerCertificates)
-	if err != nil {
-		return nil, err
+func (t *Transport) setupConn(tlsConn *tls.Conn, remotePubKey ci.PubKey) (cs.Conn, error) {
+	if remotePubKey == nil {
+		return nil, errors.New("go-libp2p-tls BUG: expected remote pub key to be set")
 	}
+
 	remotePeerID, err := peer.IDFromPublicKey(remotePubKey)
 	if err != nil {
 		return nil, err