Merge branch 'master' into reduce

* master:
  (prexisting) remove unused vars
  Fix mutation test
  Check for nil keyset
  Use IsValidEntry in Keyserver
  Separate out IsValidEntry helper
  Made logical split for mutation validation function
  update travis.yml to xenial (google#1215)

.travis.yml

@@ -1,23 +1,18 @@
 language: go
 go:
 - 1.x
-dist: trusty
-services:
-- docker-ce
+dist: xenial
+
+go_import_path: github.com/google/keytransparency
+
 cache:
   directories:
   - "$HOME/gcloud/"
 env:
   global:
-  - DOCKER_COMPOSE_VERSION="1.13.0"
   - PATH=$PATH:${HOME}/google-cloud-sdk/bin
   - CLOUDSDK_CORE_DISABLE_PROMPTS=1
 
-addons:
-  apt:
-    packages:
-    - python3-pip
-
 before_install:
   - |
     if [ ! -d $HOME/gcloud/google-cloud-sdk ]; then
@@ -47,7 +42,6 @@ after_success:
   - bash <(curl -s https://codecov.io/bash)
 
 before_deploy:
-  - pip3 install -U --user docker-compose
   - docker --version
   - docker-compose --version
   - openssl aes-256-cbc -K $encrypted_555d9b2948d2_key -iv $encrypted_555d9b2948d2_iv

cmd/keytransparency-sequencer/main.go

@@ -195,13 +195,13 @@ func main() {
 	go serveHTTPGateway(ctx, lis, dopts, grpcServer,
 		pb.RegisterKeyTransparencyAdminHandlerFromEndpoint,
 	)
-	runSequencer(ctx, conn, mconn, directoryStorage)
+	runSequencer(ctx, conn, directoryStorage)
 
 	// Shutdown.
 	glog.Errorf("Signer exiting")
 }
 
-func runSequencer(ctx context.Context, conn, mconn *grpc.ClientConn,
+func runSequencer(ctx context.Context, conn *grpc.ClientConn,
 	directoryStorage dir.Storage) {
 	electionFactory, closeFactory := getElectionFactory()
 	defer closeFactory()

cmd/keytransparency-server/main.go

@@ -121,7 +121,7 @@ func main() {
 	tmap := trillian.NewTrillianMapClient(mconn)
 
 	// Create gRPC server.
-	ksvr := keyserver.New(tlog, tmap, entry.ReduceFn, directories, logs, logs,
+	ksvr := keyserver.New(tlog, tmap, entry.IsValidEntry, directories, logs, logs,
 		prometheus.MetricFactory{})
 	grpcServer := grpc.NewServer(
 		grpc.Creds(creds),

core/keyserver/keyserver.go

@@ -81,33 +81,33 @@ type indexFunc func(ctx context.Context, d *directory.Directory, userID string)
 
 // Server holds internal state for the key server.
 type Server struct {
-	tlog        tpb.TrillianLogClient
-	tmap        tpb.TrillianMapClient
-	mutate      mutator.ReduceMutationFn
-	directories directory.Storage
-	logs        MutationLogs
-	batches     BatchReader
-	indexFunc   indexFunc
+	tlog           tpb.TrillianLogClient
+	tmap           tpb.TrillianMapClient
+	verifyMutation mutator.VerifyMutationFn
+	directories    directory.Storage
+	logs           MutationLogs
+	batches        BatchReader
+	indexFunc      indexFunc
 }
 
 // New creates a new instance of the key server.
 func New(tlog tpb.TrillianLogClient,
 	tmap tpb.TrillianMapClient,
-	mutate mutator.ReduceMutationFn,
+	verifyMutation mutator.VerifyMutationFn,
 	directories directory.Storage,
 	logs MutationLogs,
 	batches BatchReader,
 	metricsFactory monitoring.MetricFactory,
 ) *Server {
 	initMetrics.Do(func() { createMetrics(metricsFactory) })
 	return &Server{
-		tlog:        tlog,
-		tmap:        tmap,
-		mutate:      mutate,
-		directories: directories,
-		logs:        logs,
-		batches:     batches,
-		indexFunc:   indexFromVRF,
+		tlog:           tlog,
+		tmap:           tmap,
+		verifyMutation: verifyMutation,
+		directories:    directories,
+		logs:           logs,
+		batches:        batches,
+		indexFunc:      indexFromVRF,
 	}
 }
 
@@ -563,14 +563,16 @@ func (s *Server) BatchQueueUserUpdate(ctx context.Context, in *pb.BatchQueueUser
 	// - Correct profile commitment.
 	// - Correct key formats.
 	for _, u := range in.Updates {
-		if err := validateEntryUpdate(u, vrfPriv); err != nil {
+		if err := s.verifyMutation(u.Mutation); err != nil {
+			glog.Warningf("Invalid UpdateEntryRequest: %v", err)
+			return nil, status.Errorf(codes.InvalidArgument, "Invalid mutation")
+		}
+		if err = validateEntryUpdate(u, vrfPriv); err != nil {
 			glog.Warningf("Invalid UpdateEntryRequest: %v", err)
 			return nil, status.Errorf(codes.InvalidArgument, "Invalid request")
 		}
 	}
 
-	// TODO(#1177): Verify parts of the mutation that don't reference the current map value here.
-
 	// Save mutation to the database.
 	wm, err := s.logs.Send(ctx, directory.DirectoryID, in.Updates...)
 	if err != nil {

core/mutator/entry/mutation.go

@@ -19,12 +19,9 @@ import (
 	"fmt"
 
 	"github.com/golang/protobuf/proto"
+	"github.com/google/keytransparency/core/crypto/commitments"
 	"github.com/google/tink/go/keyset"
 	"github.com/google/tink/go/tink"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
-
-	"github.com/google/keytransparency/core/crypto/commitments"
 
 	pb "github.com/google/keytransparency/core/api/v1/keytransparency_go_proto"
 	tinkpb "github.com/google/tink/proto/tink_go_proto"
@@ -37,7 +34,6 @@ type Mutation struct {
 	UserID      string
 	data, nonce []byte
 
-	prevEntry       *pb.Entry
 	prevSignedEntry *pb.SignedEntry
 	entry           *pb.Entry
 	signedEntry     *pb.SignedEntry
@@ -113,19 +109,9 @@ func (m *Mutation) SerializeAndSign(signers []tink.Signer) (*pb.EntryUpdate, err
 		return nil, err
 	}
 
-	// Check authorization.
-	if err := verifyKeys(m.prevEntry.GetAuthorizedKeys(), m.entry.GetAuthorizedKeys(),
-		mutation.Entry, mutation.Signatures); err != nil {
-		return nil, status.Errorf(codes.PermissionDenied,
-			"verifyKeys(oldKeys: %v, newKeys: %v, sigs: %v): %v",
-			len(m.prevEntry.GetAuthorizedKeys().GetKey()),
-			len(m.entry.GetAuthorizedKeys().GetKey()),
-			len(mutation.GetSignatures()), err)
-	}
-
 	// Sanity check the mutation's correctness.
 	if _, err := MutateFn(m.prevSignedEntry, mutation); err != nil {
-		return nil, fmt.Errorf("presign mutation check: %v", err)
+		return nil, err
 	}
 
 	return &pb.EntryUpdate{

core/mutator/entry/mutator.go

@@ -17,14 +17,15 @@ package entry
 import (
 	"bytes"
 	"crypto/sha256"
+	"errors"
 	"fmt"
 
-	"github.com/google/keytransparency/core/mutator"
+	"github.com/golang/glog"
+	"github.com/golang/protobuf/proto"
 	"github.com/google/tink/go/keyset"
 	"github.com/google/tink/go/signature"
 
-	"github.com/golang/glog"
-	"github.com/golang/protobuf/proto"
+	"github.com/google/keytransparency/core/mutator"
 
 	pb "github.com/google/keytransparency/core/api/v1/keytransparency_go_proto"
 	tinkpb "github.com/google/tink/proto/tink_go_proto"
@@ -44,6 +45,23 @@ func MapLogItemFn(m *mutator.LogMessage, emit func(index []byte, mutation *pb.En
 	return nil
 }
 
+// IsValidEntry checks the internal consistency and correctness of a mutation.
+func IsValidEntry(signedEntry *pb.SignedEntry) error {
+	// Ensure that the mutation size is within bounds.
+	if got, want := proto.Size(signedEntry), mutator.MaxMutationSize; got > want {
+		glog.Warningf("mutation is %v bytes, want <= %v", got, want)
+		return mutator.ErrSize
+	}
+
+	newEntry := pb.Entry{}
+	if err := proto.Unmarshal(signedEntry.GetEntry(), &newEntry); err != nil {
+		return fmt.Errorf("proto.Unmarshal(): %v", err)
+	}
+
+	ks := newEntry.GetAuthorizedKeys()
+	return verifyKeys(ks, signedEntry.GetEntry(), signedEntry.GetSignatures())
+}
+
 // ReduceFn decides which of multiple updates can be applied in this revision.
 func ReduceFn(index []byte, msgs []*pb.EntryUpdate, leaves []*tpb.MapLeaf, emit func(*tpb.MapLeaf)) error {
 	if got := len(leaves); got > 1 {
@@ -87,17 +105,16 @@ func ReduceFn(index []byte, msgs []*pb.EntryUpdate, leaves []*tpb.MapLeaf, emit
 // MutateFn verifies that newSignedEntry is a valid mutation for oldSignedEntry and returns the
 // application of newSignedEntry to oldSignedEntry.
 func MutateFn(oldSignedEntry, newSignedEntry *pb.SignedEntry) (*pb.SignedEntry, error) {
-	// Ensure that the mutation size is within bounds.
-	if got, want := proto.Size(newSignedEntry), mutator.MaxMutationSize; got > want {
-		glog.Warningf("mutation is %v bytes, want <= %v", got, want)
-		return nil, mutator.ErrSize
+	if err := IsValidEntry(newSignedEntry); err != nil {
+		return nil, err
 	}
 
 	newEntry := pb.Entry{}
 	if err := proto.Unmarshal(newSignedEntry.GetEntry(), &newEntry); err != nil {
 		return nil, fmt.Errorf("proto.Unmarshal(): %v", err)
 	}
 	oldEntry := pb.Entry{}
+	// oldSignedEntry may be nil, resulting an empty oldEntry struct.
 	if err := proto.Unmarshal(oldSignedEntry.GetEntry(), &oldEntry); err != nil {
 		return nil, fmt.Errorf("proto.Unmarshal(): %v", err)
 	}
@@ -107,17 +124,21 @@ func MutateFn(oldSignedEntry, newSignedEntry *pb.SignedEntry) (*pb.SignedEntry,
 	prevEntryHash := sha256.Sum256(oldSignedEntry.GetEntry())
 	if got, want := prevEntryHash[:], newEntry.GetPrevious(); !bytes.Equal(got, want) {
 		// Check if this mutation is a replay.
-		if oldSignedEntry != nil &&
-			bytes.Equal(oldSignedEntry.Entry, newSignedEntry.Entry) {
+		if bytes.Equal(oldSignedEntry.GetEntry(), newSignedEntry.Entry) {
 			glog.Warningf("mutation is a replay of an old one")
 			return nil, mutator.ErrReplay
 		}
 		glog.Warningf("previous entry hash: %x, want %x", got, want)
 		return nil, mutator.ErrPreviousHash
 	}
 
-	if err := verifyKeys(oldEntry.GetAuthorizedKeys(), newEntry.GetAuthorizedKeys(),
-		newSignedEntry.Entry, newSignedEntry.GetSignatures()); err != nil {
+	if oldSignedEntry == nil {
+		// Skip verificaion checks if there is no previous oldSignedEntry.
+		return newSignedEntry, nil
+	}
+
+	ks := oldEntry.GetAuthorizedKeys()
+	if err := verifyKeys(ks, newSignedEntry.Entry, newSignedEntry.GetSignatures()); err != nil {
 		return nil, err
 	}
 
@@ -126,16 +147,12 @@ func MutateFn(oldSignedEntry, newSignedEntry *pb.SignedEntry) (*pb.SignedEntry,
 
 // verifyKeys verifies both old and new authorized keys based on the following
 // criteria:
-//   1. At least one signature with a key in the previous entry should exist.
-//   2. If prevAuthz is nil, at least one signature with a key from the new
-//   authorized_key set should exist.
-//   3. Signatures with no matching keys are simply ignored.
-func verifyKeys(prevAuthz, authz *tinkpb.Keyset, data []byte, sigs [][]byte) error {
-	ks := prevAuthz
-	if prevAuthz == nil {
-		ks = authz
+//   1. At least one signature with a key in the entry should exist.
+//   2. Signatures with no matching keys are simply ignored.
+func verifyKeys(ks *tinkpb.Keyset, data []byte, sigs [][]byte) error {
+	if ks == nil {
+		return errors.New("entry: nil keyset")
 	}
-
 	handle, err := keyset.NewHandleWithNoSecrets(ks)
 	if err != nil {
 		return fmt.Errorf("tink.KeysetHanldeWithNoSecret(new): %v", err)

core/mutator/entry/mutator_test.go

@@ -109,7 +109,8 @@ func TestCheckMutation(t *testing.T) {
 					AuthorizedKeys: testutil.VerifyKeysetFromPEMs(testPubKey2).Keyset(),
 				},
 			},
-			err: mutator.ErrReplay,
+			signers: testutil.SignKeysetsFromPEMs(testPrivKey1, testPrivKey2),
+			err:     mutator.ErrReplay,
 		},
 		{
 			desc: "Large mutation",
@@ -132,16 +133,19 @@ func TestCheckMutation(t *testing.T) {
 					AuthorizedKeys: testutil.VerifyKeysetFromPEMs(testPubKey2).Keyset(),
 				},
 			},
-			err: mutator.ErrPreviousHash,
+			signers: testutil.SignKeysetsFromPEMs(testPrivKey1, testPrivKey2),
+			err:     mutator.ErrPreviousHash,
 		},
 		{
 			desc: "Very first mutation, invalid previous entry hash",
 			mutation: &Mutation{
 				entry: &tpb.Entry{
-					Previous: nil,
+					Previous:       nil,
+					AuthorizedKeys: testutil.VerifyKeysetFromPEMs(testPubKey2).Keyset(),
 				},
 			},
-			err: mutator.ErrPreviousHash,
+			signers: testutil.SignKeysetsFromPEMs(testPrivKey2),
+			err:     mutator.ErrPreviousHash,
 		},
 		{
 			desc: "Very first mutation, missing previous signature",
@@ -168,7 +172,7 @@ func TestCheckMutation(t *testing.T) {
 					AuthorizedKeys: testutil.VerifyKeysetFromPEMs(testPubKey2).Keyset(),
 				},
 			},
-			signers: testutil.SignKeysetsFromPEMs(testPrivKey1),
+			signers: testutil.SignKeysetsFromPEMs(testPrivKey1, testPrivKey2),
 		},
 	} {
 		m, err := tc.mutation.sign(tc.signers)

core/mutator/mutator.go

@@ -20,6 +20,9 @@ package mutator
 import (
 	"errors"
 
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+
 	pb "github.com/google/keytransparency/core/api/v1/keytransparency_go_proto"
 	tpb "github.com/google/trillian"
 )
@@ -42,9 +45,12 @@ var (
 	ErrInvalidSig = errors.New("mutation: invalid signature")
 	// ErrUnauthorized occurs when the mutation has not been signed by a key in the
 	// previous entry.
-	ErrUnauthorized = errors.New("mutation: unauthorized")
+	ErrUnauthorized = status.Errorf(codes.PermissionDenied, "mutation: unauthorized")
 )
 
+// VerifyMutationFn verifies that a mutation is internally consistent.
+type VerifyMutationFn func(mutation *pb.SignedEntry) error
+
 // ReduceMutationFn takes all the mutations for an index an an auxiliary input
 // of existing mapleaf(s) returns a new map leaf.  ReduceMutationFn must be
 // idempotent, communative, and associative. i.e. must produce the same output

impl/integration/env.go

@@ -181,7 +181,7 @@ func NewEnv(ctx context.Context) (*Env, error) {
 
 	pb.RegisterKeyTransparencyServer(gsvr, keyserver.New(
 		logEnv.Log, mapEnv.Map,
-		entry.ReduceFn, directoryStorage,
+		entry.IsValidEntry, directoryStorage,
 		mutations, mutations,
 		monitoring.InertMetricFactory{},
 	))