You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by kawaiirice <gi...@git.apache.org> on 2016/06/29 22:48:44 UTC

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

GitHub user kawaiirice opened a pull request:

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

    TS 4593: Extend ip_allow.config to filter destination IPs

    Documentation for extending ip_allow.config to filter destination IP addresses, as proposed by Edge. 
    
    Do not merge: This document is for discussion purposes. 

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/kawaiirice/trafficserver TS-4593

Alternatively you can review and apply these changes as the patch at:

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

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #769
    
----
commit 72fc5f7347f9f2b130b07de035cd0e1f8ff0419b
Author: Quinn Lertratanakul <na...@yahoo-inc.com>
Date:   2016-06-29T22:04:35Z

    documented changes for extending ip_allow.config to filter dest_ip

----


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver issue #769: TS 4593: Extend ip_allow.config to filter destinat...

Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:

    https://github.com/apache/trafficserver/pull/769
  
    FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/478/ for details.
     



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver issue #769: TS 4593: Extend ip_allow.config to filter destinat...

Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:

    https://github.com/apache/trafficserver/pull/769
  
    @SolidWallOfCode Are we ready to land this?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kawaiirice <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver issue #769: TS 4593: Extend ip_allow.config to filter destinat...

Posted by kawaiirice <gi...@git.apache.org>.
Github user kawaiirice commented on the issue:

    https://github.com/apache/trafficserver/pull/769
  
    What do you mean by fast accept check?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver issue #769: TS 4593: Extend ip_allow.config to filter destinat...

Posted by kawaiirice <gi...@git.apache.org>.
Github user kawaiirice commented on the issue:

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


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by kawaiirice <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver issue #769: TS 4593: Extend ip_allow.config to filter destinat...

Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the issue:

    https://github.com/apache/trafficserver/pull/769
  
    Where is the change that disables the fast accept check? That's needed or a deny can't be overridden by remap.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver issue #769: TS 4593: Extend ip_allow.config to filter destinat...

Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:

    https://github.com/apache/trafficserver/pull/769
  
    [approve ci]


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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<void *>(_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<AclRecord>& 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<void *>(_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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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<IpAllow, IpAllow> 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<AclRecord> _acls;
    +  IpMap _src_map;
    +  IpMap _dest_map;
    +  Vec<AclRecord> _src_acls;
    +  Vec<AclRecord> _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<AclRecord *>(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<AclRecord*>(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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver issue #769: TS 4593: Extend ip_allow.config to filter destinat...

Posted by kawaiirice <gi...@git.apache.org>.
Github user kawaiirice commented on the issue:

    https://github.com/apache/trafficserver/pull/769
  
    Finally done!


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver issue #769: TS 4593: Extend ip_allow.config to filter destinat...

Posted by kawaiirice <gi...@git.apache.org>.
Github user kawaiirice commented on the issue:

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


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver issue #769: TS 4593: Extend ip_allow.config to filter destinat...

Posted by kawaiirice <gi...@git.apache.org>.
Github user kawaiirice commented on the issue:

    https://github.com/apache/trafficserver/pull/769
  
    Still debugging as everything is blocking and 503 is returned for every curl request. 


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver issue #769: TS 4593: Extend ip_allow.config to filter destinat...

Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:

    https://github.com/apache/trafficserver/pull/769
  
    Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/375/ for details.
     



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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<size_t>(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<size_t>(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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver issue #769: TS 4593: Extend ip_allow.config to filter destinat...

Posted by kawaiirice <gi...@git.apache.org>.
Github user kawaiirice commented on the issue:

    https://github.com/apache/trafficserver/pull/769
  
    dest_ip filtering has been tested for blocking connections at remap. 



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

Posted by SolidWallOfCode <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---