You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by meeramn <gi...@git.apache.org> on 2015/08/05 23:41:44 UTC
[GitHub] trafficserver pull request: Add support for webdav methods in ip_a...
GitHub user meeramn opened a pull request:
https://github.com/apache/trafficserver/pull/270
Add support for webdav methods in ip_allow
Add support for webdav methods in ip_allow - COPY, MOVE, LOCK, UNLOCK, PROPFIND, PROPPATCH, MKCOL, REPORT, CHECKOUT
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/meeramn/trafficserver TS-1409
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/270.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 #270
----
commit 79163f109872258936602ab5a3c8231f83cb480f
Author: Meera Mosale Nataraja <me...@gmail.com>
Date: 2015-08-05T21:34:25Z
Add support for webdav methods in ip_allow - COPY, MOVE, LOCK,
UNLOCK, PROPFIND, PROPPATCH, MKCOL, REPORT, CHECKOUT
----
---
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: Add support for webdav methods in ip_a...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-134748136
Looking at isNonstandardMethodAllowed I am OK with just using that. However it should be modified without using std::string. Also, there should be documentation added, bot the docs and ip_allow.config file.
---
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: Add support for webdav methods in ip_a...
Posted by ushachar <gi...@git.apache.org>.
Github user ushachar commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-130198618
I'm not advocating rejecting all non-standard methods out of hand -- I'm just saying they don't belong in our recognized method list.
As a general approach, I'd do this sort of thing in plugin space - but if needed it can already be done in IPAllow via isNonstandardMethodAllowed (which can be configured using raw strings) - somewhat slower, but much more generic.
---
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: Add support for webdav methods in ip_a...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-130127185
I should say, if there was a reasonable way to add functionality to ip_allow.config via plugins, that would make a lot of sense. But, I don't think there is.
---
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: Add support for webdav methods in ip_a...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-134748516
Ok, I think the consensus is that we don't change anything. Sorry Meera, but thanks for all the work. And I whole heartedly agree that we should eliminate the std:: ugliness from this 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: Add support for webdav methods in ip_a...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-130113092
I don't really see why ```ip_filter``` should not be able to deal with webdav methods. It seems reasonable to me that you should be able to filter any layered methods.
---
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: Add support for webdav methods in ip_a...
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-130071743
@meeramn : I agree with the comments that this change seems like something that can (and should?) be done in a plugin rather than in the core. It should be fairly simple to write a plugin that reads a config file with the methods that need to be filtered and be used for any method (including webdav). We have a similar internal plugin that overrides ip_allow (we basically allow everything from ip_allow and control them from the plugin) for restricting methods based on ip type (internal, external etc).
---
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: Add support for webdav methods in ip_a...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/270
---
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: Add support for webdav methods in ip_a...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-130126864
That seems draconian to rule out these methods only because they are not in the main HTTP RFCs. Since ATS is a general forward (and transparent) proxy, it seems reasonable to me to support not just HTTP, but any HTTP extension. We do that for other things, like ICP.
---
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: Add support for webdav methods in ip_a...
Posted by meeramn <gi...@git.apache.org>.
Github user meeramn commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-130058487
ushachar: I made changes to support webdav methods in ip_allow.config. I haven't implemented these methods - it just look for list of list of methods to allow or deny. Hence I'm not sure about supporting "If" header.
src_ip=::1 action=ip_allow method=<Webdav methods>
Can you please give me more information about doing this in plugin space?
I can rename it to WEBDAV as suggested.
---
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: Add support for webdav methods in ip_a...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-130105002
I'm in agreement with @ushachar and @sudheerv on this one, I do not believe webdav methods should land in the core.
---
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: Add support for webdav methods in ip_a...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-134705705
I am for putting these methods into ipallow.config. You are able to expand on the set of common methods as defined in section 9 of RFC2616. Also, we already support methods that are not listed in RFC2616 (DELETE, PURGE, and PUSH).
---
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: Add support for webdav methods in ip_a...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-130491241
@ushachar Ah, so the argument is to not add special handling for this, but still allow them to be used in ip_allow.config? If so, that seems fine.
---
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: Add support for webdav methods in ip_a...
Posted by ushachar <gi...@git.apache.org>.
Github user ushachar commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-128261744
I'm not too sure about this -- It's one thing to allow/block unknown methods that parse properly (TS-1232/TS-1407), but adding non-HTTP methods to the HTTP method list just for the ip-allow functionlity seems like another thing that can be best accomplished in plugin space....
Some technical comments:
1) Maybe rename HTTP_WKSIDX_... -> WEBDAV_WKSIDX_...
2) Some WebDAV methods should be treated like POST in that they are required to have payload (I don't think there's any method that's explicitly forbidden from having payload).
3) Should we support the "If" header? (http://www.webdav.org/specs/rfc4918.html#rfc.section.10.4.5)
---
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: Add support for webdav methods in ip_a...
Posted by ushachar <gi...@git.apache.org>.
Github user ushachar commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-131523307
@zwoop - Yes, it should already be possible to do this using the existing 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: Add support for webdav methods in ip_a...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/270#issuecomment-130400491
I'm going to side with Uri on this one as well. Adding those to the core HTTP method list seems wrong to me. As I understand, the real goal here is just for IPAllow to handle those methods. I like Uri's suggestion that we add a WEBDAV set of header tokens, e.g. `HDRTOKEN_TYPE_WEBDAV` and `HTTP_WEBDAV_XXX`. I don't see it as a big deal to have the strings in the header token heap per se.
---
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.
---