From c40774e3e41d4d8e084224144c1bf39489c810a9 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Fri, 16 May 2025 10:38:34 -0700 Subject: [PATCH 1/5] Updated testing and fixed bug in SSLSNIConfig --- src/iocore/net/SSLSNIConfig.cc | 1 + src/iocore/net/unit_tests/sni_conf_test.yaml | 10 ++++ .../net/unit_tests/test_SSLSNIConfig.cc | 51 ++++++++++++++----- .../net/unit_tests/test_YamlSNIConfig.cc | 2 +- 4 files changed, 51 insertions(+), 13 deletions(-) diff --git a/src/iocore/net/SSLSNIConfig.cc b/src/iocore/net/SSLSNIConfig.cc index 19b631e782c..bbac57eb6f7 100644 --- a/src/iocore/net/SSLSNIConfig.cc +++ b/src/iocore/net/SSLSNIConfig.cc @@ -77,6 +77,7 @@ NamedElement::operator=(NamedElement &&other) if (this != &other) { match = std::move(other.match); inbound_port_ranges = std::move(other.inbound_port_ranges); + rank = other.rank; } return *this; } diff --git a/src/iocore/net/unit_tests/sni_conf_test.yaml b/src/iocore/net/unit_tests/sni_conf_test.yaml index d8b264a5036..cffd7d890ef 100644 --- a/src/iocore/net/unit_tests/sni_conf_test.yaml +++ b/src/iocore/net/unit_tests/sni_conf_test.yaml @@ -36,3 +36,13 @@ sni: http2_buffer_water_mark: 256 - fqdn: foo.bar.com http2: false + +# test with mixed-case +- fqdn: "MiXeDcAsE.foo.com" + http2: true + http2_buffer_water_mark: 256 + inbound_port_ranges: 31337 + +# # test with mixed-case glob +- fqdn: "*.MiXeDcAsE.com" + http2: false diff --git a/src/iocore/net/unit_tests/test_SSLSNIConfig.cc b/src/iocore/net/unit_tests/test_SSLSNIConfig.cc index e04cd7837b3..d25d710a089 100644 --- a/src/iocore/net/unit_tests/test_SSLSNIConfig.cc +++ b/src/iocore/net/unit_tests/test_SSLSNIConfig.cc @@ -41,78 +41,105 @@ TEST_CASE("Test SSLSNIConfig") SECTION("The config does not match any SNIs for someport.com:577") { - auto const &actions{params.get({"someport.com", std::strlen("someport.com")}, 577)}; + auto const &actions{params.get("someport.com", 577)}; CHECK(!actions.first); } SECTION("The config does not match any SNIs for someport.com:808") { - auto const &actions{params.get({"someport.com", std::strlen("someport.com")}, 808)}; + auto const &actions{params.get("someport.com", 808)}; CHECK(!actions.first); } SECTION("The config does not match any SNIs for oneport.com:1") { - auto const &actions{params.get({"oneport.com", std::strlen("oneport.com")}, 1)}; + auto const &actions{params.get("oneport.com", 1)}; CHECK(!actions.first); } SECTION("The config does match an SNI for oneport.com:433") { - auto const &actions{params.get({"oneport.com", std::strlen("oneport.com")}, 433)}; + auto const &actions{params.get("oneport.com", 433)}; REQUIRE(actions.first); REQUIRE(actions.first->size() == 2); } SECTION("The config matches an SNI for allports.com") { - auto const &actions{params.get({"allports.com", std::strlen("allports.com")}, 1)}; + auto const &actions{params.get("allports.com", 1)}; REQUIRE(actions.first); REQUIRE(actions.first->size() == 2); } SECTION("The config matches an SNI for someport.com:1") { - auto const &actions{params.get({"someport.com", std::strlen("someport.com")}, 1)}; + auto const &actions{params.get("someport.com", 1)}; REQUIRE(actions.first); REQUIRE(actions.first->size() == 3); } SECTION("The config matches an SNI for someport.com:433") { - auto const &actions{params.get({"someport.com", std::strlen("someport.com")}, 433)}; + auto const &actions{params.get("someport.com", 433)}; REQUIRE(actions.first); REQUIRE(actions.first->size() == 3); } SECTION("The config matches an SNI for someport:8080") { - auto const &actions{params.get({"someport.com", std::strlen("someport.com")}, 8080)}; + auto const &actions{params.get("someport.com", 8080)}; REQUIRE(actions.first); REQUIRE(actions.first->size() == 2); } SECTION("The config matches an SNI for someport:65535") { - auto const &actions{params.get({"someport.com", std::strlen("someport.com")}, 65535)}; + auto const &actions{params.get("someport.com", 65535)}; REQUIRE(actions.first); REQUIRE(actions.first->size() == 2); } SECTION("The config matches an SNI for someport:482") { - auto const &actions{params.get({"someport.com", std::strlen("someport.com")}, 482)}; + auto const &actions{params.get("someport.com", 482)}; REQUIRE(actions.first); REQUIRE(actions.first->size() == 3); } SECTION("Matching order") { - std::string_view target = "foo.bar.com"; - auto const &actions{params.get(target, 443)}; + auto const &actions{params.get("foo.bar.com", 443)}; REQUIRE(actions.first); REQUIRE(actions.first->size() == 5); ///< three H2 config + early data + fqdn } + + SECTION("Test mixed-case") + { + auto const &actions{params.get("SoMePoRt.CoM", 65535)}; + REQUIRE(actions.first); + REQUIRE(actions.first->size() == 2); + } + + SECTION("Test mixed-case with wildcard in yaml config") + { + auto const &actions{params.get("AnYtHiNg.BaR.CoM", 443)}; + REQUIRE(actions.first); + REQUIRE(actions.first->size() == 4); + } + + SECTION("Test mixed-case in yaml config") + { + auto const &actions{params.get("mixedcase.foo.com", 31337)}; + REQUIRE(actions.first); + REQUIRE(actions.first->size() == 4); + } + + SECTION("Test mixed-case glob in yaml config") + { + auto const &actions{params.get("FoO.mixedcase.com", 443)}; + REQUIRE(actions.first); + REQUIRE(actions.first->size() == 3); + } } TEST_CASE("SNIConfig reconfigure callback is invoked") diff --git a/src/iocore/net/unit_tests/test_YamlSNIConfig.cc b/src/iocore/net/unit_tests/test_YamlSNIConfig.cc index e461ebe39bf..baad00fe9c5 100644 --- a/src/iocore/net/unit_tests/test_YamlSNIConfig.cc +++ b/src/iocore/net/unit_tests/test_YamlSNIConfig.cc @@ -55,7 +55,7 @@ TEST_CASE("YamlSNIConfig sets port ranges appropriately") FAIL(errorstream.str()); } REQUIRE(zret.is_ok()); - REQUIRE(conf.items.size() == 7); + REQUIRE(conf.items.size() == 9); SECTION("If no ports were specified, port range should contain all ports.") { From 98c871e20e3d51bc2110decdd879bbd3c7eb6b8c Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Fri, 16 May 2025 10:52:48 -0700 Subject: [PATCH 2/5] Minor formatting change in yaml file --- src/iocore/net/unit_tests/sni_conf_test.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/iocore/net/unit_tests/sni_conf_test.yaml b/src/iocore/net/unit_tests/sni_conf_test.yaml index cffd7d890ef..7394ae95db9 100644 --- a/src/iocore/net/unit_tests/sni_conf_test.yaml +++ b/src/iocore/net/unit_tests/sni_conf_test.yaml @@ -43,6 +43,6 @@ sni: http2_buffer_water_mark: 256 inbound_port_ranges: 31337 -# # test with mixed-case glob +# test with mixed-case glob - fqdn: "*.MiXeDcAsE.com" http2: false From 0e7f969b491d76c8147c8beac02142118e91fcb5 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Fri, 16 May 2025 11:50:16 -0700 Subject: [PATCH 3/5] Added more tests and found another bug --- src/iocore/net/unit_tests/sni_conf_test.yaml | 5 ++++ .../net/unit_tests/test_SSLSNIConfig.cc | 27 +++++++++++++++++++ .../net/unit_tests/test_YamlSNIConfig.cc | 2 +- 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/iocore/net/unit_tests/sni_conf_test.yaml b/src/iocore/net/unit_tests/sni_conf_test.yaml index 7394ae95db9..aa929361013 100644 --- a/src/iocore/net/unit_tests/sni_conf_test.yaml +++ b/src/iocore/net/unit_tests/sni_conf_test.yaml @@ -46,3 +46,8 @@ sni: # test with mixed-case glob - fqdn: "*.MiXeDcAsE.com" http2: false + +# test glob in the middle +# TODO - according to the docs this should be supported +# but it is not working in the current implementation +- fqdn: "cat.*.com" diff --git a/src/iocore/net/unit_tests/test_SSLSNIConfig.cc b/src/iocore/net/unit_tests/test_SSLSNIConfig.cc index d25d710a089..2087306bba3 100644 --- a/src/iocore/net/unit_tests/test_SSLSNIConfig.cc +++ b/src/iocore/net/unit_tests/test_SSLSNIConfig.cc @@ -140,6 +140,33 @@ TEST_CASE("Test SSLSNIConfig") REQUIRE(actions.first); REQUIRE(actions.first->size() == 3); } + + SECTION("Test empty SNI does not match") + { + auto const &actions{params.get("", 443)}; + CHECK(!actions.first); + } + + SECTION("Test SNI with special characters does not match") + { + auto const &actions{params.get("some$port.com", 443)}; + CHECK(!actions.first); + } + + // TODO - this is a bug in the regex, and it should match + SECTION("Test with invalid glob in the middle in yaml config (e.g. cat.*.com)") + { + auto const &actions{params.get("cat.dog.com", 443)}; + REQUIRE(!actions.first); + } + + // TODO - this is a bug in the regex, it should not match + SECTION("Test with invalid glob in the middle in yaml config (e.g. cat.*.com) matches") + { + auto const &actions{params.get("cat.*.com", 443)}; + REQUIRE(actions.first); + REQUIRE(actions.first->size() == 2); + } } TEST_CASE("SNIConfig reconfigure callback is invoked") diff --git a/src/iocore/net/unit_tests/test_YamlSNIConfig.cc b/src/iocore/net/unit_tests/test_YamlSNIConfig.cc index baad00fe9c5..cbd83c2eb77 100644 --- a/src/iocore/net/unit_tests/test_YamlSNIConfig.cc +++ b/src/iocore/net/unit_tests/test_YamlSNIConfig.cc @@ -55,7 +55,7 @@ TEST_CASE("YamlSNIConfig sets port ranges appropriately") FAIL(errorstream.str()); } REQUIRE(zret.is_ok()); - REQUIRE(conf.items.size() == 9); + REQUIRE(conf.items.size() == 10); SECTION("If no ports were specified, port range should contain all ports.") { From 39511cb9937e281be482e365f24d68e05869a26b Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Fri, 16 May 2025 12:00:12 -0700 Subject: [PATCH 4/5] Updated comments based on the new functionality --- src/iocore/net/unit_tests/sni_conf_test.yaml | 4 +--- src/iocore/net/unit_tests/test_SSLSNIConfig.cc | 6 ++---- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/iocore/net/unit_tests/sni_conf_test.yaml b/src/iocore/net/unit_tests/sni_conf_test.yaml index aa929361013..2f7a1dedc39 100644 --- a/src/iocore/net/unit_tests/sni_conf_test.yaml +++ b/src/iocore/net/unit_tests/sni_conf_test.yaml @@ -47,7 +47,5 @@ sni: - fqdn: "*.MiXeDcAsE.com" http2: false -# test glob in the middle -# TODO - according to the docs this should be supported -# but it is not working in the current implementation +# test glob in the middle, this will be an exact match - fqdn: "cat.*.com" diff --git a/src/iocore/net/unit_tests/test_SSLSNIConfig.cc b/src/iocore/net/unit_tests/test_SSLSNIConfig.cc index 2087306bba3..e85ae6a3d4b 100644 --- a/src/iocore/net/unit_tests/test_SSLSNIConfig.cc +++ b/src/iocore/net/unit_tests/test_SSLSNIConfig.cc @@ -153,15 +153,13 @@ TEST_CASE("Test SSLSNIConfig") CHECK(!actions.first); } - // TODO - this is a bug in the regex, and it should match - SECTION("Test with invalid glob in the middle in yaml config (e.g. cat.*.com)") + SECTION("Test with invalid glob in the middle in yaml config (e.g. cat.*.com) does not match") { auto const &actions{params.get("cat.dog.com", 443)}; REQUIRE(!actions.first); } - // TODO - this is a bug in the regex, it should not match - SECTION("Test with invalid glob in the middle in yaml config (e.g. cat.*.com) matches") + SECTION("Test with invalid glob in the middle in yaml config (e.g. cat.*.com) does an exact match") { auto const &actions{params.get("cat.*.com", 443)}; REQUIRE(actions.first); From ff9523a5a5cb11230e724ab9219af7fe2add6dca Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Mon, 19 May 2025 10:39:01 -0700 Subject: [PATCH 5/5] Added verification of the capture group --- src/iocore/net/unit_tests/test_SSLSNIConfig.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/iocore/net/unit_tests/test_SSLSNIConfig.cc b/src/iocore/net/unit_tests/test_SSLSNIConfig.cc index e85ae6a3d4b..001e2115650 100644 --- a/src/iocore/net/unit_tests/test_SSLSNIConfig.cc +++ b/src/iocore/net/unit_tests/test_SSLSNIConfig.cc @@ -125,6 +125,8 @@ TEST_CASE("Test SSLSNIConfig") auto const &actions{params.get("AnYtHiNg.BaR.CoM", 443)}; REQUIRE(actions.first); REQUIRE(actions.first->size() == 4); + // verify the capture group + REQUIRE(actions.second._fqdn_wildcard_captured_groups->at(0) == "AnYtHiNg"); } SECTION("Test mixed-case in yaml config") @@ -139,6 +141,8 @@ TEST_CASE("Test SSLSNIConfig") auto const &actions{params.get("FoO.mixedcase.com", 443)}; REQUIRE(actions.first); REQUIRE(actions.first->size() == 3); + // verify the capture group + REQUIRE(actions.second._fqdn_wildcard_captured_groups->at(0) == "FoO"); } SECTION("Test empty SNI does not match")