Validation of URLs does not comply with RFCs (e.g. app-root annotation, ingress path)

[Phil1602]

What happened:

An ingress containing an anchor within the app-root redirect annotation fails validation:

nginx.ingress.kubernetes.io/app-root: /_dashboards/app/home#/

What you expected to happen:

After updating Ingress-Nginx to 1.12.0 release, we encountered the following issue with app-root annotation. The "issue" itself was not introduced within 1.12.0 but was realized because of the new default enablement of --enable-annotation-validation (see flags.go).

So far so good, we are wondering why this specific annotation validation should fail in our case:

W0211 15:42:00.195018 7 validators.go:237] validation error on ingress my-namespace/my-ingress: annotation app-root contains invalid value /_dashboards/app/home#/

The Ingress itself is related to an OpenSearch installation which uses anchor/fragments for many aspects of client-side logic and deeplinking.

We totally aggree that this is an edge case and in our case we can solve the problem by adjusting the value to /_dashboards/app/home, since the fragment/anchor is appended properly on client side again.

But we are wondering if the validation itself is spec conform about URL/URI pattern.

• According to RFC 3986 Syntax the fragments part of the URI should be valid.

• According to RFC 9110 about HTTP semantics a fragment seems to be a valid part of a HTTP 3xx Response.

• Also RFC 9110 links to RFC 3986 for Relative reference URI format

So in our opinion, the fragment/anchor part of the redirect seems to be valid and might be worth to add to valid options?

Relevant lines of code:

Annotation Validation of URLs via regex:

• /~https://github.com/kubernetes/ingress-nginx/blob/release-1.12/internal/ingress/annotations/parser/validators.go#L47
• /~https://github.com/kubernetes/ingress-nginx/blob/release-1.12/internal/ingress/annotations/parser/validators.go#L64

Additional, specific validation to app-route annotation via net/url lib:

• /~https://github.com/kubernetes/ingress-nginx/blob/release-1.12/internal/ingress/annotations/rewrite/main.go#L183-L205

NGINX Ingress controller version (exec into the pod and run /nginx-ingress-controller --version):

-------------------------------------------------------------------------------
NGINX Ingress controller
  Release:       v1.12.0
  Build:         ba73b2c24d355f1cdcf4b31ef7c5574059f12118
  Repository:    /~https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.25.5

-------------------------------------------------------------------------------

Kubernetes version (use kubectl version): Server Version: v1.30.8-eks-2d5f260 (not related to the issue)

Environment:
Since this issue is related to annotation validation within the nginx code (see above), it is not related to any infrastructure, kernel or hardware versions at all.

How to reproduce this issue:
Ingress which triggers this issue:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: example-app-root-anchor
  annotations:
    nginx.ingress.kubernetes.io/app-root: /foo#/
spec:
  ingressClassName: nginx
  rules:
  - host: <foo.example.com>
    http:
      paths:
      - backend:
          service:
            name: foo
            port:
              number: 443
        path: /
        pathType: Prefix

Ingress-Nginx config consists of at least Medium risk level according to the docs.

annotations-risk-level: Critical

Anything else we need to know:

[longwuyuan]

Check if you know the other config for risklevel and see current risk level. I think it needs to be higher.

[longwuyuan]

/remove-kind bug

[Phil1602]

Hi @longwuyuan,

Thanks for having a look.

As already said, it's not really an "issue" for us (we have a workaround therefor).

We also already increased the risk-level using annotations-risk-level: Critical. However, the validation error itself is not bound to the risk-level per se (the app-root annotation requires at least Medium level).
Another workaround would be to simply disable the annotation validation using enable-annotation-validation: false.

But thats not my point of raising this Issue. My point was to discuss if this is correct spec-conform behavior or if a URL containing # needs to be valid for this (and other) configs.

Please check the Relevant lines of code section. The validation does not allow fragments (aka anchors aka #) since # is not a valid character for this regex.

[Phil1602]

Maybe this issue should be more generic about "URL validation is not spec-conform" rather than the specific issues about specific annotations like app-route.

The main validation of URLs in ingress-nginx seems to handcrafted (see validators.go for annotations and rules.go for ingress path).

Using handcrafted regex for proper URL validation might not be the best choice, since I can already see many cases of validated Ingress paths (.spec.rules[].http.paths[].path) which would result in false-positive Validation errors:

• path: /foo/file.json
• path: /.well-known/
• path: /foo-bar
• path: /foo_bar

All of the mentioned examples above MUST be allowed as valid paths within an Ingress resource.

RFC 3986 clearly states that the following (unreserved) characters are valid:

unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"

Probably a proper library therefor like net/url with its Parse function could help in that regard. However, this should be validated if the allowed paths of nginx and this library would match.

[longwuyuan]

Please wait for @Gacko @strongjz @tao12345666333 to comment

There is a flip side to this. I think its important to note that while making all good stuff possible, most parts of the controller code, that is not directly about the Ingress-API, does not use a framework or such resource with tons of developers behind it. Hence there is rampant misuse to say the least. Its so severe that we have had to deprecate fully functional and extremely useful, popular features, because there is not even developers to secure it and make stable reliable releases.

[frittentheke]

There is a flip side to this. I think its important to note that while making all good stuff possible, most parts of the controller code, that is not directly about the Ingress-API, does not use a framework or such resource with tons of developers behind it.

That is exactly the point. Validating well defined (I'd even go as far to call them standardized) strings / values WITHOUT a library creates even more work (bugs). Excuse my french, but writing your own regexes to validate REALLY complicated, complex and still evolving data structures such as URL / URI / email addresses ... is like rolling your own crypto.
And if they were just copy-pasted out of the NGINX source code, that would make sense. But otherwise please free yourself from attempting to cover all valid cases and catch all invalid syntax.

[Gacko]

I don't have a lot of time to go through the whole issue, but from what I read in a few seconds: Have you actually checked if everything including and behind the # is actually getting sent to the server? IIRC clients do not even send this and it's a complete client-side thing.

[Phil1602]

I don't have a lot of time to go through the whole issue, but from what I read in a few seconds: Have you actually checked if everything including and behind the # is actually getting sent to the server? IIRC clients do not even send this and it's a complete client-side thing.

The problem itself is only the validation of this annotation (as well as other configs like ingress paths).

Implementation works fine if validation is disabled. As stated above:

According to RFC 9110 about HTTP semantics a fragment seems to be a valid part of a HTTP 3xx Response.
(This is at least the case for the app-root annotation which triggers an initial HTTP 302 redirect).

For ingress paths you are right @Gacko. Fragments/Anchors do not need to be valid for paths. The net/url lib differentiates between Parse and ParseRequestURI for these cases.

[Phil1602]

TL;DR:

We have two different issues we are talking about (my bad that I mixed them into one GitHub issue, sorry). Both issues are related to handcrafted regex (kind of same cause).

Issue nginx.ingress.kubernetes.io/app-root annotation validation:

• URL part of app-root triggers a redirect
• Regex does not allow fragments (#)
• fragments are allowed in redirect responses according to RFC 9110

Issue Ingress path validation:

• Path for the actual routing configuration (aka "Request URI")
• Regex does not allow dots (.) as well as other valid chars
• EDIT: This issue seems to be already reported here strict-validate-path-type does not allow period/dot/. in Exact or Prefix path #11176

If you want me to, I could split them into distinct issues. But since they both rely on regex which do not comply to RFCs, maybe this could be a aligned fix for both.

[longwuyuan]

/triage accepted