Merge pull request #4628 from dirkmc/fix/namesys-verify-pubk

namesys: verify signature in ipns validator

core/core.go

@@ -949,14 +949,14 @@ func startListening(ctx context.Context, host p2phost.Host, cfg *config.Config)
 
 func constructDHTRouting(ctx context.Context, host p2phost.Host, dstore repo.Datastore) (routing.IpfsRouting, error) {
 	dhtRouting := dht.NewDHT(ctx, host, dstore)
-	dhtRouting.Validator[IpnsValidatorTag] = namesys.IpnsRecordValidator
+	dhtRouting.Validator[IpnsValidatorTag] = namesys.NewIpnsRecordValidator(host.Peerstore())
 	dhtRouting.Selector[IpnsValidatorTag] = namesys.IpnsSelectorFunc
 	return dhtRouting, nil
 }
 
 func constructClientDHTRouting(ctx context.Context, host p2phost.Host, dstore repo.Datastore) (routing.IpfsRouting, error) {
 	dhtRouting := dht.NewDHTClient(ctx, host, dstore)
-	dhtRouting.Validator[IpnsValidatorTag] = namesys.IpnsRecordValidator
+	dhtRouting.Validator[IpnsValidatorTag] = namesys.NewIpnsRecordValidator(host.Peerstore())
 	dhtRouting.Selector[IpnsValidatorTag] = namesys.IpnsSelectorFunc
 	return dhtRouting, nil
 }

namesys/publisher.go

@@ -3,7 +3,6 @@ package namesys
 import (
 	"bytes"
 	"context"
-	"errors"
 	"fmt"
 	"time"
 
@@ -16,25 +15,12 @@ import (
 	u "gx/ipfs/QmNiJuT8Ja3hMVpBHXv3Q6dwmperaQ6JjLtpMQgMCD7xvx/go-ipfs-util"
 	ds "gx/ipfs/QmPpegoMqhAEqjncrzArm7KVWAkCm78rqL2DPuNjhPrshg/go-datastore"
 	routing "gx/ipfs/QmTiWLZ6Fo5j4KcTVutZJ5KWRRJrbxzmxA4td8NfEdrPh7/go-libp2p-routing"
-	record "gx/ipfs/QmUpttFinNDmNPgFwKN8sZK6BUtBmA68Y4KdSBDXa8t9sJ/go-libp2p-record"
 	dhtpb "gx/ipfs/QmUpttFinNDmNPgFwKN8sZK6BUtBmA68Y4KdSBDXa8t9sJ/go-libp2p-record/pb"
 	proto "gx/ipfs/QmZ4Qi3GaRbjcx28Sme5eMH7RQjGkt8wHxt2a65oLaeFEV/gogo-protobuf/proto"
 	peer "gx/ipfs/QmZoWKhxUmZ2seW4BzX6fJkNR8hh9PsGModr7q171yq2SS/go-libp2p-peer"
 	ci "gx/ipfs/QmaPbCnUMBohSGo3KnxEa2bHqyJVVeEEcwtqJAYxerieBo/go-libp2p-crypto"
 )
 
-// ErrExpiredRecord should be returned when an ipns record is
-// invalid due to being too old
-var ErrExpiredRecord = errors.New("expired record")
-
-// ErrUnrecognizedValidity is returned when an IpnsRecord has an
-// unknown validity type.
-var ErrUnrecognizedValidity = errors.New("unrecognized validity type")
-
-// ErrInvalidPath should be returned when an ipns record path
-// is not in a valid format
-var ErrInvalidPath = errors.New("record path invalid")
-
 const PublishPutValTimeout = time.Minute
 const DefaultRecordTTL = 24 * time.Hour
 
@@ -208,7 +194,7 @@ func PublishEntry(ctx context.Context, r routing.ValueStore, ipnskey string, rec
 	}
 
 	log.Debugf("Storing ipns entry at: %s", ipnskey)
-	// Store ipns entry at "/ipns/"+b58(h(pubkey))
+	// Store ipns entry at "/ipns/"+h(pubkey)
 	return r.PutValue(timectx, ipnskey, data)
 }
 
@@ -238,114 +224,6 @@ func ipnsEntryDataForSig(e *pb.IpnsEntry) []byte {
 		[]byte{})
 }
 
-var IpnsRecordValidator = &record.ValidChecker{
-	Func: ValidateIpnsRecord,
-	Sign: true,
-}
-
-func IpnsSelectorFunc(k string, vals [][]byte) (int, error) {
-	var recs []*pb.IpnsEntry
-	for _, v := range vals {
-		e := new(pb.IpnsEntry)
-		err := proto.Unmarshal(v, e)
-		if err == nil {
-			recs = append(recs, e)
-		} else {
-			recs = append(recs, nil)
-		}
-	}
-
-	return selectRecord(recs, vals)
-}
-
-func selectRecord(recs []*pb.IpnsEntry, vals [][]byte) (int, error) {
-	var best_seq uint64
-	best_i := -1
-
-	for i, r := range recs {
-		if r == nil || r.GetSequence() < best_seq {
-			continue
-		}
-
-		if best_i == -1 || r.GetSequence() > best_seq {
-			best_seq = r.GetSequence()
-			best_i = i
-		} else if r.GetSequence() == best_seq {
-			rt, err := u.ParseRFC3339(string(r.GetValidity()))
-			if err != nil {
-				continue
-			}
-
-			bestt, err := u.ParseRFC3339(string(recs[best_i].GetValidity()))
-			if err != nil {
-				continue
-			}
-
-			if rt.After(bestt) {
-				best_i = i
-			} else if rt == bestt {
-				if bytes.Compare(vals[i], vals[best_i]) > 0 {
-					best_i = i
-				}
-			}
-		}
-	}
-	if best_i == -1 {
-		return 0, errors.New("no usable records in given set")
-	}
-
-	return best_i, nil
-}
-
-// ValidateIpnsRecord implements ValidatorFunc and verifies that the
-// given 'val' is an IpnsEntry and that that entry is valid.
-func ValidateIpnsRecord(r *record.ValidationRecord) error {
-	if r.Namespace != "ipns" {
-		return ErrInvalidPath
-	}
-
-	entry := new(pb.IpnsEntry)
-	err := proto.Unmarshal(r.Value, entry)
-	if err != nil {
-		return err
-	}
-
-	// NOTE/FIXME(#4613): We're not checking the DHT signature/author here.
-	// We're going to remove them in a followup commit and then check the
-	// *IPNS* signature. However, to do that, we need to ensure we *have*
-	// the public key and:
-	//
-	// 1. Don't want to fetch it from the network when handling PUTs.
-	// 2. Do want to fetch it from the network when handling GETs.
-	//
-	// Therefore, we'll need to either:
-	//
-	// 1. Pass some for of offline hint to the validator (e.g., using a context).
-	// 2. Ensure we pre-fetch the key when performing gets.
-	//
-	// This PR is already *way* too large so we're punting that fix to a new
-	// PR.
-	//
-	// This is not a regression, it just restores the current (bad)
-	// behavior.
-
-	// Check that record has not expired
-	switch entry.GetValidityType() {
-	case pb.IpnsEntry_EOL:
-		t, err := u.ParseRFC3339(string(entry.GetValidity()))
-		if err != nil {
-			log.Debug("failed parsing time for ipns record EOL")
-			return err
-		}
-		if time.Now().After(t) {
-			return ErrExpiredRecord
-		}
-	default:
-		return ErrUnrecognizedValidity
-	}
-	return nil
-}
-
 // InitializeKeyspace sets the ipns record for the given key to
 // point to an empty directory.
 // TODO: this doesnt feel like it belongs here

namesys/routing.go

@@ -2,7 +2,6 @@ package namesys
 
 import (
 	"context"
-	"fmt"
 	"strings"
 	"time"
 
@@ -14,8 +13,8 @@ import (
 	routing "gx/ipfs/QmTiWLZ6Fo5j4KcTVutZJ5KWRRJrbxzmxA4td8NfEdrPh7/go-libp2p-routing"
 	lru "gx/ipfs/QmVYxfoJQiZijTgPNHCHgHELvQpbsJNTg6Crmc3dQkj3yy/golang-lru"
 	proto "gx/ipfs/QmZ4Qi3GaRbjcx28Sme5eMH7RQjGkt8wHxt2a65oLaeFEV/gogo-protobuf/proto"
+	peer "gx/ipfs/QmZoWKhxUmZ2seW4BzX6fJkNR8hh9PsGModr7q171yq2SS/go-libp2p-peer"
 	mh "gx/ipfs/QmZyZDi491cCNTLfAhwcaDii2Kg4pwKRkhqQzURGDvY6ua/go-multihash"
-	ci "gx/ipfs/QmaPbCnUMBohSGo3KnxEa2bHqyJVVeEEcwtqJAYxerieBo/go-libp2p-crypto"
 	cid "gx/ipfs/QmcZfnkapfECQGcLZaf9B79NRg7cRa9EnZh4LSbkCzwNvY/go-cid"
 )
 
@@ -131,58 +130,39 @@ func (r *routingResolver) resolveOnce(ctx context.Context, name string) (path.Pa
 		return "", err
 	}
 
-	// use the routing system to get the name.
-	// /ipns/<name>
-	h := []byte("/ipns/" + string(hash))
-
-	var entry *pb.IpnsEntry
-	var pubkey ci.PubKey
-
-	resp := make(chan error, 2)
-	go func() {
-		ipnsKey := string(h)
-		val, err := r.routing.GetValue(ctx, ipnsKey)
+	// Name should be the hash of a public key retrievable from ipfs.
+	// We retrieve the public key here to make certain that it's in the peer
+	// store before calling GetValue() on the DHT - the DHT will call the
+	// ipns validator, which in turn will get the public key from the peer
+	// store to verify the record signature
+	_, err = routing.GetPublicKey(r.routing, ctx, hash)
 	if err != nil {
-			log.Debugf("RoutingResolver: dht get failed: %s", err)
-			resp <- err
-			return
+		log.Debugf("RoutingResolver: could not retrieve public key %s: %s\n", name, err)
+		return "", err
 	}
 
-		entry = new(pb.IpnsEntry)
-		err = proto.Unmarshal(val, entry)
+	pid, err := peer.IDFromBytes(hash)
 	if err != nil {
-			resp <- err
-			return
+		log.Debugf("RoutingResolver: could not convert public key hash %s to peer ID: %s\n", name, err)
+		return "", err
 	}
 
-		resp <- nil
-	}()
-
-	go func() {
-		// name should be a public key retrievable from ipfs
-		pubk, err := routing.GetPublicKey(r.routing, ctx, hash)
+	// Use the routing system to get the name.
+	// Note that the DHT will call the ipns validator when retrieving
+	// the value, which in turn verifies the ipns record signature
+	_, ipnsKey := IpnsKeysForID(pid)
+	val, err := r.routing.GetValue(ctx, ipnsKey)
 	if err != nil {
-			resp <- err
-			return
+		log.Debugf("RoutingResolver: dht get for name %s failed: %s", name, err)
+		return "", err
 	}
 
-		pubkey = pubk
-		resp <- nil
-	}()
-
-	for i := 0; i < 2; i++ {
-		err = <-resp
+	entry := new(pb.IpnsEntry)
+	err = proto.Unmarshal(val, entry)
 	if err != nil {
+		log.Debugf("RoutingResolver: could not unmarshal value for name %s: %s", name, err)
 		return "", err
 	}
-	}
-
-	// check sig with pk
-	if ok, err := pubkey.Verify(ipnsEntryDataForSig(entry), entry.GetSignature()); err != nil || !ok {
-		return "", fmt.Errorf("ipns entry for %s has invalid signature", h)
-	}
-
-	// ok sig checks out. this is a valid name.
 
 	// check for old style record:
 	valh, err := mh.Cast(entry.GetValue())
@@ -197,7 +177,7 @@ func (r *routingResolver) resolveOnce(ctx context.Context, name string) (path.Pa
 		return p, nil
 	} else {
 		// Its an old style multihash record
-		log.Debugf("encountered CIDv0 ipns entry: %s", h)
+		log.Debugf("encountered CIDv0 ipns entry: %s", valh)
 		p := path.FromCid(cid.NewCidV0(valh))
 		r.cacheSet(name, p, entry)
 		return p, nil

namesys/selector.go

+package namesys
+
+import (
+	"bytes"
+	"errors"
+
+	pb "github.com/ipfs/go-ipfs/namesys/pb"
+
+	u "gx/ipfs/QmNiJuT8Ja3hMVpBHXv3Q6dwmperaQ6JjLtpMQgMCD7xvx/go-ipfs-util"
+	proto "gx/ipfs/QmZ4Qi3GaRbjcx28Sme5eMH7RQjGkt8wHxt2a65oLaeFEV/gogo-protobuf/proto"
+)
+
+// IpnsSelectorFunc selects the best record by checking which has the highest
+// sequence number and latest EOL
+func IpnsSelectorFunc(k string, vals [][]byte) (int, error) {
+	var recs []*pb.IpnsEntry
+	for _, v := range vals {
+		e := new(pb.IpnsEntry)
+		err := proto.Unmarshal(v, e)
+		if err == nil {
+			recs = append(recs, e)
+		} else {
+			recs = append(recs, nil)
+		}
+	}
+
+	return selectRecord(recs, vals)
+}
+
+func selectRecord(recs []*pb.IpnsEntry, vals [][]byte) (int, error) {
+	var bestSeq uint64
+	besti := -1
+
+	for i, r := range recs {
+		if r == nil || r.GetSequence() < bestSeq {
+			continue
+		}
+		rt, err := u.ParseRFC3339(string(r.GetValidity()))
+		if err != nil {
+			log.Errorf("failed to parse ipns record EOL %s", r.GetValidity())
+			continue
+		}
+
+		if besti == -1 || r.GetSequence() > bestSeq {
+			bestSeq = r.GetSequence()
+			besti = i
+		} else if r.GetSequence() == bestSeq {
+			bestt, _ := u.ParseRFC3339(string(recs[besti].GetValidity()))
+			if rt.After(bestt) {
+				besti = i
+			} else if rt == bestt {
+				if bytes.Compare(vals[i], vals[besti]) > 0 {
+					besti = i
+				}
+			}
+		}
+	}
+	if besti == -1 {
+		return 0, errors.New("no usable records in given set")
+	}
+
+	return besti, nil
+}

namesys/validator.go

+package namesys
+
+import (
+	"errors"
+	"time"
+
+	pb "github.com/ipfs/go-ipfs/namesys/pb"
+	pstore "gx/ipfs/QmXauCuJzmzapetmC6W4TuDJLL1yFFrVzSHoWv8YdbmnxH/go-libp2p-peerstore"
+	peer "gx/ipfs/QmZoWKhxUmZ2seW4BzX6fJkNR8hh9PsGModr7q171yq2SS/go-libp2p-peer"
+
+	u "gx/ipfs/QmNiJuT8Ja3hMVpBHXv3Q6dwmperaQ6JjLtpMQgMCD7xvx/go-ipfs-util"
+	record "gx/ipfs/QmUpttFinNDmNPgFwKN8sZK6BUtBmA68Y4KdSBDXa8t9sJ/go-libp2p-record"
+	proto "gx/ipfs/QmZ4Qi3GaRbjcx28Sme5eMH7RQjGkt8wHxt2a65oLaeFEV/gogo-protobuf/proto"
+)
+
+// ErrExpiredRecord should be returned when an ipns record is
+// invalid due to being too old
+var ErrExpiredRecord = errors.New("expired record")
+
+// ErrUnrecognizedValidity is returned when an IpnsRecord has an
+// unknown validity type.
+var ErrUnrecognizedValidity = errors.New("unrecognized validity type")
+
+// ErrInvalidPath should be returned when an ipns record path
+// is not in a valid format
+var ErrInvalidPath = errors.New("record path invalid")
+
+// ErrSignature should be returned when an ipns record fails
+// signature verification
+var ErrSignature = errors.New("record signature verification failed")
+
+// ErrBadRecord should be returned when an ipns record cannot be unmarshalled
+var ErrBadRecord = errors.New("record could not be unmarshalled")
+
+// ErrKeyFormat should be returned when an ipns record key is
+// incorrectly formatted (not a peer ID)
+var ErrKeyFormat = errors.New("record key could not be parsed into peer ID")
+
+// ErrPublicKeyNotFound should be returned when the public key
+// corresponding to the ipns record path cannot be retrieved
+// from the peer store
+var ErrPublicKeyNotFound = errors.New("public key not found in peer store")
+
+// NewIpnsRecordValidator returns a ValidChecker for IPNS records.
+// The validator function will get a public key from the KeyBook
+// to verify the record's signature. Note that the public key must
+// already have been fetched from the network and put into the KeyBook
+// by the caller.
+func NewIpnsRecordValidator(kbook pstore.KeyBook) *record.ValidChecker {
+	// ValidateIpnsRecord implements ValidatorFunc and verifies that the
+	// given record's value is an IpnsEntry, that the entry has been correctly
+	// signed, and that the entry has not expired
+	ValidateIpnsRecord := func(r *record.ValidationRecord) error {
+		if r.Namespace != "ipns" {
+			return ErrInvalidPath
+		}
+
+		// Parse the value into an IpnsEntry
+		entry := new(pb.IpnsEntry)
+		err := proto.Unmarshal(r.Value, entry)
+		if err != nil {
+			return ErrBadRecord
+		}
+
+		// Get the public key defined by the ipns path
+		pid, err := peer.IDFromString(r.Key)
+		if err != nil {
+			log.Debugf("failed to parse ipns record key %s into peer ID", r.Key)
+			return ErrKeyFormat
+		}
+		pubk := kbook.PubKey(pid)
+		if pubk == nil {
+			log.Debugf("public key with hash %s not found in peer store", pid)
+			return ErrPublicKeyNotFound
+		}
+
+		// Check the ipns record signature with the public key
+		if ok, err := pubk.Verify(ipnsEntryDataForSig(entry), entry.GetSignature()); err != nil || !ok {
+			log.Debugf("failed to verify signature for ipns record %s", r.Key)
+			return ErrSignature
+		}
+
+		// Check that record has not expired
+		switch entry.GetValidityType() {
+		case pb.IpnsEntry_EOL:
+			t, err := u.ParseRFC3339(string(entry.GetValidity()))
+			if err != nil {
+				log.Debugf("failed parsing time for ipns record EOL in record %s", r.Key)
+				return err
+			}
+			if time.Now().After(t) {
+				return ErrExpiredRecord
+			}
+		default:
+			return ErrUnrecognizedValidity
+		}
+		return nil
+	}
+
+	return &record.ValidChecker{
+		Func: ValidateIpnsRecord,
+		Sign: false,
+	}
+}