Add gobindClient package and a gobind friendly interface to core/client/kt/verify.go by AMarcedone · Pull Request #736 · google/keytransparency

AMarcedone · 2017-08-09T02:04:03Z

This PR is the first step in modifying the go code to make it gobind friendly.

It allows an android app to make a GetEntry request (which implicitly verifies the response) or to verify a wire encoded proto GetEntryResponse obtained separately.

The code is not ready to merge (you can see lots of TODOs), but early feedback is appreciated.
Will update with a reference to the corresponding PR in keytransparency-java to give you an idea of how the code is used.

AMarcedone · 2017-08-09T02:08:34Z

This depends on google/trillian#734 being merged first. In particular, I am using this trillian: https://github.com/AMarcedone/trillian/tree/signerfactory_replace

gdbelvin · 2017-08-09T10:57:07Z

Would you mind rebasing this on #716 ?
That should provide a slightly easier interface to work with and avoids creating too many wrapper functions.

gdbelvin · 2017-08-09T10:59:38Z

cmd/keytransparency-client/cmd/root.go

 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials"
 	"google.golang.org/grpc/credentials/oauth"
+	"github.com/google/trillian/crypto/keys/pem"


Looks like this file needs a gofmt to keep the imports alphabetized
I've just configured gometalinter for this project. You might enjoy linking it up to your IDE.

gdbelvin · 2017-08-09T11:00:54Z

core/client/gobindClient/client.go

+package gobindClient
+
+import (
+	"context"


It's our convention to organize the imports as follows:

import( "golang imports" "keytransparency and trillian" "third party imports" "underscore and renamed imports" )

gdbelvin · 2017-08-09T11:02:36Z

core/client/gobindClient/client.go

+	LogPEM       []byte
+}
+
+// TODO(amarcedone) consider persisting the client or at least the trusted smr, to gain efficiency and stronger consistency guarantees.


Indeed, the design is for the client to persist the trusted SMR itself.
Please create a place for it. Later on we can store and retrieve it from storage etc.

gdbelvin · 2017-08-09T11:02:57Z

core/client/gobindClient/client.go

+
+// TODO(amarcedone) consider persisting the client or at least the trusted smr, to gain efficiency and stronger consistency guarantees.
+
+func NewBClientParams(KtURL string, MapID int64, KtTLSCertPEM, VrfPubPEM, KtSigPubKey, LogPEM []byte) *BClientParams {


Would it be easier to pass the PEMs as strings?

gdbelvin · 2017-08-09T11:04:10Z

core/client/gobindClient/client.go

+	return &BClientParams{KtURL, MapID, cKtTLSCertPEM, cVrfPubPEM, cKtSigPubKey, cLogPEM}
+}
+
+func BGetEntry(timeoutInMilliseconds int, clientParams *BClientParams, userID, appID string) ([]byte, error) {


BClientParams is interesting. It looks like it's functioning as a class object?

Could we make it a proper class object?

If not, it should at least be the first parameter in all function calls.

It can be a proper class (i.e. one can use the syntax BClientParams.MyMethod(myparam) ), but to be directly visible from the Java code it has to only contain members which are gobind friendly.

Another option to increase performance would be to create a client and save it as a global variable of this package, and have GetEntry just use this global client to make requests.
On the java side, we can ensure that there is only one copy of the client available (so only one set of client parameters such as kt address/verification keys can be used at the same time).
Even more, we could use a global variable to store a map of clients (essentially getting around the gobind limitation of not being able to pass around references to arbitrary objects by creating our own reference mechanism), but not sure if that is worth it. Probably one variable as reference would be good for now.

Any thoughts?

gdbelvin · 2017-08-09T11:05:31Z

core/client/gobindClient/client.go

+	}
+	log := client.NewLogVerifier(hasher, logPubKey)
+
+	verifier, err := keymaster.NewVerifierFromPEM(clientParams.KtSigPubKey)


consider using the trillian/crypto/keys package for this

gdbelvin · 2017-08-09T11:06:24Z

core/client/gobindClient/client.go

+// Local copy of io.Writer interface used to redirect logs.
+type BWriter interface {
+	Write(p []byte) (n int, err error)
+}


add newline?

gdbelvin · 2017-08-09T11:07:53Z

core/client/kt/verify.go

+		return err
+	}
+
+	// TODO(amarcedone) Is context.Background() appropriate here?


Context should be created from whatever the equivalent of main is, and passed in as the first argument.

AMarcedone · 2017-08-09T19:17:17Z

Regarding the rebase, this PR does not expose a commitment interface directly to gobind so I do not think it would make any difference.

…ated through gobind. Add support for fetching entries from go directly (i.e. client.GetEntry) and to verify a proto GetEntryResponse obtained separately.

…gobindClient

gdbelvin · 2017-08-23T16:34:01Z

core/client/gobindClient/client.go

+	return nil
+}
+
+func BInit(timeoutInMs int32) error {


Let's not use B as a function prefix

gdbelvin · 2017-08-23T16:34:37Z

core/client/gobindClient/client.go

+
+func BAddKtServer(ktURL string, insecureTLS bool, ktTLSCertPEM []byte, domainInfoHash []byte) error {
+	if _, exists := clients[ktURL]; exists == true {
+		fmt.Errorf("The KtServer connection for %v already exists", ktURL)


return error?

gdbelvin · 2017-08-23T16:35:12Z

core/client/gobindClient/client.go

+		fmt.Errorf("The KtServer connection for %v already exists", ktURL)
+	}
+
+	// TODO Add URL validation here.


TODO(username):

gdbelvin · 2017-08-23T16:36:45Z

core/client/gobindClient/client.go

+		return []byte{}, err
+	}
+
+	client, exists := clients[ktURL]


go style calls exists ok

gdbelvin · 2017-08-23T16:38:24Z

core/client/gobindClient/client.go

+)
+
+var (
+	initialized bool


remove initalized

gdbelvin · 2017-08-23T16:38:28Z

core/client/gobindClient/client.go

+	Vlog = log.New(ioutil.Discard, "", 0)
+)
+
+func checkInitialized() error {


gdbelvin · 2017-08-23T16:38:34Z

core/client/gobindClient/client.go

+
+func BGetEntry(ktURL, userID, appID string) ([]byte, error) {
+
+	if err := checkInitialized(); err != nil {


gdbelvin · 2017-08-23T17:01:07Z

core/client/gobindClient/client.go

+	timeout = time.Duration(ms) * time.Millisecond
+}
+
+func AddKtServer(ktURL string, insecureTLS bool, ktTLSCertPEM []byte, domainInfoHash []byte) error {


AddKTServer

gdbelvin

This is looking good. Just make Travis happy.

…gcat, as well as standard log. Optionally, vlog can be redirected to a custom java writer

gdbelvin · 2017-08-23T20:06:45Z

core/client/gobindClient/client.go

+var (
+	clients map[string]*grpcc.Client = make(map[string]*grpcc.Client)
+
+	timeout time.Duration = time.Duration(500) * time.Millisecond


just remove the extra cast

gdbelvin · 2017-08-23T20:08:17Z

core/client/gobindClient/multiLog.go

+	if m.writers == nil {
+		m.writers = []io.Writer{}
+	}
+	log.Printf("Added writer: %v", w)


remove extra logging statements

gdbelvin · 2017-08-23T20:09:53Z

core/client/gobindClient/multiLog.go

+
+type MultiIoWriter struct {
+	writers []io.Writer
+}


This belongs in it's own package. client/mutlilogger
Then you can do multilogger.New

AMarcedone mentioned this pull request Aug 9, 2017

Set up AndroidStudio project. Create Android app able to make GetEntry requests google/keytransparency-java#1

Closed

gdbelvin added the WIP label Aug 9, 2017

gdbelvin reviewed Aug 9, 2017

View reviewed changes

AMarcedone mentioned this pull request Aug 15, 2017

Add example android app able to make GetEntry requests google/keytransparency-java#2

Closed

AMarcedone force-pushed the gobind_getEntry branch from 1fc04ce to ade075c Compare August 15, 2017 18:52

AMarcedone mentioned this pull request Aug 17, 2017

Add the keytransparency android library google/keytransparency-java#3

Closed

AMarcedone added 4 commits August 22, 2017 08:49

Create the gobindClient package, which allows a client to be instanti…

c52a7f0

…ated through gobind. Add support for fetching entries from go directly (i.e. client.GetEntry) and to verify a proto GetEntryResponse obtained separately.

Remove dead code.

45079cd

Remove unused imports

4314dad

gobindClient can now autoconfig from the server DomainInfo service.

7006ebf

AMarcedone force-pushed the gobind_getEntry branch from 90f971c to 7006ebf Compare August 22, 2017 16:18

AMarcedone added 3 commits August 22, 2017 15:28

Add a list of client connection at the package level, indexed by ktURL

fb3ae6e

Revert changes to verify.go, which are not needed at the moment

6085433

Add a warning to the logs when TLS cert validation is skipped by the …

d534468

…gobindClient

gdbelvin reviewed Aug 23, 2017

View reviewed changes

core/client/gobindClient/client.go

return []byte{}, err

}

client, exists := clients[ktURL]

Copy link

Contributor

gdbelvin Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go style calls exists ok

Reorganized import

2aa81d3

gdbelvin reviewed Aug 23, 2017

View reviewed changes

core/client/gobindClient/client.go Outdated

)

var (

initialized bool

Copy link

Contributor

gdbelvin Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove initalized

gdbelvin reviewed Aug 23, 2017

View reviewed changes

core/client/gobindClient/client.go Outdated

Vlog = log.New(ioutil.Discard, "", 0)

)

func checkInitialized() error {

Copy link

Contributor

gdbelvin Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

gdbelvin reviewed Aug 23, 2017

View reviewed changes

Removed initialization. Minor nits.

22e09da

gdbelvin reviewed Aug 23, 2017

View reviewed changes

gdbelvin approved these changes Aug 23, 2017

View reviewed changes

Restructured logging. When using gobindClient, Verbose log goes to lo…

1b8173b

…gcat, as well as standard log. Optionally, vlog can be redirected to a custom java writer

gdbelvin reviewed Aug 23, 2017

View reviewed changes

AMarcedone added 2 commits August 23, 2017 18:05

Address review comments and make check issues

0b1ae8c

Capitalize gobindclient

92734f4

gdbelvin approved these changes Aug 23, 2017

View reviewed changes

gdbelvin removed the WIP label Aug 23, 2017

Merge branch 'master' into gobind_getEntry

8900333

gdbelvin merged commit df5f639 into google:master Aug 23, 2017


		// TODO(amarcedone) consider persisting the client or at least the trusted smr, to gain efficiency and stronger consistency guarantees.

		func NewBClientParams(KtURL string, MapID int64, KtTLSCertPEM, VrfPubPEM, KtSigPubKey, LogPEM []byte) *BClientParams {


		func BGetEntry(ktURL, userID, appID string) ([]byte, error) {

		if err := checkInitialized(); err != nil {

Comments

Conversation

AMarcedone commented Aug 9, 2017

Uh oh!

AMarcedone commented Aug 9, 2017

Uh oh!

gdbelvin commented Aug 9, 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!

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!

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!

AMarcedone 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!

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!

AMarcedone commented Aug 9, 2017

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 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gdbelvin commented Aug 9, 2017 •

edited

Loading

gdbelvin Aug 9, 2017 •

edited

Loading

AMarcedone Aug 9, 2017 •

edited

Loading