Add annotation validation for fcgi

internal/ingress/annotations/fastcgi/main.go

@@ -19,17 +19,49 @@ package fastcgi
 import (
 	"fmt"
 	"reflect"
+	"regexp"
 
 	networking "k8s.io/api/networking/v1"
 	"k8s.io/client-go/tools/cache"
+	"k8s.io/klog/v2"
 
 	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
 	ing_errors "k8s.io/ingress-nginx/internal/ingress/errors"
 	"k8s.io/ingress-nginx/internal/ingress/resolver"
 )
 
+const (
+	fastCGIIndexAnnotation  = "fastcgi-index"
+	fastCGIParamsAnnotation = "fastcgi-params-configmap"
+)
+
+var (
+	// fast-cgi valid parameters is just a single file name (like index.php)
+	regexValidIndexAnnotationAndKey = regexp.MustCompile(`^[A-Za-z0-9\.\-\_]+$`)
+)
+
+var fastCGIAnnotations = parser.Annotation{
+	Group: "fastcgi",
+	Annotations: parser.AnnotationFields{
+		fastCGIIndexAnnotation: {
+			Validator:     parser.ValidateRegex(*regexValidIndexAnnotationAndKey, true),
+			Scope:         parser.AnnotationScopeLocation,
+			Risk:          parser.AnnotationRiskMedium,
+			Documentation: `This annotation can be used to specify an index file`,
+		},
+		fastCGIParamsAnnotation: {
+			Validator: parser.ValidateRegex(*parser.BasicCharsRegex, true),
+			Scope:     parser.AnnotationScopeLocation,
+			Risk:      parser.AnnotationRiskMedium,
+			Documentation: `This annotation can be used to specify a ConfigMap containing the fastcgi parameters as a key/value.
+			Only ConfigMaps on the same namespace of ingress can be used. They key and value from ConfigMap are validated for unauthorized characters.`,
+		},
+	},
+}
+
 type fastcgi struct {
-	r resolver.Resolver
+	r                resolver.Resolver
+	annotationConfig parser.Annotation
 }
 
 // Config describes the per location fastcgi config
@@ -57,7 +89,10 @@ func (l1 *Config) Equal(l2 *Config) bool {
 
 // NewParser creates a new fastcgiConfig protocol annotation parser
 func NewParser(r resolver.Resolver) parser.IngressAnnotation {
-	return fastcgi{r}
+	return fastcgi{
+		r:                r,
+		annotationConfig: fastCGIAnnotations,
+	}
 }
 
 // ParseAnnotations parses the annotations contained in the ingress
@@ -70,14 +105,21 @@ func (a fastcgi) Parse(ing *networking.Ingress) (interface{}, error) {
 		return fcgiConfig, nil
 	}
 
-	index, err := parser.GetStringAnnotation("fastcgi-index", ing)
+	index, err := parser.GetStringAnnotation(fastCGIIndexAnnotation, ing, a.annotationConfig.Annotations)
 	if err != nil {
+		if ing_errors.IsValidationError(err) {
+			return fcgiConfig, err
+		}
 		index = ""
 	}
+
 	fcgiConfig.Index = index
 
-	cm, err := parser.GetStringAnnotation("fastcgi-params-configmap", ing)
+	cm, err := parser.GetStringAnnotation(fastCGIParamsAnnotation, ing, a.annotationConfig.Annotations)
 	if err != nil {
+		if ing_errors.IsValidationError(err) {
+			return fcgiConfig, err
+		}
 		return fcgiConfig, nil
 	}
 
@@ -100,7 +142,19 @@ func (a fastcgi) Parse(ing *networking.Ingress) (interface{}, error) {
 		}
 	}
 
+	for k, v := range cmap.Data {
+		if !regexValidIndexAnnotationAndKey.MatchString(k) || !parser.NGINXVariable.MatchString(v) {
+			klog.ErrorS(fmt.Errorf("fcgi contains invalid key or value"), "fcgi annotation error", "configmap", cmap.Name, "namespace", cmap.Namespace, "key", k, "value", v)
+			return fcgiConfig, ing_errors.NewValidationError(fastCGIParamsAnnotation)
+		}
+	}
+
+	fcgiConfig.Index = index
 	fcgiConfig.Params = cmap.Data
 
 	return fcgiConfig, nil
 }
+
+func (a fastcgi) GetDocumentation() parser.AnnotationFields {
+	return a.annotationConfig.Annotations
+}

internal/ingress/annotations/fastcgi/main_test.go

@@ -18,6 +18,7 @@ package fastcgi
 
 import (
 	"fmt"
+	"reflect"
 	"testing"
 
 	api "k8s.io/api/core/v1"
@@ -49,10 +50,16 @@ func buildIngress() *networking.Ingress {
 
 type mockConfigMap struct {
 	resolver.Mock
+	extraConfigMap map[string]map[string]string
 }
 
 func (m mockConfigMap) GetConfigMap(name string) (*api.ConfigMap, error) {
-	if name != "default/demo-configmap" && name != "otherns/demo-configmap" {
+	if m.extraConfigMap == nil {
+		m.extraConfigMap = make(map[string]map[string]string)
+	}
+	cmdata, ok := m.extraConfigMap[name]
+
+	if name != "default/demo-configmap" && name != "otherns/demo-configmap" && !ok {
 		return nil, fmt.Errorf("there is no configmap with name %v", name)
 	}
 
@@ -61,12 +68,17 @@ func (m mockConfigMap) GetConfigMap(name string) (*api.ConfigMap, error) {
 		return nil, fmt.Errorf("invalid configmap name")
 	}
 
+	data := map[string]string{"REDIRECT_STATUS": "200", "SERVER_NAME": "$server_name"}
+	if ok {
+		data = cmdata
+	}
+
 	return &api.ConfigMap{
 		ObjectMeta: meta_v1.ObjectMeta{
 			Namespace: cmns,
 			Name:      cmn,
 		},
-		Data: map[string]string{"REDIRECT_STATUS": "200", "SERVER_NAME": "$server_name"},
+		Data: data,
 	}, nil
 }
 
@@ -283,3 +295,111 @@ func TestConfigEquality(t *testing.T) {
 		t.Errorf("config4 should be equal to config")
 	}
 }
+
+func Test_fastcgi_Parse(t *testing.T) {
+
+	tests := []struct {
+		name          string
+		index         string
+		configmapname string
+		configmap     map[string]string
+		want          interface{}
+		wantErr       bool
+	}{
+		{
+			name:          "valid configuration",
+			index:         "indexxpto-92123.php",
+			configmapname: "default/fcgiconfig",
+			configmap: map[string]string{
+				"REQUEST_METHOD":  "$request_method",
+				"SCRIPT_FILENAME": "$document_root$fastcgi_script_name",
+			},
+			want: Config{
+				Index: "indexxpto-92123.php",
+				Params: map[string]string{
+					"REQUEST_METHOD":  "$request_method",
+					"SCRIPT_FILENAME": "$document_root$fastcgi_script_name",
+				},
+			},
+		},
+		{
+			name:          "invalid index name",
+			index:         "indexxpto-92123$xx.php",
+			configmapname: "default/fcgiconfig",
+			configmap: map[string]string{
+				"REQUEST_METHOD":  "$request_method",
+				"SCRIPT_FILENAME": "$document_root$fastcgi_script_name",
+			},
+			want:    Config{},
+			wantErr: true,
+		},
+		{
+			name:          "invalid configmap namespace",
+			index:         "indexxpto-92123.php",
+			configmapname: "otherns/fcgiconfig",
+			configmap: map[string]string{
+				"REQUEST_METHOD":  "$request_method",
+				"SCRIPT_FILENAME": "$document_root$fastcgi_script_name",
+			},
+			want:    Config{Index: "indexxpto-92123.php"},
+			wantErr: true,
+		},
+		{
+			name:          "invalid configmap namespace name",
+			index:         "indexxpto-92123.php",
+			configmapname: "otherns/fcgicon;{fig",
+			configmap: map[string]string{
+				"REQUEST_METHOD":  "$request_method",
+				"SCRIPT_FILENAME": "$document_root$fastcgi_script_name",
+			},
+			want:    Config{Index: "indexxpto-92123.php"},
+			wantErr: true,
+		},
+		{
+			name:          "invalid configmap values key",
+			index:         "indexxpto-92123.php",
+			configmapname: "default/fcgiconfig",
+			configmap: map[string]string{
+				"REQUEST_METHOD$XPTO": "$request_method",
+			},
+			want:    Config{Index: "indexxpto-92123.php"},
+			wantErr: true,
+		},
+		{
+			name:          "invalid configmap values val",
+			index:         "indexxpto-92123.php",
+			configmapname: "default/fcgiconfig",
+			configmap: map[string]string{
+				"REQUEST_METHOD_XPTO": "$request_method{test};a",
+			},
+			want:    Config{Index: "indexxpto-92123.php"},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+
+			ing := buildIngress()
+
+			data := map[string]string{}
+			data[parser.GetAnnotationWithPrefix("fastcgi-index")] = tt.index
+			data[parser.GetAnnotationWithPrefix("fastcgi-params-configmap")] = tt.configmapname
+			ing.SetAnnotations(data)
+
+			m := &mockConfigMap{
+				extraConfigMap: map[string]map[string]string{
+					tt.configmapname: tt.configmap,
+				},
+			}
+
+			got, err := NewParser(m).Parse(ing)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("fastcgi.Parse() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("fastcgi.Parse() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

internal/ingress/annotations/parser/validators.go

@@ -59,7 +59,8 @@ var SizeRegex = regexp.MustCompile("^(?i)[0-9]+[bkmg]?$")
 var (
 	// URLIsValidRegex is used on full URLs, containing query strings (:, ? and &)
 	URLIsValidRegex = regexp.MustCompile("^[" + alphaNumericChars + urlEnabledChars + "]*$")
-	// BasicChars is alphanumeric and ".", "-", "_", "~" and ":", usually used on simple host:port/path composition
+	// BasicChars is alphanumeric and ".", "-", "_", "~" and ":", usually used on simple host:port/path composition.
+	// This combination can also be used on fields that may contain characters like / (as ns/name)
 	BasicCharsRegex = regexp.MustCompile("^[/" + alphaNumericChars + "]*$")
 	// ExtendedChars is alphanumeric and ".", "-", "_", "~" and ":" plus "," and spaces, usually used on simple host:port/path composition
 	ExtendedChars = regexp.MustCompile("^[/" + extendedAlphaNumeric + "]*$")