Skip to content

Security: Follow-up on recent changes. #11873

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

Security: Follow-up on recent changes. #11873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
037e273
GitHub: Align Kubernetes version matrix.
Gacko Aug 26, 2024
9541c3b
Flags: Update `enable-annotation-validation` docs.
Gacko Aug 26, 2024
f4375ad
Chart: Fix `controller.enableAnnotationValidations`.
Gacko Aug 26, 2024
d27b270
Docs: Fix `allow-cross-namespace-resources` default value.
Gacko Aug 26, 2024
893e44d
Docs: Fix `annotations-risk-level` default value.
Gacko Aug 26, 2024
73b0aec
Docs: Fix `strict-validate-path-type` default value.
Gacko Aug 26, 2024
3f864b0
Chart: Do not render disabled `controller.allowSnippetAnnotations`.
Gacko Aug 26, 2024
323098d
Docs: Remove `allow-snippet-annotations` hint.
Gacko Aug 26, 2024
dd44c5d
Tests: Use `framework.AllowSnippetConfiguration`.
Gacko Aug 26, 2024
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
2 changes: 1 addition & 1 deletion .github/workflows/images.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ jobs:
(needs.changes.outputs.kube-webhook-certgen == 'true')
strategy:
matrix:
k8s: [v1.26.15, v1.27.13, v1.28.9, v1.29.4, v1.30.0]
k8s: [v1.28.13, v1.29.8, v1.30.4, v1.31.0]
steps:
- name: Checkout
uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
Expand Down
4 changes: 2 additions & 2 deletions charts/ingress-nginx/templates/_params.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{{- define "ingress-nginx.params" -}}
- /nginx-ingress-controller
{{- if .Values.controller.enableAnnotationValidations }}
- --enable-annotation-validation=true
{{- if not .Values.controller.enableAnnotationValidations }}
- --enable-annotation-validation=false
{{- end }}
{{- if .Values.defaultBackend.enabled }}
- --default-backend-service=$(POD_NAMESPACE)/{{ include "ingress-nginx.defaultBackend.fullname" . }}
Expand Down
4 changes: 3 additions & 1 deletion charts/ingress-nginx/templates/controller-configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ metadata:
name: {{ include "ingress-nginx.controller.fullname" . }}
namespace: {{ include "ingress-nginx.namespace" . }}
data:
allow-snippet-annotations: "{{ .Values.controller.allowSnippetAnnotations }}"
{{- if .Values.controller.allowSnippetAnnotations }}
allow-snippet-annotations: "true"
{{- end }}
{{- if .Values.controller.addHeaders }}
add-headers: {{ include "ingress-nginx.namespace" . }}/{{ include "ingress-nginx.fullname" . }}-custom-add-headers
{{- end }}
Expand Down
19 changes: 9 additions & 10 deletions docs/user-guide/nginx-configuration/configmap.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ 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-cross-namespace-resources](#allow-cross-namespace-resources) | bool | "false" | |
| [allow-snippet-annotations](#allow-snippet-annotations) | bool | "false" | |
| [annotations-risk-level](#annotations-risk-level) | string | Critical | |
| [annotations-risk-level](#annotations-risk-level) | string | High | |
| [annotation-value-word-blocklist](#annotation-value-word-blocklist) | string array | "" | |
| [hide-headers](#hide-headers) | string array | empty | |
| [access-log-params](#access-log-params) | string | "" | |
Expand Down Expand Up @@ -221,7 +221,7 @@ The following table shows a configuration option's name, type, and the default v
| [service-upstream](#service-upstream) | bool | "false" | |
| [ssl-reject-handshake](#ssl-reject-handshake) | bool | "false" | |
| [debug-connections](#debug-connections) | []string | "127.0.0.1,1.1.1.1/24" | |
| [strict-validate-path-type](#strict-validate-path-type) | bool | "false" (v1.7.x) | |
| [strict-validate-path-type](#strict-validate-path-type) | bool | "true" | |
| [grpc-buffer-size-kb](#grpc-buffer-size-kb) | int | 0 | |

## add-headers
Expand All @@ -234,34 +234,30 @@ Enables the return of the header Server from the backend instead of the generic

## allow-cross-namespace-resources

Enables users to consume cross namespace resource on annotations, when was previously enabled . _**default:**_ true
Enables users to consume cross namespace resource on annotations, when was previously enabled . _**default:**_ false

**Annotations that may be impacted with this change**:

* `auth-secret`
* `auth-proxy-set-header`
* `auth-tls-secret`
* `fastcgi-params-configmap`
* `proxy-ssl-secret`


**This option will be defaulted to false in the next major release**

## allow-snippet-annotations

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

**This option will be defaulted to false in the next major release**

## annotations-risk-level

Represents the risk accepted on an annotation. If the risk is, for instance `Medium`, annotations with risk High and Critical will not be accepted.

Accepted values are `Critical`, `High`, `Medium` and `Low`.

Defaults to `Critical` but will be changed to `High` on the next minor release
_**default:**_ `High`

## annotation-value-word-blocklist

Expand Down Expand Up @@ -1364,6 +1360,7 @@ _References:_
[http://nginx.org/en/docs/ngx_core_module.html#debug_connection](http://nginx.org/en/docs/ngx_core_module.html#debug_connection)

## strict-validate-path-type

Ingress objects contains a field called pathType that defines the proxy behavior. It can be `Exact`, `Prefix` and `ImplementationSpecific`.

When pathType is configured as `Exact` or `Prefix`, there should be a more strict validation, allowing only paths starting with "/" and
Expand All @@ -1377,6 +1374,8 @@ This means that Ingress objects that rely on paths containing regex characters s
The cluster admin should establish validation rules using mechanisms like [Open Policy Agent](https://www.openpolicyagent.org/) to
validate that only authorized users can use `ImplementationSpecific` pathType and that only the authorized characters can be used.

_**default:**_ "true"

## grpc-buffer-size-kb

Sets the configuration for the GRPC Buffer Size parameter. If not set it will use the default from NGINX.
Expand Down
2 changes: 1 addition & 1 deletion pkg/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ Requires the update-status parameter.`)
`Prefix of the Ingress annotations specific to the NGINX controller.`)

enableAnnotationValidation = flags.Bool("enable-annotation-validation", true,
`If true, will enable the annotation validation feature. This value will be defaulted to true on a future release`)
`If true, will enable the annotation validation feature. Defaults to true`)

enableSSLChainCompletion = flags.Bool("enable-ssl-chain-completion", false,
`Autocomplete SSL certificate chains with missing intermediate CA certificates.
Expand Down
21 changes: 5 additions & 16 deletions test/e2e/settings/validations/validations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,8 @@ var _ = framework.IngressNginxDescribeSerial("annotation validations", func() {
f := framework.NewDefaultFramework("validations")
//nolint:dupl // Ignore dupl errors for similar test case
ginkgo.It("should allow ingress based on their risk on webhooks", func() {
f.SetNginxConfigMapData(map[string]string{
"allow-snippet-annotations": "true",
})
defer func() {
f.SetNginxConfigMapData(map[string]string{
"allow-snippet-annotations": "false",
})
}()
disableSnippet := f.AllowSnippetConfiguration()
defer disableSnippet()

host := "annotation-validations"

Expand Down Expand Up @@ -66,14 +60,9 @@ var _ = framework.IngressNginxDescribeSerial("annotation validations", func() {
})
//nolint:dupl // Ignore dupl errors for similar test case
ginkgo.It("should allow ingress based on their risk on webhooks", func() {
f.SetNginxConfigMapData(map[string]string{
"allow-snippet-annotations": "true",
})
defer func() {
f.SetNginxConfigMapData(map[string]string{
"allow-snippet-annotations": "false",
})
}()
disableSnippet := f.AllowSnippetConfiguration()
defer disableSnippet()

host := "annotation-validations"

// Low and Medium Risk annotations should be allowed, the rest should be denied
Expand Down
1 change: 0 additions & 1 deletion test/e2e/wait-for-nginx.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ else
# TODO: remove the need to use fullnameOverride
fullnameOverride: nginx-ingress
controller:
enableAnnotationValidations: true
image:
repository: ingress-controller/controller
chroot: ${IS_CHROOT}
Expand Down