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/14 14:12:53 UTC
[GitHub] [trafficserver] JosiahWI opened a new pull request, #9845: Extract apply_ip_allow_filter
JosiahWI opened a new pull request, #9845:
URL: https://github.com/apache/trafficserver/pull/9845
This breaks up the ip allow check in HttpSM::do_http_server_open.
--
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] cmcfarlen commented on a diff in pull request #9845: Extract apply_ip_allow_filter
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1230231443
##########
proxy/http/HttpSM.cc:
##########
@@ -5261,29 +5261,29 @@ HttpSM::get_outbound_sni() const
bool
HttpSM::apply_ip_allow_filter()
{
+ bool result{true};
// Method allowed on dest IP address check
IpAllow::ACL acl = IpAllow::match(server_ip(), IpAllow::DST_ADDR);
if (ip_allow_is_request_forbidden(acl)) {
ip_allow_deny_request(acl);
- return false;
- }
-
- if (HttpTransact::is_server_negative_cached(&t_state) == true &&
- t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+ result = false;
+ } else if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+ t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
call_transact_and_set_next_state(HttpTransact::OriginDown);
- return false;
+ result = false;
}
- return true;
+ return result;
}
bool
HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
{
+ bool result{false};
if (acl.isValid()) {
if (acl.isDenyAll()) {
- return true;
+ result = true;
} else if (!acl.isAllowAll()) {
if (method() != -1) {
return !acl.isMethodAllowed(method());
Review Comment:
`result =`
--
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 #9845: Extract apply_ip_allow_filter
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1233074646
##########
proxy/http/HttpSM.h:
##########
@@ -519,6 +522,18 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
/// Update the milestones to track time spent in the plugin API.
void milestone_update_api_time();
+ sockaddr *
+ get_server_ip() const
Review Comment:
There are two IP addresses associated with the server, the method name should indicate which one. The standard terms are "local" (address used on the ATS end of the socket) and "remote" (address used on the non-ATS end of the socket). This should probably be `get_server_remote_addr`.
--
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 merged pull request #9845: Extract apply_ip_allow_filter
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845
--
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 #9845: Extract apply_ip_allow_filter
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1233337019
##########
proxy/http/HttpSM.cc:
##########
@@ -5258,6 +5258,70 @@ HttpSM::get_outbound_sni() const
return zret;
}
+bool
+HttpSM::apply_ip_allow_filter()
+{
+ bool result{true};
+ // Method allowed on dest IP address check
+ IpAllow::ACL acl = IpAllow::match(get_server_ip(), IpAllow::DST_ADDR);
+
+ if (ip_allow_is_request_forbidden(acl)) {
+ ip_allow_deny_request(acl);
+ result = false;
+ } else if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+ t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+ call_transact_and_set_next_state(HttpTransact::OriginDown);
+ result = false;
+ }
+
+ return result;
+}
+
+bool
+HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
+{
+ bool result{false};
+ if (acl.isValid()) {
+ if (acl.isDenyAll()) {
+ result = true;
+ } else if (!acl.isAllowAll()) {
+ if (get_request_method() != -1) {
+ result = !acl.isMethodAllowed(get_request_method());
+ } else {
+ int method_str_len{};
Review Comment:
Apparently, I thought you were using parens. Need new glasses.
--
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 #9845: Extract apply_ip_allow_filter
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#issuecomment-1598634645
Test "TSVConnFD" failed with a SIGSEGV (9) on the first run.
--
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 #9845: Extract apply_ip_allow_filter
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1231223111
##########
proxy/http/HttpSM.h:
##########
@@ -519,6 +522,18 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
/// Update the milestones to track time spent in the plugin API.
void milestone_update_api_time();
+ sockaddr *
+ server_ip() const
+ {
+ return &t_state.current.server->dst_addr.sa;
+ };
+
+ int
+ method() const
+ {
+ return t_state.hdr_info.server_request.method_get_wksidx();
+ };
Review Comment:
`ip_allow_is_request_forbidden` belongs in IPAllow and I tried it that way - it introduced another cyclic dependency which is probably the greater of the two evils.
--
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 #9845: Extract apply_ip_allow_filter
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1233094676
##########
proxy/http/HttpSM.cc:
##########
@@ -5258,6 +5258,70 @@ HttpSM::get_outbound_sni() const
return zret;
}
+bool
+HttpSM::apply_ip_allow_filter()
+{
+ bool result{true};
+ // Method allowed on dest IP address check
+ IpAllow::ACL acl = IpAllow::match(get_server_ip(), IpAllow::DST_ADDR);
+
+ if (ip_allow_is_request_forbidden(acl)) {
+ ip_allow_deny_request(acl);
+ result = false;
+ } else if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+ t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+ call_transact_and_set_next_state(HttpTransact::OriginDown);
+ result = false;
+ }
+
+ return result;
+}
+
+bool
+HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
+{
+ bool result{false};
+ if (acl.isValid()) {
+ if (acl.isDenyAll()) {
+ result = true;
+ } else if (!acl.isAllowAll()) {
+ if (get_request_method() != -1) {
+ result = !acl.isMethodAllowed(get_request_method());
+ } else {
+ int method_str_len{};
Review Comment:
Misread? This is zero-initialization, using the universal initialization syntax.
--
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] cmcfarlen commented on a diff in pull request #9845: Extract apply_ip_allow_filter
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1229964012
##########
proxy/http/HttpSM.cc:
##########
@@ -5258,6 +5258,68 @@ HttpSM::get_outbound_sni() const
return zret;
}
+bool
+HttpSM::apply_ip_allow_filter()
+{
+ // Method allowed on dest IP address check
+ IpAllow::ACL acl = IpAllow::match(server_ip(), IpAllow::DST_ADDR);
+
+ if (ip_allow_is_request_forbidden(acl)) {
+ ip_allow_deny_request(acl);
+ return false;
+ }
+
+ if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+ t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+ call_transact_and_set_next_state(HttpTransact::OriginDown);
+ return false;
+ }
+
+ return true;
+}
+
+bool
+HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
+{
+ if (acl.isValid()) {
+ if (acl.isDenyAll()) {
+ return true;
Review Comment:
IMO, style-wise, I like to see a result variable with the default and then a single return after calculating the answer.
##########
proxy/http/HttpSM.cc:
##########
@@ -5258,6 +5258,68 @@ HttpSM::get_outbound_sni() const
return zret;
}
+bool
+HttpSM::apply_ip_allow_filter()
+{
+ // Method allowed on dest IP address check
+ IpAllow::ACL acl = IpAllow::match(server_ip(), IpAllow::DST_ADDR);
+
+ if (ip_allow_is_request_forbidden(acl)) {
+ ip_allow_deny_request(acl);
+ return false;
+ }
+
+ if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+ t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+ call_transact_and_set_next_state(HttpTransact::OriginDown);
+ return false;
+ }
+
+ return true;
+}
+
+bool
+HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
+{
+ if (acl.isValid()) {
+ if (acl.isDenyAll()) {
+ return true;
+ } else if (!acl.isAllowAll()) {
+ if (method() != -1) {
+ return !acl.isMethodAllowed(method());
+ } else {
+ int method_str_len{};
+ auto method_str = t_state.hdr_info.server_request.method_get(&method_str_len);
+ return !acl.isNonstandardMethodAllowed(std::string_view(method_str, method_str_len));
+ }
+ }
+ }
+
+ return false;
+}
+
+void
+HttpSM::ip_allow_deny_request(const IpAllow::ACL &acl)
+{
+ if (is_debug_tag_set("ip_allow")) {
+ ip_text_buffer ipb;
+ const char *method_str;
+ int method_str_len{};
+ if (method() != -1) {
+ method_str = hdrtoken_index_to_wks(method());
+ method_str_len = strlen(method_str);
+ } else {
+ method_str = t_state.hdr_info.client_request.method_get(&method_str_len);
+ }
+ Warning("server '%s' prohibited by ip-allow policy at line %d", ats_ip_ntop(server_ip(), ipb, sizeof(ipb)), acl.source_line());
+ SMDebug("ip_allow", "Line %d denial for '%.*s' from %s", acl.source_line(), method_str_len, method_str,
+ ats_ip_ntop(server_ip(), ipb, sizeof(ipb)));
Review Comment:
Since you are cleaning this up you could pull the `ats_ip_ntop` call out 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] JosiahWI commented on pull request #9845: Extract apply_ip_allow_filter
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#issuecomment-1591810664
[approve ci fedora]
--
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 #9845: Extract apply_ip_allow_filter
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1233074891
##########
proxy/http/HttpSM.cc:
##########
@@ -5258,6 +5258,70 @@ HttpSM::get_outbound_sni() const
return zret;
}
+bool
+HttpSM::apply_ip_allow_filter()
+{
+ bool result{true};
+ // Method allowed on dest IP address check
+ IpAllow::ACL acl = IpAllow::match(get_server_ip(), IpAllow::DST_ADDR);
+
+ if (ip_allow_is_request_forbidden(acl)) {
+ ip_allow_deny_request(acl);
+ result = false;
+ } else if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+ t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+ call_transact_and_set_next_state(HttpTransact::OriginDown);
+ result = false;
+ }
+
+ return result;
+}
+
+bool
+HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
+{
+ bool result{false};
+ if (acl.isValid()) {
+ if (acl.isDenyAll()) {
+ result = true;
+ } else if (!acl.isAllowAll()) {
+ if (get_request_method() != -1) {
+ result = !acl.isMethodAllowed(get_request_method());
+ } else {
+ int method_str_len{};
+ auto method_str = t_state.hdr_info.server_request.method_get(&method_str_len);
+ result = !acl.isNonstandardMethodAllowed(std::string_view(method_str, method_str_len));
+ }
+ }
+ }
+
+ return result;
+}
+
+void
+HttpSM::ip_allow_deny_request(const IpAllow::ACL &acl)
+{
+ if (is_debug_tag_set("ip_allow")) {
+ ip_text_buffer ipb;
+ const char *method_str{};
+ int method_str_len{};
+ if (get_request_method() != -1) {
+ method_str = hdrtoken_index_to_wks(get_request_method());
Review Comment:
How does this compile? It looks like `get_request_method` returns `int` not `char*`.
--
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 #9845: Extract apply_ip_allow_filter
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1233096047
##########
proxy/http/HttpSM.cc:
##########
@@ -5258,6 +5258,70 @@ HttpSM::get_outbound_sni() const
return zret;
}
+bool
+HttpSM::apply_ip_allow_filter()
+{
+ bool result{true};
+ // Method allowed on dest IP address check
+ IpAllow::ACL acl = IpAllow::match(get_server_ip(), IpAllow::DST_ADDR);
+
+ if (ip_allow_is_request_forbidden(acl)) {
+ ip_allow_deny_request(acl);
+ result = false;
+ } else if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+ t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+ call_transact_and_set_next_state(HttpTransact::OriginDown);
+ result = false;
+ }
+
+ return result;
+}
+
+bool
+HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
+{
+ bool result{false};
+ if (acl.isValid()) {
+ if (acl.isDenyAll()) {
+ result = true;
+ } else if (!acl.isAllowAll()) {
+ if (get_request_method() != -1) {
+ result = !acl.isMethodAllowed(get_request_method());
+ } else {
+ int method_str_len{};
+ auto method_str = t_state.hdr_info.server_request.method_get(&method_str_len);
+ result = !acl.isNonstandardMethodAllowed(std::string_view(method_str, method_str_len));
+ }
+ }
+ }
+
+ return result;
+}
+
+void
+HttpSM::ip_allow_deny_request(const IpAllow::ACL &acl)
+{
+ if (is_debug_tag_set("ip_allow")) {
+ ip_text_buffer ipb;
+ const char *method_str{};
+ int method_str_len{};
+ if (get_request_method() != -1) {
+ method_str = hdrtoken_index_to_wks(get_request_method());
Review Comment:
`hdrtoken_index_to_wks` takes an `int` (an index). Renaming `get_request_method` to `get_request_method_wksidx` should clarify this code.
--
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 #9845: Extract apply_ip_allow_filter
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1233337623
##########
proxy/http/HttpSM.cc:
##########
@@ -5258,6 +5258,70 @@ HttpSM::get_outbound_sni() const
return zret;
}
+bool
+HttpSM::apply_ip_allow_filter()
+{
+ bool result{true};
+ // Method allowed on dest IP address check
+ IpAllow::ACL acl = IpAllow::match(this->get_server_remote_addr(), IpAllow::DST_ADDR);
+
+ if (ip_allow_is_request_forbidden(acl)) {
+ ip_allow_deny_request(acl);
+ result = false;
+ } else if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+ t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+ call_transact_and_set_next_state(HttpTransact::OriginDown);
+ result = false;
+ }
+
+ return result;
+}
+
+bool
+HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
+{
+ bool result{false};
+ if (acl.isValid()) {
+ if (acl.isDenyAll()) {
+ result = true;
+ } else if (!acl.isAllowAll()) {
+ if (this->get_request_method_wksidx() != -1) {
+ result = !acl.isMethodAllowed(this->get_request_method_wksidx());
+ } else {
+ int method_str_len{};
+ auto method_str = t_state.hdr_info.server_request.method_get(&method_str_len);
+ result = !acl.isNonstandardMethodAllowed(std::string_view(method_str, method_str_len));
+ }
+ }
+ }
+
+ return result;
+}
+
+void
+HttpSM::ip_allow_deny_request(const IpAllow::ACL &acl)
+{
+ if (is_debug_tag_set("ip_allow")) {
+ ip_text_buffer ipb;
+ const char *method_str{};
+ int method_str_len{};
+ if (this->get_request_method_wksidx() != -1) {
Review Comment:
What is the point of this, since it's just turned into a string anyway? In the original code, this was done separately because the WKSIDX was used to check the permission bit. That doesn't happen here, making the WKSIDX useless.
--
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 a diff in pull request #9845: Extract apply_ip_allow_filter
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1231205452
##########
proxy/http/HttpSM.h:
##########
@@ -519,6 +522,18 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
/// Update the milestones to track time spent in the plugin API.
void milestone_update_api_time();
+ sockaddr *
+ server_ip() const
+ {
+ return &t_state.current.server->dst_addr.sa;
+ };
+
+ int
+ method() const
+ {
+ return t_state.hdr_info.server_request.method_get_wksidx();
+ };
Review Comment:
Can we make these explicit getters: `get_server_ip` and `get_request_method`
--
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 #9845: Extract apply_ip_allow_filter
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1233073461
##########
proxy/http/HttpSM.cc:
##########
@@ -5258,6 +5258,70 @@ HttpSM::get_outbound_sni() const
return zret;
}
+bool
+HttpSM::apply_ip_allow_filter()
+{
+ bool result{true};
+ // Method allowed on dest IP address check
+ IpAllow::ACL acl = IpAllow::match(get_server_ip(), IpAllow::DST_ADDR);
+
+ if (ip_allow_is_request_forbidden(acl)) {
+ ip_allow_deny_request(acl);
+ result = false;
+ } else if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+ t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+ call_transact_and_set_next_state(HttpTransact::OriginDown);
+ result = false;
+ }
+
+ return result;
+}
+
+bool
+HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
+{
+ bool result{false};
+ if (acl.isValid()) {
+ if (acl.isDenyAll()) {
+ result = true;
+ } else if (!acl.isAllowAll()) {
+ if (get_request_method() != -1) {
Review Comment:
Important style point - if calling a local method, use `this->`. E.g. `this->get_request_method()`. Knowing what's part of the class and what's global at a glance is very useful.
--
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 #9845: Extract apply_ip_allow_filter
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1233074476
##########
proxy/http/HttpSM.h:
##########
@@ -519,6 +522,18 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
/// Update the milestones to track time spent in the plugin API.
void milestone_update_api_time();
+ sockaddr *
Review Comment:
Pay attention to existing style - note none of the other methods are defined inline.
--
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 #9845: Extract apply_ip_allow_filter
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1233073728
##########
proxy/http/HttpSM.cc:
##########
@@ -5258,6 +5258,70 @@ HttpSM::get_outbound_sni() const
return zret;
}
+bool
+HttpSM::apply_ip_allow_filter()
+{
+ bool result{true};
+ // Method allowed on dest IP address check
+ IpAllow::ACL acl = IpAllow::match(get_server_ip(), IpAllow::DST_ADDR);
+
+ if (ip_allow_is_request_forbidden(acl)) {
+ ip_allow_deny_request(acl);
+ result = false;
+ } else if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+ t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+ call_transact_and_set_next_state(HttpTransact::OriginDown);
+ result = false;
+ }
+
+ return result;
+}
+
+bool
+HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
+{
+ bool result{false};
+ if (acl.isValid()) {
+ if (acl.isDenyAll()) {
+ result = true;
+ } else if (!acl.isAllowAll()) {
+ if (get_request_method() != -1) {
+ result = !acl.isMethodAllowed(get_request_method());
+ } else {
+ int method_str_len{};
Review Comment:
Is this a function declaration or an initialization?
* never call a default constructor on a built in.
* use "universal initialization" syntax, e.g. `method_ptr_len{}`. That syntax was invented specifically to solve this ambiguity.
--
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 #9845: Extract apply_ip_allow_filter
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1233074339
##########
proxy/http/HttpSM.h:
##########
@@ -519,6 +522,18 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
/// Update the milestones to track time spent in the plugin API.
void milestone_update_api_time();
+ sockaddr *
+ get_server_ip() const
+ {
+ return &t_state.current.server->dst_addr.sa;
+ };
+
+ int
+ get_request_method() const
Review Comment:
If this returns a WKS index, it should indicate that in the method name, just like `method_get_wksidx`.
--
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 #9845: Extract apply_ip_allow_filter
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#issuecomment-1598634879
[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 #9845: Extract apply_ip_allow_filter
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1233337623
##########
proxy/http/HttpSM.cc:
##########
@@ -5258,6 +5258,70 @@ HttpSM::get_outbound_sni() const
return zret;
}
+bool
+HttpSM::apply_ip_allow_filter()
+{
+ bool result{true};
+ // Method allowed on dest IP address check
+ IpAllow::ACL acl = IpAllow::match(this->get_server_remote_addr(), IpAllow::DST_ADDR);
+
+ if (ip_allow_is_request_forbidden(acl)) {
+ ip_allow_deny_request(acl);
+ result = false;
+ } else if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+ t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+ call_transact_and_set_next_state(HttpTransact::OriginDown);
+ result = false;
+ }
+
+ return result;
+}
+
+bool
+HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
+{
+ bool result{false};
+ if (acl.isValid()) {
+ if (acl.isDenyAll()) {
+ result = true;
+ } else if (!acl.isAllowAll()) {
+ if (this->get_request_method_wksidx() != -1) {
+ result = !acl.isMethodAllowed(this->get_request_method_wksidx());
+ } else {
+ int method_str_len{};
+ auto method_str = t_state.hdr_info.server_request.method_get(&method_str_len);
+ result = !acl.isNonstandardMethodAllowed(std::string_view(method_str, method_str_len));
+ }
+ }
+ }
+
+ return result;
+}
+
+void
+HttpSM::ip_allow_deny_request(const IpAllow::ACL &acl)
+{
+ if (is_debug_tag_set("ip_allow")) {
+ ip_text_buffer ipb;
+ const char *method_str{};
+ int method_str_len{};
+ if (this->get_request_method_wksidx() != -1) {
Review Comment:
What is the point of this, since it's just turned into a string anyway? In the original code, this was done separately because the WKSIDX was used to check the permission bit and was therefore necessary to obtain. That doesn't happen here, making the WKSIDX useless.
--
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 #9845: Extract apply_ip_allow_filter
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#issuecomment-1592122574
[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 #9845: Extract apply_ip_allow_filter
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#issuecomment-1593331455
`ip_allow_is_request_forbidden` belongs in IPAllow and I tried it that way - it introduced another cyclic dependency which is probably the greater of the two evils.
--
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