From b909fd901e5649077349292218b5d7e89e03b75b Mon Sep 17 00:00:00 2001 From: Gary Belvin Date: Thu, 17 Aug 2017 17:24:04 +0100 Subject: [PATCH 1/4] Fix ListHistory command Cleans up a few off by on errors and poor stopping condition logic in the list history command. Fixes #763 --- cmd/keytransparency-client/grpcc/grpc_client.go | 16 +++++++++++----- core/keyserver/keyserver.go | 2 -- core/keyserver/validate.go | 6 ------ 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/cmd/keytransparency-client/grpcc/grpc_client.go b/cmd/keytransparency-client/grpcc/grpc_client.go index b3082197e..89576a4c7 100644 --- a/cmd/keytransparency-client/grpcc/grpc_client.go +++ b/cmd/keytransparency-client/grpcc/grpc_client.go @@ -170,14 +170,15 @@ func min(x, y int32) int32 { // ListHistory returns a list of profiles starting and ending at given epochs. // It also filters out all identical consecutive profiles. -// Epochs start at 1. func (c *Client) ListHistory(ctx context.Context, userID, appID string, start, end int64, opts ...grpc.CallOption) (map[*trillian.SignedMapRoot][]byte, error) { - if start <= 0 { - return nil, fmt.Errorf("start=%v, want > 0", start) + if start < 0 { + return nil, fmt.Errorf("start=%v, want >= 0", start) } var currentProfile []byte profiles := make(map[*trillian.SignedMapRoot][]byte) - for start <= end { + epochsReceived := int64(0) + epochsWant := end - start + for epochsReceived < epochsWant { resp, err := c.cli.ListEntryHistory(ctx, &tpb.ListEntryHistoryRequest{ UserId: userID, AppId: appID, @@ -187,6 +188,7 @@ func (c *Client) ListHistory(ctx context.Context, userID, appID string, start, e if err != nil { return nil, err } + epochsReceived += int64(len(resp.GetValues())) for i, v := range resp.GetValues() { Vlog.Printf("Processing entry for %v, epoch %v", userID, start+int64(i)) @@ -207,11 +209,15 @@ func (c *Client) ListHistory(ctx context.Context, userID, appID string, start, e currentProfile = profile } if resp.NextStart == 0 { - return nil, ErrIncomplete // No more data. + break // No more data. } start = resp.NextStart // Fetch the next block of results. } + if epochsReceived < epochsWant { + return nil, ErrIncomplete + } + return profiles, nil } diff --git a/core/keyserver/keyserver.go b/core/keyserver/keyserver.go index 0f4221474..b24dd6945 100644 --- a/core/keyserver/keyserver.go +++ b/core/keyserver/keyserver.go @@ -43,8 +43,6 @@ const ( defaultPageSize = 16 // Maximum allowed requested page size to prevent DOS. maxPageSize = 16 - // If no epoch is provided default to epoch 1. - defaultStartEpoch = 1 ) // Server holds internal state for the key server. diff --git a/core/keyserver/validate.go b/core/keyserver/validate.go index b81987ca0..8df8361ac 100644 --- a/core/keyserver/validate.go +++ b/core/keyserver/validate.go @@ -109,12 +109,6 @@ func validateListEntryHistoryRequest(in *tpb.ListEntryHistoryRequest, currentEpo if in.Start < 0 || in.Start > currentEpoch { return ErrInvalidStart } - // TODO(ismail): make epochs consistently start from 0 and provide a function - // such that callers don't need convert between starting 0 and 1. - // Ensure a valid start epoch is provided if the Start parameter is not set. - if in.Start == 0 { - in.Start = defaultStartEpoch - } switch { case in.PageSize < 0: From 5300887070f794d165d111008c173d0355a9f383 Mon Sep 17 00:00:00 2001 From: Gary Belvin Date: Thu, 17 Aug 2017 18:01:34 +0100 Subject: [PATCH 2/4] Start history at epoch 1. Unfortunately, epoch 0 is an invaid request --- cmd/keytransparency-client/cmd/history.go | 2 +- core/keyserver/keyserver.go | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/keytransparency-client/cmd/history.go b/cmd/keytransparency-client/cmd/history.go index 670faa7d6..a777c733c 100644 --- a/cmd/keytransparency-client/cmd/history.go +++ b/cmd/keytransparency-client/cmd/history.go @@ -98,6 +98,6 @@ func (m mapHeads) Less(i, j int) bool { return m[i].MapRevision < m[j].MapRevisi func init() { RootCmd.AddCommand(histCmd) - histCmd.PersistentFlags().Int64Var(&start, "start", 0, "Start epoch") + histCmd.PersistentFlags().Int64Var(&start, "start", 1, "Start epoch") histCmd.PersistentFlags().Int64Var(&end, "end", 0, "End epoch") } diff --git a/core/keyserver/keyserver.go b/core/keyserver/keyserver.go index b24dd6945..52e77c5d6 100644 --- a/core/keyserver/keyserver.go +++ b/core/keyserver/keyserver.go @@ -99,6 +99,10 @@ func (s *Server) GetEntry(ctx context.Context, in *tpb.GetEntryRequest) (*tpb.Ge func (s *Server) getEntry(ctx context.Context, userID, appID string, firstTreeSize, epoch int64) (*tpb.GetEntryResponse, error) { index, proof := s.vrf.Evaluate(vrf.UniqueID(userID, appID)) + if epoch == 0 { + return nil, grpc.Errorf(codes.InvalidArgument, + "Epoch 0 is inavlid. The first map revision is epoch 1.") + } getResp, err := s.tmap.GetLeaves(ctx, &trillian.GetMapLeavesRequest{ MapId: s.mapID, From e9ab50eb7ad30d983e212cccf8e2e3a53417aee0 Mon Sep 17 00:00:00 2001 From: Gary Belvin Date: Fri, 18 Aug 2017 11:24:53 +0100 Subject: [PATCH 3/4] Update integration tests --- core/fake/trillian_log_client.go | 11 +++++++++-- core/signer/signer.go | 1 + integration/testutil.go | 26 +++++++++++++++----------- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/core/fake/trillian_log_client.go b/core/fake/trillian_log_client.go index 3c6a9d441..feb69c7af 100644 --- a/core/fake/trillian_log_client.go +++ b/core/fake/trillian_log_client.go @@ -20,7 +20,9 @@ import ( "google.golang.org/grpc" ) -type logServer struct{} +type logServer struct { + treeSize int64 +} // NewFakeTrillianLogClient returns a fake trillian log client. func NewFakeTrillianLogClient() trillian.TrillianLogClient { @@ -28,6 +30,7 @@ func NewFakeTrillianLogClient() trillian.TrillianLogClient { } func (l *logServer) QueueLeaf(ctx context.Context, in *trillian.QueueLeafRequest, opts ...grpc.CallOption) (*trillian.QueueLeafResponse, error) { + l.treeSize++ return nil, nil } @@ -48,7 +51,11 @@ func (l *logServer) GetConsistencyProof(ctx context.Context, in *trillian.GetCon } func (l *logServer) GetLatestSignedLogRoot(ctx context.Context, in *trillian.GetLatestSignedLogRootRequest, opts ...grpc.CallOption) (*trillian.GetLatestSignedLogRootResponse, error) { - return &trillian.GetLatestSignedLogRootResponse{}, nil + return &trillian.GetLatestSignedLogRootResponse{ + SignedLogRoot: &trillian.SignedLogRoot{ + TreeSize: l.treeSize, + }, + }, nil } func (l *logServer) GetSequencedLeafCount(ctx context.Context, in *trillian.GetSequencedLeafCountRequest, opts ...grpc.CallOption) (*trillian.GetSequencedLeafCountResponse, error) { diff --git a/core/signer/signer.go b/core/signer/signer.go index 9412e1c94..ff0c2a154 100644 --- a/core/signer/signer.go +++ b/core/signer/signer.go @@ -113,6 +113,7 @@ func (s *Signer) Initialize(ctx context.Context) error { // add the empty map root to the log. if logRoot.GetSignedLogRoot().GetTreeSize() == 0 && mapRoot.GetMapRoot().GetMapRevision() == 0 { + glog.Infof("Initializing Trillian Log with empty map root") if err := queueLogLeaf(ctx, s.tlog, s.logID, mapRoot.GetMapRoot()); err != nil { return err } diff --git a/integration/testutil.go b/integration/testutil.go index 64f1b62e1..1b07051c9 100644 --- a/integration/testutil.go +++ b/integration/testutil.go @@ -134,15 +134,6 @@ func NewEnv(t *testing.T) *Env { t.Fatalf("CreateTree(): %v", err) } mapID := tree.TreeId - if _, err := mapEnv.MapClient.SetLeaves(ctx, &trillian.SetMapLeavesRequest{ - MapId: mapID, - Leaves: nil, - MapperData: &trillian.MapperMetadata{ - HighestFullyCompletedSeq: 0, - }, - }); err != nil { - t.Fatalf("SetLeaves(): %v", err) - } mapPubKey, err := der.UnmarshalPublicKey(tree.GetPublicKey().GetDer()) if err != nil { @@ -186,10 +177,23 @@ func NewEnv(t *testing.T) *Env { if err != nil { t.Fatalf("Dial(%v) = %v", addr, err) } - client := grpcc.New(cc, vrfPub, mapPubKey, coniks.Default, - fake.NewFakeTrillianLogVerifier()) + client := grpcc.New(cc, vrfPub, mapPubKey, coniks.Default, fake.NewFakeTrillianLogVerifier()) client.RetryCount = 0 + // Mimic first sequence event + if err := signer.Initialize(ctx); err != nil { + t.Fatalf("signer.Initialize() = %v", err) + } + if _, err := mapEnv.MapClient.SetLeaves(ctx, &trillian.SetMapLeavesRequest{ + MapId: mapID, + Leaves: nil, + MapperData: &trillian.MapperMetadata{ + HighestFullyCompletedSeq: 0, + }, + }); err != nil { + t.Fatalf("SetLeaves(): %v", err) + } + return &Env{ mapEnv: mapEnv, GRPCServer: s, From 3f676156683830c6141e68b364e986e4e7733e42 Mon Sep 17 00:00:00 2001 From: Gary Belvin Date: Fri, 18 Aug 2017 11:37:38 +0100 Subject: [PATCH 4/4] fix --- cmd/keytransparency-client/grpcc/grpc_client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/keytransparency-client/grpcc/grpc_client.go b/cmd/keytransparency-client/grpcc/grpc_client.go index 89576a4c7..8860ae6e2 100644 --- a/cmd/keytransparency-client/grpcc/grpc_client.go +++ b/cmd/keytransparency-client/grpcc/grpc_client.go @@ -177,7 +177,7 @@ func (c *Client) ListHistory(ctx context.Context, userID, appID string, start, e var currentProfile []byte profiles := make(map[*trillian.SignedMapRoot][]byte) epochsReceived := int64(0) - epochsWant := end - start + epochsWant := end - start + 1 for epochsReceived < epochsWant { resp, err := c.cli.ListEntryHistory(ctx, &tpb.ListEntryHistoryRequest{ UserId: userID,