Replace `SignedLogRoot.LogID` with `SignedLogRoot.KeyHint`. #1049

gdbelvin · 2018-03-09T18:17:05Z

Trillian logs can misrepresent their LogID in SignedLogRoot. Clients SHOULD derive the LogID associated with a SignedLogRoot object via a valid signature with that log's public key.

In situations where a SignedLogRoot could be coming from any number of known logs, the KeyHint field is provided as a shortcut to quickly identify the correct public key to use for verification.

This PR associates LogID / KeyID with the signer object, which will automatically include it when generating a SignedLogRoot object.

#958

codecov-io · 2018-03-09T18:38:37Z

Codecov Report

Merging #1049 into master will increase coverage by 0.02%.
The diff coverage is 68.18%.

@@            Coverage Diff             @@
##           master    #1049      +/-   ##
==========================================
+ Coverage   62.63%   62.65%   +0.02%     
==========================================
  Files         103      103              
  Lines        8432     8448      +16     
==========================================
+ Hits         5281     5293      +12     
- Misses       2624     2627       +3     
- Partials      527      528       +1

Impacted Files	Coverage Δ
log/sequencer.go	`75.63% <0%> (ø)`	⬆️
storage/mysql/log_storage.go	`68.86% <100%> (ø)`	⬆️
crypto/signer.go	`69.23% <71.42%> (-5.24%)`	⬇️
types/logroot.go	`92.85% <81.81%> (-7.15%)`	⬇️
util/tracker.go	`100% <0%> (+3.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b39433d...c867aa5. Read the comment docs.

daviddrysdale · 2018-03-13T14:09:33Z

'Diffbase' isn't really something that works with GitHub -- could you rebase and regenerate please?

gdbelvin · 2018-03-13T14:37:26Z

It kind of works if one reviews commit by commit, but it's certainly not great.
Rebased and ready for your inspection.

gdbelvin · 2018-03-13T14:38:53Z

storage/cloudspanner/log_storage.go

@@ -357,9 +356,6 @@ func (tx *logTX) StoreSignedLogRoot(ctx context.Context, root trillian.SignedLog
 		return err
 	}

-	if got, want := root.LogId, tx.treeID; got != want {


Now that we're not storing LogID inside LogRootV1 there is no longer duplicate fields to keep in sync.

daviddrysdale

I'm not really getting the general point here -- it looks like you're replacing an int encoded as an int with an int encoded as [8]byte; what's the gain?

daviddrysdale · 2018-03-13T14:41:34Z

crypto/signer.go

 	Hash   crypto.Hash
 	Signer crypto.Signer
 }

+// NewSigner returns a new annotated signer.


Needs a comment to explain what "annotated" means.

Done where?

Must have gotten lost. Fixed comment again.

daviddrysdale · 2018-03-13T14:49:31Z

crypto/signer.go

@@ -35,10 +35,20 @@ var sigpbHashLookup = map[crypto.Hash]sigpb.DigitallySigned_HashAlgorithm{
 // Signer is responsible for signing log-related data and producing the appropriate
 // application specific signature objects.
 type Signer struct {
+	KeyID  int64


BTW, is this used for anything other than diagnostics/logging?

At the moment it's only used for logging to my knowledge.
In the future when we have gossip, this will help gossipers identify which public key to use to verify SignedLogRoots

Now I understand better I wonder: should this be KeyHint []byte ?

That would be more consistent. Done.

daviddrysdale · 2018-03-13T14:51:48Z

crypto/signer.go

+		Signer: signer,
+	}
+}
+
 // NewSHA256Signer creates a new SHA256 based Signer.


Is this function going to stick around?

If it is, then it should probably get some comment to indicate that the resulting signed isn't associated with a particular key ID. Also, the changes to various ..._test.go files might not be needed.

I would like to remove NewSha256Signer. I've got a PR queued up for the certificatetransparency-go repo, but obviously this PR needs to be merged first.

I'll add a TODO to remove it.

daviddrysdale · 2018-03-13T14:53:43Z

quota/mysqlqm/mysql_quota_test.go

@@ -304,7 +304,9 @@ func createTree(ctx context.Context, db *sql.DB) (*trillian.Tree, error) {
 	{
 		ls := mysql.NewLogStorage(db, nil)
 		err := ls.ReadWriteTransaction(ctx, tree, func(ctx context.Context, tx storage.LogTreeTX) error {
-			return tx.StoreSignedLogRoot(ctx, trillian.SignedLogRoot{LogId: tree.TreeId, RootHash: []byte{0}, Signature: &sigpb.DigitallySigned{}})
+			return tx.StoreSignedLogRoot(ctx, trillian.SignedLogRoot{


(nit) Although it's just a test, why not use types.SerializeKeyHint here too?

I'll use a proper signer.

daviddrysdale · 2018-03-13T14:54:29Z

storage/mysql/storage_test.go

@@ -260,7 +260,6 @@ func createLogForTests(db *sql.DB) int64 {
 	l := NewLogStorage(db, nil)
 	err = l.ReadWriteTransaction(ctx, tree, func(ctx context.Context, tx storage.LogTreeTX) error {
 		if err := tx.StoreSignedLogRoot(ctx, trillian.SignedLogRoot{
-			LogId:     tree.TreeId,


daviddrysdale · 2018-03-13T14:55:09Z

trillian.proto

@@ -217,8 +217,9 @@ message SignedLogRoot {
  int64 tree_size = 3;
  sigpb.DigitallySigned signature = 4;

-  int64 log_id = 5;
+  reserved 5; // (moved to key_hint)


This is an API change - are you sure no Trillian users rely on the presence of log_id?

This is an API change indeed. I'm a little less clear on whether there are no Trillian users that use log_id. I've run the ct-go unit tests against it, so I'm pretty confident that nothing in the Google ecosystem uses it. Not sure how to detect other uses.

daviddrysdale · 2018-03-13T14:56:33Z

trillian.proto

  int64 tree_revision = 6;
+  bytes key_hint = 7;


The trillian.proto file is effectively the external API definition, so it would be good to have a comment explaining what's in the bytes, and what format it's in.

daviddrysdale · 2018-03-13T14:58:42Z

(and thanks for the rebase -- much easier to see diffs now)

gdbelvin · 2018-03-15T16:07:30Z

The idea is that LogID can be maliciously set. In keeping with the philosophy that everything a Trillian Log produces should be verified, clients derive the logID from the public key associated with a valid signature.

Using []byte also allows the KeyHint to be generalized to include things other than LogIDs. Perhaps a GUID or something in the future or for non Trillian logs.

daviddrysdale · 2018-03-15T18:14:14Z

OK, after some offline chats I think I'm catching up -- sorry for missing out on the background to all of this. Let me try explaining things back to see if I've got it right:

Thing A: as a general rule, it's safest to check a signature before doing any decoding, so you only decode things that are trusted. So we should be shipping things around the place as blob + signature-of-blob, and only after checking signature-of-blob is it safe to (say) TLS-decode blob into a structure.

But which public key do you use to verify the signature?

From the perspective of a Trillian client getting an RPC response, this is actually easy: the client only gets back a signed log root (SLR) as a response from a Trillian server after a request for a specific tree, so the client only ever tries to verify the signature with the public key associated with that <trillian-server + treeID>. Easy peasy, O(1), and no need for any identification of which tree is involved.

However, these SLRs may get gossiped around the place, at which point anyone who wants to look at them after the fact doesn't know where they came from, and can't trust anyone who claims to tell them where they came from. So checking the signature (thing A) has the O(N) problem of trying every public key they know about.

Here's where the key hint helps, to make the normal case O(1) again: use the key hint to find the right public key, verify-sig then deserialize-blob. However, if we're dealing with a big pile of SLRs from all sorts of places, the treeID isn't enough as a unique identifier -- different Trillian servers can re-use the same treeIDs, so something like (say) <trillian-log-uri, treeID> might be needed.

In other words, the key hint is context-specific:

when talking (just) to a specific Trillian server, treeID is enough
when talking more widely, it will need to have a more globally unique identifier
and so []byte allows for both possibilities.

This also means that the contents of the key hint will change as context changes. For example, a client of a specific Trillian server may get an SLR with a key hint, but when it forwards that SLR onwards into a bigger pool it would change the key hint to <serverURI, treeID> -- without needing to change the blob, signature-over-blob parts.

(Of course, anything that's having to parse a tree_hint will need to do so carefully in case of malicious contents, but any key hint format for a particular environment should be simple enough that it's possible to do safely...)

Is that roughly right?

daviddrysdale · 2018-03-15T18:15:44Z

@AlCutter too.

gdbelvin · 2018-03-15T21:37:21Z

You got it. All of the above is correct. I'll add the following two thoughts:

We might want to discourage clients from ever parsing key_hint and only have them use it as an opaque string / lookup in a table.
Great observation about the contextual nature of LogID. In order to minimize mucking around as gossip goes forward we might want to set key_hint to be a GUID? This GUID and its relationship to the public key would have to be publically maintained somewhere. Alternatively we can make key_hint the first x bytes of the public key or some other function of the public key.

daviddrysdale · 2018-03-16T10:23:41Z

trillian.proto

+  // key_hint is a hint to identify the public key for signature verification.
+  // key_hint is not authenticated and may be incorrect or missing, in which
+  // case all known public keys may be used to verify the signature.
+  // key_hint MAY contain the LogID encoded as a big endian 64 bit integer.


Maybe add something about context: "When directly communicating with a Trillian gRPC server, the key_hint will typically contain the LogID encoded as a big-endian 64-bit integer; however, in other contexts the key_hint is likely to have different contents (e.g. it could be Trillian URL + tree ID, or it could be data derived from the public key itself)."

gdbelvin · 2018-03-16T14:58:18Z

Added additional key_hint context documentation.

googlebot added the cla: yes label Mar 9, 2018

gdbelvin requested a review from daviddrysdale March 13, 2018 13:47

gdbelvin force-pushed the f/sig/keyhint branch from 56017ee to e21c52d Compare March 13, 2018 14:36

gdbelvin commented Mar 13, 2018

View reviewed changes

daviddrysdale reviewed Mar 13, 2018

View reviewed changes

gdbelvin mentioned this pull request Mar 13, 2018

Move SignedLogRoot fields into LogRootV1 #1052

Merged

gdbelvin mentioned this pull request Mar 15, 2018

Use NewSigner API google/certificate-transparency-go#180

Merged

daviddrysdale reviewed Mar 16, 2018

View reviewed changes

gdbelvin added 8 commits March 16, 2018 17:57

Add KeyHint to crypto.NewSigner

8b6d354

Add keyhint to SignedLogRoot

35b8624

SerializeKeyHint

44d3d31

Use KeyHint throughout

59489b5

Remove log_id from proto

2ce5fd8

Reviewer comments

076ebce

fix build

cf83c61

Expand key_hint doc

fe037c1

gdbelvin force-pushed the f/sig/keyhint branch from 2c21d2f to fe037c1 Compare March 16, 2018 17:57

Use KeyHint in signer

c867aa5

gdbelvin force-pushed the f/sig/keyhint branch from 5313c34 to c867aa5 Compare March 19, 2018 11:47

daviddrysdale approved these changes Mar 19, 2018

View reviewed changes

gdbelvin merged commit cf9cd7c into google:master Mar 19, 2018

gdbelvin deleted the f/sig/keyhint branch March 19, 2018 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `SignedLogRoot.LogID` with `SignedLogRoot.KeyHint`. #1049

Replace `SignedLogRoot.LogID` with `SignedLogRoot.KeyHint`. #1049

gdbelvin commented Mar 9, 2018 •

edited

Loading

codecov-io commented Mar 9, 2018 •

edited

Loading

daviddrysdale commented Mar 13, 2018

gdbelvin commented Mar 13, 2018

gdbelvin Mar 13, 2018

daviddrysdale left a comment

daviddrysdale Mar 13, 2018

gdbelvin Mar 13, 2018

daviddrysdale Mar 15, 2018

gdbelvin Mar 19, 2018

daviddrysdale Mar 13, 2018

gdbelvin Mar 13, 2018

daviddrysdale Mar 16, 2018

gdbelvin Mar 19, 2018

daviddrysdale Mar 13, 2018

gdbelvin Mar 13, 2018

gdbelvin Mar 13, 2018

daviddrysdale Mar 13, 2018

gdbelvin Mar 13, 2018

daviddrysdale Mar 13, 2018

gdbelvin Mar 13, 2018

daviddrysdale Mar 13, 2018

gdbelvin Mar 13, 2018

daviddrysdale Mar 13, 2018

gdbelvin Mar 13, 2018

daviddrysdale commented Mar 13, 2018

gdbelvin commented Mar 15, 2018

daviddrysdale commented Mar 15, 2018

daviddrysdale commented Mar 15, 2018

gdbelvin commented Mar 15, 2018 •

edited

Loading

daviddrysdale Mar 16, 2018

gdbelvin Mar 16, 2018

gdbelvin commented Mar 16, 2018

Replace SignedLogRoot.LogID with SignedLogRoot.KeyHint. #1049

Replace SignedLogRoot.LogID with SignedLogRoot.KeyHint. #1049

Conversation

gdbelvin commented Mar 9, 2018 • edited Loading

codecov-io commented Mar 9, 2018 • edited Loading

Codecov Report

daviddrysdale commented Mar 13, 2018

gdbelvin commented Mar 13, 2018

Choose a reason for hiding this comment

daviddrysdale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddrysdale commented Mar 13, 2018

gdbelvin commented Mar 15, 2018

daviddrysdale commented Mar 15, 2018

daviddrysdale commented Mar 15, 2018

gdbelvin commented Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdbelvin commented Mar 16, 2018

Replace `SignedLogRoot.LogID` with `SignedLogRoot.KeyHint`. #1049

Replace `SignedLogRoot.LogID` with `SignedLogRoot.KeyHint`. #1049

gdbelvin commented Mar 9, 2018 •

edited

Loading

codecov-io commented Mar 9, 2018 •

edited

Loading

gdbelvin commented Mar 15, 2018 •

edited

Loading