[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode closed the pull request at:

https://github.com/apache/trafficserver/pull/769


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r83323558
  
--- Diff: proxy/http/remap/RemapConfig.cc ---
@@ -1153,6 +1169,9 @@ remap_parse_config_bti(const char *path, 
BUILD_TABLE_INFO *bti)
   goto MAP_ERROR;
 }
 
+// update sticky flag
+bti->accept_check_p = bti->accept_check_p && 
(bool)bti->ip_allow_check_enabled_p;
--- End diff --

What about `bti->accept_check_p &&= bit->ip_allow_check_enabled_p;` ? Is 
the cast to `bool` required? I would think `ip_allow_check_enabled_p` to be 
declared as `bool`. Also, I'm not sure this is the correct place to update the 
sticky flag. Activating other filters should have no effect on the sticky flag, 
only defining rules. I'l look at at putting it back around line 128, after the 
update to the rule flag.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r83321967
  
--- Diff: proxy/http/HttpSM.cc ---
@@ -4726,6 +4727,44 @@ HttpSM::do_http_server_open(bool raw)
 milestones[TS_MILESTONE_SERVER_FIRST_CONNECT] = 
milestones[TS_MILESTONE_SERVER_CONNECT];
   }
 
+  // Check for remap rule. If so, only apply ip_allow filter if it is 
activated (ip_allow_check_enabled_p set).
+  // Otherwise, if no remap rule is defined, apply the ip_allow filter.
+  if(!t_state.url_remap_success || (t_state.url_remap_success && 
t_state.url_map.getMapping()->ip_allow_check_enabled_p)) {
--- End diff --

I think this can be simplified to
```
if(!t_state.url_remap_success || 
t_state.url_map.getMapping()->ip_allow_check_enabled_p))
```
The second clause can only be checked if `t_state.url_remap_success` is 
`true` - otherwise the first clause would have been `true` and the evaluation 
stopped, so there's no need to check it again.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r83322212
  
--- Diff: proxy/http/HttpTransact.cc ---
@@ -567,6 +567,16 @@ HttpTransact::BadRequest(State *s)
 }
 
 void
+HttpTransact::Forbidden(State *s)
+{
+  DebugTxn("http_trans", "[Forbidden]"
+ "parser marked request forbidden");
--- End diff --

Yes, but the descriptive text should be updated to "IPAllow marked request 
forbidden".


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r83321096
  
--- Diff: proxy/IPAllow.cc ---
@@ -287,12 +298,16 @@ IpAllow::BuildTable()
 line = tokLine(NULL, &tok_state);
   }
 
-  if (_map.getCount() == 0) {
+  if (_src_map.getCount() == 0 && _dest_map.getCount() == 0) { // TODO: 
check
 Warning("%s No entries in %s. All IP Addresses will be blocked", 
module_name, config_file_path);
   } else {
 // convert the coloring from indices to pointers.
-for (IpMap::iterator spot(_map.begin()), limit(_map.end()); spot != 
limit; ++spot) {
-  spot->setData(&_acls[reinterpret_cast(spot->data())]);
+for (IpMap::iterator spot(_src_map.begin()), limit(_src_map.end()); 
spot != limit; ++spot) {
--- End diff --

Well, how about container style for loops?
```
for ( auto& item : _src_map )
  item.setData(&_acls[reinterpret_cast(item.data())]);
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[kawaiirice]

Github user kawaiirice commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r83247044
  
--- Diff: proxy/http/HttpTransact.cc ---
@@ -567,6 +567,16 @@ HttpTransact::BadRequest(State *s)
 }
 
 void
+HttpTransact::Forbidden(State *s)
+{
+  DebugTxn("http_trans", "[Forbidden]"
+ "parser marked request forbidden");
--- End diff --

I added this Forbidden state and copied the debug message from the 
BadRequest state


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[kawaiirice]

Github user kawaiirice commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r83245499
  
--- Diff: proxy/http/HttpSM.cc ---
@@ -4726,6 +4727,44 @@ HttpSM::do_http_server_open(bool raw)
 milestones[TS_MILESTONE_SERVER_FIRST_CONNECT] = 
milestones[TS_MILESTONE_SERVER_CONNECT];
   }
 
+  // Check for remap rule. If so, only apply ip_allow filter if it is 
activated (ip_allow_flag set).
+  // Otherwise, if no remap rule is defined, apply the ip_allow filter. 
+  if(!t_state.url_remap_success || (t_state.url_remap_success && 
t_state.url_map.getMapping()->ip_allow_flag)) {
--- End diff --

Is checking `acl_record->isEmpty()` right after finding a match for the 
server ip considered too late?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r82413900
  
--- Diff: proxy/http/remap/RemapConfig.cc ---
@@ -123,6 +124,9 @@ process_filter_opt(url_mapping *mp, const 
BUILD_TABLE_INFO *bti, char *errStrBuf
 }
 errStr = remap_validate_filter_args(rpp, (const char **)bti->argv, 
bti->argc, errStrBuf, errStrBufSize);
   }
+  // Copy ip_allow_flag to url_mapping
--- End diff --

"Set the ip allow flag for this rule to the current ip allow flag state"


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r82413603
  
--- Diff: proxy/http/HttpTransact.cc ---
@@ -567,6 +567,16 @@ HttpTransact::BadRequest(State *s)
 }
 
 void
+HttpTransact::Forbidden(State *s)
+{
+  DebugTxn("http_trans", "[Forbidden]"
+ "parser marked request forbidden");
--- End diff --

Was this here prior to the patch? "parser marked" seems incorrect, unless 
it's legacy.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r82414307
  
--- Diff: proxy/http/remap/RemapConfig.cc ---
@@ -1415,6 +1434,7 @@ remap_parse_config_bti(const char *path, 
BUILD_TABLE_INFO *bti)
 return false;
   } /* end of while(cur_line != NULL) */
 
+  (void)IpAllow::enableAcceptCheck(bti->accept_check_p);
--- End diff --

Did the compiler actually complain about this, to require the `(void)`?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r82414495
  
--- Diff: proxy/http/remap/RemapConfig.h ---
@@ -53,6 +53,8 @@ struct BUILD_TABLE_INFO {
   char *paramv[BUILD_TABLE_MAX_ARGS];
   char *argv[BUILD_TABLE_MAX_ARGS];
 
+  unsigned int ip_allow_flag : 1;
--- End diff --

Hmmm, need a better name. "ip_allow_check_enabled_p" maybe?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r82414079
  
--- Diff: proxy/http/remap/RemapConfig.cc ---
@@ -1153,6 +1169,9 @@ remap_parse_config_bti(const char *path, 
BUILD_TABLE_INFO *bti)
   goto MAP_ERROR;
 }
 
+// update sticky flag
+bti->accept_check_p &= (bool)bti->ip_allow_flag;
--- End diff --

`&&=` - +logical+ and.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r82413194
  
--- Diff: proxy/http/HttpSM.cc ---
@@ -4726,6 +4727,44 @@ HttpSM::do_http_server_open(bool raw)
 milestones[TS_MILESTONE_SERVER_FIRST_CONNECT] = 
milestones[TS_MILESTONE_SERVER_CONNECT];
   }
 
+  // Check for remap rule. If so, only apply ip_allow filter if it is 
activated (ip_allow_flag set).
+  // Otherwise, if no remap rule is defined, apply the ip_allow filter. 
+  if(!t_state.url_remap_success || (t_state.url_remap_success && 
t_state.url_map.getMapping()->ip_allow_flag)) {
+
+// Method allowed on dest IP address check
+sockaddr *server_ip = &t_state.current.server->dst_addr.sa;
+IpAllow::scoped_config ip_allow;
+const AclRecord *acl_record = NULL;
+int method = t_state.hdr_info.server_request.method_get_wksidx();
+
+if(ip_allow) {
+  acl_record = ip_allow->match(server_ip, true);
+  bool deny_request = false;
+
+  if (acl_record && !acl_record->isEmpty() && 
(acl_record->_method_mask != AclRecord::ALL_METHOD_MASK)) {
+if (method != -1) {
+  deny_request = !acl_record->isMethodAllowed(method);
+} else {
+  int method_str_len;
+  const char *method_str = 
t_state.hdr_info.server_request.method_get(&method_str_len);
+  deny_request = 
!acl_record->isNonstandardMethodAllowed(std::string(method_str, 
method_str_len));
+}
+  }
+  if (deny_request) {
+if (is_debug_tag_set("ip-allow")) {
+  ip_text_buffer ipb;
+  Warning("server '%s' prohibited by ip-allow policy", 
ats_ip_ntop(server_ip, ipb, sizeof(ipb)));
+  Debug("ip-allow", "Quick filter denial on %s:%s with mask %x", 
ats_ip_ntop(&t_state.current.server->dst_addr.sa, ipb, sizeof(ipb)),
--- End diff --

"Quick filter"?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r82400518
  
--- Diff: proxy/IPAllow.h ---
@@ -129,34 +129,55 @@ class IpAllow : public ConfigInfo
   {
 return &ALL_METHOD_ACL;
   }
+  
+  /// @return The previous accept check state
+  static bool enableAcceptCheck(bool state) {
+bool temp = accept_check_p;
+accept_check_p = state;
+return temp;
+  }
+  /// @return The current accept check state
+  static bool isAcceptCheckEnabled() {
+return accept_check_p;
+  }
 
   typedef ConfigProcessor::scoped_config scoped_config;
 
 private:
   static int configid;
   static const AclRecord ALL_METHOD_ACL;
+  static bool accept_check_p;
 
+  void PrintMap(IpMap * map);
   int BuildTable();
 
   char config_file_path[PATH_NAME_MAX];
   const char *module_name;
   const char *action;
-  IpMap _map;
-  Vec _acls;
+  IpMap _src_map;
+  IpMap _dest_map;
+  Vec _src_acls;
+  Vec _dest_acls;
 };
 
 inline AclRecord *
-IpAllow::match(IpEndpoint const *ip) const
+IpAllow::match(IpEndpoint const *ip, bool is_dest_ip) const
 {
-  return this->match(&ip->sa);
+  return this->match(&ip->sa, is_dest_ip);
 }
 
 inline AclRecord *
-IpAllow::match(sockaddr const *ip) const
+IpAllow::match(sockaddr const *ip, bool is_dest_ip) const
 {
   void *raw;
-  if (_map.contains(ip, &raw)) {
-return static_cast(raw);
+  if(is_dest_ip) {
--- End diff --

I think
```
IpAllow::match(sockaddr const* ip, match_key_t key) {
void *raw = NULL;
this->map(key).contains(ip, &raw);
return static_cast(raw);
```

Then define
```
IpMap& map(match_key_t key) { return key == SRC_ADDR ? _src_map : 
_dest_map; }
```

This could be used elsewhere instead of the current ternary operator for 
cleaner code.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r78470567
  
--- Diff: proxy/IPAllow.cc ---
@@ -190,9 +231,13 @@ IpAllow::BuildTable()
 }
 
 if (*line != '\0' && *line != '#') {
-  errPtr = parseConfigLine(line, &line_info, &ip_allow_tags);
-
-  if (errPtr != NULL) {
+  if(strstr(line, ip_allow_dest_tags.match_ip) != NULL) {
--- End diff --

```
matcher_tags& allow_tags = strstr(line, ip_allow_dest_tags.match_ip) != 
NULL ? ip_allow_dest_tags : ip_allow_src_tags);
errPtr = parseConfigLine(line, &line_info, &allow_tags);
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r78470100
  
--- Diff: proxy/IPAllow.cc ---
@@ -153,6 +154,46 @@ IpAllow::Print()
   }
 }
   }
+  for (IpMap::iterator spot(_dest_map.begin()), limit(_dest_map.end()); 
spot != limit; ++spot) {
--- End diff --

Quinn will change this to be two calls to the same method, passing in the 
two maps.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #769: TS 4593: Extend ip_allow.config to filter d...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/769#discussion_r78457571
  
--- Diff: proxy/IPAllow.cc ---
@@ -271,10 +316,16 @@ IpAllow::BuildTable()
   }
 
   if (method_found) {
-_acls.push_back(AclRecord(acl_method_mask, line_num, 
nonstandard_methods, deny_nonstandard_methods));
-// Color with index because at this point the address
-// is volatile.
-_map.fill(&addr1, &addr2, reinterpret_cast(_acls.length() - 1));
+if(is_dest_ip) {
+  _dest_acls.push_back(AclRecord(acl_method_mask, line_num, 
nonstandard_methods, deny_nonstandard_methods));
--- End diff --

These cases should be more compact by selecting the map then applying the 
fill.
```
Vec& acls = is_dest_ip ? _dest_acls : _src_acls;
IpMap& map = is_dest_ip ? _dest_map : _src_map;
acls.push_back(AclRecord(acl_method_mask, line_num, nonstandard_methods, 
deny_nonstandard_methods));
map.fill(&addr1, &addr2, reinterpret_cast(_dest_acls.length() - 1));
```



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---