Hi Masaori, OK, great. This is helpful. Considering your feedback, I'm going to split up my PR into two units:
1. Adding the `add_deny` and `add_allow` actions. That seems rather non-controversial, and it helps a lot with the "Match on IP Only" policy because it adds back functionality that is important. 2. The rename to legacy/modern and deprecation idea I'll keep separate. And we can continue that discussion probably in a separate thread. Thank you again for the quick replies and hearing me out! Brian On Wed, Jul 17, 2024 at 11:54 PM Masaori Koshiba <masa...@apache.org> wrote: > > # 1. "Match on IP and Method Policy" > > > > [snip] > > > > 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? > > It's already in the "Match on IP and Method Policy" section of the docs. > If you think this needs more details, please add it. The release > announcement > should have a comment about this change, anyway. > > > https://github.com/apache/trafficserver/blob/21581ecbf1cc441cfb6fab4f935b326107963ced/doc/admin-guide/files/remap.config.en.rst#L581-L583 > > > # 2. ATS 10.0.0 > > > > [snip] > > > > 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. > > These additional actions seems harmless for the "Match on IP and Method > Policy", > so I don't object strongly. I'll leave this to you and the release manager. > > > # 3. `add_allow` and `add_deny` actions > > > > [snip] > > > > 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. > > I'd say the `deny` example was a real security issue and we had to stop ATS > 10.0.x testing on production. The problem is combination of ACL filters and > ip_allow.yaml. > > > 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. > > Thank you so much for sharing the background of your idea. Now, your > motivation is getting clear to me. In my opinion, the ACL feature had never > been implemented and documented well unfortunately. I understand ATS users > are confused by what is written in the docs and I see it's reasonable they > expect the ACL filter in the remap.config act like ip_allow.yaml. However, > it's a problem of docs not problem of matching policies. > > This is a ramdom idea, but if the consistency of behaviors between ACL > filters > in remap.config and ip_allow.yaml is the issue, another option is changing > the > behavior of ip_allow.yaml. > > > 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. > > This is the point I don't agree with. For your cases, I see the "Match on > IP > only Policy" is useful and safety. However, we can't say it's true for > everybody. I'd hear more opinions from the communiy about this. > > The use cases are different from users, this is why I made the policy > confiugurable. For my use case, I can NOT use "Match on IP only Policy", > even if it has additional actions that you are proposing here. Sorry for > repeating myself, but the behavior still has a big trap for my users. > > > Overall, what I object is: > > A). renaming policies to "legacy" and "modern" > > What is "legacy" and what is "modern" is very different by use cases. Thus, > I prefer calling them as is, becasue it just represents the behavior of the > policies. > > B). deprecate "legacy" for 10.x and remove it in 11.x. > > I don't know what will happen in the future, but we'll continue using the > "Match on IP and Method Policy" due to the the security concerns. > > Thanks again for all of your work and comments! I'd say we can agree with > we > can't agree with which matching policy is better. > > — Masaori > > On Thu, Jul 18, 2024 at 1:13 AM Brian Neradt <brian.ner...@gmail.com> > wrote: > > > > 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 > -- "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
