From 3471bb20f3c3eb44592c9b965aa830ca425a565a Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Mon, 3 Feb 2025 23:04:51 +0100 Subject: [PATCH 1/2] fix(ipns): reading records with raw []byte Value - []byte CID works again - empty value is allowed and produces NoopPath --- CHANGELOG.md | 2 ++ ipns/record.go | 36 +++++++++++++++++++--- ipns/record_test.go | 75 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f7e92be8..429341f68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,8 @@ The following emojis are used to highlight certain changes: ### Fixed +- `ipns`: Improved interop with legacy clients by restoring support for `[]byte` CID in `Value` field. `Value()` will convert it to a valid `path.Path`. Empty `Value()` will produce `NoopPath` (`/ipfs/bafkqaaa`) to avoid breaking existing code that expects a valid record to always produce a valid content path. [#830](https://github.com/ipfs/boxo/pull/830) + ### Security diff --git a/ipns/record.go b/ipns/record.go index 6dabb6017..a4da5d8e3 100644 --- a/ipns/record.go +++ b/ipns/record.go @@ -12,6 +12,7 @@ import ( ipns_pb "github.com/ipfs/boxo/ipns/pb" "github.com/ipfs/boxo/path" "github.com/ipfs/boxo/util" + "github.com/ipfs/go-cid" logging "github.com/ipfs/go-log/v2" "github.com/ipld/go-ipld-prime" "github.com/ipld/go-ipld-prime/codec/dagcbor" @@ -31,6 +32,9 @@ type ValidityType int64 // the only supported Validity type. const ValidityEOL ValidityType = 0 +// NoopValue is an identity CID that points at zero bytes. +const NoopValue = "/ipfs/bafkqaaa" + // Record represents an [IPNS Record]. // // [IPNS Record]: https://specs.ipfs.tech/ipns/ipns-record/ @@ -89,10 +93,29 @@ func (rec *Record) Value() (path.Path, error) { return nil, err } - p, err := path.NewPath(string(value)) + if len(value) == 0 { + // To maximize interop across implementations, turn empty value into zero-length identity CID. + // This is a convenience placeholder used when the value in record is empty or does + // not matter because IPNS is used for custom CBOR in the Data field. + value = []byte(NoopValue) + } + + // parse as a string with content path + if len(value) > 0 && value[0] == '/' { + p, err := path.NewPath(string(value)) + if err != nil { + return nil, multierr.Combine(ErrInvalidPath, err) + } + // done, finish fast + return p, nil + } + + // fallback: for legacy/optimization reasons, the value could be a valid CID in byte form + binaryCid, err := cid.Cast(value) if err != nil { return nil, multierr.Combine(ErrInvalidPath, err) } + p := path.FromCid(binaryCid) return p, nil } @@ -232,6 +255,11 @@ func processOptions(opts ...Option) *options { // option [WithPublicKey]. In addition, records are, by default created with V1 // compatibility. func NewRecord(sk ic.PrivKey, value path.Path, seq uint64, eol time.Time, ttl time.Duration, opts ...Option) (*Record, error) { + return newRecord(sk, []byte(value.String()), seq, eol, ttl, opts...) +} + +// newRecord is a private version of NewRecord that allows arbitrary []byte values (used internally for testing) +func newRecord(sk ic.PrivKey, value []byte, seq uint64, eol time.Time, ttl time.Duration, opts ...Option) (*Record, error) { options := processOptions(opts...) node, err := createNode(value, seq, eol, ttl) @@ -260,7 +288,7 @@ func NewRecord(sk ic.PrivKey, value path.Path, seq uint64, eol time.Time, ttl ti } if options.v1Compatibility { - pb.Value = []byte(value.String()) + pb.Value = value typ := ipns_pb.IpnsRecord_EOL pb.ValidityType = &typ pb.Sequence = &seq @@ -303,11 +331,11 @@ func NewRecord(sk ic.PrivKey, value path.Path, seq uint64, eol time.Time, ttl ti }, nil } -func createNode(value path.Path, seq uint64, eol time.Time, ttl time.Duration) (datamodel.Node, error) { +func createNode(value []byte, seq uint64, eol time.Time, ttl time.Duration) (datamodel.Node, error) { m := make(map[string]ipld.Node) var keys []string - m[cborValueKey] = basicnode.NewBytes([]byte(value.String())) + m[cborValueKey] = basicnode.NewBytes(value) keys = append(keys, cborValueKey) m[cborValidityKey] = basicnode.NewBytes([]byte(util.FormatRFC3339(eol))) diff --git a/ipns/record_test.go b/ipns/record_test.go index edf1db062..2f0972ba5 100644 --- a/ipns/record_test.go +++ b/ipns/record_test.go @@ -9,6 +9,7 @@ import ( ipns_pb "github.com/ipfs/boxo/ipns/pb" "github.com/ipfs/boxo/path" "github.com/ipfs/boxo/util" + "github.com/ipfs/go-cid" "github.com/ipld/go-ipld-prime/codec/dagcbor" basicnode "github.com/ipld/go-ipld-prime/node/basic" ic "github.com/libp2p/go-libp2p/core/crypto" @@ -45,6 +46,13 @@ func mustNewRecord(t *testing.T, sk ic.PrivKey, value path.Path, seq uint64, eol return rec } +func mustNewRawRecord(t *testing.T, sk ic.PrivKey, value []byte, seq uint64, eol time.Time, ttl time.Duration, opts ...Option) *Record { + rec, err := newRecord(sk, value, seq, eol, ttl, opts...) + require.NoError(t, err) + require.NoError(t, Validate(rec, sk.GetPublic())) + return rec +} + func mustMarshal(t *testing.T, entry *Record) []byte { data, err := MarshalRecord(entry) require.NoError(t, err) @@ -259,6 +267,12 @@ func TestCBORDataSerialization(t *testing.T) { func TestUnmarshal(t *testing.T) { t.Parallel() + sk, _, _ := mustKeyPair(t, ic.Ed25519) + + seq := uint64(0) + eol := time.Now().Add(time.Hour) + ttl := time.Minute * 10 + t.Run("Errors on invalid bytes", func(t *testing.T) { _, err := UnmarshalRecord([]byte("blah blah blah")) require.ErrorIs(t, err, ErrInvalidRecord) @@ -287,4 +301,65 @@ func TestUnmarshal(t *testing.T) { _, err = UnmarshalRecord(data) require.ErrorIs(t, err, ErrInvalidRecord) }) + + t.Run("Errors on bad binary Value() unable to represent as a Path (interop)", func(t *testing.T) { + t.Parallel() + + // create record with non-empty byte value that is not []byte CID nor a string with /ipfs/cid content path + rawByteValue := []byte{0x4A, 0x1B, 0x3C, 0x8D, 0x2E} + rec := mustNewRawRecord(t, sk, rawByteValue, seq, eol, ttl) + + // confirm raw record has same bytes + require.Equal(t, rawByteValue, rec.pb.GetValue()) + cborValue, err := rec.getBytesValue(cborValueKey) + require.NoError(t, err) + require.Equal(t, rawByteValue, cborValue) + + // confirm rec.Value() returns error + recPath, err := rec.Value() + require.ErrorIs(t, err, ErrInvalidPath) + require.Empty(t, recPath) + }) + + t.Run("Reads record with empty Value() as zero-length identity CID (interop)", func(t *testing.T) { + t.Parallel() + + // create record with empty byte value + rawByteValue := []byte{} + rec := mustNewRawRecord(t, sk, rawByteValue, seq, eol, ttl) + + // confirm raw record has empty (0-length []byte) Value + require.Empty(t, rec.pb.GetValue()) + cborValue, err := rec.getBytesValue(cborValueKey) + require.NoError(t, err) + require.Empty(t, cborValue) + + // confirm rec.Value() returns NooPath for interop + recPath, err := rec.Value() + require.NoError(t, err) + require.Equal(t, NoopValue, recPath.String()) + }) + + t.Run("Reads record with a []byte CID as Value() (interop)", func(t *testing.T) { + t.Parallel() + + // create record with a valid CID in binary form + // instead of /ipfs/cid string + // (we need to support this because this was allowed since <2018) + testCid := "bafkqablimvwgy3y" + rawCidValue := cid.MustParse(testCid).Bytes() + + rec := mustNewRawRecord(t, sk, rawCidValue, seq, eol, ttl) + + // confirm raw record has same bytes + require.Equal(t, rawCidValue, rec.pb.GetValue()) + cborValue, err := rec.getBytesValue(cborValueKey) + require.NoError(t, err) + require.Equal(t, rawCidValue, cborValue) + + // confirm rec.Value() returns path for raw CID + recPath, err := rec.Value() + require.NoError(t, err) + require.Equal(t, "/ipfs/"+testCid, recPath.String()) + }) } From bef2192112c8a2d7eed4498ead96b95f312f63c4 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Tue, 4 Feb 2025 16:01:39 +0100 Subject: [PATCH 2/2] refactor: review feedback --- ipns/record.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/ipns/record.go b/ipns/record.go index a4da5d8e3..c58b4ca56 100644 --- a/ipns/record.go +++ b/ipns/record.go @@ -85,8 +85,10 @@ func MarshalRecord(rec *Record) ([]byte, error) { return proto.Marshal(rec.pb) } -// Value returns the [path.Path] that is embedded in this IPNS Record. If the -// path is invalid, an [ErrInvalidPath] is returned. +// Value returns the [path.Path] that is embedded in this IPNS Record. +// If the path is invalid, an error is returned. +// If the value is a binary CID, it is converted to a [path.Path]. +// If the value is empty, a [NoopValue] is used instead. func (rec *Record) Value() (path.Path, error) { value, err := rec.getBytesValue(cborValueKey) if err != nil { @@ -101,7 +103,7 @@ func (rec *Record) Value() (path.Path, error) { } // parse as a string with content path - if len(value) > 0 && value[0] == '/' { + if value[0] == '/' { p, err := path.NewPath(string(value)) if err != nil { return nil, multierr.Combine(ErrInvalidPath, err)