Enforce tenant isolation for Ingress routes, snippets and ATS config

charts/ats-ingress/templates/clusterrole.yaml

@@ -28,15 +28,31 @@ metadata:
     app.kubernetes.io/instance: {{ .Release.Name }}
     app.kubernetes.io/version: {{ .Chart.AppVersion }}
 rules:
+# The controller only watches Endpoints cluster-wide (watcher/watcher.go).
+# ConfigMaps are watched in the release namespace only and are granted by a
+# namespaced Role (role.yaml); secrets, services, namespaces and events are
+# never read by the controller, so they are not granted here. TLS material
+# reaches the pod via a Secret volume mount, which needs no RBAC.
 - apiGroups:
   - ""
   resources:
-  - configmaps
   - endpoints
-  - services
-  - namespaces
-  - events
-  - secrets
+  verbs:
+  - get
+  - list
+  - watch
+- apiGroups:
+  - "k8s.trafficserver.apache.com"
+  resources:
+  - atscachingpolicies
+  verbs:
+  - get
+  - list
+  - watch
+- apiGroups:
+  - "trafficserver.apache.org"
+  resources:
+  - atssnipolicies
   verbs:
   - get
   - list

charts/ats-ingress/templates/role.yaml

@@ -0,0 +1,45 @@
+{{/*
+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.
+*/}}
+
+{{- if .Values.rbac.create -}}
+{{/*
+The controller watches ConfigMaps only in its own namespace
+(watcher/watcher.go), so the grant is a namespaced Role rather than part of
+the ClusterRole.
+*/}}
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: {{ include "ats-ingress.fullname" . }}
+  namespace: {{ .Release.Namespace }}
+  labels:
+    app.kubernetes.io/name: {{ include "ats-ingress.name" . }}
+    helm.sh/chart: {{ include "ats-ingress.chart" . }}
+    app.kubernetes.io/managed-by: {{ .Release.Service }}
+    app.kubernetes.io/instance: {{ .Release.Name }}
+    app.kubernetes.io/version: {{ .Chart.AppVersion }}
+rules:
+- apiGroups:
+  - ""
+  resources:
+  - configmaps
+  verbs:
+  - get
+  - list
+  - watch
+{{- end -}}

charts/ats-ingress/templates/rolebinding.yaml

@@ -0,0 +1,39 @@
+{{/*
+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.
+*/}}
+
+{{- if .Values.rbac.create -}}
+apiVersion: rbac.authorization.k8s.io/v1
+kind: RoleBinding
+metadata:
+  name: {{ include "ats-ingress.fullname" . }}
+  namespace: {{ .Release.Namespace }}
+  labels:
+    app.kubernetes.io/name: {{ include "ats-ingress.name" . }}
+    helm.sh/chart: {{ include "ats-ingress.chart" . }}
+    app.kubernetes.io/managed-by: {{ .Release.Service }}
+    app.kubernetes.io/instance: {{ .Release.Name }}
+    app.kubernetes.io/version: {{ .Chart.AppVersion }}
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: Role
+  name: {{ include "ats-ingress.fullname" . }}
+subjects:
+- kind: ServiceAccount
+  name: {{ include "ats-ingress.serviceAccountName" . }}
+  namespace: {{ .Release.Namespace }}
+{{- end -}}

docs/TUTORIAL.md

@@ -38,6 +38,15 @@ Check out the project's github action "Build and Integrate". It has an example o
 
 The above also shows example of configuring Apache Traffic Server [_reloadable_ configurations](https://docs.trafficserver.apache.org/en/9.2.x/admin-guide/files/records.config.en.html#reloadable) using [kubernetes configmap](https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/) resource.
 
+Because the `ats-configmap: "true"` annotation can be set by anyone with permission to write ConfigMaps in a watched namespace, the controller only applies keys from a fail-closed allowlist of operationally safe tuning records (log rolling, restart thresholds, keep-alive/transaction/connect timeouts, connection throttle, RAM-cache size). Any other key is rejected and logged. To permit additional records, set the environment variable `CONFIGMAP_RECORD_ALLOWLIST` in the ATS deployment to a comma-separated list of entries — an entry ending in `*` matches keys by prefix, any other entry must match a key exactly, and every entry must start with `proxy.config.`. For example:
+
+```yaml
+- name: CONFIGMAP_RECORD_ALLOWLIST
+  value: "proxy.config.http.cache.http,proxy.config.diags.debug.enabled,proxy.config.ssl.CA.cert.*"
+```
+
+Putting the extension in the pod spec keeps the decision with the cluster operator: extending the allowlist requires write access to the Deployment, a higher privilege than the ConfigMap write access the allowlist defends against. Records like `proxy.config.ssl.keylog_file`, `proxy.config.http.push_method_enabled`, or `proxy.config.ssl.client.verify.server.policy` should never be added.
+
 #### Namespaces for Ingresses
 
 You can specifiy the list of namespaces to look for ingress object by providing an environment variable called `INGRESS_NS`. The default is `all`, which tells the controller to look for ingress objects in all namespaces. Alternatively you can provide a comma-separated list of namespaces for the controller to look for ingresses. Similarly you can specifiy a comma-separated list of namespaces to ignore while the controller is looking for ingresses by providing `INGRESS_IGNORE_NS`.

pluginats/connect_redis.lua

@@ -31,6 +31,37 @@ function __init__(argtb)
   end
 end
 
+-- Globals a server snippet is allowed to reach. Snippets come from the
+-- ats.ingress.kubernetes.io/server-snippet annotation of tenant Ingress
+-- objects, so they must never reach process-level primitives such as
+-- os, io, require, package, loadstring, dofile or the real global table.
+local SNIPPET_ALLOWED_GLOBALS = {
+  ts = true, string = true, table = true, math = true,
+  tostring = true, tonumber = true, pairs = true, ipairs = true,
+  next = true, select = true, type = true, unpack = true,
+  pcall = true, xpcall = true, error = true, assert = true,
+}
+
+-- Build a fresh, restricted environment for one snippet execution.
+-- Reads of allowed names (plus the TS_LUA_* hook constants) are proxied
+-- to the plugin globals; everything else resolves to nil. Writes land in
+-- the throwaway table, so snippets cannot mutate plugin globals.
+local function new_snippet_env()
+  local env = {}
+  setmetatable(env, {
+    __index = function(_, key)
+      if SNIPPET_ALLOWED_GLOBALS[key] then
+        return _G[key]
+      end
+      if type(key) == 'string' and string.sub(key, 1, 7) == 'TS_LUA_' then
+        return _G[key]
+      end
+      return nil
+    end,
+  })
+  return env
+end
+
 -- helper function to split a string
 function ipport_split(s, delimiter)
   result = {}
@@ -244,8 +275,16 @@ function do_global_read_request()
           return 0
         end
       ts.debug("Snippet in the Connect Redis lua file " .. snippet)
-        local f = loadstring(snippet)
-        f()
+        local f, compile_err = loadstring(snippet)
+        if f == nil then
+          ts.error("Snippet failed to compile: " .. tostring(compile_err))
+        else
+          setfenv(f, new_snippet_env())
+          local ok, run_err = pcall(f)
+          if not ok then
+            ts.error("Snippet execution failed: " .. tostring(run_err))
+          end
+        end
       end
     end
   end

pluginats/connect_redis_test.lua

@@ -197,6 +197,84 @@ describe("Unit tests - Lua", function()
       assert.stub(ts.http.set_resp).was.called_with(301,"Redirect")
     end)
 
+    it("Test - Snippet runs in restricted sandbox", function()
+      client:select(1)
+      client:sadd("E+http://test.edge.com/app1","$attacker-ns/evil-ingress/1")
+      local evil_snippet = "ts.debug('sandbox_os:' .. tostring(os))\n" ..
+        "ts.debug('sandbox_io:' .. tostring(io))\n" ..
+        "ts.debug('sandbox_require:' .. tostring(require))\n" ..
+        "ts.debug('sandbox_loadstring:' .. tostring(loadstring))\n" ..
+        "snippet_leak_probe = 42\n"
+      client:sadd("$attacker-ns/evil-ingress/1", evil_snippet)
+
+      package.loaded["connect_redis"] = nil
+      require "connect_redis"
+      local input_args = {"snippet"}
+      __init__(input_args)
+      do_global_read_request()
+
+      -- process-level primitives are not reachable from snippets
+      assert.stub(ts.debug).was.called_with("sandbox_os:nil")
+      assert.stub(ts.debug).was.called_with("sandbox_io:nil")
+      assert.stub(ts.debug).was.called_with("sandbox_require:nil")
+      assert.stub(ts.debug).was.called_with("sandbox_loadstring:nil")
+
+      -- snippet globals stay in the sandbox, not in the plugin state
+      assert.is_nil(_G.snippet_leak_probe)
+    end)
+
+    it("Test - Snippet cannot reach metatable, environment or loader escapes", function()
+      -- Locks in the sandbox's escape-surface claims: every primitive that
+      -- could be used to break out of the restricted environment (metatable
+      -- manipulation, the real global table, environment introspection,
+      -- alternate code loaders, the debug library) must resolve to nil.
+      client:select(1)
+      client:sadd("E+http://test.edge.com/app1","$attacker-ns/escape-ingress/3")
+      local escape_snippet =
+        "ts.debug('esc_getmetatable:' .. tostring(getmetatable))\n" ..
+        "ts.debug('esc_setmetatable:' .. tostring(setmetatable))\n" ..
+        "ts.debug('esc_rawset:' .. tostring(rawset))\n" ..
+        "ts.debug('esc_rawget:' .. tostring(rawget))\n" ..
+        "ts.debug('esc_G:' .. tostring(_G))\n" ..
+        "ts.debug('esc_getfenv:' .. tostring(getfenv))\n" ..
+        "ts.debug('esc_setfenv:' .. tostring(setfenv))\n" ..
+        "ts.debug('esc_load:' .. tostring(load))\n" ..
+        "ts.debug('esc_loadfile:' .. tostring(loadfile))\n" ..
+        "ts.debug('esc_dofile:' .. tostring(dofile))\n" ..
+        "ts.debug('esc_coroutine:' .. tostring(coroutine))\n" ..
+        "ts.debug('esc_debug:' .. tostring(debug))\n" ..
+        "ts.debug('esc_package:' .. tostring(package))\n"
+      client:sadd("$attacker-ns/escape-ingress/3", escape_snippet)
+
+      package.loaded["connect_redis"] = nil
+      require "connect_redis"
+      local input_args = {"snippet"}
+      __init__(input_args)
+      do_global_read_request()
+
+      for _, name in ipairs({"getmetatable","setmetatable","rawset","rawget","G",
+                             "getfenv","setfenv","load","loadfile","dofile",
+                             "coroutine","debug","package"}) do
+        assert.stub(ts.debug).was.called_with("esc_" .. name .. ":nil")
+      end
+    end)
+
+    it("Test - Failing snippet is contained", function()
+      client:select(1)
+      client:sadd("E+http://test.edge.com/app1","$attacker-ns/broken-ingress/2")
+      client:sadd("$attacker-ns/broken-ingress/2", "os.execute('id')")
+
+      package.loaded["connect_redis"] = nil
+      require "connect_redis"
+      local input_args = {"snippet"}
+      __init__(input_args)
+
+      -- os is nil inside the sandbox; the runtime error must be caught
+      -- instead of propagating out of the plugin
+      assert.has_no.errors(function() do_global_read_request() end)
+      assert.stub(ts.error).was.called()
+    end)
+
 
   end)
 end)

tests/data/setup/traffic-server/ats-deployment.yaml

@@ -72,6 +72,11 @@ spec:
               value: "1"
             - name: POD_TLS_PATH
               value: "/etc/ats/ssl:/etc/ats/ssl/server:/etc/ats/ssl/server2"
+            # Extra records.config keys (beyond the controller's built-in
+            # safe-tuning allowlist) that the annotated ConfigMap
+            # tests/data/setup/configmaps/ats-configmap.yaml is allowed to set.
+            - name: CONFIGMAP_RECORD_ALLOWLIST
+              value: "proxy.config.http.cache.http,proxy.config.http.cache.required_headers,proxy.config.diags.debug.enabled,proxy.config.ssl.CA.cert.path,proxy.config.ssl.CA.cert.filename"
           ports:
           - containerPort: 8080
             name: http

watcher/handlerConfigmap.go

@@ -17,6 +17,8 @@ package watcher
 
 import (
 	"log"
+	"os"
+	"strings"
 
 	"github.com/apache/trafficserver-ingress-controller/endpoint"
 
@@ -29,6 +31,80 @@ type CMHandler struct {
 	Ep           *endpoint.Endpoint
 }
 
+// configMapAllowlistEnv is the environment variable through which the
+// operator may extend the built-in allowlist of records.config keys that an
+// annotated ConfigMap is permitted to set. It holds a comma-separated list
+// of entries; an entry ending in "*" matches any key with that prefix, any
+// other entry must match a key exactly. Entries must begin with
+// "proxy.config." - anything else is ignored and logged. The variable lives
+// in the pod spec, so extending the allowlist requires write access to the
+// Deployment: a strictly higher privilege than the ConfigMap write access
+// this allowlist defends against.
+const configMapAllowlistEnv = "CONFIGMAP_RECORD_ALLOWLIST"
+
+// builtinAllowedRecords is the fail-closed default allowlist of
+// records.config keys that a ConfigMap may set on the live proxy. The
+// ats-configmap annotation is attacker-suppliable by anyone who can write
+// ConfigMaps in a watched namespace, so only operationally safe tuning
+// knobs are allowed by default. Records such as proxy.config.ssl.*,
+// proxy.config.http.push_method_enabled, proxy.config.log.*,
+// proxy.config.diags.* and proxy.config.plugin.* must never be settable
+// through this path unless the operator explicitly opts them in via
+// CONFIGMAP_RECORD_ALLOWLIST on the pod spec.
+var builtinAllowedRecords = []string{
+	"proxy.config.output.logfile.rolling_*",
+	"proxy.config.restart.*",
+	"proxy.config.http.keep_alive_*",
+	"proxy.config.http.transaction_no_activity_timeout_*",
+	"proxy.config.http.transaction_active_timeout_*",
+	"proxy.config.http.connect_attempts_*",
+	"proxy.config.http.parent_proxy.connect_attempts_timeout",
+	"proxy.config.net.connections_throttle",
+	"proxy.config.cache.ram_cache.size",
+}
+
+// matchesAllowlistEntry reports whether key matches one allowlist entry:
+// prefix match when the entry ends in "*", exact match otherwise.
+func matchesAllowlistEntry(key, entry string) bool {
+	if strings.HasSuffix(entry, "*") {
+		return strings.HasPrefix(key, strings.TrimSuffix(entry, "*"))
+	}
+	return key == entry
+}
+
+// configKeyAllowed reports whether a records.config key coming from a
+// ConfigMap is on the built-in allowlist or in the operator-supplied
+// extension carried by CONFIGMAP_RECORD_ALLOWLIST.
+func configKeyAllowed(key string) bool {
+	for _, entry := range builtinAllowedRecords {
+		if matchesAllowlistEntry(key, entry) {
+			return true
+		}
+	}
+	for _, entry := range strings.Split(os.Getenv(configMapAllowlistEnv), ",") {
+		entry = strings.TrimSpace(entry)
+		if entry == "" {
+			continue
+		}
+		if !strings.HasPrefix(entry, "proxy.config.") {
+			log.Printf("Ignoring %s entry %q: allowlist entries must start with proxy.config.", configMapAllowlistEnv, entry)
+			continue
+		}
+		if matchesAllowlistEntry(key, entry) {
+			return true
+		}
+	}
+	return false
+}
+
+// configValueAllowed rejects values that do not look like a single
+// records.config token. traffic_ctl is exec'd argv-style so there is no
+// shell to inject into, but control characters and whitespace have no
+// place in a config value and only help smuggle surprises downstream.
+func configValueAllowed(value string) bool {
+	return !strings.ContainsAny(value, " \t\r\n\x00")
+}
+
 // Add for EventHandler
 func (c *CMHandler) Add(obj interface{}) {
 	c.update(obj)
@@ -51,6 +127,14 @@ func (c *CMHandler) update(newObj interface{}) {
 	}
 
 	for currKey, currVal := range cm.Data {
+		if !configKeyAllowed(currKey) {
+			log.Printf("Rejected config key %q from ConfigMap %s/%s: not in the allowlist of safe reloadable records (extend via %s in the pod spec if this key is intended)", currKey, cm.GetNamespace(), cm.GetName(), configMapAllowlistEnv)
+			continue
+		}
+		if !configValueAllowed(currVal) {
+			log.Printf("Rejected config value for key %q from ConfigMap %s/%s: contains whitespace or control characters", currKey, cm.GetNamespace(), cm.GetName())
+			continue
+		}
 		msg, err := c.Ep.ATSManager.ConfigSet(currKey, currVal) // update ATS
 		if err != nil {
 			log.Println(err)