diff --git a/charts/ats-ingress/templates/clusterrole.yaml b/charts/ats-ingress/templates/clusterrole.yaml index 94a28b8..e24cd16 100644 --- a/charts/ats-ingress/templates/clusterrole.yaml +++ b/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 diff --git a/charts/ats-ingress/templates/role.yaml b/charts/ats-ingress/templates/role.yaml new file mode 100644 index 0000000..8826021 --- /dev/null +++ b/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 -}} diff --git a/charts/ats-ingress/templates/rolebinding.yaml b/charts/ats-ingress/templates/rolebinding.yaml new file mode 100644 index 0000000..6821d25 --- /dev/null +++ b/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 -}} diff --git a/docs/TUTORIAL.md b/docs/TUTORIAL.md index 1e5d9e4..3ca2adb 100644 --- a/docs/TUTORIAL.md +++ b/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`. diff --git a/pluginats/connect_redis.lua b/pluginats/connect_redis.lua index 29d4262..9d86bf8 100644 --- a/pluginats/connect_redis.lua +++ b/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 diff --git a/pluginats/connect_redis_test.lua b/pluginats/connect_redis_test.lua index d8e7316..df6898b 100644 --- a/pluginats/connect_redis_test.lua +++ b/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) diff --git a/tests/data/setup/traffic-server/ats-deployment.yaml b/tests/data/setup/traffic-server/ats-deployment.yaml index 0c6be53..f27e4b2 100644 --- a/tests/data/setup/traffic-server/ats-deployment.yaml +++ b/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 diff --git a/watcher/handlerConfigmap.go b/watcher/handlerConfigmap.go index c84c1fe..57db8ed 100644 --- a/watcher/handlerConfigmap.go +++ b/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) diff --git a/watcher/handlerConfigmap_test.go b/watcher/handlerConfigmap_test.go index 87b915a..600a093 100644 --- a/watcher/handlerConfigmap_test.go +++ b/watcher/handlerConfigmap_test.go @@ -108,6 +108,107 @@ func TestUpdate_BasicConfigMap(t *testing.T) { } +func TestAdd_DisallowedConfigMapKeysRejected(t *testing.T) { + cmHandler := createExampleCMHandler() + exampleConfigMap := createExampleConfigMap() + + exampleConfigMap.Data = map[string]string{ + "proxy.config.http.push_method_enabled": "1", + "proxy.config.ssl.client.verify.server.policy": "DISABLED", + "proxy.config.ssl.keylog_file": "/tmp/keys.log", + "proxy.config.http.cache.required_headers": "0", + "proxy.config.diags.debug.enabled": "1", + "proxy.config.output.logfile.rolling_enabled": "1", + } + + cmHandler.Add(&exampleConfigMap) + + disallowed := []string{ + "proxy.config.http.push_method_enabled", + "proxy.config.ssl.client.verify.server.policy", + "proxy.config.ssl.keylog_file", + "proxy.config.http.cache.required_headers", + "proxy.config.diags.debug.enabled", + } + + for _, key := range disallowed { + if val, err := cmHandler.Ep.ATSManager.ConfigGet(key); err == nil { + t.Errorf("disallowed key %s should not have been applied, but got value %s", key, val) + } + } + + rEnabled, err := cmHandler.Ep.ATSManager.ConfigGet("proxy.config.output.logfile.rolling_enabled") + + if err != nil { + t.Error(err) + } else if !reflect.DeepEqual(rEnabled, "1") { + t.Errorf("returned \n%s, but expected \n%s", rEnabled, "1") + } +} + +func TestAdd_OperatorExtendedAllowlist(t *testing.T) { + // The operator extends the allowlist through the pod spec; the shipped + // e2e fixture tests/data/setup/configmaps/ats-configmap.yaml relies on + // exactly this mechanism (see tests/data/setup/traffic-server/ats-deployment.yaml). + t.Setenv("CONFIGMAP_RECORD_ALLOWLIST", " proxy.config.diags.debug.enabled, proxy.config.http.cache.*,proxy.config.ssl.CA.cert.*, not.a.proxy.record ") + + cmHandler := createExampleCMHandler() + exampleConfigMap := createExampleConfigMap() + + exampleConfigMap.Data = map[string]string{ + "proxy.config.diags.debug.enabled": "1", + "proxy.config.http.cache.required_headers": "0", + "proxy.config.ssl.CA.cert.filename": "tls.crt", + "proxy.config.http.push_method_enabled": "1", + "proxy.config.ssl.client.verify.server.policy": "DISABLED", + "not.a.proxy.record": "1", + } + + cmHandler.Add(&exampleConfigMap) + + allowed := map[string]string{ + "proxy.config.diags.debug.enabled": "1", + "proxy.config.http.cache.required_headers": "0", + "proxy.config.ssl.CA.cert.filename": "tls.crt", + } + + for key, expected := range allowed { + val, err := cmHandler.Ep.ATSManager.ConfigGet(key) + if err != nil { + t.Error(err) + } else if !reflect.DeepEqual(val, expected) { + t.Errorf("returned \n%s, but expected \n%s", val, expected) + } + } + + stillDisallowed := []string{ + "proxy.config.http.push_method_enabled", + "proxy.config.ssl.client.verify.server.policy", + "not.a.proxy.record", + } + + for _, key := range stillDisallowed { + if val, err := cmHandler.Ep.ATSManager.ConfigGet(key); err == nil { + t.Errorf("key %s outside the extended allowlist should not have been applied, but got value %s", key, val) + } + } +} + +func TestAdd_ConfigMapValueWithWhitespaceRejected(t *testing.T) { + cmHandler := createExampleCMHandler() + exampleConfigMap := createExampleConfigMap() + + exampleConfigMap.Data = map[string]string{ + "proxy.config.restart.active_client_threshold": "0\nproxy.config.ssl.keylog_file /tmp/keys.log", + } + + cmHandler.Add(&exampleConfigMap) + + if val, err := cmHandler.Ep.ATSManager.ConfigGet("proxy.config.restart.active_client_threshold"); err == nil { + t.Errorf("value with control characters should not have been applied, but got %s", val) + } +} + func createExampleConfigMap() v1.ConfigMap { exampleConfigMap := v1.ConfigMap{ ObjectMeta: meta_v1.ObjectMeta{ diff --git a/watcher/handlerIngress.go b/watcher/handlerIngress.go index 4be5649..d237dde 100644 --- a/watcher/handlerIngress.go +++ b/watcher/handlerIngress.go @@ -18,6 +18,7 @@ package watcher import ( "log" "strconv" + "sync" "github.com/apache/trafficserver-ingress-controller/endpoint" "github.com/apache/trafficserver-ingress-controller/util" @@ -29,6 +30,108 @@ import ( type IgHandler struct { ResourceName string Ep *endpoint.Endpoint + + // hostOwners maps a hostname to the namespace that first claimed it and + // a count of live Ingress claims from that namespace. A hostname's routes + // may only be written by Ingresses of the owning namespace; conflicting + // claims from other namespaces are rejected (first writer wins), so a + // tenant cannot attach its backend to another tenant's hostname. + // Held by pointer so copies of the handler share one registry and the + // struct stays safe to copy. + hostOwners *hostOwnerRegistry +} + +type hostOwnerRegistry struct { + mu sync.Mutex + owners map[string]*hostClaim +} + +type hostClaim struct { + namespace string + refs int +} + +func (g *IgHandler) hostOwnerRegistryInit() *hostOwnerRegistry { + if g.hostOwners == nil { + g.hostOwners = &hostOwnerRegistry{owners: make(map[string]*hostClaim)} + } + return g.hostOwners +} + +// claimHost records that an Ingress in namespace binds routes for host. +// It returns false when the host is already owned by a different namespace. +func (g *IgHandler) claimHost(host, namespace string) bool { + r := g.hostOwnerRegistryInit() + r.mu.Lock() + defer r.mu.Unlock() + + claim, ok := r.owners[host] + if !ok { + r.owners[host] = &hostClaim{namespace: namespace, refs: 1} + return true + } + if claim.namespace != namespace { + return false + } + claim.refs++ + return true +} + +// releaseHost drops one claim on host held by namespace. Releases by a +// namespace that does not own the host are ignored, so a rejected claimant +// cannot free another tenant's hostname by deleting its own Ingress. +func (g *IgHandler) releaseHost(host, namespace string) { + r := g.hostOwnerRegistryInit() + r.mu.Lock() + defer r.mu.Unlock() + + claim, ok := r.owners[host] + if !ok || claim.namespace != namespace { + return + } + claim.refs-- + if claim.refs <= 0 { + delete(r.owners, host) + } +} + +// ingressHosts returns the unique set of hostnames an Ingress binds routes +// for: "*" for the default backend and for rules with an empty host, plus +// every rule host. +func ingressHosts(ing *nv1.Ingress) []string { + seen := make(map[string]bool) + var hosts []string + + appendHost := func(host string) { + if host == "" { + host = "*" + } + if !seen[host] { + seen[host] = true + hosts = append(hosts, host) + } + } + + if ing.Spec.DefaultBackend != nil { + appendHost("*") + } + for _, rule := range ing.Spec.Rules { + appendHost(rule.Host) + } + return hosts +} + +// claimIngressHosts claims every host the Ingress references and returns, +// per host, whether the claim succeeded. Rejected claims are logged. +func (g *IgHandler) claimIngressHosts(ing *nv1.Ingress, namespace, name string) map[string]bool { + claimed := make(map[string]bool) + for _, host := range ingressHosts(ing) { + claimed[host] = g.claimHost(host, namespace) + if !claimed[host] { + log.Printf("Host %q in Ingress %s/%s is already owned by another namespace; skipping its routes", host, namespace, name) + } + } + return claimed } func (g *IgHandler) Add(obj interface{}) { @@ -64,8 +167,12 @@ func (g *IgHandler) add(obj interface{}) { g.Ep.RedisClient.DBOneSAdd(nameversion, snippet) } + // claim host ownership before writing any routes; hosts owned by a + // different namespace are skipped + claimed := g.claimIngressHosts(ingressObj, namespace, name) + // add default backend rules - if ingressObj.Spec.DefaultBackend != nil { + if ingressObj.Spec.DefaultBackend != nil && claimed["*"] { host := "*" scheme := "http" path := "/" @@ -105,6 +212,9 @@ func (g *IgHandler) add(obj interface{}) { if host == "" { host = "*" } + if !claimed[host] { + continue + } scheme := "http" if _, ok := tlsHosts[host]; ok { scheme = "https" @@ -129,6 +239,19 @@ func (g *IgHandler) add(obj interface{}) { } } +// seedTempHostPath makes sure the temp key for hostPath starts from the +// current live routing set before this Ingress's new members are added. +// Without the seed, the final SUNIONSTORE back into hostPath would replace +// the live set and silently drop entries contributed by other Ingresses +// that share the same host/path. +func (g *IgHandler) seedTempHostPath(m map[string]string, hostPath string) { + tempKey := "temp_" + hostPath + if _, ok := m[tempKey]; !ok { + g.Ep.RedisClient.DBOneSUnionStore(tempKey, hostPath) + m[tempKey] = hostPath + } +} + // Update for EventHandler func (g *IgHandler) Update(obj, newObj interface{}) { log.Printf("In INGRESS_HANDLER UPDATE %#v \n", newObj) @@ -238,6 +361,11 @@ func (g *IgHandler) update(obj, newObj interface{}) { } } + + // release the old spec's host claims; the new spec re-claims below + for _, host := range ingressHosts(ingressObj) { + g.releaseHost(host, namespace) + } } newNamespace := newIngressObj.GetNamespace() @@ -256,8 +384,12 @@ func (g *IgHandler) update(obj, newObj interface{}) { g.Ep.RedisClient.DBOneSAdd(nameversion, newSnippet) } + // claim host ownership before writing any routes; hosts owned by a + // different namespace are skipped + claimed := g.claimIngressHosts(newIngressObj, newNamespace, name) + // handle default backend rule - if newIngressObj.Spec.DefaultBackend != nil { + if newIngressObj.Spec.DefaultBackend != nil && claimed["*"] { host := "*" scheme := "http" path := "/" @@ -268,8 +400,8 @@ func (g *IgHandler) update(obj, newObj interface{}) { port := strconv.Itoa(int(newIngressObj.Spec.DefaultBackend.Service.Port.Number)) svcport := util.ConstructSvcPortString(newNamespace, service, port) + g.seedTempHostPath(m, hostPath) g.Ep.RedisClient.DBOneSAdd("temp_"+hostPath, svcport) - m["temp_"+hostPath] = hostPath if newSnippetErr == nil { g.Ep.RedisClient.DBOneSAdd("temp_"+hostPath, nameversion) @@ -279,8 +411,8 @@ func (g *IgHandler) update(obj, newObj interface{}) { scheme = "https" hostPath = util.ConstructHostPathString(scheme, host, path, pathType) + g.seedTempHostPath(m, hostPath) g.Ep.RedisClient.DBOneSAdd("temp_"+hostPath, svcport) - m["temp_"+hostPath] = hostPath if newSnippetErr == nil { g.Ep.RedisClient.DBOneSAdd("temp_"+hostPath, nameversion) @@ -300,6 +432,9 @@ func (g *IgHandler) update(obj, newObj interface{}) { if host == "" { host = "*" } + if !claimed[host] { + continue + } scheme := "http" if _, ok := newTlsHosts[host]; ok { scheme = "https" @@ -315,8 +450,8 @@ func (g *IgHandler) update(obj, newObj interface{}) { port := strconv.Itoa(int(httpPath.Backend.Service.Port.Number)) svcport := util.ConstructSvcPortString(newNamespace, service, port) + g.seedTempHostPath(m, hostPath) g.Ep.RedisClient.DBOneSAdd("temp_"+hostPath, svcport) - m["temp_"+hostPath] = hostPath if newSnippetErr == nil { g.Ep.RedisClient.DBOneSAdd("temp_"+hostPath, nameversion) @@ -423,6 +558,11 @@ func (g *IgHandler) delete(obj interface{}) { } } + + // release this Ingress's host claims (no-op for hosts owned elsewhere) + for _, host := range ingressHosts(ingressObj) { + g.releaseHost(host, namespace) + } } // GetResourceName returns the resource name diff --git a/watcher/handlerIngress_test.go b/watcher/handlerIngress_test.go index 6d89fd0..e59ee80 100644 --- a/watcher/handlerIngress_test.go +++ b/watcher/handlerIngress_test.go @@ -210,6 +210,81 @@ func TestDelete(t *testing.T) { } +func TestAdd_CrossNamespaceHostClaimRejected(t *testing.T) { + igHandler := createExampleIgHandler() + victimIngress := createExampleIngress() + + igHandler.add(&victimIngress) + + attackerIngress := createExampleIngress() + attackerIngress.ObjectMeta.Name = "attacker-ingress" + attackerIngress.ObjectMeta.Namespace = "attacker-namespace" + attackerIngress.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].Backend.Service.Name = "evilsvc" + + igHandler.add(&attackerIngress) + + returnedKeys := igHandler.Ep.RedisClient.GetDBOneKeyValues() + + // the attacker's backends must not appear under the victim's hostnames + expectedKeys := getExpectedKeysForAdd() + + if !util.IsSameMap(returnedKeys, expectedKeys) { + t.Errorf("returned \n%v, but expected \n%v", returnedKeys, expectedKeys) + } +} + +func TestAdd_SameNamespaceHostSharingAllowed(t *testing.T) { + igHandler := createExampleIgHandler() + firstIngress := createExampleIngress() + + igHandler.add(&firstIngress) + + secondIngress := createExampleIngress() + secondIngress.ObjectMeta.Name = "second-ingress" + secondIngress.Spec.Rules = secondIngress.Spec.Rules[:1] + secondIngress.Spec.Rules[0].IngressRuleValue.HTTP.Paths = secondIngress.Spec.Rules[0].IngressRuleValue.HTTP.Paths[:1] + secondIngress.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].Path = "/app3" + secondIngress.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].Backend.Service.Name = "appsvc3" + + igHandler.add(&secondIngress) + + returnedKeys := igHandler.Ep.RedisClient.GetDBOneKeyValues() + + expectedKeys := getExpectedKeysForAdd() + expectedKeys["E+http://test.media.com/app3"] = []string{"trafficserver-test:appsvc3:8080"} + + if !util.IsSameMap(returnedKeys, expectedKeys) { + t.Errorf("returned \n%v, but expected \n%v", returnedKeys, expectedKeys) + } +} + +func TestDelete_ReleasesHostOwnership(t *testing.T) { + igHandler := createExampleIgHandler() + victimIngress := createExampleIngress() + + igHandler.add(&victimIngress) + igHandler.delete(&victimIngress) + + otherIngress := createExampleIngress() + otherIngress.ObjectMeta.Name = "other-ingress" + otherIngress.ObjectMeta.Namespace = "other-namespace" + + igHandler.add(&otherIngress) + + returnedKeys := igHandler.Ep.RedisClient.GetDBOneKeyValues() + + found := false + for _, member := range returnedKeys["E+http://test.media.com/app1"] { + if member == "other-namespace:appsvc1:8080" { + found = true + } + } + + if !found { + t.Errorf("expected other-namespace:appsvc1:8080 to be routable after the previous owner was deleted, got \n%v", returnedKeys) + } +} + func createExampleIngressWithTLS() nv1.Ingress { exampleIngress := createExampleIngress() @@ -303,7 +378,7 @@ func createExampleIngress() nv1.Ingress { func createExampleIgHandler() IgHandler { exampleEndpoint := createExampleEndpoint() - igHandler := IgHandler{"ingresses", &exampleEndpoint} + igHandler := IgHandler{ResourceName: "ingresses", Ep: &exampleEndpoint} return igHandler } @@ -413,3 +488,40 @@ func getExampleSnippet() string { -- ts.http.set_resp(301, 'Redirect') ts.debug('Uncomment the above lines to redirect http request to https')` } + +func TestUpdate_PreservesOtherIngressRoutesOnSharedHostPath(t *testing.T) { + igHandler := createExampleIgHandler() + + // the victim Ingress owns E+http://test.media.com/app1 + victimIngress := createExampleIngress() + igHandler.add(&victimIngress) + + // a second Ingress starts out on an unrelated host + otherIngress := createExampleIngress() + otherIngress.ObjectMeta.Name = "other-ingress" + otherIngress.Spec.Rules = otherIngress.Spec.Rules[:1] + otherIngress.Spec.Rules[0].Host = "other.host.com" + otherIngress.Spec.Rules[0].IngressRuleValue.HTTP.Paths = otherIngress.Spec.Rules[0].IngressRuleValue.HTTP.Paths[:1] + + // ... and is then updated to claim the victim's host/path + updatedOtherIngress := createExampleIngress() + updatedOtherIngress.ObjectMeta.Name = "other-ingress" + updatedOtherIngress.Spec.Rules = updatedOtherIngress.Spec.Rules[:1] + updatedOtherIngress.Spec.Rules[0].Host = "test.media.com" + updatedOtherIngress.Spec.Rules[0].IngressRuleValue.HTTP.Paths = updatedOtherIngress.Spec.Rules[0].IngressRuleValue.HTTP.Paths[:1] + updatedOtherIngress.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].Backend.Service.Name = "othersvc" + + igHandler.add(&otherIngress) + igHandler.update(&otherIngress, &updatedOtherIngress) + + returnedKeys := igHandler.Ep.RedisClient.GetDBOneKeyValues() + + // the update must not remove the victim's member from the shared key + expectedKeys := getExpectedKeysForAdd() + expectedKeys["E+http://other.host.com/app1"] = []string{} + expectedKeys["E+http://test.media.com/app1"] = append(expectedKeys["E+http://test.media.com/app1"], "trafficserver-test:othersvc:8080") + + if !util.IsSameMap(returnedKeys, expectedKeys) { + t.Errorf("returned \n%v, but expected \n%v", returnedKeys, expectedKeys) + } +} diff --git a/watcher/validation_highs_test.go b/watcher/validation_highs_test.go new file mode 100644 index 0000000..8f727b9 --- /dev/null +++ b/watcher/validation_highs_test.go @@ -0,0 +1,229 @@ +/* +Licensed 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. +*/ + +// Validation harness for the HIGH/critical findings in the 2026-08-11 security +// scan (scan dir: scan-trafficserver-ingress-controller-20260811). +// +// Each test asserts the SECURE invariant the corresponding candidate patch is +// meant to establish. Against the current (unpatched) code these tests FAIL, +// and the failure output is the runtime proof that the finding is real. Once +// the candidate patch (PATCHES/bug_0N) is applied, the matching test passes — +// so this file doubles as the before/after regression check. +// +// Covered here (miniredis + FakeATSManager, no cluster needed): +// +// f003 -> TestValidation_F003_* (cross-tenant route injection on add) +// f004 -> TestValidation_F004_* (silent route deletion/takeover on update) +// f002 -> TestValidation_F002_* (unallowlisted ConfigMap key -> live ATS) +// +// Out of scope for a Go unit test (validated statically / other harness): +// +// f001 (Lua loadstring sandbox) -> needs a Lua/busted harness; none present. +// f008 (ClusterRole secrets grant) -> RBAC YAML; see the shell assertion in +// the validation report. +package watcher + +import ( + "testing" + + ep "github.com/apache/trafficserver-ingress-controller/endpoint" + "github.com/apache/trafficserver-ingress-controller/namespace" + "github.com/apache/trafficserver-ingress-controller/proxy" + "github.com/apache/trafficserver-ingress-controller/redis" + + v1 "k8s.io/api/core/v1" + nv1 "k8s.io/api/networking/v1" + meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// newTenantEndpoint builds an Endpoint wired to a fresh in-memory Redis and a +// FakeATSManager with the shipped defaults (empty ingress class => match all, +// empty namespace maps => watch all), mirroring the stock deployment posture +// the scan assumes. +func newTenantEndpoint(t *testing.T) *ep.Endpoint { + t.Helper() + + rClient, err := redis.InitForTesting() + if err != nil { + t.Fatalf("redis.InitForTesting: %v", err) + } + + nsManager := namespace.NsManager{ + NamespaceMap: make(map[string]bool), + IgnoreNamespaceMap: make(map[string]bool), + } + nsManager.Init() + + return &ep.Endpoint{ + RedisClient: rClient, + ATSManager: &proxy.FakeATSManager{ + Namespace: "trafficserver", + IngressClass: "", + Config: make(map[string]string), + }, + NsManager: &nsManager, + } +} + +// singleHostIngress builds a minimal Ingress: one namespace, one host, one exact +// path, one service backend. resourceVersion lets update() see a real change. +func singleHostIngress(name, ns, host, path, svc string, port int32, resourceVersion string) nv1.Ingress { + pt := nv1.PathTypeExact + return nv1.Ingress{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: name, + Namespace: ns, + ResourceVersion: resourceVersion, + }, + Spec: nv1.IngressSpec{ + Rules: []nv1.IngressRule{ + { + Host: host, + IngressRuleValue: nv1.IngressRuleValue{ + HTTP: &nv1.HTTPIngressRuleValue{ + Paths: []nv1.HTTPIngressPath{ + { + Path: path, + PathType: &pt, + Backend: nv1.IngressBackend{ + Service: &nv1.IngressServiceBackend{ + Name: svc, + Port: nv1.ServiceBackendPort{Number: port}, + }, + }, + }, + }, + }, + }, + }, + }, + }, + } +} + +// setContains reports whether want is a member of the route-key set. +func setContains(set []string, want string) bool { + for _, m := range set { + if m == want { + return true + } + } + return false +} + +// -------------------------------------------------------------------------- +// f003 — No host ownership check lets any tenant hijack any hostname's traffic +// handlerIngress.go:122 add() -> DBOneSAdd(hostPath, svcport), no owner check +// Candidate fix: PATCHES/bug_03 (first-writer-wins ownership index) +// -------------------------------------------------------------------------- +func TestValidation_F003_CrossTenantRouteInjection(t *testing.T) { + e := newTenantEndpoint(t) + ig := IgHandler{ResourceName: "ingresses", Ep: e} + + const host = "shop.example.com" + const routeKey = "E+http://shop.example.com/" + + // Tenant "team-victim" legitimately claims shop.example.com first. + victim := singleHostIngress("victim-ing", "team-victim", host, "/", "victim-backend", 8080, "1") + ig.add(&victim) + + // Tenant "team-attacker", in a DIFFERENT namespace, claims the SAME host. + attacker := singleHostIngress("attacker-ing", "team-attacker", host, "/", "attacker-backend", 8080, "1") + ig.add(&attacker) + + route := e.RedisClient.GetDBOneKeyValues()[routeKey] + + // SECURE invariant (first-writer-wins): once team-victim owns the host, a + // claim from another namespace must be refused — the attacker's backend + // must NOT appear in the victim's route set. + if setContains(route, "team-attacker:attacker-backend:8080") { + t.Errorf("f003 CONFIRMED: attacker backend injected into another tenant's route %q = %v; "+ + "traffic to %s can now be split to team-attacker (candidate fix: bug_03)", + routeKey, route, host) + } + if !setContains(route, "team-victim:victim-backend:8080") { + t.Errorf("f003: victim backend missing from %q = %v (unexpected)", routeKey, route) + } +} + +// -------------------------------------------------------------------------- +// f004 — Ingress update overwrites shared host/path Redis key, deleting victim +// handlerIngress.go:318/329-332 new-spec temp key is unseeded, then +// SUNIONSTORE replaces the live set. Candidate fix: PATCHES/bug_04 +// (seed new-side temp keys from live). +// -------------------------------------------------------------------------- +func TestValidation_F004_SilentRouteDeletionOnUpdate(t *testing.T) { + e := newTenantEndpoint(t) + ig := IgHandler{ResourceName: "ingresses", Ep: e} + + const host = "shop.example.com" + const routeKey = "E+http://shop.example.com/" + + // team-victim owns shop.example.com/. + victim := singleHostIngress("victim-ing", "team-victim", host, "/", "victim-backend", 8080, "1") + ig.add(&victim) + + // team-attacker starts on its OWN host, then UPDATES its Ingress to point + // at the victim's host. Because shop.example.com is only in the new spec + // (not the old), its temp key is never seeded from the live set, and the + // finalizing SUNIONSTORE replaces the live route with attacker-only. + attackerOld := singleHostIngress("attacker-ing", "team-attacker", "attacker-own.example.com", "/", "attacker-backend", 8080, "1") + ig.add(&attackerOld) + attackerNew := singleHostIngress("attacker-ing", "team-attacker", host, "/", "attacker-backend", 8080, "2") + ig.update(&attackerOld, &attackerNew) + + route := e.RedisClient.GetDBOneKeyValues()[routeKey] + + // SECURE invariant: an update by another tenant must not delete the + // victim's backend from a route the victim owns. + if !setContains(route, "team-victim:victim-backend:8080") { + t.Errorf("f004 CONFIRMED: victim backend silently deleted by another tenant's update; "+ + "%q = %v (candidate fix: bug_04)", routeKey, route) + } +} + +// -------------------------------------------------------------------------- +// f002 — Annotated ConfigMap sets arbitrary live ATS config via traffic_ctl +// handlerConfigmap.go:53 every Data key applied via ConfigSet, no allowlist +// Candidate fix: PATCHES/bug_02 (record allowlist + CONFIGMAP_RECORD_ALLOWLIST) +// -------------------------------------------------------------------------- +func TestValidation_F002_UnallowlistedConfigMapKeyApplied(t *testing.T) { + e := newTenantEndpoint(t) + cm := CMHandler{"configmap", e} + + // A ConfigMap in the ATS namespace carrying the trusted annotation, but + // with security-relevant records that no allowlist should permit. + dangerous := map[string]string{ + "proxy.config.ssl.client.verify.server.policy": "DISABLED", // upstream TLS verification off + "proxy.config.http.push_method_enabled": "1", // cache injection via PUSH + } + configMap := v1.ConfigMap{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "tuning", + Namespace: "trafficserver", + Annotations: map[string]string{"ats-configmap": "true"}, + }, + Data: dangerous, + } + cm.Add(&configMap) + + // SECURE invariant: dangerous, non-allowlisted records must NOT reach the + // live ATS configuration. + for k := range dangerous { + if v, err := cm.Ep.ATSManager.ConfigGet(k); err == nil { + t.Errorf("f002 CONFIRMED: unallowlisted record %q=%q pushed to live ATS "+ + "(candidate fix: bug_02)", k, v) + } + } +} diff --git a/watcher/watcher.go b/watcher/watcher.go index c6b8ad2..ee32b72 100644 --- a/watcher/watcher.go +++ b/watcher/watcher.go @@ -65,7 +65,7 @@ type EventHandler interface { // Watch creates necessary threads to watch over resources func (w *Watcher) Watch() error { //================= Watch for Ingress ================== - igHandler := IgHandler{"ingresses", w.Ep} + igHandler := IgHandler{ResourceName: "ingresses", Ep: w.Ep} igListWatch := cache.NewListWatchFromClient(w.Cs.NetworkingV1().RESTClient(), igHandler.GetResourceName(), v1.NamespaceAll, fields.Everything()) err := w.allNamespacesWatchFor(&igHandler, w.Cs.NetworkingV1().RESTClient(), fields.Everything(), &nv1.Ingress{}, w.ResyncPeriod, igListWatch)