From f89e7c4e35965772e5973ef9737c3d4b29fdae21 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Fri, 16 Jun 2023 12:41:17 +0000 Subject: [PATCH] Add annotation validation for fcgi --- internal/ingress/annotations/fastcgi/main.go | 62 ++++++++- .../ingress/annotations/fastcgi/main_test.go | 124 +++++++++++++++++- .../ingress/annotations/parser/validators.go | 3 +- 3 files changed, 182 insertions(+), 7 deletions(-) diff --git a/internal/ingress/annotations/fastcgi/main.go b/internal/ingress/annotations/fastcgi/main.go index 84bac4109..591ba65f0 100644 --- a/internal/ingress/annotations/fastcgi/main.go +++ b/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 +} diff --git a/internal/ingress/annotations/fastcgi/main_test.go b/internal/ingress/annotations/fastcgi/main_test.go index 35c5bbc12..3296ded65 100644 --- a/internal/ingress/annotations/fastcgi/main_test.go +++ b/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) + } + }) + } +} diff --git a/internal/ingress/annotations/parser/validators.go b/internal/ingress/annotations/parser/validators.go index e0d4951cb..0baf6d9d3 100644 --- a/internal/ingress/annotations/parser/validators.go +++ b/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 + "]*$")