Replace SignerFactory with global protobuf handlers for keys #734

RJPercival · 2017-07-12T11:42:59Z

Previously, when adding support for a new way of accessing keys (e.g. PKCS#11), there were two options:

Modify DefaultSignerFactory.
- Anyone using DefaultSignerFactory would be able to use this, but they'd also have any new dependencies imposed upon them.
Create a new SignerFactory implementation.
- Requires running a fork of Trillian.
- This new implementation would likely duplicate some code from DefaultSignerFactory, if it wanted to support some of the same ways of accessing keys.

An improved way of doing this is to instead have a global registry of functions that each accept a particular kind of protobuf message and return the corresponding private key. Functions can be added to this registry to provide support for new methods of accessing keys, or removed to disable particular methods. Disabled methods could be excluded from the build entirely using build tags, sparing users who do not use a particular method from having to install unnecessary dependencies.

TODO: In a follow-up PR, I intend to improve test coverage of the pre-existing crypto/keys/... packages.

RJPercival · 2017-07-12T12:13:41Z

Judging by Travis, it looks like this PR might be responsible for the slow integration tests I reported in #733. I'm not sure where the performance issue is yet though - I'll investigate shortly.

RJPercival · 2017-07-12T16:59:16Z

It turned out not to be a performance issue, but rather a sequencing pass failure that wasn't being (visibly) reported (see #738). I've now fixed that.

codingllama

Sending the comments I have so far. Github is warning me my page is out of date, so I'll refresh and resume the review.

codingllama · 2017-07-13T11:14:34Z

cmd/createtree/main.go

-	maxRootDuration                                                                          time.Duration
-	privateKeyType, pemKeyPath, pemKeyPass, pkcs11ConfigPath                                 string
-}
+var keyHandlers = make(map[string]func() (proto.Message, error))


FYI: I found it a bit odd that keyHandlers was declared here, but initialized elsewhere. There's more at a further comment.

It's because one of the handlers comes from a file that is conditionally built (if "pkcs11" build tag is used), so it can't be populated right here.

Were we going to move keyHandlers to their own file or package?

codingllama · 2017-07-13T11:19:37Z

cmd/createtree/main_test.go

+			wantTree: &nonDefaultTree,
+		},
+		{
+			desc:     "defaultOptsOnly",


Is this an error test because the private key won't be set? Maybe rename to "mandatoryOptsNotSet", or add a comment? At a glance it doesn't look different from "validOpts", it only became clearer after I realized runTest "fixes" a few flags.

That's correct. I'll rename it.

codingllama · 2017-07-13T11:25:30Z

cmd/createtree/main_test.go

+				return
+			}
+
+			if diff := pretty.Compare(tree, test.wantTree); diff != "" {


Please use proto.Equal() when comparing protos, pretty.Compare doesn't know about internal proto fields, so it may diff in cases where protos are meant to be equal.

The suggested idiom is:

if !proto.Equal(pb1, pb2) { diff := pretty.Compare(pb1, pb2) t.Errorf("%v: post-Foo diff (-got +want):\n%v", test.desc, diff) }

(Yep, it's a bit verbose, I know :/ )

That's not actually new code (the indentation has just changed), but I'll fix it up nonetheless.

codingllama · 2017-07-13T11:35:06Z

cmd/createtree/pem.go

+)
+
+func init() {
+	keyHandlers["PEMKeyFile"] = func() (proto.Message, error) {


I would prefer keyHandlers to be declared and initialized in the same file. I would also prefer all flags to be defined together (at main.go), but since this is in the main package we're not breaking the rule too much.

Suggestion: for the sake of isolation / cleaner interface between createtree and key logic, we could do something like:

At createtree/keys/keys.go:

package keys type handler func() (proto.Message, error) func SetHandler(name string, fn handler) { ... } func Handler(name string) (handler, bool) { ... } func NewPEMKeyHandler(path, pass) handler { ... }

At createtree/main.go:

func main() { keys.RegisterHandler("PEMKeyFile", keys.NewPEMKeyHandler(*path, *pass)) // ... }

WDYT?

keyHandlers can't be initialized in the same file, because the PKCS#11 handler needs to be behind a build tag. It needs to use the global registry / auto-registration pattern. I can move this registry out of main.go and into its own package though.

I think we figured out a way for everything except the RegisterHander function to be in it's own package?

codingllama · 2017-07-13T11:37:48Z

cmd/createtree/pem_test.go

+	}
+	wantTree.PrivateKey = keyProto
+
+	runTest(t, []*testCase{


Could we have runTest and testCase moved to a separate, test-library like file? I'd be ok with a testonly package inside createtree, for example.

It'll then lose access to the flags and won't be able to call createTree(). I can put them in their own file, but not their own package.

codingllama · 2017-07-13T11:41:42Z

cmd/createtree/pem_test.go

+
+	runTest(t, []*testCase{
+		{
+			desc: "empty pemKeyPath",


Since this is tested in an integration-like style, where we exercise it via createtree, I'd be tempted to use real handlers at main_test.go and move those test cases there. Or, possibly, just test the handlers directly here. If you take the comment on package separation, it'll force you down one of those roads regardless (which why I like it).

codingllama · 2017-07-13T11:47:22Z

cmd/flagsaver/flagsaver.go

+//     defer flagsaver.Save().Restore()
+//     // Test code that changes flags
+//   } // flags are reset to their original values here.
+package flagsaver


flagsaver isn't a command we run, so it shouldn't be placed inside cmd/. It definitely deserves its own package, though. Maybe util/flagsaver?

codingllama · 2017-07-13T11:48:17Z

cmd/flagsaver/flagsaver.go

+import "flag"
+
+// Stash holds flag values so that they can be restored at the end of a test.
+type Stash struct {


Maybe type Stash map[string]string ?

That would preclude extending this type in the future and expose the implementation detail that it stores a map of flag names to values. The only reason I can see to do this is to allow the user to examine stashed flag values - do you see that being a use case?

It works fine as a struct, but since you don't have other fields an alias could have worked too. It's fine as it is.

codingllama · 2017-07-13T11:51:07Z

cmd/flagsaver/flagsaver_test.go

+// Only a subset of the possible flag types are currently tested.
+func TestRestore(t *testing.T) {
+	tests := []struct {
+		// Test name


nit: "// name is ..."

Ditto for the other comments, although I think they could all be removed.

nit2: "desc string"? That seems to be the recommended format.

Nits addressed, but left the comments, since I'm not sure oldValue and newValue are completely self-explanatory (perhaps I just need better names?).

Looks good.

codingllama · 2017-07-13T11:52:04Z

cmd/flagsaver/flagsaver_test.go

+		// The value the flag should have just before being restored to oldValue.
+		newValue string
+	}{
+		{"RestoreDefaultIntValue", "int_flag", "", "666"},


nit: Please use field labels. It's clearer, saves you from specifying defaults, and saves us from having to touch all structs if a new field is added.

codingllama

In the future consider breaking PRs like this into smaller steps. It's a bit hard to figure out what changed and what was moved, since Github doesn't help much to identify the latter.

codingllama · 2017-07-13T11:59:51Z

crypto/keys/der.go

+		return nil, fmt.Errorf("der: unsupported private key type: %T", key2)
+	}
+
+	key3, err3 := x509.ParseECPrivateKey(keyDER)


Is the ordering here in any way important? Should we try EC first (assuming it is / will be the most popular one)?

No, these can be re-ordered.

Re-ordered to try EC first.

codingllama · 2017-07-13T12:01:41Z

crypto/keys/der_test.go

+	sf.AddHandler(&keyspb.PrivateKey{}, NewFromPrivateKeyProto)
+
+	for _, test := range []struct {
+		name     string


Suggestion: s/name/desc? Same for others (if you take it, ofc).

codingllama · 2017-07-13T12:02:24Z

crypto/keys/der_test.go

+			wantErr:  true,
+		},
+	} {
+		signer, err := sf.NewSigner(context.Background(), test.keyProto)


Suggestion: Declare ctx outside the loop and reuse it?

codingllama · 2017-07-13T12:04:35Z

crypto/keys/der_test.go

+
+	sf := NewSignerFactory()
+	sf.AddHandler(&keyspb.PrivateKey{}, NewFromPrivateKeyProto)
+


Should we test the handler for *keyspb.Specification? Ditto for the other public functions.

I'm planning on writing tests for all of the public functions in a follow-up PR - it that ok? I haven't added any new public functions, other than NewPrivateKeyProtoFromSpec(), that don't have tests (I'll add a test for that now).

Added test for NewPrivateKeyProtoFromSpec().

codingllama · 2017-07-13T12:08:44Z

crypto/keys/keys_test.go

 }

-func TestGenerateKey(t *testing.T) {
+func TestNewFromSpec(t *testing.T) {


We shuffled some things around, but tests seem to have lagged behind. Could we make sure that the tests are in the matching _test.go file, compared to the one the function is defined?

Done for TestNewFromSpec. I was intending to do this more thoroughly in the follow-up CL by replacing TestLoadPrivateKeyAndSign with individual tests for each of the public funcs (and put those tests in the appropriate _test.go file).

codingllama · 2017-07-13T12:19:45Z

crypto/keys/pkcs11_test.go

+
+func TestPkcs11(t *testing.T) {
+	// PKCS11Config support is tested by integration/log_integration.sh (when $WITH_PKCS11 == "true").
+	t.Skip("Only integration testing is implemented for PKCS#11")


Could we have a simple, happy path test? If we disable integration testing for some reason this will go without coverage.

This would be difficult because the PKCS#11 code calls out to https://github.com/letsencrypt/pkcs11key which attempts to load a dynamic library and communicate with a PKCS#11 interface. I could expose a package variable that allows the pkcs11key.New() call to be overridden during the tests, but that's about the only way I can think of making it unit testable.

At least by now having a skipped test for this, there is some (minimal) visibility of the fact that it lacks unit tests.

codingllama · 2017-07-13T12:23:04Z

crypto/keys/signer_factory.go

+	Generate ProtoGenerator
+
+	// handlers convert a protobuf message into a crypto.Signer.
+	handlers map[string]ProtoHandler


Have you considered making handlers global? Is there a situation where having factory-scoped handlers is useful?

A global handler registry would allow each handler-defining package to register its own handlers at init-time.

It's what I started with, but...

I've known people to be opposed to globals (e.g. @daviddrysdale lamented the scattering of flags through the codebase).

I preferred the explicitness of adding a handler, rather than it being a side-effect of importing a package, since it's quite an important decision - do you really want unencrypted keys in your storage, for instance? It also means it's easier to control which handlers are available to tests.

Scoping it to a SignerFactory means you can start servers/signers in-process (like the integration tests do) and not be forced to share the same set of handlers.

Having said that, I do recognise that making the handlers global and having them register on package init is a more common pattern. I can put together a commit that makes that change and see how it compares if you like?

We didn't seem to shy away from global state in other places, like the hashers registry, and an init-based registry would reduce the number of +build pkcs11 files reaching for signerFactories, so it seemed worth to suggest it.

That said, your counter-arguments and the wariness about global state are certainly strong points (I found number "2" above particularly convincing). I'm happy going forward the way it is. Thanks for the detailed response, Rob.

codingllama · 2017-07-13T12:23:52Z

crypto/keys/signer_factory.go

+func (f SignerFactory) AddHandler(keyProto proto.Message, handler ProtoHandler) {
+	keyProtoType := proto.MessageName(keyProto)
+
+	if _, alreadyExists := f.handlers[keyProtoType]; alreadyExists {


nit: s/alreadyExists/ok/, so we don't deviate from the idiom?

I wasn't entirely convinced that "ok" was an appropriate name here, as it's semantically the opposite - if it already exists, that's not really ok. Originally I was returning an error here, which meant it was very much not ok. Perhaps that doesn't matter though.

codingllama · 2017-07-13T12:28:33Z

crypto/signatures.go

+)
+
+// SignatureAlgorithm returns the algorithm used for this public key.
+// Only ECDSA and RSA keys are supported. Other key types will return sigpb.DigitallySigned_UNKNOWN.


nit: s/sigpb.DigitallySigned_UNKNOWN/sigpb.DigitallySigned_ANONYMOUS/

In a somewhat related comment, is anonymous ever valid / allowed? Should we differentiate between anonymous and unknown? (This is possibly out of scope.)

I think this is intended to mirror the DigitallySigned struct in RFC5246, which is why there isn't an "unknown" value. There are cases where anonymous is valid (see that RFC), but never in Trillian. Arguably, this should return an error rather than sigpb.DigitallySigned_ANONYMOUS.

Fixed typo.

codingllama · 2017-07-13T12:34:01Z

server/admin/admin_server_test.go

+		req                   *trillian.CreateTreeRequest
+		createErr             error
+		commitErr, wantCommit bool
+		wantErr               string


Asserting on error strings is likely to make tests brittle. Ideally, we'd use something more resilient to small changes, like error codes, but I that won't help if the codes are all the same. OTOH, it does make it easier to reason that the error is what you think it is, and not some problem.

WDYT?

I agree, and never usually check the error string but, in this case, there are a lot of different errors that could occur. I really want to know that the test is failing for the reason I think it is, rather than because the test is misconfigured, for example. Error codes would be a better solution except, as you suggest, I don't think they'd actually be different for most of these test cases (they're almost all cases of invalid arguments). Maybe that would be good enough for distinguishing the intended type of error from unintended errors though?

RJPercival · 2017-07-13T14:49:32Z

If you like, I think I could break this PR in two, with the first just doing some moving around of code?

codingllama · 2017-07-13T15:18:48Z

No need to break it now, I'll review the fixes commit-by-commit from now on. Just give me nudge when you finish replying to the comments.

gdbelvin · 2017-07-14T12:57:23Z

cmd/createtree/pem.go

+		}, nil
+	}
+
+	keyHandlers["PrivateKey"] = func() (proto.Message, error) {


Consider making each of these anonymous functions real functions for ease on the eye and ease of testing.

RJPercival · 2017-07-14T19:56:20Z

I've just completed some changes @gdbelvin requested offline, which swaps over to a global registry of key handlers with auto-registration on import and ultimately deletes SignerFactory. I'll take on improvement to cmd/createtree next.

gdbelvin · 2017-07-26T11:16:50Z

cmd/createtree/main.go

-}
+var keyHandlers = make(map[string]func() (proto.Message, error))

-func createTree(ctx context.Context, opts *createOpts) (*trillian.Tree, error) {


This is a great change of replacing opts with the relevant flags, but for the clarity of this already large PR, could I suggest that it be a separate PR?

Sure, it'll have to merge first though, since having the PKCS#11 flag behind a build tag is dependent on createOpts behind removed.

Done in #788.

gdbelvin · 2017-07-26T11:18:17Z

cmd/createtree/main.go

-	maxRootDuration                                                                          time.Duration
-	privateKeyType, pemKeyPath, pemKeyPass, pkcs11ConfigPath                                 string
-}
+var keyHandlers = make(map[string]func() (proto.Message, error))


Were we going to move keyHandlers to their own file or package?

gdbelvin · 2017-07-26T11:19:38Z

cmd/createtree/main.go

-		}
-		pemSigner, err := keys.NewFromPrivatePEMFile(
-			opts.pemKeyPath, opts.pemKeyPass)
+func newPK(format string) (*any.Any, error) {


This function should probably live in it's own file or package along with the keyHandlers

Also consider a clearer function name, say NewPublicKeyProto

gdbelvin · 2017-07-26T11:20:46Z

cmd/createtree/main.go

-		pemKeyPath:       *pemKeyPath,
-		pemKeyPass:       *pemKeyPassword,
-		pkcs11ConfigPath: *pkcs11ConfigPath,
+func keyFormats() []string {


A function comment would be great

Done (though it's now called RegisteredTypes() in the createtree/keys package).

gdbelvin · 2017-07-26T11:30:37Z

cmd/createtree/main_test.go

-	pemKeyTree.PrivateKey, err = ptypes.MarshalAny(&keyspb.PEMKeyFile{
-		Path:     pemPath,
-		Password: pemPassword,
+	runTest(t, []*testCase{


If you'd like to run the test in a sub function, consider using a proper SubTest
https://golang.org/pkg/testing/#hdr-Subtests_and_Sub_benchmarks

for _, tc := range []struct{}{}{ t.Run("A=1", func(t *testing.T) { ... }) }

I'm not sure this is so much a subtest as just delegating the actual test logic to a helper. However, I wonder if we should be using subtests for all of our tabular tests? It'd save having to prepend the test case name to every failure message, and defer would work nicely (running at the end of each test case, rather than at the end of the entire test).

The convention seems to prefer "regular" table-driven, but it may be because t.Run is a relatively recent addition, iirc. Maybe we should take this question to stylists? Meanwhile, I'd vouch for the simplest approach (no t.Run unless you really need it).

Edit: spelling

I consulted a stylist, and they too leaned away from using subtests, but mostly because they are quite new and rarely used so far. I've decided to use them, but inside of runTest(), where it was already using an anonymous function to isolate each test case.

See #788 for the use of subtests.

gdbelvin · 2017-07-26T11:34:23Z

cmd/createtree/main_test.go

+// the privateKeyFormat flag to "test". It also registers a "test" key handler.
+// Finally, it calls the test's setFlags func (if provided) to allow it to
+// change flags specific to the test, then performs the test.
+func runTest(t *testing.T, tests []*testCase) {


a slightly more descriptive name would be great

gdbelvin · 2017-07-26T11:34:23Z

cmd/createtree/main_test.go

+// the privateKeyFormat flag to "test". It also registers a "test" key handler.
+// Finally, it calls the test's setFlags func (if provided) to allow it to
+// change flags specific to the test, then performs the test.
+func runTest(t *testing.T, tests []*testCase) {


a slightly more descriptive name would be great - and the test should probably only run one test case at a time

They are only run one at a time.

Ah you mean that it should take a single testCase, rather than an array.

gdbelvin · 2017-07-26T11:35:31Z

cmd/createtree/pem.go

+)
+
+func init() {
+	keyHandlers["PEMKeyFile"] = func() (proto.Message, error) {


I think we figured out a way for everything except the RegisterHander function to be in it's own package?

codingllama

Travis looks unhappy, but other than that LGTM.

Is this one of the cases where we Travis won't pass because of the google/trillian <-> google/ct-go cycle?

codingllama · 2017-07-27T10:32:21Z

cmd/createtree/main_test.go

-	pemKeyTree.PrivateKey, err = ptypes.MarshalAny(&keyspb.PEMKeyFile{
-		Path:     pemPath,
-		Password: pemPassword,
+	runTest(t, []*testCase{


The convention seems to prefer "regular" table-driven, but it may be because t.Run is a relatively recent addition, iirc. Maybe we should take this question to stylists? Meanwhile, I'd vouch for the simplest approach (no t.Run unless you really need it).

Edit: spelling

codingllama · 2017-07-27T10:35:11Z

crypto/keys/der/der_test.go

+
+func TestFromProto(t *testing.T) {
+	// ECDSA private key in DER format.
+	keyDER, err := base64.StdEncoding.DecodeString("MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgS81mfpvtTmaINn+gtrYXn4XpxxgE655GLSKsA3hhjHmhRANCAASwBWDdgHS04V/cN0LZgc8vZaK4I1HWLLCoaOO27Z0B1aS1aqBE7g1Oo8ldSCBJAvee866kcHhZkVniPdCG2ZZG")


Suggestion: Move the DER string to a const?

codingllama · 2017-07-27T10:36:10Z

crypto/keys/der/der_test.go

+		}
+
+		if err := testonly.CheckKeyMatchesSpec(key, test.keySpec); err != nil {
+			t.Errorf("%v: NewProtoFromSpec() => %v", test.desc, err)


nit: s/NewProtoFromSpec/CheckKeyMatchesSpec/

codingllama · 2017-07-27T10:37:22Z

crypto/keys/der/proto/register_test.go

-func TestPrivateKeyProtoHandler(t *testing.T) {
+func TestProtoHandler(t *testing.T) {
 	// ECDSA private key in DER format.
 	keyDER, err := base64.StdEncoding.DecodeString("MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgS81mfpvtTmaINn+gtrYXn4XpxxgE655GLSKsA3hhjHmhRANCAASwBWDdgHS04V/cN0LZgc8vZaK4I1HWLLCoaOO27Z0B1aS1aqBE7g1Oo8ldSCBJAvee866kcHhZkVniPdCG2ZZG")


Ditto (move to const suggestion)

codingllama · 2017-07-27T10:39:30Z

crypto/keys/generate_test.go

+		}
+
+		if err := testonly.CheckKeyMatchesSpec(key, test.keySpec); err != nil {
+			t.Errorf("%v: NewFromSpec(): %v", test.desc, err)


nit: s/NewFromSpec/CheckKeyMatchesSpec/

codingllama · 2017-07-27T10:55:50Z

server/trillian_log_server/main.go

 	_ "github.com/go-sql-driver/mysql" // Load MySQL driver

+	// Register key ProtoHandlers
+	_ "github.com/google/trillian/crypto/keys/der/proto"


Suggestion: I've been splitting imports in the following blocks: golang, external libs, aliases and _ imports. I've found it more readable than mixing aliases and _ imports on the "external" block.

Eg:

import ( "context" "flag" "github.com/golang/glog" "github.com/golang/protobuf/proto" "github.com/google/trillian" "github.com/google/trillian/cmd" mysqlq "github.com/google/trillian/quota/mysql" _ "github.com/google/trillian/merkle/objhasher" // Load hashers _ "github.com/google/trillian/merkle/rfc6962" // Load hashers )

RJPercival · 2017-07-27T11:36:31Z

Yes, this breaks ct-go so Travis will never be happy. PR google/certificate-transparency-go#50 will fix ct-go once this merges.

AMarcedone · 2017-08-08T17:55:07Z

What's the ETA for merging this PR?

RJPercival · 2017-08-12T17:41:17Z

Apologies for the delay getting this merged, I was on holiday for a week and have had competing priorities. I've now broken this PR up as requested by @gdbelvin (see PRs #791, #792, #793, #794). Once those PRs are merged, I'll rebase this and hopefully it'll be of a more easily reviewable size.

RJPercival · 2017-08-15T23:51:52Z

This has now been rebased. The number of modified files has reduced from 63 to 46, and the number of changes in those files has reduced.

codingllama · 2017-08-16T11:20:09Z

.travis.yml

-  - GOFLAGS=-race WITH_ETCD=true
-  - GOFLAGS=      WITH_PKCS11=true
-  - GOFLAGS=-race WITH_PKCS11=true
+  - GOFLAGS=""


nit: Use single quotes

codingllama · 2017-08-16T11:20:50Z

.travis.yml

+  - GOFLAGS=""                     WITH_ETCD=true
+  - GOFLAGS="-race"                WITH_ETCD=true
+  - GOFLAGS="--tags pkcs11"        WITH_PKCS11=true
+  - GOFLAGS=""-race --tags pkcs11" WITH_PKCS11=true


There seems to be an extra double quote here.

codingllama · 2017-08-16T11:36:23Z

server/sequencer_manager_test.go

 	}
 }
+
+func fakeKeyProtoHandler(pb proto.Message, signer crypto.Signer, err error) (proto.Message, keys.ProtoHandler) {


Maybe rename pb to wantPB (or something to that effect)?

gdbelvin

Awesome. This is looking great!

Previously, when adding support for a new way of accessing keys (e.g. PKCS#11), there were two options: 1) Modify DefaultSignerFactory. - Anyone using DefaultSignerFactory would be able to use this, but they'd also have any new dependencies imposed upon them. 2) Create a new SignerFactory implementation. - Requires running a fork of Trillian. - This new implementation would likely duplicate some code from DefaultSignerFactory, if it wanted to support some of the same ways of accessing keys. An improved way of doing this is to instead have a global registry of functions that each accept a particular kind of protobuf message and return the corresponding private key. Functions can be added to this registry to provide support for new methods of accessing keys, or removed to disable particular methods. Disabled methods could be excluded from the build entirely using build tags, sparing users who do not use a particular method from having to install unnecessary dependencies.

Also splits cmd/createtree/main_test.go into two files: - tester.go, which contains shared helper functions. - main_test.go, which contains non-key-specific tests.

1. golang packages 2. third-party packages 3. packages with aliases 4. _ imports

RJPercival self-assigned this Jul 12, 2017

RJPercival requested a review from gdbelvin July 12, 2017 11:43

googlebot added the cla: yes label Jul 12, 2017

RJPercival force-pushed the signerfactory_replace branch 2 times, most recently from f70bd48 to 0e66d19 Compare July 12, 2017 12:01

RJPercival force-pushed the signerfactory_replace branch from 0e66d19 to db67304 Compare July 12, 2017 12:24

RJPercival mentioned this pull request Jul 12, 2017

Errors in sequencer aren't easily visible/exported anywhere #733

Open

RJPercival force-pushed the signerfactory_replace branch from db67304 to b353352 Compare July 12, 2017 16:58

RJPercival requested a review from codingllama July 12, 2017 17:00

RJPercival force-pushed the signerfactory_replace branch 2 times, most recently from b660ac7 to 1dd0803 Compare July 13, 2017 11:34

RJPercival added the ready for review label Jul 13, 2017

codingllama suggested changes Jul 13, 2017

View reviewed changes

RJPercival force-pushed the signerfactory_replace branch from e23c9e8 to 98bf15b Compare July 13, 2017 18:03

gdbelvin reviewed Jul 14, 2017

View reviewed changes

RJPercival force-pushed the signerfactory_replace branch 6 times, most recently from 82cdaf1 to 0cd23c3 Compare July 14, 2017 19:53

RJPercival force-pushed the signerfactory_replace branch 3 times, most recently from b57fa08 to 7905ec8 Compare July 14, 2017 20:58

RJPercival mentioned this pull request Jul 26, 2017

libtool is required as a dependency to build the client google/keytransparency#687

Closed

gdbelvin suggested changes Jul 26, 2017

View reviewed changes

RJPercival force-pushed the signerfactory_replace branch 3 times, most recently from 1ba8a68 to 4ddfb8f Compare July 26, 2017 16:01

RJPercival mentioned this pull request Jul 26, 2017

Delete uses of DefaultSignerFactory google/certificate-transparency-go#50

Merged

codingllama reviewed Jul 27, 2017

View reviewed changes

codingllama approved these changes Jul 27, 2017

View reviewed changes

liamsi mentioned this pull request Jul 28, 2017

Update Dockerfiles google/keytransparency#712

Closed

RJPercival mentioned this pull request Jul 28, 2017

Add --generate-key flag to createtree command #762

Merged

AMarcedone mentioned this pull request Aug 9, 2017

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

Merged

RJPercival mentioned this pull request Aug 10, 2017

Refactor createtree command #788

Merged

RJPercival force-pushed the signerfactory_replace branch 2 times, most recently from 9d85686 to ac4a6a3 Compare August 12, 2017 03:17

RJPercival force-pushed the signerfactory_replace branch 2 times, most recently from 6ac53cb to 57387ea Compare August 15, 2017 23:45

RJPercival changed the title ~~Replace SignerFactory interface with extensible struct~~ Replace SignerFactory with global protobuf handlers for keys Aug 15, 2017

RJPercival force-pushed the signerfactory_replace branch from 57387ea to 8d5f5e5 Compare August 15, 2017 23:53

codingllama approved these changes Aug 16, 2017

View reviewed changes

gdbelvin approved these changes Aug 16, 2017

View reviewed changes

Rob Percival added 3 commits August 16, 2017 15:35

Move cmd/createtree key handling into a separate package

c30ff67

Also splits cmd/createtree/main_test.go into two files: - tester.go, which contains shared helper functions. - main_test.go, which contains non-key-specific tests.

Make import order consistent

31f8169

1. golang packages 2. third-party packages 3. packages with aliases 4. _ imports

RJPercival force-pushed the signerfactory_replace branch from dc1a860 to 31f8169 Compare August 16, 2017 14:35

RJPercival merged commit 533c7a4 into google:master Aug 16, 2017

RJPercival deleted the signerfactory_replace branch August 16, 2017 14:57


		sf := NewSignerFactory()
		sf.AddHandler(&keyspb.PrivateKey{}, NewFromPrivateKeyProto)

Replace SignerFactory with global protobuf handlers for keys #734

Replace SignerFactory with global protobuf handlers for keys #734

Uh oh!

Conversation

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

Uh oh!

Uh oh!

RJPercival commented Jul 12, 2017

Uh oh!

RJPercival commented Jul 12, 2017

Uh oh!

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

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!

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!

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

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

RJPercival commented Jul 12, 2017 •

edited

Loading