WIP: minimalistic Monitor #709

liamsi · 2017-07-18T21:46:58Z

~~This will be split up into small separate PRs~~ (related to #679).
~~As discussed with Gary this will be merge as a single PR.~~

After further offline discussion this will be split up into two parts:

A monitor "skeleton" which does everything besides the verification
the verification logic and other changes to core (like to the protos)

Implemented:

fetch mutations from server
main function
Docker/docker-compose
basic HTTP/grpc API (will be renamed according to this)
signing and publishing result/"gossiping"
verification (of mutations, SMHs); untested

Missing:

"cleanup" (impl vs core)
proper storage of received SMH

gdbelvin · 2017-07-26T13:19:21Z

cmd/keytransparency-monitor/Dockerfile

+
+ENV VERBOSITY=1
+
+ADD keytransparency/genfiles/* /kt/


Try following rob's example

Done. I opened an issue for our other Dockerfiles (#712)

gdbelvin · 2017-07-26T13:20:17Z

cmd/keytransparency-monitor/main.go

+	ktURL      = flag.String("kt-url", "localhost:8080", "URL of key-server.")
+	ktPEM      = flag.String("kt-key", "genfiles/server.crt", "Path to kt-server's public key")
+	// TODO(ismail): are the IDs actually needed for verification operations?
+	mapID = flag.Int64("map-id", 0, "Trillian map ID")


you can remove mapID and logID

mapID is currently needed by the verifier. I've added a TODO to remove it as soon as the verifier is updated (similar to #708)

gdbelvin · 2017-07-26T13:22:04Z

cmd/keytransparency-monitor/main.go

+	"google.golang.org/grpc/credentials"
+	"google.golang.org/grpc/reflection"
+)
+


Add TODO for a prometheus variable that tracks Valid / Invalid for each Tree Head

gdbelvin · 2017-07-26T13:22:48Z

core/proto/keytransparency_v1_types/gen.go

 // limitations under the License.

-//go:generate protoc -I=. -I=$GOPATH/src/ -I=$GOPATH/src/github.com/google/trillian/ -I=$GOPATH/src/github.com/googleapis/googleapis --go_out=:. keytransparency_v1_types.proto
+//go:generate protoc -I=. -I=$GOPATH/src/ -I=$GOPATH/src/github.com/google/trillian/vendor/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis/ -I=$GOPATH/src/github.com/google/trillian/ --go_out=:. keytransparency_v1_types.proto


revert local changes

try rebasing

gdbelvin · 2017-07-26T13:26:32Z

core/proto/keytransparency_v1_types/keytransparency_v1_types.proto

+}
+
+message GetMonitoringResponse {
+  // smr contains the map root for the sparse Merkle Tree signed with the


For convenience: a pass / fail field.

Proof fields for each type of failure.

Invalid root sig

Invalid mutation from epoch e to e+1 (current interval)

inclusion to leaf at epoch e

inclusion to leaf at epoch e+1

Set of mutations does not produce new map root

map root A

inclusion proofs for changed leaves.

TODO: put this in a markdown document

Thanks! I've updated your comment to clarify that the "invalid mutations" case is only from epoch e to e+1. My understanding still is that the "storage-less" monitor doesn't have all information if it doesn't additionally store the inclusion proofs (and the corresponding index) at each epoch e. As the monitor can not know in advance (at epoch e) if there will be an error at e+1. Is that correct or do I miss sth?

@gdbelvin I've added documenting this in a markdown document to the issue's TODO list so we won't forget about it.

Note to myself: the new inclusion proof (e+1) can be computed using the mutations and the old inclusion proof.

I've added proto messages for the above cases here: 5ed5f8a

Let me know what you think.

gdbelvin · 2017-07-26T13:41:25Z

impl/monitor/monitor.go

+}
+
+// GetSignedMapRootStream is a streaming API similar to GetSignedMapRoot.
+func (s *server) GetSignedMapRootStream(in *ktpb.GetMonitoringRequest, stream mspb.MonitorService_GetSignedMapRootStreamServer) error {


Remove functions that you don't want to implement. -- including from proto

removed the stream API as I'm definitely not finishing with this PR.

gdbelvin · 2017-07-26T13:43:36Z

impl/monitor/monitor.go

+		got.GetMapRevision() == want.GetMapRevision() {
+		// We already processed this SMR. Do not update seen SMRs. Do not scroll
+		// pages for further mutations. Return empty mutations list.
+		glog.Info("Already processed this SMR. Continuing.")


include revision number

gdbelvin · 2017-07-26T13:44:17Z

impl/monitor/monitor.go

+		return nil, nil
+	}
+
+	mutations := make([]*ktpb.Mutation, pageSize*2)


Put the code to iterate through pages in a function

gdbelvin · 2017-07-26T13:44:42Z

impl/monitor/monitor.go

+	// Verify the mutation’s validity against the previous leaf.
+	// Compute the new leaf and store the intermediate hashes locally.
+	// Compute the new root using local intermediate hashes from epoch e.
+	for _, m := range mutations {


Put verification code in it's own function too

done. though, the verification code does not really verify anything yet

gdbelvin · 2017-07-26T13:47:01Z

impl/monitor/monitor.go

+
+	tree *tv.Verifier
+	// TODO(ismail) abstract this into a storage interface and have an in-memory version
+	seenSMRs          []*trillian.SignedMapRoot


Maybe create a struct for this:

SMR

Was it valid? / Error type / status

Time observed

gdbelvin · 2017-07-31T14:59:56Z

cmd/keytransparency-monitor/main.go

+	"strings"
+	"time"
+
+	mopb "github.com/google/keytransparency/impl/proto/monitor_v1_service"


please place this import group at the bottom of import(

gdbelvin · 2017-07-31T15:00:29Z

cmd/keytransparency-monitor/main.go

+
+var (
+	addr     = flag.String("addr", ":8099", "The ip:port combination to listen on")
+	keyFile  = flag.String("key", "genfiles/server.key", "TLS private key file")


Key and cert are slightly ambiguous. Should we use tls-key and tls-cert?

gdbelvin · 2017-07-31T15:01:37Z

cmd/keytransparency-monitor/main.go

+	ktURL      = flag.String("kt-url", "localhost:8080", "URL of key-server.")
+	ktPEM      = flag.String("kt-key", "genfiles/server.crt", "Path to kt-server's public key")
+	// TODO(ismail): remove mapID
+	mapID = flag.Int64("map-id", 0, "Trillian map ID")


You will need the public key of the map and log

gdbelvin · 2017-07-31T15:14:03Z

core/proto/keytransparency_v1_types/keytransparency_v1_types.proto

+  // smr will be empty. The epochs are encoded into the smr map_revision.
+  trillian.SignedMapRoot smr = 1;
+
+  // seen_timestamp_nanos contains the time in nanoseconds where this particular


We might need to create a signed version of this, but let's discuss that in the design document

We might reuse SignedEntryTimestamp in trillian.proto

Brainstorming a few options:

Update the signature field in SMR to be a set of signature timestamps.

Defining an equals method on SignedMapRoot to determine that signatures of of the same object.

Possible further ideas from RoughTime

gdbelvin · 2017-07-31T15:15:32Z

impl/monitor/monitor.go

+}
+
+func (s *Server) pollMutations(ctx context.Context, opts ...grpc.CallOption) ([]*ktpb.Mutation, error) {
+	req := &ktpb.GetMutationsRequest{PageSize: pageSize, Epoch: s.nextRevToQuery()}


When creating protobufs, consider putting each item on it's own line, or even creating the request proto inside the GetMutation line

gdbelvin · 2017-07-31T15:18:46Z

impl/monitor/monitor.go

+
+	mutations := make([]*ktpb.Mutation, pageSize*2)
+	mutations = append(mutations, resp.GetMutations()...)
+	if err := s.pageMutations(ctx, resp, mutations, opts...); err != nil {


Is there a reason pageMutations isn't creating and returning the list of mutations inside of it?
In Go, we try to avoid returning values through arguments.

gdbelvin · 2017-07-31T15:20:09Z

impl/monitor/monitor.go

+	// Update seen/processed signed map roots:
+	s.proccessedSMRs = append(s.proccessedSMRs,
+		&ktpb.GetMonitoringResponse{
+			Smr: &trillian.SignedMapRoot{


Should this just be resp.GetSmr() with an updated sig field?

As discussed there will be 2 PRs. One monitor that doesn't do anything and just signs what it sees and a separate pull for the verification logic.

gdbelvin · 2017-07-31T15:20:42Z

impl/monitor/monitor.go

+	return mutations, nil
+}
+
+func (s *Server) verifyMutations(ms []*ktpb.Mutation, expectedRoot []byte) error {


the verification logic probably deserves it's own file

gdbelvin · 2017-07-31T15:21:14Z

impl/monitor/monitor.go

+	for _, m := range ms {
+		idx := m.GetProof().GetLeaf().GetIndex()
+		nbrs := m.GetProof().GetInclusion()
+		if err := s.tree.VerifyProof(nbrs, idx, m.GetProof().GetLeaf().GetLeafValue(),


Try using the trillian verification logic

Will be moved out of this PR. But done

gdbelvin · 2017-07-31T15:21:44Z

impl/monitor/monitor.go

+	return nil
+}
+
+func (s *Server) lastSeenSMR() *trillian.SignedMapRoot {


Functions that implement the proto interface also probably deserve their own file

fix some typos

# Conflicts: # core/client/kt/requests.go # core/mutator/entry/entry.go # core/mutator/entry/entry_test.go # impl/proto/monitor_v1_service/gen.go

# Conflicts: # docker-compose.yml

gdbelvin · 2017-08-18T13:14:13Z

cmd/keytransparency-monitor/main.go

+		return nil, err
+	}
+	var creds credentials.TransportCredentials
+	if caFile != "" {


follow the example in the key transparency client, splitting up getTransportCreds

gdbelvin · 2017-08-18T13:15:39Z

core/monitor/monitor.go

+	trusted     trillian.SignedLogRoot
+}
+
+func New(hasher hashers.MapHasher, mapPubKey, logPubKey crypto.PublicKey, logVerifier merkle.LogVerifier) *Monitor {


Maybe pass in the tree structs and compute the rest locally

gdbelvin · 2017-08-18T13:17:44Z

core/monitor/verify.go

+	newLeaves := make([]merkle.HStar2LeafHash, 0, len(muts))
+	mutator := entry.New()
+	oldProofNodes := make(map[string][]byte)
+	hasher := coniks.Default


Pull from tree config

gdbelvin · 2017-08-18T13:18:54Z

core/monitor/verify.go

+		// store the proof hashes locally to recompute the tree below:
+		sibIDs := newLeafnID.Siblings()
+		for level, proof := range m.GetProof().GetInclusion() {
+			pID := sibIDs[level]


try iterating over sibIDs

liamsi · 2017-09-01T10:18:05Z

Closing this PR in favor of #776 and #768

liamsi added the WIP label Jul 18, 2017

liamsi added this to the Build Monitor milestone Jul 18, 2017

gdbelvin reviewed Jul 26, 2017

View reviewed changes

liamsi force-pushed the mvp_monitor branch from c56c6f2 to aacda4a Compare July 28, 2017 10:10

liamsi mentioned this pull request Jul 28, 2017

Monitor Implementation #679

Closed

12 tasks

liamsi force-pushed the mvp_monitor branch 6 times, most recently from a7bcc64 to 96723ec Compare July 31, 2017 14:36

gdbelvin reviewed Jul 31, 2017

View reviewed changes

liamsi added 7 commits August 14, 2017 12:38

Add helper method to Entry to avoid code duplication (tests)

0a2850d

Use helper method to Entry to avoid code duplication

4d3a88d

gofmt

f2840cd

fix some typos

compile again

01fba58

TODO verification

16db4f7

Work in progress: compute new root hash

4c8831b

Minor cleanup

a0c9404

liamsi force-pushed the mvp_monitor branch from 1e9db9e to a0c9404 Compare August 14, 2017 12:59

liamsi added 4 commits August 14, 2017 14:48

Merge branch 'master' into mvp_monitor

972eb31

Some cleanup, further splitting up into core/impl

7c54e26

Merge branch 'master' into mvp_monitor

fda51ba

# Conflicts: # core/client/kt/requests.go # core/mutator/entry/entry.go # core/mutator/entry/entry_test.go # impl/proto/monitor_v1_service/gen.go

Merge branch 'master' into mvp_monitor

3e6ed89

liamsi force-pushed the mvp_monitor branch from 983e914 to 3e6ed89 Compare August 15, 2017 11:09

liamsi added 2 commits August 18, 2017 14:06

Merge remote-tracking branch 'origin/master' into mvp_monitor

bac1ab6

Merge remote-tracking branch 'origin/master' into mvp_monitor

08e7e11

# Conflicts: # docker-compose.yml

gdbelvin reviewed Aug 18, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into mvp_monitor

de1845f

liamsi mentioned this pull request Aug 18, 2017

Monitor verification logic #768

Merged

liamsi and others added 6 commits August 18, 2017 16:21

Some of the review comments (this should go to google#768)

c3a0907

Minor changes: compiles

48b5f69

Keep imports relative

352d299

obsolete change to mutation service proto's gen.go

6f75fda

Use "autoconfig" from domain info, split transportCreds

d2aaf7d

remove libtool dependency from monitor dockerfile

e487368

liamsi mentioned this pull request Aug 21, 2017

Non-verifying monitor #776

Merged

liamsi closed this Sep 1, 2017

WIP: minimalistic Monitor #709

WIP: minimalistic Monitor #709

Uh oh!

Conversation

liamsi commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin Jul 26, 2017 • edited by liamsi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamsi Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

liamsi commented Jul 18, 2017 •

edited

Loading

gdbelvin Jul 26, 2017 •

edited by liamsi

Loading

liamsi Jul 26, 2017 •

edited

Loading

gdbelvin Jul 31, 2017 •

edited

Loading

gdbelvin Jul 31, 2017 •

edited

Loading

gdbelvin Aug 9, 2017 •

edited

Loading

gdbelvin Jul 31, 2017 •

edited

Loading

gdbelvin Jul 31, 2017 •

edited

Loading