Skip to content

Commit 142c117

Browse files
eliottnesseliottness
committed
fix(appsec/nginx): harden security, error handling, and test coverage
Address review findings: validate moduleMountPath against nginx directive injection, add SecurityContext to init container (PSA restricted-compliant), propagate reconciler Start() failure, handle AddFunc events after restart, emit K8s events on permanent reconciliation failure, aggregate ConfigMap deletion errors, and add constructor nil guards. Add 10 new tests covering Deleted() cleanup, ConfigMap update retry loops, and reconciler reconcile paths. Fix doc.go inaccuracies around marker-based injection and sidecar terminology.
1 parent 6cfe461 commit 142c117

10 files changed

Lines changed: 694 additions & 30 deletions

File tree

‎pkg/clusteragent/appsec/config/config.go‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,31 @@ type Config struct {
160160
Product
161161
}
162162

163+
// validateNginxConfig validates that required nginx configuration fields are set
164+
// and that paths do not contain characters that could lead to nginx directive injection.
165+
func validateNginxConfig(config Nginx) error {
166+
var errs []error
167+
168+
if config.InitImage == "" {
169+
errs = append(errs, errors.New("nginx.init_image is required"))
170+
} else if strings.ContainsAny(config.InitImage, ";\n\r\t{}") {
171+
errs = append(errs, fmt.Errorf("nginx.init_image contains invalid characters: %q", config.InitImage))
172+
}
173+
174+
if config.ModuleMountPath == "" {
175+
errs = append(errs, errors.New("nginx.module_mount_path is required"))
176+
} else {
177+
if !strings.HasPrefix(config.ModuleMountPath, "/") {
178+
errs = append(errs, fmt.Errorf("nginx.module_mount_path must be an absolute path, got: %q", config.ModuleMountPath))
179+
}
180+
if strings.ContainsAny(config.ModuleMountPath, ";\n\r \t{}") {
181+
errs = append(errs, fmt.Errorf("nginx.module_mount_path contains invalid characters: %q", config.ModuleMountPath))
182+
}
183+
}
184+
185+
return errors.Join(errs...)
186+
}
187+
163188
// validateSidecarConfig validates that required sidecar configuration fields are set
164189
func validateSidecarConfig(config Sidecar) error {
165190
var errs []error
@@ -245,10 +270,12 @@ func FromComponent(cfg config.Component, logger log.Component) Config {
245270
fallthrough
246271
case InjectionModeSidecar:
247272
staticAnnotations[AppsecProcessorResourceAnnotation] = "localhost"
248-
// Validate required sidecar configuration
249273
if err := validateSidecarConfig(sidecarConfig); err != nil {
250274
logger.Errorf("Invalid sidecar configuration: %v", err)
251275
}
276+
if err := validateNginxConfig(nginxConfig); err != nil {
277+
logger.Errorf("Invalid nginx configuration: %v", err)
278+
}
252279
case InjectionModeExternal:
253280
staticAnnotations[AppsecProcessorResourceAnnotation] = processor.String()
254281
// Validate required external configuration

‎pkg/clusteragent/appsec/doc.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ The appsec injector follows a controller pattern with these key components:
2828
- InjectionPattern: An interface for proxy-specific injection implementations
2929
- ProxyDetector: Auto-detection of supported proxies in the cluster
3030
- Configuration: Unified configuration for processor deployment and injection
31+
- ConfigMapReconciler: Watches original ConfigMaps for changes and re-syncs DD-owned copies (nginx only)
3132
3233
The controller watches proxy resources using Kubernetes informers and maintains a work queue for
3334
processing add/modify/delete events with exponential backoff retry logic.
@@ -45,7 +46,7 @@ Currently supported proxy types:
4546
- SIDECAR mode: Currently unsupported due to Kubernetes validation constraints on localhost references
4647
4748
- ingress-nginx (ProxyTypeIngressNginx): Injects the nginx-datadog WAF module (.so) into controller pods
48-
- SIDECAR mode only: Adds init container + emptyDir volume, redirects --configmap to DD-owned ConfigMap
49+
- Pod mutation mode only (uses init container, not a running sidecar): Adds init container + emptyDir volume, redirects --configmap to DD-owned ConfigMap
4950
- Auto-detects via IngressClass with spec.controller == "k8s.io/ingress-nginx"
5051
- Version detection from controller image tag for matching init container image
5152
@@ -371,9 +372,9 @@ spec.controller == "k8s.io/ingress-nginx".
371372
372373
The DD-owned ConfigMap:
373374
- Copies all keys from the original ConfigMap verbatim
374-
- Prepends load_module + thread_pool + env DD_AGENT_HOST to main-snippet
375-
- Prepends datadog_appsec_enabled + datadog_waf_thread_pool_name to http-snippet
376-
- Uses comment markers for idempotent injection
375+
- Injects load_module + thread_pool + env DD_AGENT_HOST into main-snippet using idempotent comment markers
376+
- Injects datadog_appsec_enabled + datadog_waf_thread_pool_name into http-snippet using idempotent comment markers
377+
- Comment markers (# datadog-appsec-begin / # datadog-appsec-end) enable safe re-application and clean removal
377378
- Has an ownerReference to the original ConfigMap for garbage collection
378379
- Is labeled with appsec.datadoghq.com/proxy-type=ingress-nginx for cleanup
379380

‎pkg/clusteragent/appsec/injector.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,8 @@ func (si *securityInjector) runLeader(ctx context.Context, proxyType appsecconfi
264264

265265
if s, ok := pattern.(appsecconfig.Starter); ok {
266266
if err := s.Start(ctx); err != nil {
267-
si.logger.Warnf("error starting reconciler for %s: %v", proxyType, err)
267+
si.logger.Errorf("failed to start reconciler for %s: %v", proxyType, err)
268+
return fmt.Errorf("failed to start reconciler for %s: %w", proxyType, err)
268269
}
269270
}
270271

‎pkg/clusteragent/appsec/nginx/configmap_reconciler.go‎

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
log "github.com/DataDog/datadog-agent/comp/core/log/def"
1717
appsecconfig "github.com/DataDog/datadog-agent/pkg/clusteragent/appsec/config"
1818

19+
corev1 "k8s.io/api/core/v1"
1920
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2021
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2122
"k8s.io/client-go/dynamic"
@@ -30,9 +31,10 @@ import (
3031
// It keeps no in-memory state; all context is derived from ConfigMap
3132
// labels and annotations at reconcile time.
3233
type configMapReconciler struct {
33-
client dynamic.Interface
34-
logger log.Component
35-
config appsecconfig.Config
34+
client dynamic.Interface
35+
logger log.Component
36+
config appsecconfig.Config
37+
eventRecorder eventRecorder
3638
}
3739

3840
type reconcileItem struct {
@@ -59,17 +61,20 @@ func (r *configMapReconciler) Start(ctx context.Context) error {
5961

6062
informer := informerFactory.ForResource(configMapGVR).Informer()
6163

64+
enqueue := func(obj any) {
65+
u, ok := obj.(*unstructured.Unstructured)
66+
if !ok {
67+
return
68+
}
69+
queue.Add(reconcileItem{
70+
namespace: u.GetNamespace(),
71+
originalName: u.GetName(),
72+
})
73+
}
74+
6275
if _, err := informer.AddEventHandler(cache.ResourceEventHandlerFuncs{
63-
UpdateFunc: func(_, newObj any) {
64-
u, ok := newObj.(*unstructured.Unstructured)
65-
if !ok {
66-
return
67-
}
68-
queue.Add(reconcileItem{
69-
namespace: u.GetNamespace(),
70-
originalName: u.GetName(),
71-
})
72-
},
76+
AddFunc: enqueue,
77+
UpdateFunc: func(_, newObj any) { enqueue(newObj) },
7378
}); err != nil {
7479
queue.ShutDown()
7580
return fmt.Errorf("failed to add ConfigMap reconciler event handler: %w", err)
@@ -140,7 +145,19 @@ func (r *configMapReconciler) reconcile(ctx context.Context, queue workqueue.Typ
140145
if queue.NumRequeues(item) < 5 {
141146
queue.AddRateLimited(item)
142147
} else {
143-
r.logger.Errorf("ConfigMap reconciler: giving up on %s/%s after retries", item.namespace, item.originalName)
148+
r.logger.Errorf("ConfigMap reconciler: giving up on %s/%s after retries: %v", item.namespace, item.originalName, err)
149+
r.eventRecorder.recorder.Eventf(
150+
&corev1.ObjectReference{
151+
Kind: "ConfigMap",
152+
Name: item.originalName,
153+
Namespace: item.namespace,
154+
APIVersion: "v1",
155+
},
156+
corev1.EventTypeWarning,
157+
"ReconciliationFailed",
158+
"Failed to reconcile DD ConfigMap after retries: %v",
159+
err,
160+
)
144161
queue.Forget(item)
145162
}
146163
return

‎pkg/clusteragent/appsec/nginx/configmap_reconciler_test.go‎

Lines changed: 222 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,222 @@
1+
// Unless explicitly stated otherwise all files in this repository are licensed
2+
// under the Apache License Version 2.0.
3+
// This product includes software developed at Datadog (https://www.datadoghq.com/).
4+
// Copyright 2025-present Datadog, Inc.
5+
6+
//go:build kubeapiserver && test
7+
8+
package nginx
9+
10+
import (
11+
"testing"
12+
13+
"github.com/stretchr/testify/assert"
14+
"github.com/stretchr/testify/require"
15+
16+
logmock "github.com/DataDog/datadog-agent/comp/core/log/mock"
17+
appsecconfig "github.com/DataDog/datadog-agent/pkg/clusteragent/appsec/config"
18+
19+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
20+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
21+
"k8s.io/apimachinery/pkg/runtime"
22+
"k8s.io/apimachinery/pkg/runtime/schema"
23+
dynamicfake "k8s.io/client-go/dynamic/fake"
24+
"k8s.io/client-go/tools/record"
25+
"k8s.io/client-go/util/workqueue"
26+
)
27+
28+
func newTestConfigMapReconciler(t *testing.T, objects ...runtime.Object) (*configMapReconciler, *dynamicfake.FakeDynamicClient) {
29+
t.Helper()
30+
31+
logger := logmock.New(t)
32+
scheme := runtime.NewScheme()
33+
client := dynamicfake.NewSimpleDynamicClientWithCustomListKinds(scheme,
34+
map[schema.GroupVersionResource]string{
35+
configMapGVR: "ConfigMapList",
36+
ingressClassGVR: "IngressClassList",
37+
},
38+
objects...,
39+
)
40+
41+
config := appsecconfig.Config{
42+
Product: appsecconfig.Product{
43+
Nginx: appsecconfig.Nginx{
44+
ModuleMountPath: "/modules_mount",
45+
},
46+
},
47+
Injection: appsecconfig.Injection{
48+
CommonLabels: map[string]string{
49+
"app.kubernetes.io/part-of": "datadog",
50+
"app.kubernetes.io/component": "datadog-appsec-injector",
51+
"app.kubernetes.io/managed-by": "datadog-cluster-agent",
52+
},
53+
CommonAnnotations: map[string]string{
54+
"annotation": "value",
55+
},
56+
},
57+
}
58+
59+
return &configMapReconciler{
60+
client: client,
61+
logger: logger,
62+
config: config,
63+
eventRecorder: eventRecorder{
64+
recorder: record.NewFakeRecorder(100),
65+
},
66+
}, client
67+
}
68+
69+
func TestReconcilerReconcile_Success(t *testing.T) {
70+
ctx := t.Context()
71+
originalName := "ingress-nginx-controller"
72+
ddName := ddConfigMapName(originalName)
73+
originalCM := &unstructured.Unstructured{
74+
Object: map[string]interface{}{
75+
"apiVersion": "v1",
76+
"kind": "ConfigMap",
77+
"metadata": map[string]interface{}{
78+
"name": originalName,
79+
"namespace": "ingress-nginx",
80+
"uid": "original-uid",
81+
"labels": map[string]interface{}{
82+
watchedConfigMapLabel: "true",
83+
},
84+
"annotations": map[string]interface{}{
85+
ddConfigMapAnnotation: ddName,
86+
},
87+
},
88+
"data": map[string]interface{}{
89+
mainSnippetKey: "worker_connections 4096;",
90+
httpSnippetKey: "keepalive_timeout 75;",
91+
},
92+
},
93+
}
94+
existingDDCM := &unstructured.Unstructured{
95+
Object: map[string]interface{}{
96+
"apiVersion": "v1",
97+
"kind": "ConfigMap",
98+
"metadata": map[string]interface{}{
99+
"name": ddName,
100+
"namespace": "ingress-nginx",
101+
"resourceVersion": "1",
102+
},
103+
"data": map[string]interface{}{
104+
mainSnippetKey: "stale-main-snippet",
105+
},
106+
},
107+
}
108+
109+
r, client := newTestConfigMapReconciler(t, originalCM, existingDDCM)
110+
queue := workqueue.NewTypedRateLimitingQueue(workqueue.DefaultTypedItemBasedRateLimiter[reconcileItem]())
111+
defer queue.ShutDown()
112+
113+
item := reconcileItem{namespace: "ingress-nginx", originalName: originalName}
114+
queue.Add(item)
115+
received, quit := queue.Get()
116+
require.False(t, quit)
117+
require.Equal(t, item, received)
118+
119+
r.reconcile(ctx, queue, received)
120+
assert.Equal(t, 0, queue.NumRequeues(item))
121+
122+
ddCM, err := client.Resource(configMapGVR).Namespace("ingress-nginx").Get(ctx, ddName, metav1.GetOptions{})
123+
require.NoError(t, err)
124+
assert.Equal(t, "value", ddCM.GetAnnotations()["annotation"])
125+
assert.Equal(t, "datadog", ddCM.GetLabels()["app.kubernetes.io/part-of"])
126+
assert.Equal(t, string(appsecconfig.ProxyTypeIngressNginx), ddCM.GetLabels()[appsecconfig.AppsecProcessorProxyTypeAnnotation])
127+
data, found, err := unstructured.NestedStringMap(ddCM.UnstructuredContent(), "data")
128+
require.NoError(t, err)
129+
require.True(t, found)
130+
assert.Contains(t, data[mainSnippetKey], "load_module /modules_mount/ngx_http_datadog_module.so;")
131+
assert.Contains(t, data[mainSnippetKey], "worker_connections 4096;")
132+
assert.Contains(t, data[httpSnippetKey], "datadog_appsec_enabled on;")
133+
assert.Contains(t, data[httpSnippetKey], "keepalive_timeout 75;")
134+
}
135+
136+
func TestReconcilerReconcile_MissingAnnotation(t *testing.T) {
137+
ctx := t.Context()
138+
originalName := "ingress-nginx-controller"
139+
originalCM := &unstructured.Unstructured{
140+
Object: map[string]interface{}{
141+
"apiVersion": "v1",
142+
"kind": "ConfigMap",
143+
"metadata": map[string]interface{}{
144+
"name": originalName,
145+
"namespace": "ingress-nginx",
146+
"labels": map[string]interface{}{
147+
watchedConfigMapLabel: "true",
148+
},
149+
},
150+
},
151+
}
152+
153+
r, client := newTestConfigMapReconciler(t, originalCM)
154+
queue := workqueue.NewTypedRateLimitingQueue(workqueue.DefaultTypedItemBasedRateLimiter[reconcileItem]())
155+
defer queue.ShutDown()
156+
157+
item := reconcileItem{namespace: "ingress-nginx", originalName: originalName}
158+
queue.Add(item)
159+
received, quit := queue.Get()
160+
require.False(t, quit)
161+
162+
r.reconcile(ctx, queue, received)
163+
assert.Equal(t, 0, queue.NumRequeues(item))
164+
165+
_, err := client.Resource(configMapGVR).Namespace("ingress-nginx").Get(ctx, ddConfigMapName(originalName), metav1.GetOptions{})
166+
assert.Error(t, err)
167+
}
168+
169+
func TestReconcilerReconcile_AnnotationMismatch(t *testing.T) {
170+
ctx := t.Context()
171+
originalName := "ingress-nginx-controller"
172+
mismatchedDDName := ddConfigMapName("different-original")
173+
originalCM := &unstructured.Unstructured{
174+
Object: map[string]interface{}{
175+
"apiVersion": "v1",
176+
"kind": "ConfigMap",
177+
"metadata": map[string]interface{}{
178+
"name": originalName,
179+
"namespace": "ingress-nginx",
180+
"labels": map[string]interface{}{
181+
watchedConfigMapLabel: "true",
182+
},
183+
"annotations": map[string]interface{}{
184+
ddConfigMapAnnotation: mismatchedDDName,
185+
},
186+
},
187+
},
188+
}
189+
existingDDCM := &unstructured.Unstructured{
190+
Object: map[string]interface{}{
191+
"apiVersion": "v1",
192+
"kind": "ConfigMap",
193+
"metadata": map[string]interface{}{
194+
"name": mismatchedDDName,
195+
"namespace": "ingress-nginx",
196+
"resourceVersion": "1",
197+
},
198+
"data": map[string]interface{}{
199+
mainSnippetKey: "unchanged",
200+
},
201+
},
202+
}
203+
204+
r, client := newTestConfigMapReconciler(t, originalCM, existingDDCM)
205+
queue := workqueue.NewTypedRateLimitingQueue(workqueue.DefaultTypedItemBasedRateLimiter[reconcileItem]())
206+
defer queue.ShutDown()
207+
208+
item := reconcileItem{namespace: "ingress-nginx", originalName: originalName}
209+
queue.Add(item)
210+
received, quit := queue.Get()
211+
require.False(t, quit)
212+
213+
r.reconcile(ctx, queue, received)
214+
assert.Equal(t, 0, queue.NumRequeues(item))
215+
216+
ddCM, err := client.Resource(configMapGVR).Namespace("ingress-nginx").Get(ctx, mismatchedDDName, metav1.GetOptions{})
217+
require.NoError(t, err)
218+
data, found, err := unstructured.NestedStringMap(ddCM.UnstructuredContent(), "data")
219+
require.NoError(t, err)
220+
require.True(t, found)
221+
assert.Equal(t, "unchanged", data[mainSnippetKey])
222+
}

0 commit comments

Comments
 (0)