Address review comments

fabiolb · Feb 16, 2018 · 9126048 · 9126048
1 parent cfbb0cd
commit 9126048
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 8 deletions.
diff --git a/proxy/tcp/tls_clienthello.go b/proxy/tcp/tls_clienthello.go
@@ -7,7 +7,7 @@ import "errors"
 // handshake message header.
 // The function requires at least the first 9 bytes of the tls conversation
 // in "data".
-// 0, error is returned if the data does not follow the
+// An error is returned if the data does not follow the
 // specification (https://tools.ietf.org/html/rfc5246) or if the client hello
 // is fragmented over multiple records.
 func clientHelloBufferSize(data []byte) (int, error) {

diff --git a/proxy/tcp/tls_clienthello_test.go b/proxy/tcp/tls_clienthello_test.go
@@ -10,74 +10,83 @@ func TestClientHelloBufferSize(t *testing.T) {
 		name string
 		data []byte
 		size int
+		fail bool
 	}{
 		{
 			name: "valid data",
 			// Largest possible client hello message
 			//                            |- 16384 -|      |----- 16380 ----|
 			data: []byte{0x16, 0x03, 0x01, 0x40, 0x00, 0x01, 0x00, 0x3f, 0xfc},
 			size: 16384 + 5, // max record length + record header
+			fail: false,
 		},
 		{
 			name: "not enough data",
 			data: []byte{0x16, 0x03, 0x01, 0x40, 0x00, 0x01, 0x00, 0x3f},
 			size: 0,
+			fail: true,
 		},
 		{
 			name: "not a TLS record",
 			data: []byte{0x15, 0x03, 0x01, 0x01, 0xF4, 0x01, 0x00, 0x01, 0xeb},
 			size: 0,
+			fail: true,
 		},
 
 		{
 			name: "TLS record too large",
 			//                             | max + 1 |
 			data: []byte{0x16, 0x03, 0x01, 0x40, 0x01, 0x01, 0x00, 0x3f, 0xfc},
 			size: 0,
+			fail: true,
 		},
 
 		{
 			name: "TLS record length zero",
 			//                            |----------|
 			data: []byte{0x16, 0x03, 0x01, 0x00, 0x00, 0x01, 0x00, 0x3f, 0xfc},
 			size: 0,
+			fail: true,
 		},
 
 		{
 			name: "Not a client hello",
 			//                                        |----|
 			data: []byte{0x16, 0x03, 0x01, 0x40, 0x00, 0x02, 0x00, 0x3f, 0xfc},
 			size: 0,
+			fail: true,
 		},
 
 		{
 			name: "Invalid handshake message record length",
 			//                                              |----- 0 --------|
 			data: []byte{0x16, 0x03, 0x01, 0x40, 0x00, 0x01, 0x00, 0x00, 0x00},
 			size: 0,
+			fail: true,
 		},
 
 		{
 			name: "Fragmentation (handshake message larger than record)",
 			//                            |-  500 ---|      |----- 497 ------|
 			data: []byte{0x16, 0x03, 0x01, 0x01, 0xF4, 0x01, 0x00, 0x01, 0xf1},
 			size: 0,
+			fail: true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			got, err := clientHelloBufferSize(tt.data)
-			want := tt.size
-			if got != want {
-				t.Fatalf("want size %d, got %d", want, got)
-			}
 
-			// Function doc says returned length of 0 should be accompanied by an error
-			if got == 0 && err == nil {
-				t.Fatalf("expected error, got nil")
+			if tt.fail && err == nil {
+				t.Fatal("expected error, got nil")
+			} else if !tt.fail && err != nil {
+				t.Fatalf("expected error to be nil, got %s", err)
 			}
 
+			if want := tt.size; got != want {
+				t.Fatalf("want size %d, got %d", want, got)
+			}
 		})
 	}
 }