TS 4593: Extend ip_allow.config to filter destination IPs #769

kawaiirice · 2016-06-29T22:48:42Z

Documentation for extending ip_allow.config to filter destination IP addresses, as proposed by Edge. @SolidWallOfCode @shinrich

kawaiirice · 2016-07-01T18:01:23Z

dest_ip filtering has been implemented and tested for blocking connections at remap.
The test used was dest_ip=10.0.0.0-10.0.0.255 action=ip_deny method=ALL in ip_allow.config and map http://127.0.0.1:8080 http://10.0.0.10 in remap.config.

Also tested for normal functionality in default/normal configurations of traffic server.

zwoop · 2016-07-23T16:21:10Z

[approve ci]

atsci · 2016-07-23T16:23:15Z

Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/375/ for details.

atsci · 2016-07-23T16:35:28Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/478/ for details.

zwoop · 2016-08-18T00:29:54Z

@SolidWallOfCode Are we ready to land this?

SolidWallOfCode · 2016-09-12T21:23:11Z

proxy/IPAllow.cc

-            // 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));


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));

SolidWallOfCode · 2016-09-12T23:03:52Z

Where is the change that disables the fast accept check? That's needed or a deny can't be overridden by remap. Currently if in the IP allow configuration all methods are denied, the connection is dropped immediately after the ACCEPT event. That can't be done if remap configuration is to override such a deny (which is the essential feature requested for this fix).

SolidWallOfCode · 2016-10-07T14:08:55Z

proxy/IPAllow.cc

 const AclRecord IpAllow::ALL_METHOD_ACL(AclRecord::ALL_METHOD_MASK);

 int IpAllow::configid = 0;
+bool IpAllow::accept_check_p = true;


Comments, please.

SolidWallOfCode · 2016-10-07T14:11:52Z

proxy/IPAllow.cc

      errPtr = parseConfigLine(line, &line_info, &ip_allow_tags);
-
-      if (errPtr != NULL) {
+      


Clang format or tweak your editor to cleanup trailing space.

SolidWallOfCode · 2016-10-07T14:16:32Z

proxy/IPAllow.cc

    // 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) {


Let's convert these to lambda's now that we have eleventy.

Well, how about container style for loops?

for ( auto& item : _src_map ) item.setData(&_acls[reinterpret_cast<size_t>(item.data())]);

SolidWallOfCode · 2016-10-07T14:24:26Z

proxy/IPAllow.h

  void Print();
-  AclRecord *match(IpEndpoint const *ip) const;
-  AclRecord *match(sockaddr const *ip) const;
+  AclRecord *match(IpEndpoint const *ip, bool is_dest_ip) const;


Let's change this to an enum like

enum class match_key_t { SRC_ADDR, DST_ADDR };

This should make the call site clearer.

SolidWallOfCode · 2016-10-07T14:26:29Z

proxy/IPAllow.h

    return &ALL_METHOD_ACL;
  }
+
+  /// @return The previous accept check state


Add a comment that this is independent of the configuration and is therefore a class global so it can be changed without changing the configuration.

SolidWallOfCode · 2016-10-07T14:31:25Z

proxy/IPAllow.h

  void *raw;
-  if (_map.contains(ip, &raw)) {
-    return static_cast<AclRecord *>(raw);
+  if(is_dest_ip) {


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.

SolidWallOfCode · 2016-10-07T15:31:28Z

proxy/http/HttpSM.cc


+  // 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)) {


For performance reasons we should probably have a short cut that prevents as much of this as possible if there are no destination address rules. That might be worth having a specific method in the configuration object.

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

SolidWallOfCode · 2016-10-07T15:32:42Z

proxy/http/HttpSM.cc

+    // 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;


Move these inside the if - no need to compute them if there's no IP allow configuration.

SolidWallOfCode · 2016-10-07T15:33:53Z

proxy/http/HttpSM.cc

+        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)),


"Quick filter"?

SolidWallOfCode · 2016-10-07T15:36:12Z

proxy/http/HttpTransact.cc

+void
+HttpTransact::Forbidden(State *s)
+{
+  DebugTxn("http_trans", "[Forbidden]"


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

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

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

SolidWallOfCode · 2016-10-07T15:37:49Z

proxy/http/remap/RemapConfig.cc

    }
    errStr = remap_validate_filter_args(rpp, (const char **)bti->argv, bti->argc, errStrBuf, errStrBufSize);
  }
+  // Copy ip_allow_flag to url_mapping


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

SolidWallOfCode · 2016-10-07T15:38:35Z

proxy/http/remap/RemapConfig.cc

    }

+    // update sticky flag
+    bti->accept_check_p &= (bool)bti->ip_allow_flag;


&&= - logical and.

SolidWallOfCode · 2016-10-07T15:39:20Z

proxy/http/remap/RemapConfig.cc

    return false;
  } /* end of while(cur_line != NULL) */

+  (void)IpAllow::enableAcceptCheck(bti->accept_check_p);


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

SolidWallOfCode · 2016-10-07T15:39:52Z

proxy/http/remap/RemapConfig.h

  char *paramv[BUILD_TABLE_MAX_ARGS];
  char *argv[BUILD_TABLE_MAX_ARGS];

+  unsigned int ip_allow_flag : 1;


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

…led from remap. (#1) Merged

SolidWallOfCode

Worked with @kawaiirice to get this fixed up.

SolidWallOfCode · 2016-10-13T22:02:40Z

proxy/http/HttpSM.cc


+  // 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)) {


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.

SolidWallOfCode · 2016-10-13T22:04:23Z

proxy/http/HttpTransact.cc

+void
+HttpTransact::Forbidden(State *s)
+{
+  DebugTxn("http_trans", "[Forbidden]"


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

SolidWallOfCode · 2016-10-13T22:13:13Z

proxy/http/remap/RemapConfig.cc

    }

+    // update sticky flag
+    bti->accept_check_p = bti->accept_check_p && (bool)bti->ip_allow_check_enabled_p;


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.

kawaiirice · 2016-11-02T21:11:06Z

Finally done!

…e#769) A number of tests were flakey because of race conditions between the AuTest framework finishing the Default TestRun process and ending the test, and therefore the traffic server process, before traffic server had time to exercise the expected functionality of the test. This improves the Ready conditions for these tests which should improve their reliability. (cherry picked from commit c175aa4)

* Stricten field name check * Add a test --------- Co-authored-by: Masakazu Kitajo <maskit@apache.org>

zwoop added Core Configuration labels Jul 23, 2016

zwoop added this to the 7.0.0 milestone Jul 23, 2016

zwoop assigned SolidWallOfCode Jul 23, 2016

kawaiirice force-pushed the TS-4593 branch from 26ba705 to bc33e1d Compare September 9, 2016 17:58

SolidWallOfCode reviewed Sep 12, 2016
View reviewed changes

zwoop modified the milestones: 7.1.0, 7.0.0 Sep 15, 2016

SolidWallOfCode reviewed Oct 7, 2016

View reviewed changes

TS-4593: Extend IPAllow to filter outbound connections and be control…

0c025b0

…led from remap. (#1) Merged

SolidWallOfCode approved these changes Nov 2, 2016

View reviewed changes

kawaiirice force-pushed the TS-4593 branch from 1c9b266 to 0c025b0 Compare November 2, 2016 21:09

SolidWallOfCode merged commit b92a6b3 into apache:master Nov 2, 2016

masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request May 29, 2025

Stricten field name check (apache#769)

e9fc8d6

* Stricten field name check * Add a test --------- Co-authored-by: Masakazu Kitajo <maskit@apache.org>

		errPtr = parseConfigLine(line, &line_info, &ip_allow_tags);

		if (errPtr != NULL) {

TS 4593: Extend ip_allow.config to filter destination IPs #769

TS 4593: Extend ip_allow.config to filter destination IPs #769

Uh oh!

Conversation

kawaiirice commented Jun 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kawaiirice commented Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zwoop commented Jul 23, 2016

Uh oh!

atsci commented Jul 23, 2016

Uh oh!

atsci commented Jul 23, 2016

Uh oh!

zwoop commented Aug 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kawaiirice commented Nov 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

kawaiirice commented Jun 29, 2016 •

edited

Loading

kawaiirice commented Jul 1, 2016 •

edited

Loading

SolidWallOfCode commented Sep 12, 2016 •

edited

Loading

SolidWallOfCode Oct 7, 2016 •

edited

Loading

SolidWallOfCode Oct 7, 2016 •

edited

Loading