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

Says invalid fcgi value, but it isn't #10462

Closed
withinboredom opened this issue Sep 29, 2023 · 23 comments · Fixed by #10528
Closed

Says invalid fcgi value, but it isn't #10462

withinboredom opened this issue Sep 29, 2023 · 23 comments · Fixed by #10528
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@withinboredom
Copy link

withinboredom commented Sep 29, 2023

What happened:

Upgrade to 4.9.0:

 main.go:146] "fcgi annotation error" err="fcgi contains invalid key or value" configmap="php-config-map" namespace="main" key="SCRIPT_FILENAME" value="/app/src/index.php"
E0929 21:35:47.438810       7 annotations.go:189] "ingress contains invalid annotation value" err="annotation fastcgi-params-configmap contains invalid value"
E0929 21:35:47.438839       7 main.go:96] "invalid ingress configuration" err="annotation fastcgi-params-configmap contains invalid value" ingress="main/php"

and now seeing this when trying to update an ingress. Changing any key to a hardcoded value (and not a variable) results in this error. For example, setting DOCUMENT_ROOT to "/app" results in the same error but for DOCUMENT_ROOT.

What you expected to happen:

To be able to hard-code variables sent to PHP.

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

-------------------------------------------------------------------------------
NGINX Ingress controller
  Release:       v1.9.0
  Build:         4bd3d6b8a00b01b009f225a5593ce502cce5c26b
  Repository:    https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.21.6

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

Kubernetes version (use kubectl version):

WARNING: This version information is deprecated and will be replaced with the output from kubectl version --short.  Use --output=yaml|json to get the full version.
Client Version: version.Info{Major:"1", Minor:"26", GitVersion:"v1.26.3", GitCommit:"9e644106593f3f4aa98f8a84b23db5fa378900bd", GitTreeState:"clean", BuildDate:"2023-03-15T13:40:17Z", GoVersion:"go1.19.7", Compiler:"gc", Platform:"linux/amd64"}
Kustomize Version: v4.5.7
Server Version: version.Info{Major:"1", Minor:"27", GitVersion:"v1.27.6+k3s1", GitCommit:"bd04941a294793ec92e8703d5e5da14107902e88", GitTreeState:"clean", BuildDate:"2023-09-20T23:05:58Z", GoVersion:"go1.20.8", Compiler:"gc", Platform:"linux/amd64"}

Environment:

  • Cloud provider or hardware configuration: Bare Metal

  • OS (e.g. from /etc/os-release): Ubuntu 22.04.3 LTS

  • Kernel (e.g. uname -a): Linux cameo 5.15.0-84-generic Fix sort for catch all server #93-Ubuntu SMP Tue Sep 5 17:16:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

  • Install tools:

    • k3s
  • Basic cluster related info:

    • kubectl version
    • kubectl get nodes -o wide
  • How was the ingress-nginx-controller installed:
    helm

How to reproduce this issue:

  1. Create a cluster + install nginx ingress however you want (pods aren't really necessary because you can't save the ingress)
  2. Create a config map
apiVersion: v1
kind: ConfigMap
metadata:
  name: php-config-map
  namespace: default
data:
  CONTENT_LENGTH: $content_length
  CONTENT_TYPE: $content_type
  DOCUMENT_ROOT: $document_root
  DOCUMENT_URI: $document_uri
  HTTPS: $https
  HTTP_X_FORWARDED_FOR: $remote_addr
  HTTP_X_FORWARDED_HOST: $best_http_host
  HTTP_X_FORWARDED_PORT: $pass_port
  HTTP_X_FORWARDED_PROTO: $pass_access_scheme
  HTTP_X_FORWARDED_SCHEME: $pass_access_scheme
  HTTP_X_ORIGINAL_FORWARDED_FOR: $http_x_forwarded_for
  HTTP_X_REAL_IP: $remote_addr
  HTTP_X_REQUEST_ID: $req_id
  HTTP_X_SCHEME: $pass_access_scheme
  QUERY_STRING: $query_string
  REDIRECT_STATUS: '200'
  REMOTE_ADDR: $remote_addr
  REMOTE_PORT: $remote_port
  REQUEST_METHOD: $request_method
  REQUEST_SCHEME: $scheme
  REQUEST_URI: $request_uri
  SCRIPT_NAME: $fastcgi_script_name
  SCRIPT_FILENAME: /app/index.php # <--------------------- this is important
  SERVER_ADDR: $server_addr
  SERVER_NAME: $server_name
  SERVER_PORT: $server_port
  SERVER_PROTOCOL: $server_protocol

Now create an ingress that uses the config map:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: php
  namespace: default
  annotations:
    nginx.ingress.kubernetes.io/backend-protocol: FCGI
    nginx.ingress.kubernetes.io/fastcgi-index: index.php
    nginx.ingress.kubernetes.io/fastcgi-params-configmap: default/php-config-map
spec:
  ingressClassName: nginx
  rules:
    - host: example.com
      http:
        paths:
          - path: /login/
            pathType: Prefix
            backend:
              service:
                name: php
                port:
                  number: 9000

See the error mentioned at the beginning.

@withinboredom withinboredom added the kind/bug Categorizes issue or PR as related to a bug. label Sep 29, 2023
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Sep 29, 2023
@rikatz
Copy link
Contributor

rikatz commented Oct 1, 2023

@withinboredom I have fixed it on release v1.9.1 that will be released this week, it should accept again fcgi, or FCGI, etc.

If you are willing to test and provide feedback, please change your image on deploy to registry.k8s.io/ingress-nginx/controller:v1.9.1@sha256:605a737877de78969493a4b1213b21de4ee425d2926906857b98050f57a95b25.

Thanks

@withinboredom
Copy link
Author

Awesome, I will give it a go this evening!

@gregth
Copy link

gregth commented Oct 5, 2023

I am bumping onto the same issue.

Has the fix been included in @sha256:605a737877de78969493a4b1213b21de4ee425d2926906857b98050f57a95b25 image, or not? Cc: @rikatz

I might be be getting wrong, but seems like the code in the current main branch does not allow using hardcoded values. I see they validate against the following regexes:

	// NGINXVariable allows entries with alphanumeric characters, -, _ and the special "$"
	NGINXVariable = regexp.MustCompile(`^[A-Za-z0-9\-\_\$\{\}]*$`)

Can we use hard-coded values, like that, similar to the examples demonstrated in the docs https://kubernetes.github.io/ingress-nginx/user-guide/fcgi-services/?

  SCRIPT_FILENAME: /var/www/html/public/index.php

@surgiie
Copy link

surgiie commented Oct 6, 2023

Also experiencing this issue. This appears to have been published in the https://github.com/kubernetes/ingress-nginx/releases/tag/controller-v1.9.1 release?

@jkabat
Copy link

jkabat commented Oct 11, 2023

Unfortunately Im still getting the same error on 1.9.1

"fcgi annotation error" err="fcgi contains invalid key or value" configmap="api-configuration" namespace="staging" key="SCRIPT_FILENAME" value="/app/public/api.php"

@ronaldijsfontein
Copy link

We experience the same issue on 1.9.1

@surgiie
Copy link

surgiie commented Oct 11, 2023

Any roadmap to a fix?

@rikatz
Copy link
Contributor

rikatz commented Oct 11, 2023

No, at the moment we are running to release a new version with http2 CVE fix

@ametad
Copy link

ametad commented Oct 12, 2023

I tried out this tutorial: https://kubernetes.github.io/ingress-nginx/user-guide/fcgi-services/

And I got also the error with ingress-nginx version 1.9.1 version :

k apply -f example-app-ingress.yaml 
configmap/example-cm created
Error from server (BadRequest): error when creating "example-app-ingress.yaml": admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: annotation fastcgi-params-configmap contains invalid value

Ingress definition example-app-ingress.yaml:

# The ConfigMap MUST be created first for the ingress controller to be able to
# find it when the Ingress object is created.

apiVersion: v1
kind: ConfigMap
metadata:
  name: example-cm
data:
  SCRIPT_FILENAME: "/var/www/html/public/index.php"
  DOCUMENT_ROOT: "/var/www/html/public"
---

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    nginx.ingress.kubernetes.io/backend-protocol: "FCGI"
    nginx.ingress.kubernetes.io/fastcgi-index: "index.php"
    nginx.ingress.kubernetes.io/fastcgi-params-configmap: "example-cm"
  name: example-app
spec:
  ingressClassName: nginx
  rules:
    - host: example-app.test
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: example-service
                port:
                  name: fastcgi

But with ingress-nginx version 1.8.2 there is no problem and it just works correctly!

@withinboredom
Copy link
Author

withinboredom commented Oct 12, 2023

No, at the moment we are running to release a new version with http2 CVE fix

@rikatz -- what's the point of a fix for that if nobody using the fpm feature can upgrade?

@rikatz
Copy link
Contributor

rikatz commented Oct 12, 2023

Well, for a matter of bugs, I think security has priority over bug on a feature :)

I am super busy with other things and honestly had to stop during a PTO to do that release, so I'm happy to review a PR that fixes fpm bug as well and try a new release, otherwise we need to wait until I have some time to fix it.

I've mentioned during today's community meeting that this bug is on my radar, but I didn't had time to fix it

@rikatz
Copy link
Contributor

rikatz commented Oct 12, 2023

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority labels Oct 12, 2023
@rikatz
Copy link
Contributor

rikatz commented Oct 12, 2023

Also, @withinboredom please check your tone. We are volunteers and are running low on contributors, we are always happy to help.

Release 1.8 is still supported, if fpm is working there, you can downgrade for it while waiting for a fix

@rikatz rikatz added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Oct 12, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 12, 2023
@withinboredom
Copy link
Author

I apologize for my tone. It wasn't meant to be antagonistic, just typed it quickly while doing something else. If anything, I think it was meant to be an attempt at sarcastic or funny? Either way, taking into consideration your context, and reading it again, I can clearly see the hostility you may have seen. This was not my intention, at all. I sincerely apologize for that.

If you (or someone) can point me in the right general direction, I can investigate the issue and contribute with a fix, or if I hit the limits of my capabilities, report my findings here. Like, should I start in the validation webhook or is there a better place to start digging?

@rikatz
Copy link
Contributor

rikatz commented Oct 13, 2023

No worries, thanks for understanding it.

var regexValidIndexAnnotationAndKey = regexp.MustCompile(`^[A-Za-z0-9.\-\_]+$`)
is where the validation happens. While it should not be happening at this point (as validations should be turned off by default), this is the regex called to validate the file name.

@rikatz
Copy link
Contributor

rikatz commented Oct 13, 2023

Actually sorry, the validation for cmap key/value is on this line/regex:

if !regexValidIndexAnnotationAndKey.MatchString(k) || !parser.NGINXVariable.MatchString(v) {

@surgiie
Copy link

surgiie commented Oct 17, 2023

@rikatz What is the expected release for this fix? Particularly on the helm chart.

@rikatz
Copy link
Contributor

rikatz commented Oct 22, 2023

Hi all, I will TRY to do a new release with this fix tomorrow, Oct 22nd

@zxzharmlesszxz
Copy link

I have similar problem so still waiting for fix

@rikatz
Copy link
Contributor

rikatz commented Oct 27, 2023

This was fixed on v1.9.4 released yesterday

@BloodyIron
Copy link

This was fixed on v1.9.4 released yesterday

Hey bud, sorry didn't mean to wake anyone. I immediately deleted my comment after realising upping my version could address my problem. I'm still working through this so hoping to say "it works" ... if it works :) But again, deleted my comment to avoid noise. So sorry that it made noise anyways :(

@BloodyIron
Copy link

Uhh it LOOKS to be working for me (1.9.4) but I'm having other issues probably unrelated. So perhaps consider this a positive sample.

@fabpico
Copy link

fabpico commented Dec 12, 2023

Happened also on 1.9.3. Fixed with upgrade to 1.9.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.