Add support for caching complete responses to the cache range requests plugin #8816

elsloo · 2022-05-02T20:24:00Z

Adds support for caching full object responses.
Refactored logic to be more efficient.
Added new plugin parameter to relevant docs.

…s plugin * Adds support for caching full object responses. * Refactored logic to be more efficient. * Added new plugin parameter to relevant docs.

elsloo · 2022-05-02T20:45:52Z

[approve ci fedora]

elsloo · 2022-05-03T14:04:18Z

[approve ci rocky]

elsloo · 2022-05-03T14:25:01Z

[approve ci autest]

elsloo · 2022-05-03T15:22:12Z

[approve ci clang-analyzer]

* Status code must be flipped to 200 prior to performing cacheability check * Reverts to prior logic for the partial content case

traeak · 2022-05-05T12:42:20Z

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.

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

elsloo · 2022-05-05T14:22:41Z

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.

elsloo · 2022-05-05T20:17:02Z

@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.

traeak

Changes look good and test fine.

traeak · 2022-05-10T14:21:59Z

If possible adding an autest would be nice.

elsloo · 2022-05-16T21:01:30Z

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.

… with the full plugin stack.

elsloo · 2022-05-25T16:51:10Z

@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.

…lice and cachekey.

elsloo · 2022-05-27T14:34:17Z

[approve ci autest]

…e CI sandbox.

traeak

Looks and tests fine.

…s plugin (#8816) * Add support for caching complete responses to the cache range requests plugin * Adds support for caching full object responses. * Refactored logic to be more efficient. * Added new plugin parameter to relevant docs. * Revert 206 flip behavior * Status code must be flipped to 200 prior to performing cacheability check * Reverts to prior logic for the partial content case * Update docs to reflect actual behavior. * Adds an AuTest to validate the behavior of caching complete responses with the full plugin stack. * Update docs to provide more detail on the expected use case. * Removed a trailing space from the AuTest. * Ran autopep8 * Added a few test cases to cover when the CRR plugin is used without slice and cachekey. * Fix test case numbering. * Fix test name/comment. * Update AuTest check on cachekey to use non-greedy regex to work in the CI sandbox. (cherry picked from commit 39744f7)

zwoop · 2022-06-08T21:37:07Z

Cherry-picked to v9.2.x

* asf/9.2.x: Updated ChangeLog Add proxy.process.hostdb.total_serve_stale (apache#8873) Allow for long Http* error.log lines (apache#8855) mkdfa.c is not being used and doesn't compile with gcc 12.1.1 (apache#8838) Add compatibility define when building with OpenSSL3 (apache#8837) Make post-early-return Au test more robust. (apache#8832) Add support for caching complete responses to the cache range requests plugin (apache#8816) Fixes issues with the CRR plugin introduced in apache#8488 (apache#8828) slice and cache_range_requests: allow header override (apache#8666) (apache#8898) Removed references to the throttle option from the slice plugin. (apache#8373) (apache#8897) cache_range_requests plugin: don't require 206 Partial Content reason string (apache#8488) Improve option processing in cache promote (apache#8501) Change parent_select Init func to constructor (apache#8853) Fix "is is" typos. (apache#8866) Eliminate duplicate words. (apache#8870) money_trace: allow custom header, change span-id gen, opt to create if none (apache#8655) Update HostDBContinuation timeout handling to clear pending queue. (apache#8480) Upgrade to Proxy Verifier 2.4.0. (apache#8884) Change ats_scoped_obj to std::unique_ptr . (apache#8882)

Add support for caching complete responses to the cache range request…

d85de02

…s plugin * Adds support for caching full object responses. * Refactored logic to be more efficient. * Added new plugin parameter to relevant docs.

elsloo added Plugins Enhancement labels May 2, 2022

elsloo requested a review from bryancall as a code owner May 2, 2022 20:24

bryancall assigned elsloo May 2, 2022

bryancall requested review from traeak and ywkaras and removed request for ywkaras May 2, 2022 23:03

randall added this to the 10.0.0 milestone May 3, 2022

elsloo added 2 commits May 4, 2022 17:35

Revert 206 flip behavior

1b9e157

* Status code must be flipped to 200 prior to performing cacheability check * Reverts to prior logic for the partial content case

Update docs to reflect actual behavior.

30ef52b

traeak previously approved these changes May 10, 2022

View reviewed changes

elsloo added 2 commits May 25, 2022 10:49

Adds an AuTest to validate the behavior of caching complete responses…

a23ace7

… with the full plugin stack.

Update docs to provide more detail on the expected use case.

84a3fd9

elsloo dismissed traeak’s stale review via 84a3fd9 May 25, 2022 16:50

elsloo added 2 commits May 25, 2022 10:55

Removed a trailing space from the AuTest.

a7ccf64

Ran autopep8

c5015fb

elsloo requested a review from traeak May 25, 2022 18:05

elsloo added 2 commits May 25, 2022 15:50

Added a few test cases to cover when the CRR plugin is used without s…

6e907ac

…lice and cachekey.

Fix test case numbering.

13091ab

Fix test name/comment.

28c029d

Update AuTest check on cachekey to use non-greedy regex to work in th…

204fa7a

…e CI sandbox.

traeak approved these changes May 27, 2022

View reviewed changes

elsloo merged commit 39744f7 into apache:master May 27, 2022

zwoop modified the milestones: 10.0.0, 9.2.0 Jun 8, 2022

Add support for caching complete responses to the cache range requests plugin #8816

Add support for caching complete responses to the cache range requests plugin #8816

Uh oh!

Conversation

elsloo commented May 2, 2022

Uh oh!

elsloo commented May 2, 2022

Uh oh!

elsloo commented May 3, 2022

Uh oh!

elsloo commented May 3, 2022

Uh oh!

elsloo commented May 3, 2022

Uh oh!

traeak commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elsloo commented May 5, 2022

Uh oh!

elsloo commented May 5, 2022

Uh oh!

traeak left a comment

Choose a reason for hiding this comment

Uh oh!

traeak commented May 10, 2022

Uh oh!

elsloo commented May 16, 2022

Uh oh!

elsloo commented May 25, 2022

Uh oh!

elsloo commented May 27, 2022

Uh oh!

traeak left a comment

Choose a reason for hiding this comment

Uh oh!

zwoop commented Jun 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

traeak commented May 5, 2022 •

edited

Loading