Enable security features by default

[rikatz]

What this PR does / why we need it:

This PR:

• Lowers the acceptable annotation risk from Critical to High
• Disables cross namespace consumption
• Enables strict path validation

Also makes validation enabled by default

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• CVE Report (Scanner found CVE and adding report)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

[netlify]

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	771614f
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/66c29aafb403c10008db347a

[aojea]

/priority critical-important
/assign @aojea
/cc @robscott

[k8s-ci-robot]

@aojea: The label(s) priority/critical-important cannot be applied, because the repository doesn't have them.

In response to this:

/priority critical-important
/assign @aojea
/cc @robscott

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[aojea]

/priority critical-urgent

for context kubernetes/kubernetes#126744 and

https://www.cvedetails.com/vulnerability-list/vendor_id-15867/product_id-94170/Kubernetes-Ingress-nginx.html

The proliferation of annotations and the complexity to sanitize the inputs is causing a lot of damage to the security reputation of the project. We should aim for a secure by default setup that does not put on risk the non-advanced users

[Gacko]

Actually there might be more to enable by default, like strict path type checking. I'm currently on vacation, but I'll try to contribute here in the next days.

[longwuyuan]

/triage accepted
/kind bug

[aojea]

/assign @kubernetes/ingress-nginx-maintainers

[rikatz]

I did enabled the strict path already :)

…

On Tue, 20 Aug 2024 at 13:40 Antonio Ojea ***@***.***> wrote: /assign @kubernetes/ingress-nginx-maintainers <https://github.com/orgs/kubernetes/teams/ingress-nginx-maintainers> — Reply to this email directly, view it on GitHub <#11819 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABWZQBNKMUBYXWDHFZ77CHTZSNWP7AVCNFSM6AAAAABMWTGKVKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJZGI4TGOBVHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[longwuyuan]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: longwuyuan, rikatz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rikatz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Gacko]

The changes in the chart in conjunction with those in the controller effectively break the flag. Before you could opt-in for annotation validation by enabling the flag in the chart, now, if you disable it, nothing happens as we do not pass false to the controller in the deployment.

I'll create a follow-up PR to fix this.

[Gacko]

Also we forgot to update the help text in flags.go.

[samip5]

Please in the future, could you not introduce breaking changes in a minor version bump? :(

[strongjz]

Please in the future, could you not introduce breaking changes in a minor version bump? :(

We have operated like this in the past: We create a new feature, disable it by default, and then, after another minor release, enable it by default. This is all in service of securing the ingress controller by default. We discuss these types of changes in our community meetings, in Slack in #ingress-dev, and in the release notes.

[samip5]

Please in the future, could you not introduce breaking changes in a minor version bump? :(

We have operated like this in the past: We create a new feature, disable it by default, and then, after another minor release, enable it by default. This is all in service of securing the ingress controller by default. We discuss these types of changes in our community meetings, in Slack in #ingress-dev, and in the release notes.

That may be true, but according to semver (https://semver.org/), this is considered a breaking change and should have been a major as such.