Address code review feedback

3 files changed, 26 insertions, 12 deletions

diff --git a/vespalib/src/tests/net/tls/protocol_snooping/protocol_snooping_test.cpp b/vespalib/src/tests/net/tls/protocol_snooping/protocol_snooping_test.cpp
index e4cf06f6631..2d203047835 100644
--- a/vespalib/src/tests/net/tls/protocol_snooping/protocol_snooping_test.cpp
+++ b/vespalib/src/tests/net/tls/protocol_snooping/protocol_snooping_test.cpp
@@ -3,7 +3,7 @@
 #include <vespa/vespalib/net/tls/protocol_snooping.h>
 
 using namespace vespalib;
-using namespace vespalib::net::tls;
+using namespace vespalib::net::tls::snooping;
 
 TEST("min_header_bytes_to_observe() is 8") {
     EXPECT_EQUAL(8u, min_header_bytes_to_observe());
@@ -28,9 +28,14 @@ TEST("Mismatching handshake header byte 1 returns HandshakeMismatch") {
     EXPECT_EQUAL(TlsSnoopingResult::HandshakeMismatch, do_snoop(buf));
 }
 
-TEST("Mismatching handshake header byte 2 returns HandshakeMismatch") {
+TEST("Mismatching major version byte returns ProtocolVersionMismatch") {
     const unsigned char buf[] = { 22, 2, 1, 10, 255, 1, 0, 10 };
-    EXPECT_EQUAL(TlsSnoopingResult::HandshakeMismatch, do_snoop(buf));
+    EXPECT_EQUAL(TlsSnoopingResult::ProtocolVersionMismatch, do_snoop(buf));
+}
+
+TEST("Mismatching minor version byte returns ProtocolVersionMismatch") {
+    const unsigned char buf[] = { 22, 3, 0, 10, 255, 1, 0, 10 };
+    EXPECT_EQUAL(TlsSnoopingResult::ProtocolVersionMismatch, do_snoop(buf));
 }
 
 TEST("Oversized record returns RecordSizeRfcViolation") {
@@ -42,6 +47,11 @@ TEST("Oversized record returns RecordSizeRfcViolation") {
     EXPECT_EQUAL(TlsSnoopingResult::RecordSizeRfcViolation, do_snoop(buf2));
 }
 
+TEST("Undersized record returns RecordSizeRfcViolation") {
+    const unsigned char buf1[] = { 22, 3, 1, 0, 3, 1, 0, 0 };
+    EXPECT_EQUAL(TlsSnoopingResult::RecordSizeRfcViolation, do_snoop(buf1));
+}
+
 TEST("Non-ClientHello handshake record returns RecordNotClientHello") {
     const unsigned char buf[] = { 22, 3, 1, 10, 255, 2, 0, 10 };
     //                                               ^ 1 == ClientHello
@@ -60,4 +70,9 @@ TEST("Expected ClientHello record size mismatch returns ExpectedRecordSizeMismat
     EXPECT_EQUAL(TlsSnoopingResult::ExpectedRecordSizeMismatch, do_snoop(buf));
 }
 
+TEST("Valid ClientHello record size with LSB < 4 returns ProbablyTls") {
+    const unsigned char buf[] = { 22, 3, 1, 10, 3, 1, 0, 9 };
+    EXPECT_EQUAL(TlsSnoopingResult::ProbablyTls, do_snoop(buf));
+}
+
 TEST_MAIN() { TEST_RUN_ALL(); }
diff --git a/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.cpp b/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.cpp
index 04a0a29be64..6ccfc7f27ac 100644
--- a/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.cpp
+++ b/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.cpp
@@ -4,7 +4,7 @@
 #include <cstdlib>
 #include <stdint.h>
 
-namespace vespalib::net::tls {
+namespace vespalib::net::tls::snooping {
 
 namespace {
 
@@ -12,17 +12,17 @@ namespace {
 
 // From RFC 5246:
 // 0x16 - Handshake content type byte of TLSCiphertext record
-// 0x03 - First byte of 2-byte ProtocolVersion, always 3 on TLSv1.2 and v1.3
 inline bool is_tls_handshake_packet(const char* buf) {
-    return ((buf[0] == 0x16) && (buf[1] == 0x03));
+    return (buf[0] == 0x16);
 }
 
+// First byte of 2-byte ProtocolVersion, always 3 on TLSv1.2 and v1.3
 // Next is the TLS minor version, either 1 or 3 depending on version (though the
 // RFCs say it _should_ be 1 for backwards compatibility reasons).
 // Yes, the TLS spec says that you should technically ignore the protocol version
 // field here, but we want all the signals we can get.
 inline bool is_expected_tls_protocol_version(const char* buf) {
-    return ((buf[2] == 0x01) || (buf[2] == 0x03));
+    return ((buf[1] == 0x03) && ((buf[2] == 0x01) || (buf[2] == 0x03)));
 }
 
 // Length is big endian u16 in bytes 3, 4
@@ -70,7 +70,7 @@ TlsSnoopingResult snoop_client_hello_header(const char* buf) noexcept {
     // where we control frame sizes and where such fragmentation should not take place.
     // We also do not support TLSv1.3 0-RTT which may trigger early data.
     uint16_t length = tls_record_length(buf);
-    if (length > (16384 + 2048)) {
+    if ((length < 4) || (length > (16384 + 2048))) {
         return TlsSnoopingResult::RecordSizeRfcViolation;
     }
     if (!is_client_hello_handshake_record(buf)) {
@@ -95,8 +95,8 @@ const char* to_string(TlsSnoopingResult result) noexcept {
     case TlsSnoopingResult::RecordNotClientHello:       return "RecordNotClientHello";
     case TlsSnoopingResult::ClientHelloRecordTooBig:    return "ClientHelloRecordTooBig";
     case TlsSnoopingResult::ExpectedRecordSizeMismatch: return "ExpectedRecordSizeMismatch";
-    default: return "Unknown TlsSnoopingResult";
     }
+    abort();
 }
 
 std::ostream& operator<<(std::ostream& os, TlsSnoopingResult result) {
@@ -120,9 +120,8 @@ const char* describe_result(TlsSnoopingResult result) noexcept {
         return "ClientHello record is too big (fragmented?)";
     case TlsSnoopingResult::ExpectedRecordSizeMismatch:
         return "ClientHello vs Handshake header record size mismatch";
-    default:
-        return "Unknown TlsSnoopingResult";
     }
+    abort();
 }
 
 }
diff --git a/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.h b/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.h
index 59f6ecf6e9d..f53e136e597 100644
--- a/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.h
+++ b/vespalib/src/vespa/vespalib/net/tls/protocol_snooping.h
@@ -4,7 +4,7 @@
 #include <iosfwd>
 #include <stddef.h>
 
-namespace vespalib::net::tls {
+namespace vespalib::net::tls::snooping {
 
 constexpr inline size_t min_header_bytes_to_observe() { return 8; }