Hi Masaori, Thank you for the thoughtful and swift feedback.
# 1. "Match on IP and Method Policy" > Let me clarify one thing about the current default confing called "Match > on IP and Method Policy". It employs the "first match wins" policy. This > is an improvement from ATS 9.2.x, making Inline Filter, Named Filter, and > ip_allow.yaml work well. This is a good clarification. Is there something I can change to make this more clear in the docs? Or is this just a comment for the wider audience? # 2. ATS 10.0.0 > I would prefer to avoid making any additional changes of ACL in the last > minuetes of ATS 10.0.0 release unless there is a serious bug. The target > of this discussion should be ATS 10.1.0 or 11.0.0. We actually need `add_deny` and `add_allow` for 10.0.0 because I'm convinced now after the discussions between you, Matty, and me, that the modern policy needs these behaviors too. These new actions are small, safe, and valuable for 10.0.0. Note that they don't add any new implementation - they just add new names for an implementation we already have. Further, "Match on IP and Method" is benefited by them to to help with the transition to the other. Let's definitely get these actions into 10.0.0. # 3. `add_allow` and `add_deny` actions > I understand that if the "Match on IP only Policy" has `add_allow` and > `add_deny` actions, it can be superset. However, I don't see any reason to > make "Match on IP only Policy" as the default config and deprecate the > "Match on IP and Method Policy". > The biggest concern with having "Match on IP only Policy" as the default > is, > it might accidentaly allow methods that can be a security hole. > For example, when a user configures a remap rule like below, as you > described, > ``` > map http://www.example.com/ http://internal.example.com/ \ > @action=deny @method=POST > ``` > This allows ALL requests except POST request with "Match on IP only > Policy". > That means PURGE and PUSH requests are allowed despite they're denied in > ip_allow.yaml in default. > I assume, you're proposing the `add_deny` action for this case and > suggesting > use it instead of the `deny` action. However, even if it has additional > knobs, > this is still a trap. A clarification and a thought here: 1. I think you understand this, but just to make sure this is clear for others in case my email didn't state this explicitly: I am not proposing any default behavior change over what we currently have implemented for ATS 10. The current default "Match on IP and Method" policy will still be the default, just renamed to "legacy". 2. The impetus for the initial change I made around ACL filters is that the `allow` and `deny` actions, while named the same between ip_allow.yaml rules and ACL filter rules, currently behave very differently. Differently enough that when I made these ACL filter actions behave the same as ip_allow.yaml, it caused issues with your configuration on upgrade. It is confusing that these actions of the same name act differently. That said, you convinced me ofthe pain of the incompatible change into 10, and for that reason we should keep the old behavior the default for 10.0.0. Now, concerning your ACL filter `deny` example: a `deny` action of POST allows all other methods for ip_allow.yaml too. If that is a security issue for ACL filters, it is for ip_allow.yaml as well. I don't think we would make that argument. On the other hand, the current behavior does come with a significant security risk. We at Yahoo have regularly seen properties that, assuming these actions behave the same between ip_allow.yaml and ACL filters, do something like this: ``` map http://www.example.com/ http://internal.example.com/ \ @action=allow @method=GET @method=HEAD ``` Intuitively, they expect this ACL to allow both GET and HEAD requests and deny all other methods, just like the `allow` action would in ip_allow.yaml. Instead, this adds no denial of methods because it acts like the new `add_allow` action. This is a security risk via confusing configuration. And this is not a theoretical security risk, it is one we face internally at Yahoo with some periodicity (maybe every couple years). And I can't really blame the ops guys who do this - their expectation makes sense. The 9.x docs (which by the way, still are not updated with any of the clarifications you and I have made) don't state the difference in behavior - they just say that these ACL filter rules act like ip_allow.yaml rules. In addition to being confusing, the legacy 9.x behavior cannot even do the intention of the above. With the new policy, they can both add methods to allow or deny via `add_allow` or `add_deny`, or they can set a whole new ip_allow.yaml rule for the target via `allow` or `deny` actions. This makes the above work as expected and effectively locks down a target to just GET and HEAD requests. Thus the new behavior is both clearer and more explicit for the `add_deny` and `add_allow` behavior, and it adds a very valuable ability to close off all methods except for those in an allow list. The legacy behavior objectively offers no advantages over the new behavior. Thus we should transition people off the old policy, name it legacy, and deprecate it and remove it in 11. Again, I really appreciate your timely response Masaori. That's really helpful for our discussion. I'm open to discussing this further in a meeting as well if that will be helpful (with the important points and decision being made here in email per our policy, of course). Brian On Wed, Jul 17, 2024 at 12:30 AM Masaori Koshiba <masa...@apache.org> wrote: > Thank you for starting this discussion. I have three comments: > > # 1. "Match on IP and Method Policy" > > Let me clarify one thing about the current default confing called "Match > on IP and Method Policy". It employs the "first match wins" policy. This > is an improvement from ATS 9.2.x, making Inline Filter, Named Filter, and > ip_allow.yaml work well. > > - e.g. > deny the PUT method in ip_allow.yaml for general cases, but allow > it on a specific remap rule. > > # 2. ATS 10.0.0 > > I would prefer to avoid making any additional changes of ACL in the last > minuetes of ATS 10.0.0 release unless there is a serious bug. The target > of this discussion should be ATS 10.1.0 or 11.0.0. > > # 3. `add_allow` and `add_deny` actions > > I understand that if the "Match on IP only Policy" has `add_allow` and > `add_deny` actions, it can be superset. However, I don't see any reason to > make "Match on IP only Policy" as the default config and deprecate the > "Match on IP and Method Policy". > > The biggest concern with having "Match on IP only Policy" as the default > is, > it might accidentaly allow methods that can be a security hole. > > For example, when a user configures a remap rule like below, as you > described, > > ``` > map http://www.example.com/ http://internal.example.com/ \ > @action=deny @method=POST > ``` > > This allows ALL requests except POST request with "Match on IP only > Policy". > That means PURGE and PUSH requests are allowed despite they're denied in > ip_allow.yaml in default. > > I assume, you're proposing the `add_deny` action for this case and > suggesting > use it instead of the `deny` action. However, even if it has additional > knobs, > this is still a trap. > > Thanks, > Masaori > > On Wed, Jul 17, 2024 at 6:32 AM Brian Neradt <brian.ner...@gmail.com> > wrote: > > > > dev@trafficserver.apache.org: > > > > Matty Williams, Masaori Koshiba, and I have been working on polishing up > > ACL filter rules for 10.0.x. Masaori and Matty worked on a configuration > > that keeps 10.0.x ACL filter action behavior, by default, the same as it > > was for ATS 9.x via the following PRs: > > > > > > - https://github.com/apache/trafficserver/pull/11433 > > - https://github.com/apache/trafficserver/pull/11448 > > > > While collaborating with them on that work, it occurred to me that the > > additive action behavior of 9.x ACL filter `allow` and `deny` actions can > > be valuable for the new 10.x behavior as well. As a result, I suggest the > > following incremental change upon our work so far to ACL filters for > 10.x: > > > > - Name our two ACL action modes "legacy" and "modern". The legacy > policy > > makes the allow and deny ACL filter actions behave as they do for > 9.x: they > > additively layer allowed or denied methods on top of other > ip_allow.yaml > > rules. The modern behavior makes the allow and deny rules act like > ip_allow > > rules: they fully specify how requests will be accepted or denied in > the > > rule itself. > > - Add `add_allow` and `add_deny` ACL filter actions in addition to the > > already existing `allow` and `deny` actions. These actions behave the > same > > between both legacy and modern ACL filter action behavior modes: they > act > > like `allow` and `deny` actions behaved in 9.x and before, adding > methods > > to allow or deny lists. > > - The legacy mode will be introduced as deprecated for 10.x and will > be > > removed in 11.x. ACL rules will behave as modern from 11.x on. > > > > Note that with this change, modern mode is a feature superset on top of > > legacy: that is, it allows a user to specify both ip_allow like rules via > > `allow` and `deny` actions while additively setting allowed or denied > > methods via `add_allow` and `add_deny` actions. > > > > To give some examples to illustrate these behaviors (these examples are > > given in our docs in the draft PR mentioned below): > > > > map http://www.example.com/ http://internal.example.com/ @action=deny > > > @method=POST > > > > > > In legacy mode, this behaves just like it does in 9.x: it adds denial of > > POST to requests with the http://www.example.com target, while all other > > requests will be handled by other ip_allow or other ACL filter rules. > This > > is not changed by my suggestion here (other than calling it "legacy" > mode). > > > > In modern mode, this behaves like an ip_allow.yaml rule: POST requests to > > http://www.example.com will be denied, while all other requests will be > > allowed. (Naturally, this will be an unlikely rule for anyone to > configure, > > either as a modern ACL filter or ip_allow rule, but for this is helpful > for > > the purpose of comparison between the two behaviors.) > > > > The same rule using an `add_deny` action would be the following: > > > > map http://www.example.com/ http://internal.example.com/ > @action=add_deny > > > @method=POST > > > > > > In both legacy and modern modes, this rule behaves the same: it acts > like a > > `deny` rule would have for 9.x. That is, it adds denial of POST requests > on > > top of any other ip_allow or ACL filter rule that my otherwise apply to > > requests to http://www.example.com. > > > > In case it is helpful, here is a draft PR for the implementation of this > > suggestion: > > https://github.com/apache/trafficserver/pull/11555 > > > > Note that the production change is almost trivial: most of the change > > involves renaming the configuration and its associated member variables > or > > updating some tests. > > > > Please let me know if you have any thoughts or concerns. > > > > Thank you, > > Brian Neradt > > -- > > "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 > -- "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
