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

Implement annotation validation #9673

Merged
merged 14 commits into from
Jul 22, 2023
49 changes: 49 additions & 0 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,55 @@ jobs:
name: e2e-test-reports-${{ matrix.k8s }}
path: 'test/junitreports/report*.xml'

kubernetes-validations:
name: Kubernetes with Validations
runs-on: ubuntu-latest
needs:
- changes
- build
if: |
(needs.changes.outputs.go == 'true') || ${{ inputs.run_e2e }}

strategy:
matrix:
k8s: [v1.27.1]

steps:
- name: Checkout
uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3

- name: cache
uses: actions/download-artifact@9bc31d5ccc31df68ecc42ccf4149144866c47d8a # v3.0.2
with:
name: docker.tar.gz

- name: Create Kubernetes ${{ matrix.k8s }} cluster
id: kind
run: |
kind create cluster --image=kindest/node:${{ matrix.k8s }} --config test/e2e/kind.yaml

- name: Load images from cache
run: |
echo "loading docker images..."
pigz -dc docker.tar.gz | docker load

- name: Run e2e tests
env:
KIND_CLUSTER_NAME: kind
SKIP_CLUSTER_CREATION: true
SKIP_IMAGE_CREATION: true
ENABLE_VALIDATIONS: true
run: |
kind get kubeconfig > $HOME/.kube/kind-config-kind
make kind-e2e-test

- name: Upload e2e junit-reports
uses: actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce # v3.1.2
if: success() || failure()
with:
name: e2e-test-reports-${{ matrix.k8s }}
path: 'test/junitreports/report*.xml'


kubernetes-chroot:
name: Kubernetes chroot
Expand Down
1 change: 1 addition & 0 deletions charts/ingress-nginx/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ As of version `1.26.0` of this chart, by simply not providing any clusterIP valu
| controller.dnsConfig | object | `{}` | Optionally customize the pod dnsConfig. |
| controller.dnsPolicy | string | `"ClusterFirst"` | Optionally change this to ClusterFirstWithHostNet in case you have 'hostNetwork: true'. By default, while using host network, name resolution uses the host's DNS. If you wish nginx-controller to keep resolving names inside the k8s network, use ClusterFirstWithHostNet. |
| controller.electionID | string | `""` | Election ID to use for status update, by default it uses the controller name combined with a suffix of 'leader' |
| controller.enableAnnotationValidations | bool | `false` | |
| controller.enableMimalloc | bool | `true` | Enable mimalloc as a drop-in replacement for malloc. # ref: https://github.com/microsoft/mimalloc # |
| controller.enableTopologyAwareRouting | bool | `false` | This configuration enables Topology Aware Routing feature, used together with service annotation service.kubernetes.io/topology-aware-hints="auto" Defaults to false |
| controller.existingPsp | string | `""` | Use an existing PSP instead of creating one |
Expand Down
3 changes: 3 additions & 0 deletions charts/ingress-nginx/templates/_params.tpl
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
{{- define "ingress-nginx.params" -}}
- /nginx-ingress-controller
{{- if .Values.controller.enableAnnotationValidations }}
- --enable-annotation-validation=true
{{- end }}
{{- if .Values.defaultBackend.enabled }}
- --default-backend-service=$(POD_NAMESPACE)/{{ include "ingress-nginx.defaultBackend.fullname" . }}
{{- end }}
Expand Down
1 change: 1 addition & 0 deletions charts/ingress-nginx/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ commonLabels: {}

controller:
name: controller
enableAnnotationValidations: false
image:
## Keep false as default for now!
chroot: false
Expand Down
1 change: 1 addition & 0 deletions docs/user-guide/cli-arguments.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ They are set in the container spec of the `ingress-nginx-controller` Deployment
| `--default-backend-service` | Service used to serve HTTP requests not matching any known server name (catch-all). Takes the form "namespace/name". The controller configures NGINX to forward requests to the first port of this Service. |
| `--default-server-port` | Port to use for exposing the default server (catch-all). (default 8181) |
| `--default-ssl-certificate` | Secret containing a SSL certificate to be used by the default HTTPS server (catch-all). Takes the form "namespace/name". |
| `--enable-annotation-validation` | If true, will enable the annotation validation feature. This value will be defaulted to true on a future release. |
| `--disable-catch-all` | Disable support for catch-all Ingresses. (default false) |
| `--disable-full-test` | Disable full test of all merged ingresses at the admission stage and tests the template of the ingress being created or updated (full test of all ingresses is enabled by default). |
| `--disable-svc-external-name` | Disable support for Services of type ExternalName. (default false) |
Expand Down
26 changes: 26 additions & 0 deletions docs/user-guide/nginx-configuration/configmap.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +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-snippet-annotations](#allow-snippet-annotations)|bool|true||
|[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||
|[access-log-params](#access-log-params)|string|""||
Expand Down Expand Up @@ -239,13 +241,37 @@ Sets custom headers from named configmap before sending traffic to the client. S

Enables the return of the header Server from the backend instead of the generic nginx string. _**default:**_ is disabled

## allow-cross-namespace-resources

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

**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:**_ `true`

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

## annotation-value-word-blocklist

Contains a comma-separated value of chars/words that are well known of being used to abuse Ingress configuration
Expand Down
36 changes: 33 additions & 3 deletions internal/ingress/annotations/alias/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,44 @@ import (
"k8s.io/ingress-nginx/internal/ingress/resolver"
)

const (
serverAliasAnnotation = "server-alias"
)

var aliasAnnotation = parser.Annotation{
Group: "alias",
Annotations: parser.AnnotationFields{
serverAliasAnnotation: {
Validator: parser.ValidateArrayOfServerName,
Scope: parser.AnnotationScopeIngress,
Risk: parser.AnnotationRiskHigh, // High as this allows regex chars
Documentation: `this annotation can be used to define additional server
aliases for this Ingress`,
},
},
}

type alias struct {
r resolver.Resolver
r resolver.Resolver
annotationConfig parser.Annotation
}

// NewParser creates a new Alias annotation parser
func NewParser(r resolver.Resolver) parser.IngressAnnotation {
return alias{r}
return alias{
r: r,
annotationConfig: aliasAnnotation,
}
}

func (a alias) GetDocumentation() parser.AnnotationFields {
return a.annotationConfig.Annotations
}

// Parse parses the annotations contained in the ingress rule
// used to add an alias to the provided hosts
func (a alias) Parse(ing *networking.Ingress) (interface{}, error) {
val, err := parser.GetStringAnnotation("server-alias", ing)
val, err := parser.GetStringAnnotation(serverAliasAnnotation, ing, a.annotationConfig.Annotations)
if err != nil {
return []string{}, err
}
Expand All @@ -61,3 +86,8 @@ func (a alias) Parse(ing *networking.Ingress) (interface{}, error) {

return l, nil
}

func (a alias) Validate(anns map[string]string) error {
maxrisk := parser.StringRiskToRisk(a.r.GetSecurityConfiguration().AnnotationsRiskLevel)
return parser.CheckAnnotationRisk(anns, maxrisk, aliasAnnotation.Annotations)
}
35 changes: 24 additions & 11 deletions internal/ingress/annotations/alias/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (
"k8s.io/ingress-nginx/internal/ingress/resolver"
)

var annotation = parser.GetAnnotationWithPrefix("server-alias")
var annotation = parser.GetAnnotationWithPrefix(serverAliasAnnotation)

func TestParse(t *testing.T) {
ap := NewParser(&resolver.Mock{})
Expand All @@ -36,16 +36,20 @@ func TestParse(t *testing.T) {
}

testCases := []struct {
annotations map[string]string
expected []string
annotations map[string]string
expected []string
skipValidation bool
wantErr bool
}{
{map[string]string{annotation: "a.com, b.com, , c.com"}, []string{"a.com", "b.com", "c.com"}},
{map[string]string{annotation: "www.example.com"}, []string{"www.example.com"}},
{map[string]string{annotation: "*.example.com,www.example.*"}, []string{"*.example.com", "www.example.*"}},
{map[string]string{annotation: `~^www\d+\.example\.com$`}, []string{`~^www\d+\.example\.com$`}},
{map[string]string{annotation: ""}, []string{}},
{map[string]string{}, []string{}},
{nil, []string{}},
{map[string]string{annotation: "a.com, b.com, , c.com"}, []string{"a.com", "b.com", "c.com"}, false, false},
{map[string]string{annotation: "www.example.com"}, []string{"www.example.com"}, false, false},
{map[string]string{annotation: "*.example.com,www.example.*"}, []string{"*.example.com", "www.example.*"}, false, false},
{map[string]string{annotation: `~^www\d+\.example\.com$`}, []string{`~^www\d+\.example\.com$`}, false, false},
{map[string]string{annotation: `www.xpto;lala`}, []string{}, false, true},
{map[string]string{annotation: `www.xpto;lala`}, []string{"www.xpto;lala"}, true, false}, // When we skip validation no error should happen
{map[string]string{annotation: ""}, []string{}, false, true},
{map[string]string{}, []string{}, false, true},
{nil, []string{}, false, true},
}

ing := &networking.Ingress{
Expand All @@ -58,7 +62,16 @@ func TestParse(t *testing.T) {

for _, testCase := range testCases {
ing.SetAnnotations(testCase.annotations)
result, _ := ap.Parse(ing)
if testCase.skipValidation {
parser.EnableAnnotationValidation = false
}
defer func() {
parser.EnableAnnotationValidation = true
}()
result, err := ap.Parse(ing)
if (err != nil) != testCase.wantErr {
t.Errorf("ParseAliasAnnotation() annotation: %s, error = %v, wantErr %v", testCase.annotations, err, testCase.wantErr)
}
if !reflect.DeepEqual(result, testCase.expected) {
t.Errorf("expected %v but returned %v, annotations: %s", testCase.expected, result, testCase.annotations)
}
Expand Down
17 changes: 12 additions & 5 deletions internal/ingress/annotations/annotations.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ import (
"k8s.io/ingress-nginx/internal/ingress/annotations/fastcgi"
"k8s.io/ingress-nginx/internal/ingress/annotations/globalratelimit"
"k8s.io/ingress-nginx/internal/ingress/annotations/http2pushpreload"
"k8s.io/ingress-nginx/internal/ingress/annotations/ipallowlist"
"k8s.io/ingress-nginx/internal/ingress/annotations/ipdenylist"
"k8s.io/ingress-nginx/internal/ingress/annotations/ipwhitelist"
"k8s.io/ingress-nginx/internal/ingress/annotations/loadbalancing"
"k8s.io/ingress-nginx/internal/ingress/annotations/log"
"k8s.io/ingress-nginx/internal/ingress/annotations/mirror"
Expand Down Expand Up @@ -109,14 +109,14 @@ type Ingress struct {
UpstreamHashBy upstreamhashby.Config
LoadBalancing string
UpstreamVhost string
Whitelist ipwhitelist.SourceRange
Denylist ipdenylist.SourceRange
XForwardedPrefix string
SSLCipher sslcipher.Config
Logs log.Config
ModSecurity modsecurity.Config
Mirror mirror.Config
StreamSnippet string
Allowlist ipallowlist.SourceRange
}

// Extractor defines the annotation parsers to be used in the extraction of annotations
Expand Down Expand Up @@ -159,7 +159,7 @@ func NewAnnotationExtractor(cfg resolver.Resolver) Extractor {
"UpstreamHashBy": upstreamhashby.NewParser(cfg),
"LoadBalancing": loadbalancing.NewParser(cfg),
"UpstreamVhost": upstreamvhost.NewParser(cfg),
"Whitelist": ipwhitelist.NewParser(cfg),
"Allowlist": ipallowlist.NewParser(cfg),
strongjz marked this conversation as resolved.
Show resolved Hide resolved
"Denylist": ipdenylist.NewParser(cfg),
"XForwardedPrefix": xforwardedprefix.NewParser(cfg),
"SSLCipher": sslcipher.NewParser(cfg),
Expand All @@ -173,16 +173,23 @@ func NewAnnotationExtractor(cfg resolver.Resolver) Extractor {
}

// Extract extracts the annotations from an Ingress
func (e Extractor) Extract(ing *networking.Ingress) *Ingress {
func (e Extractor) Extract(ing *networking.Ingress) (*Ingress, error) {
pia := &Ingress{
ObjectMeta: ing.ObjectMeta,
}

data := make(map[string]interface{})
for name, annotationParser := range e.annotations {
if err := annotationParser.Validate(ing.GetAnnotations()); err != nil {
return nil, errors.NewRiskyAnnotations(name)
}
val, err := annotationParser.Parse(ing)
klog.V(5).InfoS("Parsing Ingress annotation", "name", name, "ingress", klog.KObj(ing), "value", val)
if err != nil {
if errors.IsValidationError(err) {
klog.ErrorS(err, "ingress contains invalid annotation value")
return nil, err
}
if errors.IsMissingAnnotations(err) {
continue
}
Expand Down Expand Up @@ -220,5 +227,5 @@ func (e Extractor) Extract(ing *networking.Ingress) *Ingress {
klog.ErrorS(err, "unexpected error merging extracted annotations")
}

return pia
return pia, nil
}
32 changes: 25 additions & 7 deletions internal/ingress/annotations/annotations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,11 @@ func TestSSLPassthrough(t *testing.T) {

for _, foo := range fooAnns {
ing.SetAnnotations(foo.annotations)
r := ec.Extract(ing).SSLPassthrough
if r != foo.er {
r, err := ec.Extract(ing)
if err != nil {
t.Errorf("Errors should be null: %v", err)
}
if r.SSLPassthrough != foo.er {
t.Errorf("Returned %v but expected %v", r, foo.er)
}
}
Expand All @@ -158,8 +161,11 @@ func TestUpstreamHashBy(t *testing.T) {

for _, foo := range fooAnns {
ing.SetAnnotations(foo.annotations)
r := ec.Extract(ing).UpstreamHashBy.UpstreamHashBy
if r != foo.er {
r, err := ec.Extract(ing)
if err != nil {
t.Errorf("error should be null: %v", err)
}
if r.UpstreamHashBy.UpstreamHashBy != foo.er {
t.Errorf("Returned %v but expected %v", r, foo.er)
}
}
Expand All @@ -185,7 +191,11 @@ func TestAffinitySession(t *testing.T) {

for _, foo := range fooAnns {
ing.SetAnnotations(foo.annotations)
r := ec.Extract(ing).SessionAffinity
rann, err := ec.Extract(ing)
if err != nil {
t.Errorf("error should be null: %v", err)
}
r := rann.SessionAffinity
t.Logf("Testing pass %v %v", foo.affinitytype, foo.cookiename)

if r.Type != foo.affinitytype {
Expand Down Expand Up @@ -228,7 +238,11 @@ func TestCors(t *testing.T) {

for _, foo := range fooAnns {
ing.SetAnnotations(foo.annotations)
r := ec.Extract(ing).CorsConfig
rann, err := ec.Extract(ing)
if err != nil {
t.Errorf("error should be null: %v", err)
}
r := rann.CorsConfig
t.Logf("Testing pass %v %v %v %v %v", foo.corsenabled, foo.methods, foo.headers, foo.origin, foo.credentials)

if r.CorsEnabled != foo.corsenabled {
Expand Down Expand Up @@ -277,7 +291,11 @@ func TestCustomHTTPErrors(t *testing.T) {

for _, foo := range fooAnns {
ing.SetAnnotations(foo.annotations)
r := ec.Extract(ing).CustomHTTPErrors
rann, err := ec.Extract(ing)
if err != nil {
t.Errorf("error should be null: %v", err)
}
r := rann.CustomHTTPErrors

// Check that expected codes were created
for i := range foo.er {
Expand Down
Loading