Copilot commented on code in PR #12864:
URL: https://github.com/apache/trafficserver/pull/12864#discussion_r2790469297
##########
src/iocore/net/ProxyProtocol.cc:
##########
@@ -547,6 +547,84 @@ ProxyProtocol::get_tlv(const uint8_t tlvCode) const
return std::nullopt;
}
+/*
+ * PP2_TYPE_SSL
+ * struct pp2_tlv_ssl {
+ * uint8_t client;
+ * uint32_t verify;
+ * struct pp2_tlv sub_tlv[0];
+ * };
+ */
+
+std::optional<std::string_view>
+ProxyProtocol::_get_tlv_ssl_subtype(uint8_t subtype) const
+{
+ if (auto v = tlv.find(PP2_TYPE_SSL); v != tlv.end() && v->second.length() !=
0) {
+ auto ssl = v->second;
+
+ // Is the client connected over TLS
+ if ((ssl.data()[0] & 0x01) == 0) {
+ // Not over TLS
+ return std::nullopt;
+ }
+
+ if (ssl.length() < 5) {
+ return std::nullopt;
+ }
+
+ // Find the given subtype
+ uint16_t len = ssl.length();
+ const char *p = ssl.data() + 5; // Skip client (uint8_t) + verify
(uint32_t)
+ const char *end = ssl.data() + len;
+ while (p != end) {
+ if (end - p < 3) {
+ // The size of a sub TLV entry must be 3 bytes or more
+ Dbg(dbg_ctl_proxyprotocol_v2, "Remaining data (%ld bytes) is not
enough for a sub TLV field", end - p);
+ return std::nullopt;
+ }
+
+ // Type
+ uint8_t type = *p;
+ p += 1;
+
+ // Length
+ uint16_t length = ntohs(*reinterpret_cast<const uint16_t *>(p));
+ p += 2;
+
Review Comment:
Reading the 16-bit sub-TLV length via `ntohs(*reinterpret_cast<const
uint16_t *>(p))` is undefined behavior (potential unaligned access +
type-punning from `char*`). Please parse the length with `memcpy` into a
`uint16_t` (or use an existing unaligned/buffer-reader helper) before calling
`ntohs`.
##########
src/iocore/net/unit_tests/test_ProxyProtocol.cc:
##########
@@ -327,6 +330,9 @@ TEST_CASE("PROXY Protocol v2 Parser",
"[ProxyProtocol][ProxyProtocolv2]")
CHECK(pp_info.tlv[PP2_TYPE_ALPN] == "h2");
CHECK(pp_info.tlv[PP2_TYPE_AUTHORITY] == "abc");
+
+ CHECK(pp_info.get_tlv_ssl_cipher() == "XYZ");
+ CHECK(pp_info.get_tlv_ssl_version() == "TLS");
Review Comment:
The new SSL-subtype accessor logic checks the PP2_TYPE_SSL 'client' flag and
validates the nested TLV structure, but the unit test coverage only exercises
the happy-path (client flag set, both sub-TLVs present). Consider adding at
least one test case where PP2_TYPE_SSL is present but the client flag does not
indicate TLS (or the sub-TLV is truncated) to verify
`get_tlv_ssl_cipher/version()` return `nullopt` and parsing remains robust.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
