Hi dev@trafficserver.apache.org, I've been working with our remap.config ACL feature some for the IP Category work I've been doing. While adding ACL autests, I've noticed some unfriendly behaviors that I'd like to address for ATS 10. For reference, here's the documentation for remap.config ACLs:
https://docs.trafficserver.apache.org/en/latest/admin-guide/files/remap.config.en.html#acl-filters Leif already addressed a big one with this ATS 10 only change: https://github.com/apache/trafficserver/pull/9631 In there he made ACL rules function like remap: at most one ACL rule runs, the first one that matches. As he describes it there, that makes ACLs behave more like ip_allow.yaml rules. While considering the changes in ACL behavior here, be aware that change is already incompatible with 9. *At a high level, in this email I suggest a few tweaks to the ACL feature to make it more like ip_allow.yaml.* The ip_allow.yaml is more more like what people expect since it functions generally like iptables firewall rules: rules are processed top down and the first one that matches "wins". Here's the tweaks I've noticed while writing tests for the feature: 1. Currently, an ACL @action=allow rule never denies anything. In ip_allow.yaml, an allow rule lists a set of methods which are allowed for a range of IPs and any request from a corresponding IP that comes in with a method not in the allow list is denied. With ACLs on the other hand, the rule doesn't match if an incoming request comes in with a method not in the allow list and therefore the rule is not processed and the transaction is not rejected. I believe this is simply a bug in the implementation of ACL processing for @action=allow, but since fixing it changes current behavior, I want to state it here. 2. Leif's #9631 <https://github.com/apache/trafficserver/pull/9631> patch referenced above made it so that if there are named ACLs activated as well as potentially a remap.config in-line ACL, then only the first matching ACL, if any, is run. This makes sense. But I suggest switching the order the rules are processed such that the first ACL to be checked is the in-line ACL, then any named activated ACLs. This seems more natural since the in-line ACL is the most specific and should therefore take precedence before the broader activated named ACLs which potentially apply to multiple rules. 3. Currently, remap.config ACLs always run in conjunction with ip_allow.yaml rules, unless ip_allow.yaml is disabled via the special-purpose `.deactivatefilter ip_allow` named ACL. Thus, ip_allow.yaml is always either all-on or all-off. We can keep the `.deactivatefilter ip_allow` toggle because it might have helpful purposes, but I suggest making the ACLs first class citizens along with ip_allow.yaml rules such that if any of the ACLs "match", then the ip_allow.yaml rules implicitly do not run. Thus the ACLs and ip_allow.yaml rules would layer from (1) remap.config in-line ACL, (2) named activated ACLs, and then (3) ip_allow.yaml rules, with the first rule matching being used and none of the others being used. I've found these changes all straightforward to implement and test. A PR with them is here: https://github.com/apache/trafficserver/pull/11033 Let me know if there are any concerns to these changes for ATS 10. Thanks, Brian -- "Come to Me, all who are weary and heavy-laden, and I will give you rest. Take My yoke upon you and learn from Me, for I am gentle and humble in heart, and you will find rest for your souls. For My yoke is easy and My burden is light." ~ Matthew 11:28-30
