Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable user snippets per default #10393

Merged
merged 2 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,4 @@ cmd/plugin/release/ingress-nginx.yaml
cmd/plugin/release/*.tar.gz
cmd/plugin/release/LICENSE
tmp/
test/junitreports/
2 changes: 1 addition & 1 deletion charts/ingress-nginx/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ As of version `1.26.0` of this chart, by simply not providing any clusterIP valu
| controller.admissionWebhooks.service.servicePort | int | `443` | |
| controller.admissionWebhooks.service.type | string | `"ClusterIP"` | |
| controller.affinity | object | `{}` | Affinity and anti-affinity rules for server scheduling to nodes # Ref: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#affinity-and-anti-affinity # |
| controller.allowSnippetAnnotations | bool | `true` | This configuration defines if Ingress Controller should allow users to set their own *-snippet annotations, otherwise this is forbidden / dropped when users add those annotations. Global snippets in ConfigMap are still respected |
| controller.allowSnippetAnnotations | bool | `false` | This configuration defines if Ingress Controller should allow users to set their own *-snippet annotations, otherwise this is forbidden / dropped when users add those annotations. Global snippets in ConfigMap are still respected |
| controller.annotations | object | `{}` | Annotations to be added to the controller Deployment or DaemonSet # |
| controller.autoscaling.annotations | object | `{}` | |
| controller.autoscaling.behavior | object | `{}` | |
Expand Down
2 changes: 1 addition & 1 deletion charts/ingress-nginx/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ controller:
# their own *-snippet annotations, otherwise this is forbidden / dropped
# when users add those annotations.
# Global snippets in ConfigMap are still respected
allowSnippetAnnotations: true
allowSnippetAnnotations: false
# -- Required for use with CNI based kubernetes installations (such as ones set up by kubeadm),
# since CNI and hostport don't mix yet. Can be deprecated once https://github.com/kubernetes/kubernetes/issues/23920
# is merged
Expand Down
4 changes: 2 additions & 2 deletions cmd/plugin/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ import (

// The default deployment and service names for ingress-nginx
const (
DefaultIngressDeploymentName = "ingress-nginx-controller"
DefaultIngressServiceName = "ingress-nginx-controller"
DefaultIngressDeploymentName = "ingress-nginx-controller" //#nosec G101
DefaultIngressServiceName = "ingress-nginx-controller" //#nosec G101
DefaultIngressContainerName = "controller"
)

Expand Down
4 changes: 2 additions & 2 deletions docs/user-guide/nginx-configuration/configmap.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ The following table shows a configuration option's name, type, and the default v
|[add-headers](#add-headers)|string|""||
|[allow-backend-server-header](#allow-backend-server-header)|bool|"false"||
|[allow-cross-namespace-resources](#allow-cross-namespace-resources)|bool|"true"||
|[allow-snippet-annotations](#allow-snippet-annotations)|bool|true||
|[allow-snippet-annotations](#allow-snippet-annotations)|bool|false||
|[annotations-risk-level](#annotations-risk-level)|string|Critical||
|[annotation-value-word-blocklist](#annotation-value-word-blocklist)|string array|""||
|[hide-headers](#hide-headers)|string array|empty||
Expand Down Expand Up @@ -257,7 +257,7 @@ Enables users to consume cross namespace resource on annotations, when was previ

## allow-snippet-annotations

Enables Ingress to parse and add *-snippet annotations/directives created by the user. _**default:**_ `true`
Enables Ingress to parse and add *-snippet annotations/directives created by the user. _**default:**_ `false`

Warning: We recommend enabling this option only if you TRUST users with permission to create Ingress objects, as this
may allow a user to add restricted configurations to the final nginx.conf file
Expand Down
2 changes: 1 addition & 1 deletion internal/ingress/annotations/fastcgi/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import (

const (
fastCGIIndexAnnotation = "fastcgi-index"
fastCGIParamsAnnotation = "fastcgi-params-configmap"
fastCGIParamsAnnotation = "fastcgi-params-configmap" //#nosec G101
)

// fast-cgi valid parameters is just a single file name (like index.php)
Expand Down
4 changes: 2 additions & 2 deletions internal/ingress/annotations/modsecurity/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func TestParse(t *testing.T) {
Spec: networking.IngressSpec{},
}

for _, testCase := range testCases {
for i, testCase := range testCases {
ing.SetAnnotations(testCase.annotations)
result, err := ap.Parse(ing)
if err != nil {
Expand All @@ -77,7 +77,7 @@ func TestParse(t *testing.T) {
if !ok {
t.Errorf("unexpected type: %T", result)
}
if !config.Equal(&testCase.expected) {
if !config.Equal(&testCases[i].expected) {
t.Errorf("expected %v but returned %v, annotations: %s", testCase.expected, result, testCase.annotations)
}
}
Expand Down
2 changes: 1 addition & 1 deletion internal/ingress/annotations/proxy/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ const (
proxyRedirectToAnnotation = "proxy-redirect-to"
proxyBufferingAnnotation = "proxy-buffering"
proxyHTTPVersionAnnotation = "proxy-http-version"
proxyMaxTempFileSizeAnnotation = "proxy-max-temp-file-size"
proxyMaxTempFileSizeAnnotation = "proxy-max-temp-file-size" //#nosec G101
)

var validUpstreamAnnotation = regexp.MustCompile(`^((error|timeout|invalid_header|http_500|http_502|http_503|http_504|http_403|http_404|http_429|non_idempotent|off)\s?)+$`)
Expand Down
4 changes: 2 additions & 2 deletions internal/ingress/annotations/sslcipher/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,13 @@ func TestParse(t *testing.T) {
Spec: networking.IngressSpec{},
}

for _, testCase := range testCases {
for i, testCase := range testCases {
ing.SetAnnotations(testCase.annotations)
result, err := ap.Parse(ing)
if (err != nil) != testCase.expectErr {
t.Fatalf("expected error: %t got error: %t err value: %s. %+v", testCase.expectErr, err != nil, err, testCase.annotations)
}
if !reflect.DeepEqual(result, &testCase.expected) {
if !reflect.DeepEqual(result, &testCases[i].expected) {
t.Errorf("expected %v but returned %v, annotations: %s", testCase.expected, result, testCase.annotations)
}
}
Expand Down
2 changes: 1 addition & 1 deletion internal/ingress/controller/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,7 @@ func NewDefault() Configuration {
defGlobalExternalAuth := GlobalExternalAuth{"", "", "", "", "", append(defResponseHeaders, ""), "", "", "", []string{}, map[string]string{}, false}

cfg := Configuration{
AllowSnippetAnnotations: true,
AllowSnippetAnnotations: false,
AllowCrossNamespaceResources: true,
AllowBackendServerHeader: false,
AnnotationValueWordBlocklist: "",
Expand Down
8 changes: 4 additions & 4 deletions internal/ingress/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -1057,7 +1057,7 @@ func (n *NGINXController) createUpstreams(data []*ingress.Ingress, du *ingress.B
continue
}

for _, path := range rule.HTTP.Paths {
for i, path := range rule.HTTP.Paths {
if path.Backend.Service == nil {
// skip non-service backends
klog.V(3).Infof("Ingress %q and path %q does not contain a service backend, using default backend", ingKey, path.Path)
Expand Down Expand Up @@ -1087,7 +1087,7 @@ func (n *NGINXController) createUpstreams(data []*ingress.Ingress, du *ingress.B

// add the service ClusterIP as a single Endpoint instead of individual Endpoints
if anns.ServiceUpstream {
endpoint, err := n.getServiceClusterEndpoint(svcKey, &path.Backend)
endpoint, err := n.getServiceClusterEndpoint(svcKey, &rule.HTTP.Paths[i].Backend)
if err != nil {
klog.Errorf("Failed to determine a suitable ClusterIP Endpoint for Service %q: %v", svcKey, err)
} else {
Expand Down Expand Up @@ -1844,7 +1844,7 @@ func ingressForHostPath(hostname, path string, servers []*ingress.Server) []*net
continue
}

for _, location := range server.Locations {
for i, location := range server.Locations {
if location.Path != path {
continue
}
Expand All @@ -1853,7 +1853,7 @@ func ingressForHostPath(hostname, path string, servers []*ingress.Server) []*net
continue
}

ingresses = append(ingresses, &location.Ingress.Ingress)
ingresses = append(ingresses, &server.Locations[i].Ingress.Ingress)
}
}

Expand Down
2 changes: 1 addition & 1 deletion internal/ingress/metric/collectors/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@ limitations under the License.
package collectors

// PrometheusNamespace default metric namespace
var PrometheusNamespace = "nginx_ingress_controller"
var PrometheusNamespace = "nginx_ingress_controller" //#nosec G101
2 changes: 1 addition & 1 deletion internal/net/ssl/ssl.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ var FakeSSLCertificateUID = "00000000-0000-0000-0000-000000000000"
var oidExtensionSubjectAltName = asn1.ObjectIdentifier{2, 5, 29, 17}

const (
fakeCertificateName = "default-fake-certificate"
fakeCertificateName = "default-fake-certificate" //#nosec G101
)

// getPemFileName returns absolute file path and file name of pem cert related to given fullSecretName
Expand Down
Loading
Loading