Repository: libp2p/go-libp2p
Pull request #1661 (2022-07-26 12:08:07)

mdns: don't discover ourselves

marten-seemann: Looks like we had a test case `TestSelfDiscovery` asserting that we discover ourselves. I'm not why that would be useful. @Stebalien, wdyt?
MGMCN: Thank you for your reply.
Yes, I saw the test.
Maybe I overlooked some details?
But I don't know the meaning of finding ourselves.
MGMCN: <img width="765" alt="image" src="https://user-images.githubusercontent.com/49364055/181014684-c1a70afc-45c9-43be-a610-24839e6b5a6b.png">

Because after this code modification, both "TestSelfDiscovery" and "TestOtherDiscovery" failed. I have a bold guess. 

Before we modify the code, whether these two tests passed because peer found itself. 

But this is just my assumption.

Looking forward to your reply.

Stebalien: > Looks like we had a test case TestSelfDiscovery asserting that we discover ourselves. I'm not why that would be useful. @Stebalien, wdyt?

I have no idea...

Patch:
diff --git a/p2p/discovery/mdns/mdns.go b/p2p/discovery/mdns/mdns.go
index a84cf807..cbae29a2 100644
--- a/p2p/discovery/mdns/mdns.go
+++ b/p2p/discovery/mdns/mdns.go
@@ -176,6 +176,9 @@ func (s *mdnsService) startResolver(ctx context.Context) {
 				continue
 			}
 			for _, info := range infos {
+				if info.ID == s.host.ID() {
+					continue
+				}
 				go s.notifee.HandlePeerFound(info)
 			}
 		}
diff --git a/p2p/security/tls/crypto.go b/p2p/security/tls/crypto.go
index 24a06939..698cccf4 100644
--- a/p2p/security/tls/crypto.go
+++ b/p2p/security/tls/crypto.go
@@ -1,6 +1,7 @@
 package libp2ptls
 
 import (
+	"crypto"
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rand"
@@ -38,9 +39,37 @@ type Identity struct {
 	config tls.Config
 }
 
+// IdentityConfig is used to configure an Identity
+type IdentityConfig struct {
+	CertTemplate *x509.Certificate
+}
+
+// IdentityOption transforms an IdentityConfig to apply optional settings.
+type IdentityOption func(r *IdentityConfig)
+
+// WithCertTemplate specifies the template to use when generating a new certificate.
+func WithCertTemplate(template *x509.Certificate) IdentityOption {
+	return func(c *IdentityConfig) {
+		c.CertTemplate = template
+	}
+}
+
 // NewIdentity creates a new identity
-func NewIdentity(privKey ic.PrivKey) (*Identity, error) {
-	cert, err := keyToCertificate(privKey)
+func NewIdentity(privKey ic.PrivKey, opts ...IdentityOption) (*Identity, error) {
+	config := IdentityConfig{}
+	for _, opt := range opts {
+		opt(&config)
+	}
+
+	var err error
+	if config.CertTemplate == nil {
+		config.CertTemplate, err = certTemplate()
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	cert, err := keyToCertificate(privKey, config.CertTemplate)
 	if err != nil {
 		return nil, err
 	}
@@ -167,60 +196,79 @@ func PubKeyFromCertChain(chain []*x509.Certificate) (ic.PubKey, error) {
 	return pubKey, nil
 }
 
-func keyToCertificate(sk ic.PrivKey) (*tls.Certificate, error) {
-	certKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
-	if err != nil {
-		return nil, err
-	}
-
+// GenerateSignedExtension uses the provided private key to sign the public key, and returns the
+// signature within a pkix.Extension.
+// This extension is included in a certificate to cryptographically tie it to the libp2p private key.
+func GenerateSignedExtension(sk ic.PrivKey, pubKey crypto.PublicKey) (pkix.Extension, error) {
 	keyBytes, err := ic.MarshalPublicKey(sk.GetPublic())
 	if err != nil {
-		return nil, err
+		return pkix.Extension{}, err
 	}
-	certKeyPub, err := x509.MarshalPKIXPublicKey(certKey.Public())
+	certKeyPub, err := x509.MarshalPKIXPublicKey(pubKey)
 	if err != nil {
-		return nil, err
+		return pkix.Extension{}, err
 	}
 	signature, err := sk.Sign(append([]byte(certificatePrefix), certKeyPub...))
 	if err != nil {
-		return nil, err
+		return pkix.Extension{}, err
 	}
 	value, err := asn1.Marshal(signedKey{
 		PubKey:    keyBytes,
 		Signature: signature,
 	})
+	if err != nil {
+		return pkix.Extension{}, err
+	}
+
+	return pkix.Extension{Id: extensionID, Critical: extensionCritical, Value: value}, nil
+}
+
+// keyToCertificate generates a new ECDSA private key and corresponding x509 certificate.
+// The certificate includes an extension that cryptographically ties it to the provided libp2p
+// private key to authenticate TLS connections.
+func keyToCertificate(sk ic.PrivKey, certTmpl *x509.Certificate) (*tls.Certificate, error) {
+	certKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		return nil, err
+	}
+
+	// after calling CreateCertificate, these will end up in Certificate.Extensions
+	extension, err := GenerateSignedExtension(sk, certKey.Public())
+	if err != nil {
+		return nil, err
+	}
+	certTmpl.ExtraExtensions = append(certTmpl.ExtraExtensions, extension)
+
+	certDER, err := x509.CreateCertificate(rand.Reader, certTmpl, certTmpl, certKey.Public(), certKey)
 	if err != nil {
 		return nil, err
 	}
+	return &tls.Certificate{
+		Certificate: [][]byte{certDER},
+		PrivateKey:  certKey,
+	}, nil
+}
 
+// certTemplate returns the template for generating an Identity's TLS certificates.
+func certTemplate() (*x509.Certificate, error) {
 	bigNum := big.NewInt(1 << 62)
 	sn, err := rand.Int(rand.Reader, bigNum)
 	if err != nil {
 		return nil, err
 	}
+
 	subjectSN, err := rand.Int(rand.Reader, bigNum)
 	if err != nil {
 		return nil, err
 	}
-	tmpl := &x509.Certificate{
+
+	return &x509.Certificate{
 		SerialNumber: sn,
 		NotBefore:    time.Now().Add(-time.Hour),
 		NotAfter:     time.Now().Add(certValidityPeriod),
 		// According to RFC 3280, the issuer field must be set,
 		// see https://datatracker.ietf.org/doc/html/rfc3280#section-4.1.2.4.
 		Subject: pkix.Name{SerialNumber: subjectSN.String()},
-		// after calling CreateCertificate, these will end up in Certificate.Extensions
-		ExtraExtensions: []pkix.Extension{
-			{Id: extensionID, Critical: extensionCritical, Value: value},
-		},
-	}
-	certDER, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, certKey.Public(), certKey)
-	if err != nil {
-		return nil, err
-	}
-	return &tls.Certificate{
-		Certificate: [][]byte{certDER},
-		PrivateKey:  certKey,
 	}, nil
 }
 
@@ -229,7 +277,7 @@ func keyToCertificate(sk ic.PrivKey) (*tls.Certificate, error) {
 // x86->x86: AES, ARM->x86: ChaCha, x86->ARM: ChaCha and ARM->ARM: Chacha
 // This function returns true if we don't have AES hardware support, and false otherwise.
 // Thus, ARM servers will always use their own cipher suite preferences (ChaCha first),
-// and x86 servers will aways use the client's cipher suite preferences.
+// and x86 servers will always use the client's cipher suite preferences.
 func preferServerCipherSuites() bool {
 	// Copied from the Go TLS implementation.
 
diff --git a/version.json b/version.json
index 14e1da56..c6a304fe 100644
--- a/version.json
+++ b/version.json
@@ -1,3 +1,3 @@
 {
-  "version": "v0.21.0-rc"
+  "version": "v0.21.0"
 }


Test patch:
diff --git a/.github/workflows/interop-test.yml b/.github/workflows/interop-test.yml
new file mode 100644
index 00000000..06134982
--- /dev/null
+++ b/.github/workflows/interop-test.yml
@@ -0,0 +1,16 @@
+on: [push, pull_request]
+name: Interoperability Testing
+
+jobs:
+  run-ping-interop-cross-version:
+    uses: "libp2p/test-plans/.github/workflows/run-composition.yml@master"
+    with:
+      composition_file: "ping/_compositions/go-cross-versions.toml"
+      # NOTE: during a pull request run, github creates a merge commit referenced in `github.sha`
+      # that merge commit is not a regular commit. You won't find it with a regular `git checkout SHA` and
+      # tools like `go get repo@SHA` won't find it.
+      #
+      # As a workaround, we generate a path to the actual pull request's commit, it looks like:
+      # `github.com/external-org/go-libp2p@latest-commit-on-their-branch`
+      custom_git_target: github.com/${{ github.event.pull_request.head.repo.full_name || github.event.repository.full_name }}
+      custom_git_reference: ${{ github.event.pull_request.head.sha || github.sha }}
diff --git a/p2p/discovery/mdns/mdns_test.go b/p2p/discovery/mdns/mdns_test.go
index 8b5e000a..60a49cae 100644
--- a/p2p/discovery/mdns/mdns_test.go
+++ b/p2p/discovery/mdns/mdns_test.go
@@ -46,27 +46,6 @@ func (n *notif) GetPeers() []peer.AddrInfo {
 	return infos
 }
 
-func TestSelfDiscovery(t *testing.T) {
-	notif := &notif{}
-	hostID := setupMDNS(t, notif)
-	assert.Eventuallyf(
-		t,
-		func() bool {
-			var found bool
-			for _, info := range notif.GetPeers() {
-				if info.ID == hostID {
-					found = true
-					break
-				}
-			}
-			return found
-		},
-		5*time.Second,
-		5*time.Millisecond,
-		"expected peer to find itself",
-	)
-}
-
 func TestOtherDiscovery(t *testing.T) {
 	const n = 4
 
@@ -78,9 +57,12 @@ func TestOtherDiscovery(t *testing.T) {
 		hostIDs[i] = setupMDNS(t, notif)
 	}
 
-	containsAllHostIDs := func(ids []peer.ID) bool {
+	containsAllHostIDs := func(ids []peer.ID, currentHostID peer.ID) bool {
 		for _, id := range hostIDs {
 			var found bool
+			if currentHostID == id {
+				continue
+			}
 			for _, i := range ids {
 				if id == i {
 					found = true
@@ -97,13 +79,13 @@ func TestOtherDiscovery(t *testing.T) {
 	assert.Eventuallyf(
 		t,
 		func() bool {
-			for _, notif := range notifs {
+			for i, notif := range notifs {
 				infos := notif.GetPeers()
 				ids := make([]peer.ID, 0, len(infos))
 				for _, info := range infos {
 					ids = append(ids, info.ID)
 				}
-				if !containsAllHostIDs(ids) {
+				if !containsAllHostIDs(ids, hostIDs[i]) {
 					return false
 				}
 			}
diff --git a/p2p/security/tls/crypto_test.go b/p2p/security/tls/crypto_test.go
new file mode 100644
index 00000000..e2a28bbb
--- /dev/null
+++ b/p2p/security/tls/crypto_test.go
@@ -0,0 +1,48 @@
+package libp2ptls
+
+import (
+	"crypto/x509"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestNewIdentityCertificates(t *testing.T) {
+	_, key := createPeer(t)
+	cn := "a.test.name"
+	email := "unittest@example.com"
+
+	t.Run("NewIdentity with default template", func(t *testing.T) {
+		// Generate an identity using the default template
+		id, err := NewIdentity(key)
+		assert.NoError(t, err)
+
+		// Extract the x509 certificate
+		x509Cert, err := x509.ParseCertificate(id.config.Certificates[0].Certificate[0])
+		assert.NoError(t, err)
+
+		// verify the common name and email are not set
+		assert.Empty(t, x509Cert.Subject.CommonName)
+		assert.Empty(t, x509Cert.EmailAddresses)
+	})
+
+	t.Run("NewIdentity with custom template", func(t *testing.T) {
+		tmpl, err := certTemplate()
+		assert.NoError(t, err)
+
+		tmpl.Subject.CommonName = cn
+		tmpl.EmailAddresses = []string{email}
+
+		// Generate an identity using the custom template
+		id, err := NewIdentity(key, WithCertTemplate(tmpl))
+		assert.NoError(t, err)
+
+		// Extract the x509 certificate
+		x509Cert, err := x509.ParseCertificate(id.config.Certificates[0].Certificate[0])
+		assert.NoError(t, err)
+
+		// verify the common name and email are set
+		assert.Equal(t, cn, x509Cert.Subject.CommonName)
+		assert.Equal(t, email, x509Cert.EmailAddresses[0])
+	})
+}
diff --git a/p2p/security/tls/transport_test.go b/p2p/security/tls/transport_test.go
index d49730f3..1d272f0f 100644
--- a/p2p/security/tls/transport_test.go
+++ b/p2p/security/tls/transport_test.go
@@ -85,12 +85,7 @@ func TestHandshakeSucceeds(t *testing.T) {
 	clientID, clientKey := createPeer(t)
 	serverID, serverKey := createPeer(t)
 
-	handshake := func(t *testing.T) {
-		clientTransport, err := New(clientKey)
-		require.NoError(t, err)
-		serverTransport, err := New(serverKey)
-		require.NoError(t, err)
-
+	handshake := func(t *testing.T, clientTransport *Transport, serverTransport *Transport) {
 		clientInsecureConn, serverInsecureConn := connect(t)
 
 		serverConnChan := make(chan sec.SecureConn)
@@ -129,15 +124,54 @@ func TestHandshakeSucceeds(t *testing.T) {
 		require.Equal(t, string(b), "foobar")
 	}
 
-	t.Run("with extension not critical", func(t *testing.T) {
-		handshake(t)
+	// Use standard transports with default TLS configuration
+	clientTransport, err := New(clientKey)
+	require.NoError(t, err)
+	serverTransport, err := New(serverKey)
+	require.NoError(t, err)
+
+	t.Run("standard TLS with extension not critical", func(t *testing.T) {
+		handshake(t, clientTransport, serverTransport)
+	})
+
+	t.Run("standard TLS with extension critical", func(t *testing.T) {
+		extensionCritical = true
+		t.Cleanup(func() { extensionCritical = false })
+
+		handshake(t, clientTransport, serverTransport)
+	})
+
+	// Use transports with custom TLS certificates
+
+	// override client identity to use a custom certificate
+	clientCertTmlp, err := certTemplate()
+	require.NoError(t, err)
+
+	clientCertTmlp.Subject.CommonName = "client.test.name"
+	clientCertTmlp.EmailAddresses = []string{"client-unittest@example.com"}
+
+	clientTransport.identity, err = NewIdentity(clientKey, WithCertTemplate(clientCertTmlp))
+	require.NoError(t, err)
+
+	// override server identity to use a custom certificate
+	serverCertTmpl, err := certTemplate()
+	require.NoError(t, err)
+
+	serverCertTmpl.Subject.CommonName = "server.test.name"
+	serverCertTmpl.EmailAddresses = []string{"server-unittest@example.com"}
+
+	serverTransport.identity, err = NewIdentity(serverKey, WithCertTemplate(serverCertTmpl))
+	require.NoError(t, err)
+
+	t.Run("custom TLS with extension not critical", func(t *testing.T) {
+		handshake(t, clientTransport, serverTransport)
 	})
 
-	t.Run("with extension critical", func(t *testing.T) {
+	t.Run("custom TLS with extension critical", func(t *testing.T) {
 		extensionCritical = true
 		t.Cleanup(func() { extensionCritical = false })
 
-		handshake(t)
+		handshake(t, clientTransport, serverTransport)
 	})
 }