feat: replace EmbedPublicKey by option

Author: hacdias

ipns/record.go

@@ -17,6 +17,7 @@ import (
 	"github.com/ipld/go-ipld-prime/datamodel"
 	basicnode "github.com/ipld/go-ipld-prime/node/basic"
 	ic "github.com/libp2p/go-libp2p/core/crypto"
+	ic_pb "github.com/libp2p/go-libp2p/core/crypto/pb"
 	"github.com/libp2p/go-libp2p/core/peer"
 	"github.com/multiformats/go-multibase"
 	"go.uber.org/multierr"
@@ -194,14 +195,21 @@ const (
 )
 
 type options struct {
-	compatibleWithV1 bool
+	v1Compatibility bool
+	embedPublicKey  *bool
 }
 
 type Option func(*options)
 
-func CompatibleWithV1(compatible bool) Option {
-	return func(opts *options) {
-		opts.compatibleWithV1 = compatible
+func WithV1Compatibility(compatible bool) Option {
+	return func(o *options) {
+		o.v1Compatibility = compatible
+	}
+}
+
+func WithPublicKey(embedded bool) Option {
+	return func(o *options) {
+		o.embedPublicKey = &embedded
 	}
 }
 
@@ -243,7 +251,7 @@ func NewRecord(sk ic.PrivKey, value path.Path, seq uint64, eol time.Time, ttl ti
 		SignatureV2: sig2,
 	}
 
-	if options.compatibleWithV1 {
+	if options.v1Compatibility {
 		pb.Value = []byte(value)
 		typ := ipns_pb.IpnsEntry_EOL
 		pb.ValidityType = &typ
@@ -263,6 +271,17 @@ func NewRecord(sk ic.PrivKey, value path.Path, seq uint64, eol time.Time, ttl ti
 		pb.SignatureV1 = sig1
 	}
 
+	// By default, embed public key if it's not a Ed25519 key. Otherwise, only if
+	// the user has explicitly asked for it to be embedded.
+	if (options.embedPublicKey == nil && sk.Type() != ic_pb.KeyType_Ed25519) ||
+		(options.embedPublicKey != nil && *options.embedPublicKey) {
+		pkBytes, err := ic.MarshalPublicKey(sk.GetPublic())
+		if err != nil {
+			return nil, err
+		}
+		pb.PubKey = pkBytes
+	}
+
 	return &Record{
 		pb:   pb,
 		node: node,
@@ -396,31 +415,6 @@ func compare(a, b *Record) (int, error) {
 	return 0, nil
 }
 
-// EmbedPublicKey embeds the given public key in the given [Record]. While not
-// strictly required, some nodes (e.g., DHT servers), may reject IPNS Records
-// that do not embed their public keys as they may not be able to validate them
-// efficiently.
-func EmbedPublicKey(r *Record, pk ic.PubKey) error {
-	// Try extracting the public key from the ID. If we can, do not embed it.
-	pid, err := peer.IDFromPublicKey(pk)
-	if err != nil {
-		return err
-	}
-	if _, err := pid.ExtractPublicKey(); err != peer.ErrNoPublicKey {
-		// Either a *real* error or nil.
-		return err
-	}
-
-	// We failed to extract the public key from the peer ID, embed it.
-	pkBytes, err := ic.MarshalPublicKey(pk)
-	if err != nil {
-		return err
-	}
-
-	r.pb.PubKey = pkBytes
-	return nil
-}
-
 // ExtractPublicKey extracts a [crypto.PubKey] matching the given [peer.ID] from
 // the IPNS Record, if possible.
 func ExtractPublicKey(r *Record, pid peer.ID) (ic.PubKey, error) {

ipns/record_test.go

@@ -107,7 +107,7 @@ func TestNewRecord(t *testing.T) {
 	t.Run("V1+V2 with option", func(t *testing.T) {
 		t.Parallel()
 
-		rec := mustNewRecord(t, sk, testPath, seq, eol, ttl, CompatibleWithV1(true))
+		rec := mustNewRecord(t, sk, testPath, seq, eol, ttl, WithV1Compatibility(true))
 		require.NotEmpty(t, rec.pb.SignatureV1)
 
 		_, err := rec.PubKey()
@@ -116,51 +116,48 @@ func TestNewRecord(t *testing.T) {
 		fieldsMatch(t, rec, testPath, seq, eol, ttl)
 		fieldsMatchV1(t, rec, testPath, seq, eol, ttl)
 	})
-}
-
-func TestEmbedPublicKey(t *testing.T) {
-	t.Parallel()
-
-	sk, pk, pid := mustKeyPair(t, ic.RSA)
-
-	seq := uint64(0)
-	eol := time.Now().Add(time.Hour)
-	ttl := time.Minute * 10
 
-	rec := mustNewRecord(t, sk, testPath, seq, eol, ttl)
+	t.Run("Public key embedded by default for non-ed25519 keys", func(t *testing.T) {
+		t.Parallel()
 
-	_, err := rec.PubKey()
-	require.ErrorIs(t, err, ErrPublicKeyNotFound)
+		for _, keyType := range []int{ic.RSA, ic.Secp256k1, ic.ECDSA} {
+			sk, _, _ := mustKeyPair(t, keyType)
+			rec := mustNewRecord(t, sk, testPath, seq, eol, ttl)
+			fieldsMatch(t, rec, testPath, seq, eol, ttl)
 
-	err = EmbedPublicKey(rec, pk)
-	require.NoError(t, err)
+			pk, err := rec.PubKey()
+			require.NoError(t, err)
+			require.True(t, pk.Equals(sk.GetPublic()))
+		}
+	})
 
-	recPK, err := rec.PubKey()
-	require.NoError(t, err)
+	t.Run("Public key not embedded by default for ed25519 keys", func(t *testing.T) {
+		t.Parallel()
 
-	recPID, err := peer.IDFromPublicKey(recPK)
-	require.NoError(t, err)
+		sk, _, _ := mustKeyPair(t, ic.Ed25519)
+		rec := mustNewRecord(t, sk, testPath, seq, eol, ttl)
+		fieldsMatch(t, rec, testPath, seq, eol, ttl)
 
-	require.Equal(t, pid, recPID)
+		_, err := rec.PubKey()
+		require.ErrorIs(t, err, ErrPublicKeyNotFound)
+	})
 }
 
 func TestExtractPublicKey(t *testing.T) {
 	t.Parallel()
 
 	t.Run("Returns expected public key when embedded in Peer ID", func(t *testing.T) {
 		sk, pk, pid := mustKeyPair(t, ic.Ed25519)
-		rec := mustNewRecord(t, sk, testPath, 0, time.Now().Add(time.Hour), time.Minute*10)
+		rec := mustNewRecord(t, sk, testPath, 0, time.Now().Add(time.Hour), time.Minute*10, WithPublicKey(false))
 
 		pk2, err := ExtractPublicKey(rec, pid)
 		require.Nil(t, err)
 		require.Equal(t, pk, pk2)
 	})
 
-	t.Run("Returns expected public key when embedded in Record", func(t *testing.T) {
+	t.Run("Returns expected public key when embedded in Record (by default)", func(t *testing.T) {
 		sk, pk, pid := mustKeyPair(t, ic.RSA)
 		rec := mustNewRecord(t, sk, testPath, 0, time.Now().Add(time.Hour), time.Minute*10)
-		err := EmbedPublicKey(rec, pk)
-		require.Nil(t, err)
 
 		pk2, err := ExtractPublicKey(rec, pid)
 		require.Nil(t, err)
@@ -169,7 +166,7 @@ func TestExtractPublicKey(t *testing.T) {
 
 	t.Run("Errors when not embedded in Record or Peer ID", func(t *testing.T) {
 		sk, _, pid := mustKeyPair(t, ic.RSA)
-		rec := mustNewRecord(t, sk, testPath, 0, time.Now().Add(time.Hour), time.Minute*10)
+		rec := mustNewRecord(t, sk, testPath, 0, time.Now().Add(time.Hour), time.Minute*10, WithPublicKey(false))
 
 		pk, err := ExtractPublicKey(rec, pid)
 		require.Error(t, err)

ipns/validation_test.go

@@ -70,12 +70,12 @@ func TestOrdering(t *testing.T) {
 func TestValidator(t *testing.T) {
 	t.Parallel()
 
-	check := func(t *testing.T, sk ic.PrivKey, keybook peerstore.KeyBook, key string, val []byte, eol time.Time, exp error) {
+	check := func(t *testing.T, sk ic.PrivKey, keybook peerstore.KeyBook, key string, val []byte, eol time.Time, exp error, opts ...Option) {
 		validator := Validator{keybook}
 		data := val
 		if data == nil {
 			// do not call mustNewRecord because that validates the record!
-			rec, err := NewRecord(sk, testPath, 1, eol, 0)
+			rec, err := NewRecord(sk, testPath, 1, eol, 0, opts...)
 			require.NoError(t, err)
 			data = mustMarshal(t, rec)
 		}
@@ -99,9 +99,10 @@ func TestValidator(t *testing.T) {
 		check(t, sk, kb, RoutingKey(pid), nil, ts.Add(time.Hour*-1), ErrExpiredRecord)
 		check(t, sk, kb, RoutingKey(pid), []byte("bad data"), ts.Add(time.Hour), ErrBadRecord)
 		check(t, sk, kb, "/ipns/"+"bad key", nil, ts.Add(time.Hour), ErrKeyFormat)
-		check(t, sk, emptyKB, RoutingKey(pid), nil, ts.Add(time.Hour), ErrPublicKeyNotFound)
-		check(t, sk2, kb, RoutingKey(pid2), nil, ts.Add(time.Hour), ErrPublicKeyNotFound)
-		check(t, sk2, kb, RoutingKey(pid), nil, ts.Add(time.Hour), ErrSignature)
+		check(t, sk, emptyKB, RoutingKey(pid), nil, ts.Add(time.Hour), ErrPublicKeyNotFound, WithPublicKey(false))
+		check(t, sk2, kb, RoutingKey(pid2), nil, ts.Add(time.Hour), ErrPublicKeyNotFound, WithPublicKey(false))
+		check(t, sk2, kb, RoutingKey(pid), nil, ts.Add(time.Hour), ErrPublicKeyMismatch)
+		check(t, sk2, kb, RoutingKey(pid), nil, ts.Add(time.Hour), ErrSignature, WithPublicKey(false))
 		check(t, sk, kb, "//"+string(pid), nil, ts.Add(time.Hour), ErrInvalidPath)
 		check(t, sk, kb, "/wrong/"+string(pid), nil, ts.Add(time.Hour), ErrInvalidPath)
 	})
@@ -128,14 +129,14 @@ func TestValidator(t *testing.T) {
 		kb, err := pstoremem.NewPeerstore()
 		require.NoError(t, err)
 
-		sk, pk, pid := mustKeyPair(t, ic.RSA)
-		rec := mustNewRecord(t, sk, testPath, 1, eol, 0)
+		sk, _, pid := mustKeyPair(t, ic.RSA)
+		rec := mustNewRecord(t, sk, testPath, 1, eol, 0, WithPublicKey(false))
 
 		// Fails with RSA key without embedded public key.
 		check(t, sk, kb, RoutingKey(pid), mustMarshal(t, rec), eol, ErrPublicKeyNotFound)
 
 		// Embeds public key, must work now.
-		require.NoError(t, EmbedPublicKey(rec, pk))
+		rec = mustNewRecord(t, sk, testPath, 1, eol, 0)
 		check(t, sk, kb, RoutingKey(pid), mustMarshal(t, rec), eol, nil)
 
 		// Force bad public key. Validation fails.
@@ -163,8 +164,8 @@ func TestValidate(t *testing.T) {
 
 		v := Validator{}
 
-		rec1 := mustNewRecord(t, sk, path.FromString("/path/1"), 1, eol, 0, CompatibleWithV1(true))
-		rec2 := mustNewRecord(t, sk, path.FromString("/path/2"), 2, eol, 0, CompatibleWithV1(true))
+		rec1 := mustNewRecord(t, sk, path.FromString("/path/1"), 1, eol, 0, WithV1Compatibility(true))
+		rec2 := mustNewRecord(t, sk, path.FromString("/path/2"), 2, eol, 0, WithV1Compatibility(true))
 
 		best, err := v.Select(ipnsRoutingKey, [][]byte{mustMarshal(t, rec1), mustMarshal(t, rec2)})
 		require.NoError(t, err)