You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by bn...@apache.org on 2022/09/08 17:25:59 UTC
[trafficserver] branch 10-Dev updated: Updates to origin side ALPN configuration implementation (#9074)
This is an automated email from the ASF dual-hosted git repository.
bneradt pushed a commit to branch 10-Dev
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/10-Dev by this push:
new a48f6cd4d Updates to origin side ALPN configuration implementation (#9074)
a48f6cd4d is described below
commit a48f6cd4dbefa81d3e0d626af47848ead551ce6e
Author: Brian Neradt <br...@gmail.com>
AuthorDate: Thu Sep 8 12:25:54 2022 -0500
Updates to origin side ALPN configuration implementation (#9074)
ATS will now emit an ERROR instead of a WARNING if there are issues with
the user configured ALPN string. Also, the ALPN parsing is updated to
use the more capable TextView API.
---
include/records/I_RecHttp.h | 4 ++
src/records/RecHttp.cc | 48 +++++++++++-----------
src/records/unit_tests/test_RecHttp.cc | 9 +++-
.../tls/tls_client_alpn_configuration.test.py | 6 +--
4 files changed, 39 insertions(+), 28 deletions(-)
diff --git a/include/records/I_RecHttp.h b/include/records/I_RecHttp.h
index a0ee4b374..8dad83c09 100644
--- a/include/records/I_RecHttp.h
+++ b/include/records/I_RecHttp.h
@@ -542,5 +542,9 @@ extern void ts_session_protocol_well_known_name_indices_init();
* @return True if the conversion was successful, false otherwise. Note that
* the wire format does not support an empty protocol list, therefore this
* function returns false if @a protocols is an empty string.
+ *
+ * TODO: ideally this would take a ts::TextView for @a protocols, but currently
+ * ts::TextView does not have a char* constructor while std::string_view does.
+ * Once that is added, this can be seemlessly switched to a ts::TextView.
*/
bool convert_alpn_to_wire_format(std::string_view protocols, unsigned char *wire_format_buffer, int &wire_format_buffer_len);
diff --git a/src/records/RecHttp.cc b/src/records/RecHttp.cc
index 2fde6e5b1..a2e7cb580 100644
--- a/src/records/RecHttp.cc
+++ b/src/records/RecHttp.cc
@@ -33,6 +33,8 @@
#include <unordered_set>
#include <tscore/IpMapConf.h>
+using ts::TextView;
+
SessionProtocolNameRegistry globalSessionProtocolNameRegistry;
/* Protocol session well-known protocol names.
@@ -844,8 +846,12 @@ SessionProtocolNameRegistry::nameFor(int idx) const
}
bool
-convert_alpn_to_wire_format(std::string_view protocols, unsigned char *wire_format_buffer, int &wire_format_buffer_len)
+convert_alpn_to_wire_format(std::string_view protocols_sv, unsigned char *wire_format_buffer, int &wire_format_buffer_len)
{
+ // TODO: once the protocols_sv is switched to be a TextView (see the TODO
+ // comment in this functions doxygen comment), then rename the input
+ // parameter to be simply `protocols` and remove this next line.
+ TextView protocols(protocols_sv);
// Callers expect wire_format_buffer_len to be zero'd out in the event of an
// error. To simplify the error handling from doing this on every return, we
// simply zero them out here at the start.
@@ -859,44 +865,37 @@ convert_alpn_to_wire_format(std::string_view protocols, unsigned char *wire_form
// Parse the comma separated protocol string into a list of protocol names.
std::vector<std::string_view> alpn_protocols;
- std::string_view protocol;
- size_t pos = 0;
+ TextView protocol;
int computed_alpn_array_len = 0;
- while (pos < protocols.size()) {
- size_t next_pos = protocols.find(',', pos);
- if (next_pos == std::string_view::npos) {
- protocol = protocols.substr(pos);
- pos = protocols.size();
- } else {
- protocol = protocols.substr(pos, next_pos - pos);
- pos = next_pos + 1;
- }
+
+ while (protocols) {
+ protocol = protocols.take_prefix_at(',').trim_if(&isspace);
if (protocol.empty()) {
- Warning("Empty protocol name in configured ALPN list: %.*s", static_cast<int>(protocols.size()), protocols.data());
+ Error("Empty protocol name in configured ALPN list: \"%.*s\"", static_cast<int>(protocols.size()), protocols.data());
return false;
}
if (protocol.size() > 255) {
// The length has to fit in one byte.
- Warning("A protocol name larger than 255 bytes in configured ALPN list: %.*s", static_cast<int>(protocols.size()),
- protocols.data());
+ Error("A protocol name larger than 255 bytes in configured ALPN list: \"%.*s\"", static_cast<int>(protocols.size()),
+ protocols.data());
return false;
}
// Check whether we recognize the protocol.
auto const protocol_index = globalSessionProtocolNameRegistry.indexFor(protocol);
if (protocol_index == SessionProtocolNameRegistry::INVALID) {
- Warning("Unknown protocol name in configured ALPN list: %.*s", static_cast<int>(protocol.size()), protocol.data());
+ Error("Unknown protocol name in configured ALPN list: \"%.*s\"", static_cast<int>(protocol.size()), protocol.data());
return false;
}
// We currently only support HTTP/1.x protocols toward the origin.
if (!HTTP_PROTOCOL_SET.contains(protocol_index)) {
- Warning("Unsupported non-HTTP/1.x protocol name in configured ALPN list: %.*s", static_cast<int>(protocol.size()),
- protocol.data());
+ Error("Unsupported non-HTTP/1.x protocol name in configured ALPN list: \"%.*s\"", static_cast<int>(protocol.size()),
+ protocol.data());
return false;
}
// But not HTTP/0.9.
if (protocol_index == TS_ALPN_PROTOCOL_INDEX_HTTP_0_9) {
- Warning("Unsupported \"http/0.9\" protocol name in configured ALPN list: %.*s", static_cast<int>(protocol.size()),
- protocol.data());
+ Error("Unsupported \"http/0.9\" protocol name in configured ALPN list: \"%.*s\"", static_cast<int>(protocol.size()),
+ protocol.data());
return false;
}
@@ -904,15 +903,15 @@ convert_alpn_to_wire_format(std::string_view protocols, unsigned char *wire_form
computed_alpn_array_len += protocol_wire_format.size();
if (computed_alpn_array_len > orig_wire_format_buffer_len) {
// We have exceeded the size of the output buffer.
- Warning("The output ALPN length (%d bytes) is larger than the output buffer size of %d bytes", computed_alpn_array_len,
- orig_wire_format_buffer_len);
+ Error("The output ALPN length (%d bytes) is larger than the output buffer size of %d bytes", computed_alpn_array_len,
+ orig_wire_format_buffer_len);
return false;
}
alpn_protocols.push_back(protocol_wire_format);
}
if (alpn_protocols.empty()) {
- Warning("No protocols specified in ALPN list: %.*s", static_cast<int>(protocols.size()), protocols.data());
+ Error("No protocols specified in ALPN list: \"%.*s\"", static_cast<int>(protocols.size()), protocols.data());
return false;
}
@@ -925,6 +924,7 @@ convert_alpn_to_wire_format(std::string_view protocols, unsigned char *wire_form
end += len;
}
wire_format_buffer_len = computed_alpn_array_len;
- Debug("ssl_alpn", "Successfully converted ALPN list to wire format: %.*s", static_cast<int>(protocols.size()), protocols.data());
+ Debug("ssl_alpn", "Successfully converted ALPN list to wire format: \"%.*s\"", static_cast<int>(protocols.size()),
+ protocols.data());
return true;
}
diff --git a/src/records/unit_tests/test_RecHttp.cc b/src/records/unit_tests/test_RecHttp.cc
index 8177549ea..6dcf4f0e0 100644
--- a/src/records/unit_tests/test_RecHttp.cc
+++ b/src/records/unit_tests/test_RecHttp.cc
@@ -197,12 +197,19 @@ std::vector<ConvertAlpnToWireFormatTestCase> convertAlpnToWireFormatTestCases =
true
},
{
- "Multiple protocols: HTTP/0.9, HTTP/1.0, HTTP/1.1",
+ "Multiple protocols: HTTP/1.0, HTTP/1.1",
"http/1.1,http/1.0",
{0x08, 'h', 't', 't', 'p', '/', '1', '.', '1', 0x08, 'h', 't', 't', 'p', '/', '1', '.', '0'},
18,
true
},
+ {
+ "Whitespace: verify that we gracefully handle padded whitespace",
+ "http/1.1, http/1.0",
+ {0x08, 'h', 't', 't', 'p', '/', '1', '.', '1', 0x08, 'h', 't', 't', 'p', '/', '1', '.', '0'},
+ 18,
+ true
+ },
};
// clang-format on
diff --git a/tests/gold_tests/tls/tls_client_alpn_configuration.test.py b/tests/gold_tests/tls/tls_client_alpn_configuration.test.py
index 45d1cbb74..1e4fac508 100644
--- a/tests/gold_tests/tls/tls_client_alpn_configuration.test.py
+++ b/tests/gold_tests/tls/tls_client_alpn_configuration.test.py
@@ -134,12 +134,12 @@ class TestAlpnFunctionality:
)
if alpn_is_malformed:
- ts.Disk.diags_log.Content += Testers.ContainsExpression(
- "WARNING.*ALPN",
+ ts.Disk.diags_log.Content = Testers.ContainsExpression(
+ "ERROR.*ALPN",
"There should be no ALPN parse warnings.")
else:
ts.Disk.diags_log.Content += Testers.ExcludesExpression(
- "WARNING.*ALPN",
+ "ERROR.*ALPN",
"There should be no ALPN parse warnings.")
return ts