diff --git a/pkg/linters/templates/README.md b/pkg/linters/templates/README.md index 2b92d784..dc5522ed 100644 --- a/pkg/linters/templates/README.md +++ b/pkg/linters/templates/README.md @@ -14,7 +14,7 @@ Proper template validation prevents runtime issues, ensures applications are pro | [pdb](#pdb) | Validates PodDisruptionBudgets for deployments and statefulsets | ✅ | enabled | | [kube-rbac-proxy](#kube-rbac-proxy) | Validates kube-rbac-proxy CA certificates in namespaces | ✅ | enabled | | [service-port](#service-port) | Validates services use named target ports | ✅ | enabled | -| [ingress-rules](#ingress-rules) | Validates Ingress configuration snippets | ✅ | enabled | +| [ingress-rules](#ingress-rules) | Rejects unsafe Ingress snippet annotations and validates HSTS | ✅ | enabled | | [httproute-rules](#httproute-rules) | Validates that every Ingress has a companion HTTPRoute backed by a ListenerSet | ✅ | enabled | | [prometheus-rules](#prometheus-rules) | Validates Prometheus rules with promtool and proper templates | ✅ | enabled | | [grafana-dashboards](#grafana-dashboards) | Validates Grafana dashboard templates | ✅ | enabled | @@ -885,29 +885,47 @@ linters-settings: ### ingress-rules -**Purpose:** Ensures Ingress resources include required security configuration snippets, specifically the Strict-Transport-Security (HSTS) header for enforcing HTTPS connections. +**Purpose:** Reports unsafe ingress-nginx snippet annotations and ensures that +Ingresses using `configuration-snippet` preserve HSTS during migration. **Description:** -Validates that Ingress objects with `nginx.ingress.kubernetes.io/configuration-snippet` annotation contain the required HSTS header configuration using the `helm_lib_module_ingress_configuration_snippet` helper. +The rule reports an error when an Ingress uses any of these Critical +annotations: + +- `nginx.ingress.kubernetes.io/configuration-snippet`; +- `nginx.ingress.kubernetes.io/server-snippet`; +- `nginx.ingress.kubernetes.io/auth-snippet`; +- `nginx.ingress.kubernetes.io/modsecurity-snippet`; +- `nginx.ingress.kubernetes.io/stream-snippet`. + +These annotations allow arbitrary NGINX configuration and require manual +migration. The rule does not inspect arbitrary directives or suggest +replacements for them. **What it checks:** -1. Ingresses with `nginx.ingress.kubernetes.io/configuration-snippet` annotation -2. Configuration snippet contains `add_header Strict-Transport-Security` -3. Recommends using `helm_lib_module_ingress_configuration_snippet` helper +1. Every unsafe snippet annotation produces a migration error. +2. An Ingress using `configuration-snippet` must also preserve HSTS through + either: + - `nginx.ingress.kubernetes.io/ingress-nginx-hsts: "true"` (preferred), or + - the canonical legacy HSTS directive produced by + `helm_lib_module_ingress_configuration_snippet` and shown below. +3. The canonical legacy HSTS directive is accepted during migration, but the + unsafe annotation error remains. **Why it matters:** -HSTS (HTTP Strict-Transport-Security): -- Forces browsers to use HTTPS only -- Prevents protocol downgrade attacks -- Protects against man-in-the-middle attacks -- Required for security compliance +- Snippet annotations require unsafe ingress-nginx controller options and allow + arbitrary NGINX directives. +- The dedicated HSTS annotation preserves the required fixed policy without + enabling arbitrary snippets. +- Keeping the legacy HSTS check during migration avoids weakening existing + Ingress security. **Examples:** -❌ **Incorrect** - Missing HSTS header: +❌ **Unsafe and missing HSTS:** ```yaml # templates/ingress.yaml @@ -934,13 +952,10 @@ spec: name: http ``` -**Error:** -``` -Error: Ingress annotation "nginx.ingress.kubernetes.io/configuration-snippet" does not contain required snippet "{{ include "helm_lib_module_ingress_configuration_snippet" . | nindent 6 }}". -Object: dashboard -``` +This produces an error for the unsafe annotation and another error because +neither the preferred nor legacy HSTS configuration is present. -✅ **Correct** - Using Helm library helper: +⚠️ **Legacy HSTS during migration:** ```yaml # templates/ingress.yaml @@ -968,13 +983,24 @@ spec: name: http ``` -The helper includes the HSTS header: +The helper renders the legacy HSTS header: ```nginx add_header Strict-Transport-Security "max-age=31536000; includeSubDomains" always; ``` -✅ **Correct** - Ingress without configuration-snippet (not checked): +This temporarily satisfies the HSTS check, but the unsafe annotation error +remains. + +✅ **Preferred HSTS configuration:** + +```yaml +metadata: + annotations: + nginx.ingress.kubernetes.io/ingress-nginx-hsts: "true" +``` + +✅ **Ingress without snippet annotations:** ```yaml # templates/ingress.yaml @@ -2658,26 +2684,33 @@ Object: namespace = d8-my-module - d8-my-module ``` -### Issue: Ingress missing HSTS configuration +### Issue: Unsafe Ingress annotation or missing HSTS configuration **Symptom:** ``` -Error: Ingress annotation "nginx.ingress.kubernetes.io/configuration-snippet" does not contain required snippet +Error: Ingress annotation "nginx.ingress.kubernetes.io/configuration-snippet" is unsafe and requires manual migration. +Error: Ingress annotation "nginx.ingress.kubernetes.io/configuration-snippet" requires annotation "nginx.ingress.kubernetes.io/ingress-nginx-hsts" to be set to "true" to preserve HSTS. ``` -**Cause:** Ingress configuration-snippet missing Strict-Transport-Security header. +**Cause:** The Ingress uses an unsafe snippet annotation. A +`configuration-snippet` without either the dedicated HSTS annotation or the +legacy canonical HSTS directive also risks losing HSTS during migration. **Solutions:** -1. **Use Helm helper:** +1. **Preserve HSTS with the safe annotation:** ```yaml annotations: - nginx.ingress.kubernetes.io/configuration-snippet: | -{{- include "helm_lib_module_ingress_configuration_snippet" . | nindent 6 }} + nginx.ingress.kubernetes.io/ingress-nginx-hsts: "true" ``` -2. **Exclude Ingress:** +2. **Migrate the remaining snippet directives to safe annotations or other + Kubernetes resources, then remove the unsafe snippet annotation.** Adding + the HSTS annotation resolves only the HSTS error; the unsafe annotation + error remains until the snippet is removed or excluded. + +3. **Temporarily exclude the Ingress if no safe migration is available:** ```yaml # .dmtlint.yaml diff --git a/pkg/linters/templates/rules/ingress.go b/pkg/linters/templates/rules/ingress.go index 2385387c..936107aa 100644 --- a/pkg/linters/templates/rules/ingress.go +++ b/pkg/linters/templates/rules/ingress.go @@ -32,10 +32,21 @@ import ( ) const ( - IngressRuleName = "ingress-rules" - snippet = `{{ include "helm_lib_module_ingress_configuration_snippet" . | nindent 6 }}` + IngressRuleName = "ingress-rules" + nginxAnnotationPrefix = "nginx.ingress.kubernetes.io/" + configurationSnippetAnnotation = nginxAnnotationPrefix + "configuration-snippet" + ingressNginxHSTSAnnotation = nginxAnnotationPrefix + "ingress-nginx-hsts" + legacyHSTSDirective = `add_header Strict-Transport-Security "max-age=31536000; includeSubDomains" always;` ) +var unsafeIngressAnnotations = []string{ + configurationSnippetAnnotation, + nginxAnnotationPrefix + "server-snippet", + nginxAnnotationPrefix + "auth-snippet", + nginxAnnotationPrefix + "modsecurity-snippet", + nginxAnnotationPrefix + "stream-snippet", +} + type IngressRule struct { pkg.RuleMeta pkg.KindRule @@ -87,12 +98,37 @@ func (r *IngressRule) checkObject(object storage.StoreObject) { return } - for key, value := range ingress.GetAnnotations() { - if key == "nginx.ingress.kubernetes.io/configuration-snippet" { - if !strings.Contains(value, "add_header Strict-Transport-Security") { - errorList.WithObjectID(object.Unstructured.GetName()). - Errorf("Ingress annotation %q does not contain required snippet %q.", key, snippet) - } + annotations := ingress.GetAnnotations() + objectErrors := errorList.WithObjectID(object.Unstructured.GetName()) + + for _, annotation := range unsafeIngressAnnotations { + if _, found := annotations[annotation]; !found { + continue } + + objectErrors.Errorf("Ingress annotation %q is unsafe and requires manual migration.", annotation) } + + configurationSnippet, found := annotations[configurationSnippetAnnotation] + if !found { + return + } + + hasSafeHSTS := annotations[ingressNginxHSTSAnnotation] == "true" + + hasLegacyHSTS := hasLegacyHSTSDirective(configurationSnippet) + if !hasSafeHSTS && !hasLegacyHSTS { + objectErrors.Errorf("Ingress annotation %q requires annotation %q to be set to %q to preserve HSTS.", + configurationSnippetAnnotation, ingressNginxHSTSAnnotation, "true") + } +} + +func hasLegacyHSTSDirective(configurationSnippet string) bool { + for line := range strings.SplitSeq(configurationSnippet, "\n") { + if strings.TrimSpace(line) == legacyHSTSDirective { + return true + } + } + + return false } diff --git a/pkg/linters/templates/rules/ingress_test.go b/pkg/linters/templates/rules/ingress_test.go new file mode 100644 index 00000000..45144df1 --- /dev/null +++ b/pkg/linters/templates/rules/ingress_test.go @@ -0,0 +1,207 @@ +/* +Copyright 2026 Flant JSC + +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. +*/ + +package rules + +import ( + "testing" + + "github.com/gojuno/minimock/v3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/deckhouse/dmt/internal/mocks" + "github.com/deckhouse/dmt/internal/storage" + "github.com/deckhouse/dmt/pkg" + "github.com/deckhouse/dmt/pkg/errors" +) + +func TestIngressRuleReportsUnsafeAnnotationsAsErrors(t *testing.T) { + for _, annotation := range unsafeIngressAnnotations { + t.Run(annotation, func(t *testing.T) { + value := "value" + if annotation == configurationSnippetAnnotation { + value = legacyHSTSDirective + } + + findings := runIngressRule(t, "Ingress", map[string]string{annotation: value}, nil) + + require.Len(t, findings, 1) + assert.Equal(t, pkg.Error, findings[0].Level) + assert.Contains(t, findings[0].Text, annotation) + }) + } +} + +func TestIngressRuleHSTSCompatibility(t *testing.T) { + tests := []struct { + name string + annotations map[string]string + wantErrors int + wantHSTSError bool + }{ + { + name: "safe HSTS annotation", + annotations: map[string]string{ + configurationSnippetAnnotation: "proxy_set_header X-Test value;", + ingressNginxHSTSAnnotation: "true", + }, + wantErrors: 1, + }, + { + name: "legacy HSTS directive", + annotations: map[string]string{ + configurationSnippetAnnotation: " " + legacyHSTSDirective, + }, + wantErrors: 1, + }, + { + name: "commented legacy HSTS directive", + annotations: map[string]string{ + configurationSnippetAnnotation: "# " + legacyHSTSDirective, + }, + wantErrors: 2, + wantHSTSError: true, + }, + { + name: "legacy HSTS directive disables HSTS", + annotations: map[string]string{ + configurationSnippetAnnotation: `add_header Strict-Transport-Security "max-age=0" always;`, + }, + wantErrors: 2, + wantHSTSError: true, + }, + { + name: "report-only header is not HSTS", + annotations: map[string]string{ + configurationSnippetAnnotation: `add_header Strict-Transport-Security-Report-Only "max-age=31536000" always;`, + }, + wantErrors: 2, + wantHSTSError: true, + }, + { + name: "HSTS is missing", + annotations: map[string]string{ + configurationSnippetAnnotation: "proxy_set_header X-Test value;", + }, + wantErrors: 2, + wantHSTSError: true, + }, + { + name: "configuration snippet is empty", + annotations: map[string]string{ + configurationSnippetAnnotation: "", + }, + wantErrors: 2, + wantHSTSError: true, + }, + { + name: "safe HSTS annotation is false", + annotations: map[string]string{ + configurationSnippetAnnotation: "proxy_set_header X-Test value;", + ingressNginxHSTSAnnotation: "false", + }, + wantErrors: 2, + wantHSTSError: true, + }, + { + name: "safe HSTS annotation is empty", + annotations: map[string]string{ + configurationSnippetAnnotation: "proxy_set_header X-Test value;", + ingressNginxHSTSAnnotation: "", + }, + wantErrors: 2, + wantHSTSError: true, + }, + { + name: "safe HSTS annotation has another value", + annotations: map[string]string{ + configurationSnippetAnnotation: "proxy_set_header X-Test value;", + ingressNginxHSTSAnnotation: "TRUE", + }, + wantErrors: 2, + wantHSTSError: true, + }, + { + name: "safe annotations without configuration snippet", + annotations: map[string]string{ + ingressNginxHSTSAnnotation: "true", + nginxAnnotationPrefix + "proxy-ssl-use-controller-certificate": "true", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + findings := runIngressRule(t, "Ingress", tt.annotations, nil) + + require.Len(t, findings, tt.wantErrors) + + for _, finding := range findings { + assert.Equal(t, pkg.Error, finding.Level) + } + + if tt.wantHSTSError { + assert.Contains(t, findings[len(findings)-1].Text, ingressNginxHSTSAnnotation) + } + }) + } +} + +func TestIngressRuleReportsUnsafeAnnotationsInStableOrder(t *testing.T) { + findings := runIngressRule(t, "Ingress", map[string]string{ + nginxAnnotationPrefix + "stream-snippet": "stream {}", + nginxAnnotationPrefix + "server-snippet": "return 200;", + }, nil) + + require.Len(t, findings, 2) + assert.Contains(t, findings[0].Text, nginxAnnotationPrefix+"server-snippet") + assert.Contains(t, findings[1].Text, nginxAnnotationPrefix+"stream-snippet") +} + +func TestIngressRuleSkipsExcludedAndNonIngressResources(t *testing.T) { + exclude := []pkg.KindRuleExclude{{Kind: "Ingress", Name: "test"}} + annotations := map[string]string{configurationSnippetAnnotation: "value"} + + assert.Empty(t, runIngressRule(t, "Ingress", annotations, exclude)) + assert.Empty(t, runIngressRule(t, "Deployment", annotations, nil)) +} + +func runIngressRule(t *testing.T, kind string, annotations map[string]string, exclude []pkg.KindRuleExclude) []pkg.LinterError { + t.Helper() + + object := unstructured.Unstructured{} + object.SetAPIVersion("networking.k8s.io/v1") + object.SetKind(kind) + object.SetName("test") + object.SetAnnotations(annotations) + + objects := map[storage.ResourceIndex]storage.StoreObject{ + {Kind: kind, Name: object.GetName()}: { + Unstructured: object, + AbsPath: "/test/ingress.yaml", + }, + } + + module := mocks.NewModuleMock(minimock.NewController(t)) + module.GetStorageMock.Return(objects) + + errorList := errors.NewLintRuleErrorsList() + NewIngressRule(exclude, module, errorList).Check(t.Context()) + + return errorList.GetErrors() +} diff --git a/test/e2e/README.md b/test/e2e/README.md index 737301c0..0acc185f 100644 --- a/test/e2e/README.md +++ b/test/e2e/README.md @@ -132,7 +132,7 @@ go test ./test/e2e/ -run 'TestE2E//' -v | `templates/vpa-misconfigured` | `vpa` (updateMode `Auto`, missing `resourcePolicy.containerPolicies`) | | `templates/pdb-mismatch` | `pdb` (PDB selector does not match controller pod labels) | | `templates/pdb-helm-hook` | `pdb` (PDB carries helm hook annotations) | -| `templates/ingress-snippet` | `ingress-rules` (configuration-snippet missing HSTS) | +| `templates/ingress-snippet` | `ingress-rules` (unsafe configuration-snippet error and missing HSTS error) | | `templates/monitoring-missing-yaml` | `prometheus-rules` + `grafana-dashboards` (monitoring/ without templates/monitoring.yaml) | | `templates/grafana-dashboard` | `grafana-dashboards` (deprecated panel type, missing prometheus datasource variable) | | `templates/prometheus-promtool` | `prometheus-rules` (invalid PromQL via promtool) | diff --git a/test/e2e/testdata/templates/ingress-snippet/expected.yaml b/test/e2e/testdata/templates/ingress-snippet/expected.yaml index 266218e2..ea3e766a 100644 --- a/test/e2e/testdata/templates/ingress-snippet/expected.yaml +++ b/test/e2e/testdata/templates/ingress-snippet/expected.yaml @@ -1,9 +1,13 @@ description: > - An Ingress whose configuration-snippet annotation does not include the - required HSTS snippet must be flagged by the ingress-rules rule. + An Ingress with an unsafe configuration-snippet and no HSTS protection must + produce a migration error and an HSTS error. module: module expect: - linter: templates rule: ingress-rules level: error - textContains: "does not contain required snippet" + textContains: "is unsafe and requires manual migration" + - linter: templates + rule: ingress-rules + level: error + textContains: "to preserve HSTS"