You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/05/02 20:24:01 UTC
[GitHub] [trafficserver] elsloo opened a new pull request, #8816: Add support for caching complete responses to the cache range requests plugin
elsloo opened a new pull request, #8816:
URL: https://github.com/apache/trafficserver/pull/8816
* Adds support for caching full object responses.
* Refactored logic to be more efficient.
* Added new plugin parameter to relevant docs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] elsloo commented on pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
elsloo commented on PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816#issuecomment-1115349796
[approve ci fedora]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] elsloo merged pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
elsloo merged PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] elsloo commented on pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
elsloo commented on PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816#issuecomment-1139678107
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
zwoop commented on PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816#issuecomment-1150439350
Cherry-picked to v9.2.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] elsloo commented on pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
elsloo commented on PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816#issuecomment-1119007096
@traeak I was able to reproduce this issue and in the process discovered that the content revalidation case is not being handled correctly, in addition to what you observed. This is due to changes introduced in PR #8488 and instead of pushing the fix into this PR, I wanted to separate the fix from this feature and instead opened #8828.
Please retest, possibly with both PRs. If necessary I can push the fix into this PR to ease the testing burden.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] elsloo commented on pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
elsloo commented on PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816#issuecomment-1116139212
[approve ci rocky]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] elsloo commented on pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
elsloo commented on PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816#issuecomment-1128131040
> If possible adding an autest would be nice.
I will add an AuTest and update the docs to address your initial feedback about the risk before merging this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] elsloo commented on pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
elsloo commented on PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816#issuecomment-1116163603
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] elsloo commented on pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
elsloo commented on PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816#issuecomment-1118616880
> A 200 is being cached with a cachekey that's been modified by the CRR plugin. Any origin request that returns 200 responses for a range request could be exploited to overload the cache with multiple full asset. **Docs should have warning signs all over this if the CRR plugin's endpoint is exposed.**
When run as a standalone plugin, and when allowing the cache range requests plugin to modify the cache key, this statement is true. However, when this modification is used with the cache key plugin, the slice plugin, and with the cache key manipulation within cache range requests plugin disabled, the statement is no longer true. In my testing, the cache key becomes anchored to the slice reference block, and consequently, that is the only cache key used for the object.
I understand your concern, but in my opinion, having this plugin directly exposed to the open internet without the slice plugin or a normalization tier is already a huge concern because of the damage a user can do by polluting the cache with random ranges on any object. That is basically the same "exploit," just with fewer bytes per cache key. Nevertheless, I believe the possibility still exists with the default cache range requests plugin, regardless of whether this new plugin parameter is used. The only actual difference is the rate at which you can inflict damage.
I can add a comment to recommend only using this when using this style of configuration if that will help reduce the risk of accidental use.
> But there's also a bug uncovered in testing:
>
> Here's the request I was using. Request strips the range request header at the synthetic origin:
>
> ```
> curl -x localhost:18080 "http://crr/~p.txt/~s.5M/~ui.200/a" -r 0-100 -H "X-Dtp: ~rmhdrs.Range" -H "x-debug: x-cache-key" -Lv
> ```
>
> When running a test on this first time around I got a 200 response with a modified cache key.
>
> ```
> < HTTP/1.1 200 OK
> < Accept-Ranges: bytes
> < Cache-Control: max-age=123
> < Content-Length: 5242880
> < Content-Type: text/plain
> < Last-Modified: Thu, 05 May 2022 12:36:40 GMT
> < Date: Thu, 05 May 2022 12:38:43 GMT
> < Age: 0
> < Proxy-Connection: keep-alive
> < Via: http/1.1 ipcdn-cache-58.cdnlab.comcast.net (ApacheTrafficServer/10.0.0 [uScMsSfWpSeN:t cCMpSs ])
> < Server: ATS/10.0.0
> < X-Cache-Key: http://localhost/~p.txt/~s.5M/~ui.200/a-bytes=0-100
> ```
>
> Second request I got back the whole asset but with a 206 response:
>
> ```
> < HTTP/1.1 206 OK
> < Accept-Ranges: bytes
> < Cache-Control: max-age=123
> < Content-Length: 5242880
> < Content-Type: text/plain
> < Last-Modified: Thu, 05 May 2022 12:36:40 GMT
> < Date: Thu, 05 May 2022 12:38:43 GMT
> < Age: 59
> < Proxy-Connection: keep-alive
> < Via: http/1.1 ipcdn-cache-58.cdnlab.comcast.net (ApacheTrafficServer/10.0.0 [uScHs f p eN:t cCHp s ])
> < Server: ATS/10.0.0
> < X-Cache-Key: http://localhost/~p.txt/~s.5M/~ui.200/a-bytes=0-100
> ```
This should be relatively easy to fix, but I have not observed this issue due to not running this as a standalone plugin. In my case, I was getting 200s consistently for any object that was smaller than the slice block size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] elsloo commented on pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
elsloo commented on PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816#issuecomment-1137546378
@traeak I just pushed two commits that add an AuTest and updates the docs with a warning and detailed description of the intended use case for this feature. Please take a look at both additions and provide any necessary feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak commented on pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
traeak commented on PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816#issuecomment-1118500691
When running a test on this first time around I got a 200 response with a modified cache key.
```
< HTTP/1.1 200 OK
< Accept-Ranges: bytes
< Cache-Control: max-age=123
< Content-Length: 5242880
< Content-Type: text/plain
< Last-Modified: Thu, 05 May 2022 12:36:40 GMT
< Date: Thu, 05 May 2022 12:38:43 GMT
< Age: 0
< Proxy-Connection: keep-alive
< Via: http/1.1 ipcdn-cache-58.cdnlab.comcast.net (ApacheTrafficServer/10.0.0 [uScMsSfWpSeN:t cCMpSs ])
< Server: ATS/10.0.0
< X-Cache-Key: http://localhost/~p.txt/~s.5M/~ui.200/a-bytes=0-100
```
Second request I got back the whole asset but with a 206 response:
```
< HTTP/1.1 206 OK
< Accept-Ranges: bytes
< Cache-Control: max-age=123
< Content-Length: 5242880
< Content-Type: text/plain
< Last-Modified: Thu, 05 May 2022 12:36:40 GMT
< Date: Thu, 05 May 2022 12:38:43 GMT
< Age: 59
< Proxy-Connection: keep-alive
< Via: http/1.1 ipcdn-cache-58.cdnlab.comcast.net (ApacheTrafficServer/10.0.0 [uScHs f p eN:t cCHp s ])
< Server: ATS/10.0.0
< X-Cache-Key: http://localhost/~p.txt/~s.5M/~ui.200/a-bytes=0-100
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] elsloo commented on pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
elsloo commented on PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816#issuecomment-1116226353
[approve ci clang-analyzer]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak commented on pull request #8816: Add support for caching complete responses to the cache range requests plugin
Posted by GitBox <gi...@apache.org>.
traeak commented on PR #8816:
URL: https://github.com/apache/trafficserver/pull/8816#issuecomment-1122465041
If possible adding an autest would be nice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org