api/v0: reject Metadata.ProtocolID == 0

Integers in Go default to the zero value, meaning that if the user
forgets to set the protocol ID, it could transparently default to a
confusing value. For instance:

	metadata := v0.Metadata{Data: someData}

This could lead to confusing decode errors later on, or to metadata
being mistaken for the wrong kind.

Instead, reject this zero value. The protocol ID is a multihash anyway,
and we want users to either select registered indexer metadata
multihashes, or a multihash in the reserved range for testing.

The check happens at both encode and decode, for extra safety.
This required adding an error return on the encode side.
While we're there, make Metadata implement encoding's Binary interfaces.

Updates #94.

api/v0/ingest/schema/utils.go

@@ -4,7 +4,7 @@ package schema
 import (
 	"context"
 
-	"github.com/filecoin-project/storetheindex/api/v0"
+	v0 "github.com/filecoin-project/storetheindex/api/v0"
 	"github.com/ipfs/go-cid"
 	"github.com/ipld/go-ipld-prime"
 	cidlink "github.com/ipld/go-ipld-prime/linking/cid"
@@ -162,6 +162,11 @@ func NewAdvertisementWithFakeSig(
 	provider string,
 	addrs []string) (Advertisement, Link_Advertisement, error) {
 
+	encMetadata, err := metadata.MarshalBinary()
+	if err != nil {
+		return nil, nil, err
+	}
+
 	var ad Advertisement
 	if previousID != nil {
 		ad = &_Advertisement{
@@ -170,7 +175,7 @@ func NewAdvertisementWithFakeSig(
 			Addresses:  GoToIpldStrings(addrs),
 			Entries:    _Link{x: entries},
 			ContextID:  _Bytes{x: contextID},
-			Metadata:   _Bytes{x: metadata.Encode()},
+			Metadata:   _Bytes{x: encMetadata},
 			IsRm:       _Bool{x: isRm},
 		}
 	} else {
@@ -180,7 +185,7 @@ func NewAdvertisementWithFakeSig(
 			Addresses:  GoToIpldStrings(addrs),
 			Entries:    _Link{x: entries},
 			ContextID:  _Bytes{x: contextID},
-			Metadata:   _Bytes{x: metadata.Encode()},
+			Metadata:   _Bytes{x: encMetadata},
 			IsRm:       _Bool{x: isRm},
 		}
 	}
@@ -208,6 +213,11 @@ func newAdvertisement(
 	provider string,
 	addrs []string) (Advertisement, error) {
 
+	encMetadata, err := metadata.MarshalBinary()
+	if err != nil {
+		return nil, err
+	}
+
 	var ad Advertisement
 	if previousID != nil {
 		ad = &_Advertisement{
@@ -216,7 +226,7 @@ func newAdvertisement(
 			Addresses:  GoToIpldStrings(addrs),
 			Entries:    _Link{x: entries},
 			ContextID:  _Bytes{x: contextID},
-			Metadata:   _Bytes{x: metadata.Encode()},
+			Metadata:   _Bytes{x: encMetadata},
 			IsRm:       _Bool{x: isRm},
 		}
 	} else {
@@ -226,7 +236,7 @@ func newAdvertisement(
 			Addresses:  GoToIpldStrings(addrs),
 			Entries:    _Link{x: entries},
 			ContextID:  _Bytes{x: contextID},
-			Metadata:   _Bytes{x: metadata.Encode()},
+			Metadata:   _Bytes{x: encMetadata},
 			IsRm:       _Bool{x: isRm},
 		}
 	}

api/v0/ingest/schema/utils_test.go

@@ -5,7 +5,7 @@ import (
 	"io"
 	"testing"
 
-	"github.com/filecoin-project/storetheindex/api/v0"
+	v0 "github.com/filecoin-project/storetheindex/api/v0"
 	"github.com/filecoin-project/storetheindex/test/util"
 	"github.com/ipfs/go-datastore"
 	ipld "github.com/ipld/go-ipld-prime"
@@ -38,12 +38,15 @@ func mkLinkSystem(ds datastore.Batching) ipld.LinkSystem {
 	return lsys
 }
 
+const protocolID = 0x300000
+
 func genCidsAndAdv(t *testing.T, lsys ipld.LinkSystem, priv crypto.PrivKey, previous Link_Advertisement) ([]multihash.Multihash, ipld.Link, Advertisement, Link_Advertisement) {
 	mhs := util.RandomMultihashes(10)
 	p, _ := peer.Decode("12D3KooWKRyzVWW6ChFjQjK4miCty85Niy48tpPV95XdKu1BcvMA")
 	ctxID := []byte("test-context-id")
 	metadata := v0.Metadata{
-		Data: mhs[0],
+		ProtocolID: protocolID,
+		Data:       mhs[0],
 	}
 	addr := "/ip4/127.0.0.1/tcp/9999"
 	cidsLnk, err := NewListOfMhs(lsys, mhs)

api/v0/metadata.go

@@ -2,11 +2,21 @@ package v0
 
 import (
 	"bytes"
+	"encoding"
+	"fmt"
 
 	"github.com/multiformats/go-multicodec"
 	"github.com/multiformats/go-varint"
 )
 
+type ErrInvalidMetadata struct {
+	Message string
+}
+
+func (e ErrInvalidMetadata) Error() string {
+	return fmt.Sprintf("storetheindex: invalid metadata: %v", e.Message)
+}
+
 // Metadata is data that provides information about retrieving
 // data for an index, from a particular provider.
 type Metadata struct {
@@ -17,30 +27,47 @@ type Metadata struct {
 	Data []byte
 }
 
+var (
+	_ encoding.BinaryMarshaler   = (*Metadata)(nil)
+	_ encoding.BinaryUnmarshaler = (*Metadata)(nil)
+)
+
 // Equal determines if two Metadata values are equal.
 func (m Metadata) Equal(other Metadata) bool {
 	return m.ProtocolID == other.ProtocolID && bytes.Equal(m.Data, other.Data)
 }
 
-// EncodeMetadata serializes Metadata to []byte.
-func (m Metadata) Encode() []byte {
+// MarshalBinary implements encoding.BinaryMarshaler.
+func (m Metadata) MarshalBinary() ([]byte, error) {
+	if m.ProtocolID == 0 {
+		return nil, &ErrInvalidMetadata{Message: "encountered protocol ID 0 on encode"}
+	}
+
 	varintSize := varint.UvarintSize(uint64(m.ProtocolID))
 	buf := make([]byte, varintSize+len(m.Data))
 	varint.PutUvarint(buf, uint64(m.ProtocolID))
 	if len(m.Data) != 0 {
 		copy(buf[varintSize:], m.Data)
 	}
-	return buf
+	return buf, nil
 }
 
-// DecodeMetadata deserializes []byte into Metadata.
-func DecodeMetadata(data []byte) (Metadata, error) {
-	protocol, len, err := varint.FromUvarint(data)
+// UnmarshalBinary implements encoding.BinaryUnmarshaler.
+func (m *Metadata) UnmarshalBinary(data []byte) error {
+	protocol, protoLen, err := varint.FromUvarint(data)
 	if err != nil {
-		return Metadata{}, err
+		return err
+	}
+	if protocol == 0 {
+		return &ErrInvalidMetadata{Message: "encountered protocol ID 0 on decode"}
 	}
-	return Metadata{
-		ProtocolID: multicodec.Code(protocol),
-		Data:       data[len:],
-	}, nil
+
+	m.ProtocolID = multicodec.Code(protocol)
+
+	// We can't hold onto the input data. Make a copy.
+	innerData := data[protoLen:]
+	m.Data = make([]byte, len(innerData))
+	copy(m.Data, innerData)
+
+	return nil
 }

api/v0/metadata_test.go

@@ -12,14 +12,17 @@ func TestEncodeDecode(t *testing.T) {
 		ProtocolID: testProtoID,
 		Data:       []byte("test-data"),
 	}
-	metadataBytes := origMetadata.Encode()
+	metadataBytes, err := origMetadata.MarshalBinary()
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	if len(metadataBytes) == 0 {
 		t.Fatal("did not encode metadata")
 	}
 
-	metadata, err := DecodeMetadata(metadataBytes)
-	if err != nil {
+	var metadata Metadata
+	if err := metadata.UnmarshalBinary(metadataBytes); err != nil {
 		t.Fatal(err)
 	}
 	if metadata.ProtocolID != origMetadata.ProtocolID {
@@ -32,6 +35,13 @@ func TestEncodeDecode(t *testing.T) {
 		t.Fatal("metadata no equal after decode")
 	}
 
+	// Zero the bytes and ensure the decoded struct still works.
+	// This will fail if UnmarshalBinary did not copy the inner data bytes.
+	copy(metadataBytes, make([]byte, 1024))
+	if !metadata.Equal(origMetadata) {
+		t.Fatal("metadata no equal after buffer zeroing")
+	}
+
 	metadata.ProtocolID = origMetadata.ProtocolID + 1
 	if metadata.Equal(origMetadata) {
 		t.Fatal("metadata should not be equal")

internal/handler/finder_handler.go

@@ -4,7 +4,7 @@ import (
 	"fmt"
 
 	"github.com/filecoin-project/go-indexer-core"
-	"github.com/filecoin-project/storetheindex/api/v0"
+	v0 "github.com/filecoin-project/storetheindex/api/v0"
 	"github.com/filecoin-project/storetheindex/api/v0/finder/model"
 	"github.com/filecoin-project/storetheindex/internal/registry"
 	"github.com/filecoin-project/storetheindex/internal/syserr"
@@ -75,7 +75,8 @@ func (h *FinderHandler) MakeFindResponse(mhashes []multihash.Multihash) (*model.
 }
 
 func providerResultFromValue(value indexer.Value, addrs []multiaddr.Multiaddr) (model.ProviderResult, error) {
-	metadata, err := v0.DecodeMetadata(value.MetadataBytes)
+	var metadata v0.Metadata
+	err := metadata.UnmarshalBinary(value.MetadataBytes)
 	if err != nil {
 		return model.ProviderResult{}, fmt.Errorf("could not decode metadata: %s", err)
 	}

internal/handler/ingest_handler.go

@@ -103,11 +103,15 @@ func (h *IngestHandler) IndexContent(data []byte) error {
 	if err != nil {
 		return err
 	}
+	encMetadata, err := ingReq.Metadata.MarshalBinary()
+	if err != nil {
+		return err
+	}
 
 	value := indexer.Value{
 		ProviderID:    ingReq.ProviderID,
 		ContextID:     ingReq.ContextID,
-		MetadataBytes: ingReq.Metadata.Encode(),
+		MetadataBytes: encMetadata,
 	}
 	err = h.indexer.Put(value, ingReq.Multihash)
 	if err != nil {

internal/ingest/linksystem.go

@@ -6,7 +6,7 @@ import (
 	"net/http"
 
 	"github.com/filecoin-project/go-indexer-core"
-	"github.com/filecoin-project/storetheindex/api/v0"
+	v0 "github.com/filecoin-project/storetheindex/api/v0"
 	"github.com/filecoin-project/storetheindex/api/v0/ingest/schema"
 	"github.com/filecoin-project/storetheindex/internal/registry"
 	"github.com/filecoin-project/storetheindex/internal/syserr"
@@ -252,8 +252,8 @@ func (i *legIngester) processEntries(adCid cid.Cid, p peer.ID, nentries ipld.Nod
 		return err
 	}
 
-	// Check for valud metadata
-	_, err = v0.DecodeMetadata(metadataBytes)
+	// Check for valid metadata
+	err = new(v0.Metadata).UnmarshalBinary(metadataBytes)
 	if err != nil {
 		log.Errorf("Error decoding metadata: %s", err)
 		return err

server/admin/http/handler.go

@@ -7,7 +7,6 @@ import (
 	"net/http"
 
 	"github.com/filecoin-project/go-indexer-core"
-	"github.com/filecoin-project/storetheindex/api/v0"
 	"github.com/filecoin-project/storetheindex/internal/importer"
 	"github.com/filecoin-project/storetheindex/internal/ingest"
 	"github.com/gorilla/mux"
@@ -136,7 +135,7 @@ func (h *adminHandler) importManifest(w http.ResponseWriter, r *http.Request) {
 	value := indexer.Value{
 		ProviderID:    provID,
 		ContextID:     contextID,
-		MetadataBytes: v0.Metadata{}.Encode(),
+		MetadataBytes: nil, // TODO: redesign into endpoints that take metadata too
 	}
 	batchErr := batchIndexerEntries(batchSize, out, value, h.indexer)
 	err = <-batchErr
@@ -190,7 +189,7 @@ func (h *adminHandler) importCidList(w http.ResponseWriter, r *http.Request) {
 	value := indexer.Value{
 		ProviderID:    provID,
 		ContextID:     contextID,
-		MetadataBytes: v0.Metadata{}.Encode(),
+		MetadataBytes: nil, // TODO: redesign into endpoints that take metadata too
 	}
 	batchErr := batchIndexerEntries(batchSize, out, value, h.indexer)
 	err = <-batchErr

server/finder/test/test.go

@@ -14,7 +14,7 @@ import (
 	"github.com/filecoin-project/go-indexer-core/cache/radixcache"
 	"github.com/filecoin-project/go-indexer-core/engine"
 	"github.com/filecoin-project/go-indexer-core/store/storethehash"
-	"github.com/filecoin-project/storetheindex/api/v0"
+	v0 "github.com/filecoin-project/storetheindex/api/v0"
 	"github.com/filecoin-project/storetheindex/api/v0/finder/client"
 	"github.com/filecoin-project/storetheindex/api/v0/finder/model"
 	"github.com/filecoin-project/storetheindex/config"
@@ -105,10 +105,14 @@ func FindIndexTest(ctx context.Context, t *testing.T, c client.Finder, ind index
 		ProtocolID: protocolID,
 		Data:       []byte(mhs[0]),
 	}
+	encMetadata, err := metadata.MarshalBinary()
+	if err != nil {
+		t.Fatal(err)
+	}
 	v := indexer.Value{
 		ProviderID:    p,
 		ContextID:     ctxID,
-		MetadataBytes: metadata.Encode(),
+		MetadataBytes: encMetadata,
 	}
 	populateIndex(ind, mhs[:10], v, t)
 
@@ -133,13 +137,12 @@ func FindIndexTest(ctx context.Context, t *testing.T, c client.Finder, ind index
 
 	provResult := model.ProviderResult{
 		ContextID: v.ContextID,
-		Metadata:  metadata,
 		Provider: peer.AddrInfo{
 			ID:    v.ProviderID,
 			Addrs: info.AddrInfo.Addrs,
 		},
 	}
-	provResult.Metadata, err = v0.DecodeMetadata(v.MetadataBytes)
+	err = provResult.Metadata.UnmarshalBinary(v.MetadataBytes)
 	if err != nil {
 		t.Fatal(err)
 	}

server/ingest/test/test.go

@@ -12,7 +12,7 @@ import (
 	"github.com/filecoin-project/go-indexer-core/cache/radixcache"
 	"github.com/filecoin-project/go-indexer-core/engine"
 	"github.com/filecoin-project/go-indexer-core/store/storethehash"
-	"github.com/filecoin-project/storetheindex/api/v0"
+	v0 "github.com/filecoin-project/storetheindex/api/v0"
 	"github.com/filecoin-project/storetheindex/api/v0/ingest/client"
 	"github.com/filecoin-project/storetheindex/config"
 	"github.com/filecoin-project/storetheindex/internal/registry"
@@ -161,10 +161,15 @@ func IndexContent(t *testing.T, cl client.Ingest, providerID peer.ID, privateKey
 		t.Fatal("no content values returned")
 	}
 
+	encMetadata, err := metadata.MarshalBinary()
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	expectValue := indexer.Value{
 		ProviderID:    providerID,
 		ContextID:     contextID,
-		MetadataBytes: metadata.Encode(),
+		MetadataBytes: encMetadata,
 	}
 	ok = false
 	for i := range vals {