TLS LogRootV1 Format #1037

gdbelvin · 2018-03-02T12:41:25Z

This PR is part of a multi step process updating the SignedLogRoot struct to look like the following:

message SignedLogRoot {
   bytes key_hint
   bytes log_root
   bytes signature
}

This PR defines the serialization and deserialization functions for log_root.

#958

daviddrysdale

BTW, any Log-specific comments usually apply to the Map- variants too

daviddrysdale · 2018-03-02T14:16:26Z

crypto/data_formats.go

+func ParseLogRoot(b []byte) (*LogRootV1, error) {
+	// Verify version
+	version := binary.BigEndian.Uint16(b)
+	if got, want := version, uint16(trillian.LogRootFormat_LOG_ROOT_FORMAT_V1); got != want {


Personally, I'm not a big fan of using the got, want:=... thing outside of unit tests, unless there's a calculation to avoid repeating -- I think it's easier to see what's going on with:

if version != uint16(trillian.LogRootFormat_LOG_ROOT_FORMAT_V1) {

(But I know other opinions vary...)

daviddrysdale · 2018-03-02T14:17:42Z

crypto/data_formats.go

+	Metadata       []byte `tls:"minlen:0,maxlen:65535"`
+}
+
+// ParseLogRoot returns a *SignedLogRootV1


Comment is incorrect; also, it doesn't really add any extra information over and above the method signature.

Updated the comment to
ParseLogRoot verifies that b has the LOG_ROOT_FORMAT_V1 tag and returns a *LogRootV1

daviddrysdale · 2018-03-02T14:19:59Z

crypto/data_formats.go

+	TimestampNanos uint64
+	Revision       uint64
+	Metadata       []byte `tls:"minlen:0,maxlen:65535"`
+}


Do we need LogID in the structure too?

We discussed including the LogID in the struct, but decided against it in order to prevent a malicious log from falsely representing it's LogID. Clients should derive the LogID from the valid signature associated with SignedLogRoot

daviddrysdale · 2018-03-02T14:36:45Z

crypto/data_formats.go

+
+// LogRootV1 contains the fields verified by SignedLogRoot
+type LogRootV1 struct {
+	Version        uint16


IIUC this field will always have value 1 (i.e. LogRootV1.Version==LOG_ROOT_FORMAT_V1 is an invariant)? If so, a comment to that effect might be helpful.

Also, if this is effectively an enum, it might be worth using the tls.Enum type (with a size:2 tag).

More generally, I wonder whether using variants might be more future proof -- e.g. something in TLS-speak like:

enum { v1(1), (65535)} Version; struct { uint64 tree_size; opaque root_hash<0..128>; uint64 timestamp_nanos; uint64 revision; opaque metadata<0..65535>; } LogRootV1; struct { Version version; select(version) { case v1: LogRootV1; } } LogRoot;

is then easily extendable to:

enum { v1(1), v2(2), (65535)} Version; ... struct { uint64 tree_size; opaque root_hash<0..128>; uint64 timestamp_nanos; uint64 revision; opaque metadata<0..65535>; opaque something_else<0..255>; } LogRootV2; struct { Version version; select(version) { case v1: LogRootV1; case v2: LogRootV2; } } LogRoot;

(might well be overkill though)

Great idea :-)

daviddrysdale · 2018-03-02T14:46:25Z

crypto/data_formats.go

@@ -53,3 +55,66 @@ func HashLogRoot(root trillian.SignedLogRoot) ([]byte, error) {
 	}
 	return hash[:], nil
 }
+
+// LogRootV1 contains the fields verified by SignedLogRoot


Having these type definitions in crypto/data_formats.go feels a bit out of the way, given that this is a data type that forms part of the external API to Trillian. It's not a proto structure so we can't directly have it in the top-level .proto files that define the API, but we could do something like:

Add a top-level types.go file with these API types as Go types.

Add (in a later PR) comments to the .proto files that say something like:
// The log_root field holds the TLS-serialization of the following // structure (described in RFC5246 notation): // struct { // uint16 version; // uint64 tree_size; // opaque root_hash<0..128>; // uint64 timestamp_nanos; // uint64 revision; // opaque metadata<0..65535>; // } LogRootV1;

That way the .proto files would contain an unambiguous description of the API details, and a non-Go user of Trillian would have enough information to be able to use the API.

I agree on both points. Unfortunately, I can't add non-proto code to a proto package.
I'm tempted to move the proto definitions to api/v1/trillian_proto so we can use the top level trillian package name for this type of stuff.

In the following pull requests, I'll be defining methods to verify SignedLogRoot that will return LogRootV1 upon verification. That was the rational for keeping these methods in the crypto package. Should I create a crypto/signature package?

daviddrysdale · 2018-03-02T14:48:34Z

crypto/data_formats_test.go

+		if err != nil {
+			t.Errorf("SerializeLogRoot(%v): %v", logRoot, err)
+		}
+		r2, err := ParseLogRoot(b)


Just call this got and skip the aliasing below.

Done, but are we sure that skipping the aliasing best matches go style?

I didn't think the aliasing was the point, more that things are named something-like "got" and "want". E.g. the example in the docs doesn't introduce a if want := tt.want; ... alias

daviddrysdale · 2018-03-02T14:51:04Z

crypto/data_formats_test.go

 	"testing"

 	"github.com/google/trillian"
 	"github.com/google/trillian/testonly"
+	"github.com/kr/pretty"


This is not the pretty package we use everywhere else.

daviddrysdale · 2018-03-02T14:51:56Z

crypto/data_formats_test.go

+func TestLogRoot(t *testing.T) {
+	for _, logRoot := range []*LogRootV1{
+		{
+			Version:  1,


Need test cases exercising version overwriting during serialization.

Refactored to remove this sharp edge.

daviddrysdale · 2018-03-02T14:55:57Z

crypto/data_formats_test.go

+		{
+			logRoot: func() []byte {
+				b, _ := SerializeLogRoot(&LogRootV1{})
+				b[0] = 1


A comment would be helpful here.

Also/alternatively, it might be nice if the tests distinguished errors -- I think this one is checking that a wrong version (0x101) gets bounced (and so the error will be something containing "Invalid MapRoot.Version"), whereas the test below is a plain parse failure.

It looks like the error types are internal to the tls package. It's difficult to reliably discern what the error was from the outside?

I often do something like if !strings.Contains(err.Error(), test.wantErr) { and put a relevant substring in the test definition ("failed to parse", "trailing data", etc.) -- that seems to get a decent balance between specificity and brittleness.

codecov-io · 2018-03-06T12:14:07Z

Codecov Report

Merging #1037 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1037      +/-   ##
==========================================
+ Coverage   62.56%   62.69%   +0.12%     
==========================================
  Files         101      103       +2     
  Lines        8379     8408      +29     
==========================================
+ Hits         5242     5271      +29     
  Misses       2610     2610              
  Partials      527      527

Impacted Files	Coverage Δ
crypto/data_formats.go	`77.77% <ø> (ø)`	⬆️
types/maproot.go	`100% <100%> (ø)`
types/logroot.go	`100% <100%> (ø)`
trees/trees.go	`83.67% <0%> (-1.52%)`	⬇️
trees/types.go	`100% <0%> (ø)`	⬆️

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 c5bde67...e073651. Read the comment docs.

gdbelvin · 2018-03-06T12:15:08Z

Updated and ready for a second look

daviddrysdale · 2018-03-08T13:56:04Z

types/logroot.go

+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+


Can we have a package comment in exactly one of the files in types ? (Some stricter linters check for this.)

daviddrysdale · 2018-03-08T14:00:35Z

types/logroot.go

+	if b == nil {
+		return nil, fmt.Errorf("nil log root")
+	}
+	// Verify version


(nit) As it stands, this comment doesn't really add anything that the code doesn't say clearly. But it could -- eg. by explaining why you do the version check first.

I'll remove the comment. As it stands, this check is not strictly needed because of the way we're using TLS variants, but it's a nice belt and braces check for others that might look to use this as a hint for other languages and parsers.

daviddrysdale · 2018-03-08T14:02:23Z

types/logroot.go

+	Metadata       []byte `tls:"minlen:0,maxlen:65535"`
+}
+
+// LogRoot contains the fields serialized into SignedLogRoot.LogRoot


If this is going to be API documentation, it would be good to say how serialized

Added a description of the TLS serialization

daviddrysdale · 2018-03-08T14:04:20Z

types/logroot.go

+	V1      *LogRootV1 `tls:"selector:Version,val:1"`
+}
+
+// ParseLogRoot verifies that b has the LOG_ROOT_FORMAT_V1 tag and returns a *LogRootV1


Be nice to describe the expected form of b -- it's not just bytes, it's supposed to be bytes that hold a TLS-serialized LogRoot structure.

daviddrysdale · 2018-03-08T14:05:46Z

types/logroot.go

+	return logRoot.V1, nil
+}
+
+// SerializeLogRoot returns a canonical TLS serialization of the log root.


Maybe include a reference or two to RFC 5246 section 4 scattered around -- I don't think TLS encoding is as well-known as other encodings.

daviddrysdale · 2018-03-08T14:18:55Z

crypto/data_formats_test.go

+		{
+			logRoot: func() []byte {
+				b, _ := SerializeLogRoot(&LogRootV1{})
+				b[0] = 1


I often do something like if !strings.Contains(err.Error(), test.wantErr) { and put a relevant substring in the test definition ("failed to parse", "trailing data", etc.) -- that seems to get a decent balance between specificity and brittleness.

daviddrysdale · 2018-03-08T14:25:32Z

types/logroot_test.go

+		{
+			logRoot: []byte("foo"),
+			wantErr: true,
+		},


Could we have a more adversarial test case or two here? One that starts with a valid version field but has nonsense thereafter? Or which has a 129 byte root_hash (which will decode fine but should hit a runtime limit)

daviddrysdale · 2018-03-08T14:26:45Z

crypto/data_formats_test.go

+func TestLogRoot(t *testing.T) {
+	for _, logRoot := range []*LogRootV1{
+		{
+			Version:  1,


daviddrysdale

(As before log* comments => map*... )

Looking good, just doc changes + a test suggestion

gdbelvin · 2018-03-08T15:28:46Z

Added tests and documentation for both logs and maps.

I often do something like if !strings.Contains(err.Error(), test.wantErr)

is a good idea but it doesn't work well when err is nil. Do you know of any reliable function to stringify err and not panic on nil?

daviddrysdale · 2018-03-08T17:42:27Z

types/logroot.go

+	"github.com/google/trillian"
+)
+
+// LogRootV1 holds the TLS-deserializtion of the following structure


"deserialization"

daviddrysdale · 2018-03-08T17:42:42Z

types/logroot.go

+	Metadata       []byte `tls:"minlen:0,maxlen:65535"`
+}
+
+// LogRoot holds the TLS-deserializtion of the following structure


"deserialization"

daviddrysdale · 2018-03-09T15:00:13Z

types/logroot_test.go

+			Metadata: []byte{},
+		},
+	} {
+		b, err := logRoot.MarshalBinary()


Could alter the test so it handles objects of encoding.BinaryMarshaller and encoding.BinaryUnmarshaller interfaces ? That would implicitly check that the interfaces are indeed satisfied.

gdbelvin requested a review from daviddrysdale March 2, 2018 12:41

googlebot added the cla: yes label Mar 2, 2018

gdbelvin requested a review from AlCutter March 2, 2018 12:41

gdbelvin force-pushed the f/sig/logroot_format branch 3 times, most recently from 8283200 to 3dd45fd Compare March 2, 2018 13:23

daviddrysdale reviewed Mar 2, 2018

View reviewed changes

google deleted a comment from codecov-io Mar 6, 2018

gdbelvin force-pushed the f/sig/logroot_format branch from 3dd45fd to ab4818d Compare March 6, 2018 12:14

gdbelvin force-pushed the f/sig/logroot_format branch 2 times, most recently from d91d546 to 29bb73f Compare March 8, 2018 10:59

daviddrysdale reviewed Mar 8, 2018

View reviewed changes

gdbelvin added 7 commits March 8, 2018 15:02

Define enums for signature formats

0e80915

LogRoot format

427b7d4

Use TLS variants

1843670

Move LogRoot and MapRoot to types package

5b8786b

add glog so ci tests pass

a50a47e

Add nil checks to types package

e8cf97c

Add documentation and tests

42bf9b0

gdbelvin force-pushed the f/sig/logroot_format branch from 29bb73f to 42bf9b0 Compare March 8, 2018 15:27

Use MarshalBinary interface

65c2eb5

daviddrysdale reviewed Mar 9, 2018

View reviewed changes

Use marshler interfaces in tests

e073651

daviddrysdale approved these changes Mar 9, 2018

View reviewed changes

gdbelvin merged commit 16362da into google:master Mar 9, 2018

gdbelvin deleted the f/sig/logroot_format branch March 9, 2018 16:44

gdbelvin mentioned this pull request Mar 9, 2018

Use LogRootV1 with SignLogRoot / VerifyLogRoot #1048

Merged

gdbelvin mentioned this pull request Mar 26, 2018

Better tests for types package #1070

Merged

daviddrysdale mentioned this pull request Apr 17, 2018

Update changelog for 1.0.9 #1089

Closed

TLS LogRootV1 Format #1037

TLS LogRootV1 Format #1037

Conversation

gdbelvin commented Mar 2, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 6, 2018 • edited Loading

Codecov Report

gdbelvin commented Mar 6, 2018

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

Choose a reason for hiding this comment

gdbelvin commented Mar 8, 2018 • edited Loading

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

gdbelvin commented Mar 2, 2018 •

edited

Loading

codecov-io commented Mar 6, 2018 •

edited

Loading

gdbelvin commented Mar 8, 2018 •

edited

Loading