cache_range_requests plugin: detect and handle TSCacheUrlSet failures…

doc/admin-guide/plugins/cache_range_requests.en.rst

@@ -137,6 +137,29 @@ In order for this to properly work in a CDN each cache in the
 chain *SHOULD* also contain a remap rule with the
 :program:`cache_range_requests` plugin with this option set.
 
+Don't modify the Cache Key
+--------------------------
+
+.. option:: --no-modify-cachekey
+.. option:: -n
+
+With each transaction TSCacheUrlSet may only be called once.  When
+using the `cache_range_requests` plugin in conjunction with the
+`cachekey` plugin the option `--include-headers=Range` should be
+added as a `cachekey` parameter with this option.  Configuring this
+incorrectly *WILL* result in cache poisoning.
+
+.. code::
+
+       map http://ats/ http://parent/ \
+           @plugin=cachekey.so @pparam=--include-headers=Range \
+           @plugin=cache_range_requests.so @pparam=--no-modify-cachekey
+
+*Without this `cache_range_requests` plugin option*
+
+*IF* the TSCacheUrlSet call in cache_range_requests fails, an error is
+generated in the logs and the cache_range_requests plugin will disable
+transaction caching in order to avoid cache poisoning.
 
 Configuration examples
 ======================
@@ -146,23 +169,23 @@ Global plugin
 
 .. code::
 
-    cache_range_requests.so --ps-cachekey --consider-ims
+    cache_range_requests.so --ps-cachekey --consider-ims --no-modify-cachekey
 
 or
 
 .. code::
 
-    cache_range_requests.so -p -c
+    cache_range_requests.so -p -c -n
 
 Remap plugin
 ------------
 
 .. code::
 
-    map http://ats http://parent @plugin=cache_range_requests.so @pparam=--ps-cachekey @pparam=--consider-ims
+    map http://ats http://parent @plugin=cache_range_requests.so @pparam=--ps-cachekey @pparam=--consider-ims @pparam=--no-modify-cachekey
 
 or
 
 .. code::
 
-    map http://ats http://parent @plugin=cache_range_requests.so @pparam=-p @pparam=-c
+    map http://ats http://parent @plugin=cache_range_requests.so @pparam=-p @pparam=-c @pparam=-n

plugins/cache_range_requests/cache_range_requests.cc

@@ -49,6 +49,7 @@ typedef enum parent_select_mode {
 struct pluginconfig {
   parent_select_mode_t ps_mode{PS_DEFAULT};
   bool consider_ims_header{false};
+  bool modify_cache_key{true};
 };
 
 struct txndata {
@@ -96,6 +97,7 @@ create_pluginconfig(int argc, char *const argv[])
   static const struct option longopts[] = {
     {const_cast<char *>("ps-cachekey"), no_argument, nullptr, 'p'},
     {const_cast<char *>("consider-ims"), no_argument, nullptr, 'c'},
+    {const_cast<char *>("no-modify-cachekey"), no_argument, nullptr, 'n'},
     {nullptr, 0, nullptr, 0},
   };
 
@@ -118,6 +120,10 @@ create_pluginconfig(int argc, char *const argv[])
       DEBUG_LOG("Plugin considers the '%.*s' header", (int)X_IMS_HEADER.size(), X_IMS_HEADER.data());
       pc->consider_ims_header = true;
     } break;
+    case 'n': {
+      DEBUG_LOG("Plugin doesn't modify cache key");
+      pc->modify_cache_key = false;
+    } break;
     default: {
     } break;
     }
@@ -203,12 +209,16 @@ range_header_check(TSHttpTxn txnp, pluginconfig *const pc)
             TSfree(req_url);
           }
 
-          // set the cache key.
-          if (TS_SUCCESS != TSCacheUrlSet(txnp, cache_key_url, cache_key_url_length)) {
-            DEBUG_LOG("failed to change the cache url to %s.", cache_key_url);
-          }
-
           if (nullptr != pc) {
+            // set the cache key if configured to.
+            if (pc->modify_cache_key && TS_SUCCESS != TSCacheUrlSet(txnp, cache_key_url, cache_key_url_length)) {
+              ERROR_LOG("failed to change the cache url to %s.", cache_key_url);
+              ERROR_LOG("Disabling cache for this transaction to avoid cache poisoning.");
+              TSHttpTxnServerRespNoStoreSet(txnp, 1);
+              TSHttpTxnRespCacheableSet(txnp, 0);
+              TSHttpTxnReqCacheableSet(txnp, 0);
+            }
+
             // Optionally set the parent_selection_url to the cache_key url or path
             if (PS_DEFAULT != pc->ps_mode) {
               TSMLoc ps_loc = nullptr;

tests/gold_tests/pluginTest/cache_range_requests/cache_range_requests_cachekey.test.py

@@ -0,0 +1,198 @@
+'''
+'''
+#  Licensed to the Apache Software Foundation (ASF) under one
+#  or more contributor license agreements.  See the NOTICE file
+#  distributed with this work for additional information
+#  regarding copyright ownership.  The ASF licenses this file
+#  to you under the Apache License, Version 2.0 (the
+#  "License"); you may not use this file except in compliance
+#  with the License.  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+import os
+import time
+
+Test.Summary = '''
+cache_range_requests with cachekey
+'''
+
+## Test description:
+# Preload the cache with the entire asset to be range requested.
+# Reload remap rule with cache_range_requests plugin
+# Request content through the cache_range_requests plugin
+
+Test.SkipUnless(
+    Condition.PluginExists('cache_range_requests.so'),
+    Condition.PluginExists('cachekey.so'),
+    Condition.PluginExists('xdebug.so'),
+)
+Test.ContinueOnFail = False
+Test.testName = "cache_range_requests_cachekey"
+
+# Define and configure ATS, enable traffic_ctl config reload
+ts = Test.MakeATSProcess("ts", command="traffic_server")
+
+# Define and configure origin server
+server = Test.MakeOriginServer("server", lookup_key="{%uuid}")
+
+# default root
+req_chk = {"headers":
+  "GET / HTTP/1.1\r\n" +
+  "Host: www.example.com\r\n" +
+  "uuid: none\r\n" +
+  "\r\n",
+  "timestamp": "1469733493.993",
+  "body": ""
+}
+
+res_chk = {"headers":
+  "HTTP/1.1 200 OK\r\n" +
+  "Connection: close\r\n" +
+  "\r\n",
+  "timestamp": "1469733493.993",
+  "body": ""
+}
+
+server.addResponse("sessionlog.json", req_chk, res_chk)
+
+body = "lets go surfin now"
+bodylen = len(body)
+
+# this request should work
+req_full = {"headers":
+  "GET /path HTTP/1.1\r\n" +
+  "Host: www.example.com\r\n" +
+  "Accept: */*\r\n" +
+	"uuid: full\r\n" +
+  "\r\n",
+  "timestamp": "1469733493.993",
+  "body": ""
+}
+
+res_full = {"headers":
+  "HTTP/1.1 206 Partial Content\r\n" +
+  "Accept-Ranges: bytes\r\n" +
+  'Etag: "foo"\r\n' +
+  "Cache-Control: public, max-age=500\r\n" +
+  "Connection: close\r\n" +
+  "\r\n",
+  "timestamp": "1469733493.993",
+  "body": body
+}
+
+server.addResponse("sessionlog.json", req_full, res_full)
+
+# this request should work
+req_good = {"headers":
+  "GET /path HTTP/1.1\r\n" +
+  "Host: www.example.com\r\n" +
+  "Accept: */*\r\n" +
+  "Range: bytes=0-\r\n" +
+	"uuid: range_full\r\n" +
+  "\r\n",
+  "timestamp": "1469733493.993",
+  "body": ""
+}
+
+res_good = {"headers":
+  "HTTP/1.1 206 Partial Content\r\n" +
+  "Accept-Ranges: bytes\r\n" +
+  'Etag: "foo"\r\n' +
+  "Cache-Control: public, max-age=500\r\n" +
+  "Content-Range: bytes 0-{0}/{0}\r\n".format(bodylen) +
+  "Connection: close\r\n" +
+  "\r\n",
+  "timestamp": "1469733493.993",
+  "body": body
+}
+
+server.addResponse("sessionlog.json", req_good, res_good)
+
+# this request should fail with a cache_range_requests asset
+req_fail = {"headers":
+  "GET /path HTTP/1.1\r\n" +
+  "Host: www.fail.com\r\n" +
+  "Accept: */*\r\n" +
+  "Range: bytes=0-\r\n" +
+	"uuid: range_fail\r\n" +
+  "\r\n",
+  "timestamp": "1469733493.993",
+  "body": ""
+}
+
+res_fail = {"headers":
+  "HTTP/1.1 206 Partial Content\r\n" +
+  "Accept-Ranges: bytes\r\n" +
+  'Etag: "foo"\r\n' +
+  "Cache-Control: public, max-age=500\r\n" +
+  "Content-Range: bytes 0-{0}/{0}\r\n".format(bodylen) +
+  "Connection: close\r\n" +
+  "\r\n",
+  "timestamp": "1469733493.993",
+  "body": body
+}
+
+server.addResponse("sessionlog.json", req_fail, res_fail)
+
+# cache range requests plugin remap, working config
+ts.Disk.remap_config.AddLine(
+  'map http://www.example.com http://127.0.0.1:{}'.format(server.Variables.Port) +
+    ' @plugin=cachekey.so @pparam=--include-headers=Range' +
+    ' @plugin=cache_range_requests.so @pparam=--no-modify-cachekey',
+)
+
+# improperly configured cache_range_requests with cachekey
+ts.Disk.remap_config.AddLine(
+  'map http://www.fail.com http://127.0.0.1:{}'.format(server.Variables.Port) +
+    ' @plugin=cachekey.so @pparam=--static-prefix=foo'
+    ' @plugin=cache_range_requests.so',
+)
+
+# cache debug
+ts.Disk.plugin_config.AddLine('xdebug.so')
+
+# minimal configuration
+ts.Disk.records_config.update({
+  'proxy.config.diags.debug.enabled': 1,
+  'proxy.config.diags.debug.tags': 'cache_range_requests',
+  'proxy.config.http.cache.http': 1,
+  'proxy.config.http.wait_for_cache': 1,
+})
+
+curl_and_args = 'curl -s -D /dev/stdout -o /dev/stderr -x localhost:{} -H "x-debug: x-cache"'.format(ts.Variables.port)
+
+# 0 Test - Fetch full asset into cache (ensure cold)
+tr = Test.AddTestRun("full asset fetch")
+ps = tr.Processes.Default
+ps.StartBefore(server, ready=When.PortOpen(server.Variables.Port))
+ps.StartBefore(Test.Processes.ts, ready=When.PortOpen(ts.Variables.port))
+ps.Command = curl_and_args + ' http://www.example.com/path -H "uuid: full"'
+ps.ReturnCode = 0
+ps.Streams.stdout.Content = Testers.ContainsExpression("X-Cache: miss", "expected cache miss for load")
+tr.StillRunningAfter = ts
+
+# 1 Test - Fetch whole asset into cache via range request (ensure cold)
+tr = Test.AddTestRun("0- asset fetch")
+ps = tr.Processes.Default
+ps.Command = curl_and_args + ' http://www.example.com/path -r 0- -H "uuid: range_full"'
+ps.ReturnCode = 0
+ps.Streams.stdout.Content = Testers.ContainsExpression("X-Cache: miss", "expected cache miss for load")
+tr.StillRunningAfter = ts
+
+# 2 Test - Ensure assert happens instead of possible cache poisoning.
+tr = Test.AddTestRun("Attempt poisoning")
+ps = tr.Processes.Default
+ps.Command = curl_and_args + ' http://www.fail.com/path -r 0- -H "uuid: range_fail"'
+ps.ReturnCode = 0
+tr.StillRunningAfter = ts
+
+ts.Disk.diags_log.Content = Testers.ContainsExpression("ERROR", "error condition hit")
+ts.Disk.diags_log.Content = Testers.ContainsExpression("failed to change the cache url", "ensure failure for misconfiguration")
+ts.Disk.diags_log.Content = Testers.ContainsExpression("Disabling cache for this transaction to avoid cache poisoning", "ensure transaction caching disabled")

tests/gold_tests/pluginTest/cache_range_requests/cache_range_requests_ims.test.py

@@ -33,7 +33,7 @@
     Condition.PluginExists('xdebug.so'),
 )
 Test.ContinueOnFail = False
-Test.testName = "cache_range_requests"
+Test.testName = "cache_range_requests_ims"
 
 # Define and configure ATS
 ts = Test.MakeATSProcess("ts", command="traffic_server")