strict-validate-path-type does not allow period/dot/. in Exact or Prefix path

[rouke-broersma]

What happened:

The documentation mentions the following text:

When pathType is configured as Exact or Prefix, there should be a more strict validation, allowing only paths starting with "/" and containing only alphanumeric characters and "-", "_" and additional "/".

However the term alphanumeric characters is not specified and the term does not seem to be universally specified either. Some sources for example include special characters in alphanumeric characters, which seems to defeat the purpose of the strict checking.

The current text, depending on the definition of alphanumeric characters, seems to suggest that for example the period . character is not allowed when using Exact or Prefix. This would mean that a path of https://example.com/index.html, https://example.com/.well-known/openid-configuration or https://example.com/.well-known/acme-challenge/3857265 would be invalid for path type Exact and Prefix. I cannot imagine that it's intentional that all these super common url's would be unsupported by Exact or Prefix.

What you expected to happen:

Define the term alphanumeric characters as used by this project and allow . in Exact and Prefix path types.

/kind documentation
/remove-kind bug

[longwuyuan]

Then does this https://kubernetes.io/docs/concepts/overview/working-with-objects/names/ page also has the words alphanumeric characters

[longwuyuan]

also https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/

[longwuyuan]

And I could be wrong, but period/dot/. is known as a separator

[rouke-broersma]

@longwuyuan You're linking documentation in the context of Kubernetes objects, but these requirements are not the same as used by ingress-nginx for path validation. This documentation does indeed mention that alphanumeric means [a-z0-9A-Z] in this context, but ingress-nginx does not define what alphanumeric means. If alphanumeric in the context of ingress-nginx means the same as the definition as defined in the documents you linked, then I think this should be mentioned in the docs for strict-validate-path-type.

I looked up the validation of the path as used by ingress-nginx here:

ingress-nginx/internal/ingress/inspector/rules.go

Line 37 in e44cab7

validPathType = regexp.MustCompile(`(?i)^/[[:alnum:]\_\-/]*$`)

And I could be wrong, but period/dot/. is known as a separator

Be that as it may, this is not supported by the path validation in ingress-nginx so you can't even host static files with a file extenstion (index.html for example) using Exact or Prefix paths when strict-validate-path-type is enabled.

See: https://regex101.com/r/WjwI0c/1

I think this is an oversight and that . should be supported in Exact and Prefix path types. So actually perhaps this is not only a documentation issue but also either a bug or a feature request.

[longwuyuan]

thanks @rouke-broersma , maybe we should keep adding thoughts here so it helps make progress.

• Could you paste a sample ingress manifest and the log messages for an attempt to create that sample
• Would you opine that the suggested changes improve security or risk stays the same as now or risk is increased

[rouke-broersma]

@longwuyuan here is an ingress from my existing homelab:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: bazarr-auth
  namespace: authentik
spec:
  rules:
    - host: bazarr.example.com
      http:
        paths:
          - backend:
              service:
                name: ak-outpost-authentik-embedded-outpost
                port:
                  number: 9000
            path: /outpost.goauthentik.io
            pathType: Prefix

Log message:

admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: ingress contains invalid paths: path /outpost.goauthentik.io cannot be used with pathType Prefix

I would say that since / is required in the path and period/dot/. is not a special character when in the path that there is no increased security risk in allowing this character in Exact and Prefix.

I would say that allowing the commonly used period/dot/. in Exact and Prefix instead of only in ImplementationSpecific increases security because ImplementationSpecific is not validated but a user is now forced to use it.

[longwuyuan]

• I see your path starts with "forward slash" char
• I see docs saying paths starting with "forward slash" are allowed
  

[rouke-broersma]

@longwuyuan I know / is allowed. In fact / is required. I'm saying since / is required there is no risk in allowing .. If / was not required then allowing . might be a risk, because a user could create a ingress like this:

    - host: bazarr.example.com
      http:
        paths:
            path: .outpost.goauthentik.io
            pathType: Prefix

This might change the host part instead of only the path part of the ingress, which could be a concern. However since / is required, this is not a concern.

[longwuyuan]

I think I am trying to see enough text in this issue description here that settles the following thoughts

• What precisely makes the use of phrase "alphanumeric chars" in this project's docs, go out of context, when this project is a sub of K8S sig-network, and K8S docs use the phrase and even explain the phrase clearly as screenshot pasted earlier

• For security sake, if a user is required to set pathType as "ImplementationSpecific", to spec the path field with a dot/./period/ characters, then what is the problem created for the user. I have not seen many daily use URLs that contain a period character anyways and neither do people substantially often create subfolders/subdirectories with period/dot character in the name of the folder to be served over a browser or in api-calls

• There is another open issue where not allowing the dollar/$ char broke a working annotation, in production, so the project does ack improvements needed. And without feedack like this one, it would be impossible to improve/fix issues. So thank you very much.

• But actionable tasks come out of elaborate clear data and that data comes out of triage and these discussions. There is an acute shortage of developer time so data here helps reduce time a developer needs to research the issue

[rouke-broersma]

What precisely makes the use of phrase "alphanumeric chars" in this project's docs, go out of context, when this project is a sub of K8S sig-network, and K8S docs use the phrase and even explain the phrase clearly as screenshot pasted earlier

Ingress-nginx implements it's own regex for the path validation, which is not actually exactly the same as the docs you linked. The ingress-nginx docs also do not link to this other documentation, so while one can make the assumption that this is a term shared within the k8s sig-network, it would be an improvement to the docs if it actually mentions this if this is in fact the case. As it stands there is no reason to assume both usages of the term are the same, since the ingress-nginx docs are also not hosted in the same location. You are more familiar with the kubernetes project so for you it makes sense that they are related. For me as an outsider it is not as obvious, so I turned to Google to tell me what alphanumerical characters means, and there I did not find one consistent definition. Some did include . and some did not.

For security sake, if a user is required to set pathType as "ImplementationSpecific", to spec the path field with a dot/./period/ characters, then what is the problem created for the user. I have not seen many daily use URLs that contain a period character anyways and neither do people substantially often create subfolders/subdirectories with period/dot character in the name of the folder to be served over a browser or in api-calls

ImplementationSpecific allows special characters that are dangerous. A user is forced to choose a more dangerous, non-validated path type, to be able to use a non-dangerous character in the path. It is my opinion that this decreases security, because a user is more often forced to choose a less-secure method.

I disagree with you that the . is an uncommon character in the path of a url. Sure, these days it is also common to no longer include the file name and extension in paths, but there are many sites that still serve content where the file extension is in the path (index.html, index.php etc). File names an extensions are an integral part to serving web content.

On top of that the /.well-known/ construct is also used a lot, for example in oidc and in the HTTP01 ACME TLS certificate challenge protocol: https://letsencrypt.org/docs/challenge-types/

But actionable tasks come out of elaborate clear data and that data comes out of triage and these discussions. There is an acute shortage of developer time so data here helps reduce time a developer needs to research the issue

In this case I believe the actual change is very minor, it would only involve changing the validating regex I linked above and adding a test case.

That said I believe this discussion is currently highly relevant because strict-validate-path-type will be turned on by default in the next release, and I believe that many users will be caught off-guard by . being an invalid character since it's so integral to how webservers work. Bug reports from these users will also eat up valuable developer time, so imo either fixing this or alternatively explicitly mentioning in the docs that the . character is invalid for these path types will save a lot of time in the future.

[strongjz]

Thank you for pointing this out; many recent changes were due to security issues with path validation and annotations. They were made quickly to prevent these issues. Please feel free to update the documentation to remind folks that they should be Implementation Specific if they have dots in their paths.

/kind documentation
/good-first-issue

[k8s-ci-robot]

@strongjz:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

Thank you for pointing this out; many recent changes were due to security issues with path validation and annotations. They were made quickly to prevent these issues. Please feel free to update the documentation to remind folks that they should be Implementation Specific if they have dots in their paths.

/kind documentation
/good-first-issue

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/test-infra repository.

[strongjz]

/triage accepted
/priority backlog

[strongjz]

@rouke-broersma are you going to open a PR to update the documentation around this?

[rouke-broersma]

@rouke-broersma are you going to open a PR to update the documentation around this?

I can do that.

Just to be absolutely sure, the current behavior is as intended and changing this is not acceptable to the project?

Changing the behavior would ultimately have my preference, because I consider the current behavior less secure from a user perspective

[github-actions]

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

[AlbinoDrought]

For what it's worth, it seems the exclusion of . is intentional and related to kubernetes/kubernetes#126815 (CVE-2022-4886). The CVE workaround can be found in #9967, which introduces this validation. I don't think there's a CVE-2022-4886 proof of concept available, so I'm not sure why . is a potentially evil character in this context (maybe Go templating qualifier?)

[strongjz]

@rouke-broersma please only update the docs.

[halkeye]

so as the default changed, the synapse chart I was using has failed trying to add a path for /.well-known/matrix
https://gitlab.com/ananace/charts/-/blob/master/charts/matrix-synapse/templates/ingress.yaml?ref_type=heads#L180-191

It sounds like from this thread I should submit a PR that updates them to implementation specific, which disables validation right? Or is there still debate if periods are going to be allowed?

[rouke-broersma]

so as the default changed, the synapse chart I was using has failed trying to add a path for /.well-known/matrix https://gitlab.com/ananace/charts/-/blob/master/charts/matrix-synapse/templates/ingress.yaml?ref_type=heads#L180-191

It sounds like from this thread I should submit a PR that updates them to implementation specific, which disables validation right? Or is there still debate if periods are going to be allowed?

Period is unfortunately not going to be allowed so you will have to change the path type to ImplementationSpecific.

[longwuyuan]

I think @Gacko put in a PR to allow period and other chars in some places but (as @rouke-broersma poonted out) that pathType value required is ImplementationSpecific

[rouke-broersma]

I think @Gacko put in a PR to allow period and other chars in some places but (as @rouke-broersma poonted out) that pathType value required is ImplementationSpecific

Could you link the pr? I could not find it

[longwuyuan]

@Gacko hoping you can comment here about the PR in which you added dot/period and other characters as valid characters.

[bgehman]

Period is unfortunately not going to be allowed so you will have to change the path type to ImplementationSpecific.

Instead of fixing the validation regex so that it properly validates URL Paths (and . is definitely a valid char in URL paths) -- we are instead telling everyone to use ImplementationSpecific to bypass the controllers new strict URL path validations -- and this will somehow lead to better security?

https://www.ietf.org/rfc/rfc3986.txt

2.3. Unreserved Characters
Characters that are allowed in a URI but do not have a reserved
purpose are called unreserved. These include uppercase and lowercase
letters, decimal digits, hyphen, period, underscore, and tilde.
unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"

It seems to me the original issue (CVE) is that the controller is treating the path as a regex string? I am not seeing in the docs where controllers are supposed to do that. If people want to use regex defined paths, they should use ImplementationSpecific (and its great ingress-nginx provides that capability for people who want it).

However, Prefix/Exact pathTypes should be treated as literal (not regex) strings, and its validation should conform to the published RFCs.

[rouke-broersma]

I agree, which is why I reported this issue.