You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "JosiahWI (via GitHub)" <gi...@apache.org> on 2023/06/01 20:22:46 UTC
[GitHub] [trafficserver] JosiahWI opened a new pull request, #9767: Filter SNI based on port
JosiahWI opened a new pull request, #9767:
URL: https://github.com/apache/trafficserver/pull/9767
This adds an optional port configuration to the fqdn in sni.yaml. Two forms are supported:
```yaml
sni:
# single port
- fqdn: example.com:433
# inclusive range of ports
- fqdn: example.com:500:600
```
If the port is omitted, it defaults to all ports. Incoming requests that are checked against the SNIs are only matched if the incoming destination port is within the range for that SNI. This allows to treat incoming requests differently based on the incoming port, or to fall back to remap on certain ports.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1240016287
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
It doesn't look like a right direction to me. What design change are you planning?
With this change for port number filtering, although port number is not directly related to SNI, port number becomes essential to support SNI (or more specifically, SNIAction) on a `NetVConnection` implementation. I'd require a `NetVConnection` implementation to have the capability of providing the local port number by adding `virtual int _get_local_port() = 0` to `TLSSNISupport`. If one cannot fulfill the requirement, the implementation can't say it supports "SNI".
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1577266617
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1604110350
Rebased on master due to merge conflicts.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1237259521
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
This is not great. What requires this cast?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9767: Filter SNI based on port
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1572768416
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1573943324
> Interesting. To be clear, are you suggesting we remove the corresponding tunnel_route's ability to use matched groups from the fqdn?
Oh that relies on pcre? I didn't know it. Then it doesn't need to be completely removed, but as #9736 explains, using many pcre has negative impact in performance. And current pattern matching is not quite right. If you have `fqdn: *.example.com`, it actually matches `foo.bar.example.com.evil.com`, which sounds scary although I don't think of any bad ways to use the bug.
I suggest having `name` (and `pattern` if we can't remove pcre matching) for server name matching.
- `name`: Follows the regular server name matching (include just one wildcard for a subdomain) which is used for cert validation.
- `pattern`: PCRE, which requires user to set a real regex (no implicit `^` and `$`)
And for the new feature, I'd suggest a separate key `ports` as a sibling of "name".
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] jpeach commented on pull request #9767: Filter SNI based on port
Posted by "jpeach (via GitHub)" <gi...@apache.org>.
jpeach commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1574549310
> This adds an optional port configuration to the fqdn in sni.yaml. Two forms are supported:
>
> ```yaml
> sni:
> # single port
> - fqdn: example.com:443
> # inclusive range of ports
> - fqdn: example.com:443-446
> ```
I don't think that overloading the `fqdn` field is the right approach. Since there is a structured config file here, you can just add a new field, maybe calling it `inbound_port_range` or something that's relatively obvious.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1222054351
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -111,80 +113,72 @@ int
SNIConfigParams::load_sni_config()
{
for (auto &item : yaml_sni.items) {
- auto ai = sni_action_list.emplace(sni_action_list.end());
- ai->set_glob_name(item.fqdn);
+ auto &ai = sni_action_list.emplace_back();
+ ai.set_glob_name(item.fqdn);
+ if (!item.port_ranges.empty()) {
+ auto const [min, max]{item.port_ranges[0]};
Review Comment:
Why is this lookin at only the first element?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1222066118
##########
iocore/net/YamlSNIConfig.cc:
##########
@@ -178,6 +220,28 @@ template <> struct convert<YamlSNIConfig::Item> {
} else {
return false; // servername must be present
}
+
+ if (node[TS_inbound_port_range]) {
+ swoc::TextView port_view{node[TS_inbound_port_range].Scalar()};
+ auto min{port_view.split_prefix_at('-')};
+ if (!min) {
+ min = port_view;
+ }
+ auto const &max{port_view};
+
+ swoc::TextView parsed_min;
+ long min_port{swoc::svtoi(min, &parsed_min)};
+ swoc::TextView parsed_max;
+ long max_port{swoc::svtoi(max, &parsed_max)};
+ if (parsed_min != min || min_port < 1 || parsed_max != max || max_port > std::numeric_limits<uint16_t>::max() ||
+ max_port < min_port) {
+ std::string out;
Review Comment:
Use `ts::bw_dbg` instead of `out`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1222119783
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -111,80 +113,72 @@ int
SNIConfigParams::load_sni_config()
{
for (auto &item : yaml_sni.items) {
- auto ai = sni_action_list.emplace(sni_action_list.end());
- ai->set_glob_name(item.fqdn);
+ auto &ai = sni_action_list.emplace_back();
+ ai.set_glob_name(item.fqdn);
+ if (!item.port_ranges.empty()) {
+ auto const [min, max]{item.port_ranges[0]};
+ ai.ports = {static_cast<uint16_t>(min), static_cast<uint16_t>(max)};
+ }
Debug("ssl", "name: %s", item.fqdn.data());
- // set SNI based actions to be called in the ssl_servername_only callback
- if (item.offer_h2.has_value()) {
- ai->actions.push_back(std::make_unique<ControlH2>(item.offer_h2.value()));
- }
- if (item.verify_client_level != 255) {
- ai->actions.push_back(
- std::make_unique<VerifyClient>(item.verify_client_level, item.verify_client_ca_file, item.verify_client_ca_dir));
- }
- if (item.host_sni_policy != 255) {
- ai->actions.push_back(std::make_unique<HostSniPolicy>(item.host_sni_policy));
- }
- if (item.valid_tls_version_min_in >= 0 || item.valid_tls_version_max_in >= 0) {
- ai->actions.push_back(std::make_unique<TLSValidProtocols>(item.valid_tls_version_min_in, item.valid_tls_version_max_in));
- } else {
- if (!item.protocol_unset) {
- ai->actions.push_back(std::make_unique<TLSValidProtocols>(item.protocol_mask));
- }
- }
- if (item.tunnel_destination.length() > 0) {
- ai->actions.push_back(
- std::make_unique<TunnelDestination>(item.tunnel_destination, item.tunnel_type, item.tunnel_prewarm, item.tunnel_alpn));
- }
- if (!item.client_sni_policy.empty()) {
- ai->actions.push_back(std::make_unique<OutboundSNIPolicy>(item.client_sni_policy));
- }
- if (item.http2_buffer_water_mark.has_value()) {
- ai->actions.push_back(std::make_unique<HTTP2BufferWaterMark>(item.http2_buffer_water_mark.value()));
+ item.populate_sni_actions(ai.actions);
+ if (set_next_hop_properties(item) == 1) {
+ return 1;
}
+ }
- ai->actions.push_back(std::make_unique<ServerMaxEarlyData>(item.server_max_early_data));
+ return 0;
+}
- ai->actions.push_back(std::make_unique<SNI_IpAllow>(item.ip_allow, item.fqdn));
+int
Review Comment:
I was hesitant to mess with other people's API decisions when I started, but I'm starting to grow out of that, so I'll change it.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1224742773
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -214,19 +205,24 @@ SNIConfigParams::get(std::string_view servername) const
return {nullptr, {}};
}
-int
+bool
SNIConfigParams::initialize()
{
std::string sni_filename = RecConfigReadConfigPath("proxy.config.ssl.servername.filename");
+ return initialize(sni_filename);
+}
+bool
+SNIConfigParams::initialize(std::string const &sni_filename)
Review Comment:
Don't use `std::string const&` parameters, use `std::string_view`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1240016287
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
It doesn't look like a right direction to me. What design change are you planning?
With this change for port number filtering, although port number is not directly related to SNI, port number becomes essential to support SNI (or more specifically, SNIAction) on a `NetVConnectoin` implementation. I'd require a `NetVConnection` implementation to have the capability of providing the local port number by adding `virtual int _get_local_port() = 0` to `TLSSNISupport`. If one cannot fulfill the requirement, the implementation can't say it supports "SNI".
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1610023316
> Probably last question. What would happen if I accidentally set inbound_port_range on an entry for outbound setting? Crash? Be ignored? Any warning message?
Talked offline and it turned out that I misunderstood how sni.yaml works.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] jpeach commented on pull request #9767: Filter SNI based on port
Posted by "jpeach (via GitHub)" <gi...@apache.org>.
jpeach commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1574547100
> Oh that relies on pcre? I didn't know it. Then it doesn't need to be completely removed, but as #9736 explains, using many pcre has negative impact in performance. And current pattern matching is not quite right. If you have `fqdn: *.example.com`, it actually matches `foo.bar.example.com.evil.com`, which sounds scary although I don't think of any bad ways to use the bug.
IIRC this is explicitly forbidden by the RFC. I implemented this in the very first version of SNI support, and then reverted it for that reason.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1572838578
Almost nitpicking but I wouldn't call the setting value "fqdn". Since this change would be for 10.x, shouldn't we change the setting name accordingly? Also, the setting currently allows using pcre (see #9736). Using a colon for as a delimiter may make the setting parse more complicated. It's probably a time to make an incompatible change and define a clear setting format. IMO, we should not allow pcre.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1224781319
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -271,22 +267,22 @@ SNIConfig::reconfigure()
Debug("ssl", "Reload SNI file");
SNIConfigParams *params = new SNIConfigParams;
- int retStatus = params->initialize();
- if (!retStatus) {
+ bool retStatus = params->initialize();
+ if (retStatus) {
_configid = configProcessor.set(_configid, params);
prewarmManager.reconfigure();
} else {
delete params;
}
std::string sni_filename = RecConfigReadConfigPath("proxy.config.ssl.servername.filename");
- if (!retStatus || TSSystemState::is_initializing()) {
+ if (retStatus || TSSystemState::is_initializing()) {
Note("%s finished loading", sni_filename.c_str());
} else {
Error("%s failed to load", sni_filename.c_str());
}
- return !retStatus;
+ return retStatus ? 1 : 0;
Review Comment:
Don't you think that's less clear?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1238975664
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
There are lots of other places where it is explicitly dynamic_cast to an `SSLNetVConnection*`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1238421948
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
I took a look this, and I think it might be time to refactor.
```cpp
class SSLNetVConnection : public UnixNetVConnection,
public ALPNSupport,
public TLSSessionResumptionSupport,
public TLSSNISupport,
public TLSEarlyDataSupport,
public TLSTunnelSupport,
public TLSCertSwitchSupport,
public TLSBasicSupport
```
This is where we obtain the TLSSNIConfig:
```cpp
TLSSNISupport *
TLSSNISupport::getInstance(SSL *ssl)
{
return static_cast<TLSSNISupport *>(SSL_get_ex_data(ssl, _ex_data_index));
}
```
and SSLUtils.cc does the actual calls to perform SNI actions, on the `TLSSNISupport` object it got from this... apparently some global data stored using OpenSSL? Am I reading that right?
So there's no NetVConnection in sight. The only way we can obtain it is to `dynamic_cast`. I'm not the only person who's noticed this:
```cpp
// TODO: change design to avoid this dynamic_cast
auto ssl_vc = dynamic_cast<SSLNetVConnection *>(snis);
```
@masaori335 put in this TODO, maybe he's thought some about how to do this. I suggest we aim to do the design change in a separate PR, as it will probably be nontrivial.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1239023425
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
The existing dynamic_casts were written before QUIC, and we need to fix them. New code should try not to introduce a new one.
Here are two options but there may be better ways.
- Get a port number from SSL object
- Add a protected virtual function, which returns a port number, to SNISupport
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1608454918
Overall looks good.
Probably last question. What would happen if I accidentally set `inbound_port_range` on an entry for outbound setting? Crash? Be ignored? Any warning message?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1238421948
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
I took a look this, and I think it might be time to refactor.
```cpp
class SSLNetVConnection : public UnixNetVConnection,
public ALPNSupport,
public TLSSessionResumptionSupport,
public TLSSNISupport,
public TLSEarlyDataSupport,
public TLSTunnelSupport,
public TLSCertSwitchSupport,
public TLSBasicSupport
```
This is where we obtain the TLSSNIConfig:
```cpp
TLSSNISupport *
TLSSNISupport::getInstance(SSL *ssl)
{
return static_cast<TLSSNISupport *>(SSL_get_ex_data(ssl, _ex_data_index));
}
```
and SSLUtils.cc does the actual calls to perform SNI actions, on the `TLSSNISupport` object it got from this... apparently some global data stored using OpenSSL? Am I reading that right?
So there's no `SSLNetVConnection` in sight. The only way we can obtain it is to `dynamic_cast`. You're not the only person who's wrinkled his nose at this:
```cpp
// TODO: change design to avoid this dynamic_cast
auto ssl_vc = dynamic_cast<SSLNetVConnection *>(snis);
```
@masaori335 put in this TODO, maybe he's thought some about how to do this. I suggest we aim to do the design change in a separate PR, as it will probably be nontrivial.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1240206960
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
I haven't thought about how to redesign yet. Maybe we should do that first in a separate PR before adding this feature, because it's clear that it's much more complicated than it looked initially.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1577088861
https://datatracker.ietf.org/doc/html/rfc6125#section-6.4.3
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1581457981
It's a good idea to change it from a vector to a single item. I wanted an easy way to check that it was "empty", but I could instead check that it is set to a default value (of all ports).
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1224734509
##########
doc/admin-guide/files/sni.yaml.en.rst:
##########
@@ -52,11 +52,34 @@ for a more detailed description of HTTP/2 connection coalescing.
.. _override-host-sni-policy:
.. _override-h2-properties:
+The following fields make up the key for each item in the configuration file.
+
========================= ========= ========================================================================================
Key Direction Meaning
========================= ========= ========================================================================================
-fqdn Both Fully Qualified Domain Name. This item is used if the SNI value matches this.
+fqdn Both Fully Qualified Domain Name.
+
+inbound_port_range Inbound The port range for the inbound connection in the form ``port`` or
+ ``min:max``.
Review Comment:
Why does this use ';' and the example '-'? Don't use colon, it's already problematic in this context. '-' is the goto separator for a range.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1238421948
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
I took a look this, and I think it might be time to refactor.
```cpp
class SSLNetVConnection : public UnixNetVConnection,
public ALPNSupport,
public TLSSessionResumptionSupport,
public TLSSNISupport,
public TLSEarlyDataSupport,
public TLSTunnelSupport,
public TLSCertSwitchSupport,
public TLSBasicSupport
```
This is where we obtain the TLSSNIConfig:
```cpp
TLSSNISupport *
TLSSNISupport::getInstance(SSL *ssl)
{
return static_cast<TLSSNISupport *>(SSL_get_ex_data(ssl, _ex_data_index));
}
```
and SSLUtils.cc does the actual calls to perform SNI actions, on the `TLSSNISupport` object it got from this... apparently some global data stored using OpenSSL? Am I reading that right?
So there's no `SSLNetVConnection` in sight. The only way we can obtain it is to `dynamic_cast`. You're not the only person who's noticed this:
```cpp
// TODO: change design to avoid this dynamic_cast
auto ssl_vc = dynamic_cast<SSLNetVConnection *>(snis);
```
@masaori335 put in this TODO, maybe he's thought some about how to do this. I suggest we aim to do the design change in a separate PR, as it will probably be nontrivial.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1222068264
##########
iocore/net/YamlSNIConfig.cc:
##########
@@ -178,6 +220,28 @@ template <> struct convert<YamlSNIConfig::Item> {
} else {
return false; // servername must be present
}
+
+ if (node[TS_inbound_port_range]) {
+ swoc::TextView port_view{node[TS_inbound_port_range].Scalar()};
+ auto min{port_view.split_prefix_at('-')};
+ if (!min) {
+ min = port_view;
+ }
+ auto const &max{port_view};
+
+ swoc::TextView parsed_min;
+ long min_port{swoc::svtoi(min, &parsed_min)};
+ swoc::TextView parsed_max;
+ long max_port{swoc::svtoi(max, &parsed_max)};
+ if (parsed_min != min || min_port < 1 || parsed_max != max || max_port > std::numeric_limits<uint16_t>::max() ||
+ max_port < min_port) {
+ std::string out;
+ swoc::bwprint(out, "bad port range: {}-{}", min, max);
+ throw YAML::ParserException(node[TS_fqdn].Mark(), out);
+ }
+
+ item.port_ranges.emplace_back(std::make_pair(min_port, max_port));
Review Comment:
Shouldn't that be `port_rang_type(mn_port, max_port)` instead of `makepair`?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1224837495
##########
iocore/net/SSLSNIConfig.h:
##########
@@ -60,12 +65,19 @@ struct PcreFreer {
struct NamedElement {
NamedElement() {}
+ NamedElement(NamedElement const &other) = delete;
+ NamedElement &operator=(NamedElement const &other) = delete;
NamedElement(NamedElement &&other);
NamedElement &operator=(NamedElement &&other);
+ ~NamedElement() = default;
void set_glob_name(std::string name);
void set_regex_name(const std::string ®ex_name);
+ inline static constexpr in_port_t MAX_PORT_VALUE{std::numeric_limits<in_port_t>::max()};
+ using port_range_t = swoc::DiscreteRange<in_port_t>;
Review Comment:
Maybe I'll sleep on it. I don't think there's any harm in DRYing it up, but it's only in two places so my initial reaction was to wait until it happened in a third place.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] jpeach commented on pull request #9767: Filter SNI based on port
Posted by "jpeach (via GitHub)" <gi...@apache.org>.
jpeach commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1577721762
> https://datatracker.ietf.org/doc/html/rfc6125#section-6.4.3
https://issues.apache.org/jira/browse/TS-1135 😂
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1604224300
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1238969108
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
Or are you saying we can't get the connection at all? We have to find the incoming port in order to filter on it.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1238967886
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
Ahh, yes, this looks like a mistake in my change. We need to dynamic_cast to an `I_NetVConnection*` I think.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit merged pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit merged PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1239951726
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
I added a base class inbetween the NetVConnection implementations and the UnixNetVConnection base class. This is probably not the cleanest solution, but it is easy, and I am thinking it would be better to do more extensive design changes separately. Will this work?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9767: Filter SNI based on port
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1572800487
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1222053444
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -111,80 +113,72 @@ int
SNIConfigParams::load_sni_config()
{
for (auto &item : yaml_sni.items) {
- auto ai = sni_action_list.emplace(sni_action_list.end());
- ai->set_glob_name(item.fqdn);
+ auto &ai = sni_action_list.emplace_back();
+ ai.set_glob_name(item.fqdn);
+ if (!item.port_ranges.empty()) {
+ auto const [min, max]{item.port_ranges[0]};
+ ai.ports = {static_cast<uint16_t>(min), static_cast<uint16_t>(max)};
Review Comment:
use `in_port_t` instead of `uint16_t`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1222067055
##########
iocore/net/YamlSNIConfig.cc:
##########
@@ -178,6 +220,28 @@ template <> struct convert<YamlSNIConfig::Item> {
} else {
return false; // servername must be present
}
+
+ if (node[TS_inbound_port_range]) {
+ swoc::TextView port_view{node[TS_inbound_port_range].Scalar()};
+ auto min{port_view.split_prefix_at('-')};
+ if (!min) {
+ min = port_view;
+ }
+ auto const &max{port_view};
+
+ swoc::TextView parsed_min;
+ long min_port{swoc::svtoi(min, &parsed_min)};
+ swoc::TextView parsed_max;
+ long max_port{swoc::svtoi(max, &parsed_max)};
+ if (parsed_min != min || min_port < 1 || parsed_max != max || max_port > std::numeric_limits<uint16_t>::max() ||
+ max_port < min_port) {
+ std::string out;
+ swoc::bwprint(out, "bad port range: {}-{}", min, max);
+ throw YAML::ParserException(node[TS_fqdn].Mark(), out);
Review Comment:
Depending on compact you want this, `swoc::bwprint` returns a reference to the input `std::string` so you coul do
```
throw YAML::ParserException(node[TS_fqdn].Mark(), swoc::bwprint(ts::bw_dbg, ...)));
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1224806416
##########
iocore/net/SSLSNIConfig.h:
##########
@@ -60,12 +65,19 @@ struct PcreFreer {
struct NamedElement {
NamedElement() {}
+ NamedElement(NamedElement const &other) = delete;
+ NamedElement &operator=(NamedElement const &other) = delete;
NamedElement(NamedElement &&other);
NamedElement &operator=(NamedElement &&other);
+ ~NamedElement() = default;
void set_glob_name(std::string name);
void set_regex_name(const std::string ®ex_name);
+ inline static constexpr in_port_t MAX_PORT_VALUE{std::numeric_limits<in_port_t>::max()};
+ using port_range_t = swoc::DiscreteRange<in_port_t>;
Review Comment:
This shows up twice. Promote to "ts_ip.h"?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1238421948
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
I took a look this, and I think it might be time to refactor.
```
class SSLNetVConnection : public UnixNetVConnection,
public ALPNSupport,
public TLSSessionResumptionSupport,
public TLSSNISupport,
public TLSEarlyDataSupport,
public TLSTunnelSupport,
public TLSCertSwitchSupport,
public TLSBasicSupport
```
This is where we obtain the TLSSNIConfig:
```cpp
TLSSNISupport *
TLSSNISupport::getInstance(SSL *ssl)
{
return static_cast<TLSSNISupport *>(SSL_get_ex_data(ssl, _ex_data_index));
}
```
and SSLUtils.cc does the actual calls to perform SNI actions, on the `TLSSNISupport` object it got from this... apparently some global data stored using OpenSSL? Am I reading that right?
So there's no NetVConnection in sight. The only way we can obtain it is to `dynamic_cast`. I'm not the only person who's noticed this:
```cpp
// TODO: change design to avoid this dynamic_cast
auto ssl_vc = dynamic_cast<SSLNetVConnection *>(snis);
```
@masaori335 put in this TODO, maybe he's thought some about how to do this. I suggest we aim to do the design change in a separate PR, as it will probably be nontrivial.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1240203139
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
Do you have an example of where we need to use SNI and do not want the `NetVConnection` to have a local port number? If that's because there is none, that implies we need to re-evaluate this whole idea of filtering by port.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1238857820
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
No, the goal is not to use `SSLNetVConnection` directly. You should not try to get the instance in any way. This doesn't work if the connection is QUIC.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1240353332
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
It's fine to have a local port number. I'm just talking about how to access it.
`NetVConnection` can keep current `get_local_port()`, and I'm suggesting a protected new function `_get_local_prot()` so `TLSSNISupport` can access it without depending on any `NetVConnection`. The new function is an interface between `TLSSNISupport` and something that supports `TLSSNISupport`. `TLSSNISupport` itself doesn't need `NetVConnection` (i.e. `TLSSNISupport` doesn't need to transfer data over network).
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1240507571
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
Ah, very good! I was looking for how to move `get_local_port()` into `TLSSNISupport`, but it did not occur to me to simply use a different name! I like that approach, so let's go with that.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1585085312
Use `std::optional` if you want to check for empty. Or if you use `DiscreteRange` it has an invalid / empty state. Although you don't have to check that because `contains` always fails on an empty range so it's really not worth the bother.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9767: Filter SNI based on port
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1240398633
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
This is what I'm suggesting. You can simply call `this->_get_local_port()` here without dynamic_cast.
https://github.com/apache/trafficserver/compare/master...maskit:export_local_port?expand=1
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1238421948
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername, std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
I took a look this, and I think it might be time to refactor.
```cpp
class SSLNetVConnection : public UnixNetVConnection,
public ALPNSupport,
public TLSSessionResumptionSupport,
public TLSSNISupport,
public TLSEarlyDataSupport,
public TLSTunnelSupport,
public TLSCertSwitchSupport,
public TLSBasicSupport
```
This is where we obtain the TLSSNIConfig:
```cpp
TLSSNISupport *
TLSSNISupport::getInstance(SSL *ssl)
{
return static_cast<TLSSNISupport *>(SSL_get_ex_data(ssl, _ex_data_index));
}
```
and SSLUtils.cc does the actual calls to perform SNI actions, on the `TLSSNISupport` object it got from this... apparently some global data stored using OpenSSL? Am I reading that right?
So there's no NetVConnection in sight. The only way we can obtain it is to `dynamic_cast`. You're not the only person who's wrinkled his nose at this:
```cpp
// TODO: change design to avoid this dynamic_cast
auto ssl_vc = dynamic_cast<SSLNetVConnection *>(snis);
```
@masaori335 put in this TODO, maybe he's thought some about how to do this. I suggest we aim to do the design change in a separate PR, as it will probably be nontrivial.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1609339949
Could you provide an example of such a configuration? I expect the `inbound_port_range` to be checked on the incoming connection, and then ignored on the outgoing connection, but I would like to verify this in one of the autests.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1222055829
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -111,80 +113,72 @@ int
SNIConfigParams::load_sni_config()
{
for (auto &item : yaml_sni.items) {
- auto ai = sni_action_list.emplace(sni_action_list.end());
- ai->set_glob_name(item.fqdn);
+ auto &ai = sni_action_list.emplace_back();
+ ai.set_glob_name(item.fqdn);
+ if (!item.port_ranges.empty()) {
+ auto const [min, max]{item.port_ranges[0]};
+ ai.ports = {static_cast<uint16_t>(min), static_cast<uint16_t>(max)};
+ }
Debug("ssl", "name: %s", item.fqdn.data());
- // set SNI based actions to be called in the ssl_servername_only callback
- if (item.offer_h2.has_value()) {
- ai->actions.push_back(std::make_unique<ControlH2>(item.offer_h2.value()));
- }
- if (item.verify_client_level != 255) {
- ai->actions.push_back(
- std::make_unique<VerifyClient>(item.verify_client_level, item.verify_client_ca_file, item.verify_client_ca_dir));
- }
- if (item.host_sni_policy != 255) {
- ai->actions.push_back(std::make_unique<HostSniPolicy>(item.host_sni_policy));
- }
- if (item.valid_tls_version_min_in >= 0 || item.valid_tls_version_max_in >= 0) {
- ai->actions.push_back(std::make_unique<TLSValidProtocols>(item.valid_tls_version_min_in, item.valid_tls_version_max_in));
- } else {
- if (!item.protocol_unset) {
- ai->actions.push_back(std::make_unique<TLSValidProtocols>(item.protocol_mask));
- }
- }
- if (item.tunnel_destination.length() > 0) {
- ai->actions.push_back(
- std::make_unique<TunnelDestination>(item.tunnel_destination, item.tunnel_type, item.tunnel_prewarm, item.tunnel_alpn));
- }
- if (!item.client_sni_policy.empty()) {
- ai->actions.push_back(std::make_unique<OutboundSNIPolicy>(item.client_sni_policy));
- }
- if (item.http2_buffer_water_mark.has_value()) {
- ai->actions.push_back(std::make_unique<HTTP2BufferWaterMark>(item.http2_buffer_water_mark.value()));
+ item.populate_sni_actions(ai.actions);
+ if (set_next_hop_properties(item) == 1) {
+ return 1;
}
+ }
- ai->actions.push_back(std::make_unique<ServerMaxEarlyData>(item.server_max_early_data));
+ return 0;
+}
- ai->actions.push_back(std::make_unique<SNI_IpAllow>(item.ip_allow, item.fqdn));
+int
Review Comment:
Why are these returning `int`? These should all return `bool` or `swoc::Errata`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1581396391
Why is a `vector` used if only one range is supported?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] JosiahWI commented on a diff in pull request #9767: Filter SNI based on port
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1222062023
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -111,80 +113,72 @@ int
SNIConfigParams::load_sni_config()
{
for (auto &item : yaml_sni.items) {
- auto ai = sni_action_list.emplace(sni_action_list.end());
- ai->set_glob_name(item.fqdn);
+ auto &ai = sni_action_list.emplace_back();
+ ai.set_glob_name(item.fqdn);
+ if (!item.port_ranges.empty()) {
+ auto const [min, max]{item.port_ranges[0]};
Review Comment:
There is only one element; I'm planning to implement support for multiple port ranges immediately after this PR is finished.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1222069046
##########
iocore/net/YamlSNIConfig.h:
##########
@@ -71,6 +76,7 @@ struct YamlSNIConfig {
struct Item {
std::string fqdn;
+ std::vector<std::pair<uint16_t, uint16_t>> port_ranges;
Review Comment:
Use `port_range_type` here?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9767: Filter SNI based on port
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#issuecomment-1572848217
> IMO, we should not allow pcre.
Interesting. To be clear, are you suggesting we remove the corresponding tunnel_route's ability to use matched groups from the fqdn?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1224746027
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -271,22 +267,22 @@ SNIConfig::reconfigure()
Debug("ssl", "Reload SNI file");
SNIConfigParams *params = new SNIConfigParams;
- int retStatus = params->initialize();
- if (!retStatus) {
+ bool retStatus = params->initialize();
+ if (retStatus) {
_configid = configProcessor.set(_configid, params);
prewarmManager.reconfigure();
} else {
delete params;
}
std::string sni_filename = RecConfigReadConfigPath("proxy.config.ssl.servername.filename");
- if (!retStatus || TSSystemState::is_initializing()) {
+ if (retStatus || TSSystemState::is_initializing()) {
Note("%s finished loading", sni_filename.c_str());
} else {
Error("%s failed to load", sni_filename.c_str());
}
- return !retStatus;
+ return retStatus ? 1 : 0;
Review Comment:
You can also do `!!restStatus`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9767: Filter SNI based on port
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1224747907
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -306,12 +302,12 @@ SNIConfig::release(SNIConfigParams *params)
// setting proxy.config.http.host_sni_policy and is possibly overridden if the sni policy
// contains a host_sni_policy entry
bool
-SNIConfig::test_client_action(const char *servername, const IpEndpoint &ep, int &host_sni_policy)
+SNIConfig::test_client_action(const char *servername, uint16_t dest_incoming_port, const IpEndpoint &ep, int &host_sni_policy)
Review Comment:
`in_port_t`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org