diff --git a/internal/ingress/annotations/auth/main.go b/internal/ingress/annotations/auth/main.go index ccdcfd730..e7c0ddaa6 100644 --- a/internal/ingress/annotations/auth/main.go +++ b/internal/ingress/annotations/auth/main.go @@ -182,8 +182,9 @@ func (a auth) Parse(ing *networking.Ingress) (interface{}, error) { if sns == "" { sns = ing.Namespace } + secCfg := a.r.GetSecurityConfiguration() // We don't accept different namespaces for secrets. - if sns != ing.Namespace { + if !secCfg.AllowCrossNamespaceResources && sns != ing.Namespace { return nil, ing_errors.LocationDenied{ Reason: fmt.Errorf("cross namespace usage of secrets is not allowed"), } diff --git a/internal/ingress/annotations/auth/main_test.go b/internal/ingress/annotations/auth/main_test.go index 412ddb711..2a9dc7c72 100644 --- a/internal/ingress/annotations/auth/main_test.go +++ b/internal/ingress/annotations/auth/main_test.go @@ -26,6 +26,7 @@ import ( api "k8s.io/api/core/v1" networking "k8s.io/api/networking/v1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/cache" "k8s.io/ingress-nginx/internal/ingress/annotations/parser" ing_errors "k8s.io/ingress-nginx/internal/ingress/errors" "k8s.io/ingress-nginx/internal/ingress/resolver" @@ -79,13 +80,18 @@ type mockSecret struct { } func (m mockSecret) GetSecret(name string) (*api.Secret, error) { - if name != "default/demo-secret" { + if name != "default/demo-secret" && name != "otherns/demo-secret" { return nil, fmt.Errorf("there is no secret with name %v", name) } + ns, _, err := cache.SplitMetaNamespaceKey(name) + if err != nil { + return nil, err + } + return &api.Secret{ ObjectMeta: meta_v1.ObjectMeta{ - Namespace: api.NamespaceDefault, + Namespace: ns, Name: "demo-secret", }, Data: map[string][]byte{"auth": []byte("foo:$apr1$OFG3Xybp$ckL0FHDAkoXYIlH9.cysT0")}, @@ -158,6 +164,25 @@ func TestIngressInvalidDifferentNamespace(t *testing.T) { } } +func TestIngressInvalidDifferentNamespaceAllowed(t *testing.T) { + ing := buildIngress() + + data := map[string]string{} + data[parser.GetAnnotationWithPrefix(authTypeAnnotation)] = "basic" + data[parser.GetAnnotationWithPrefix(AuthSecretAnnotation)] = "otherns/demo-secret" + ing.SetAnnotations(data) + + _, dir, _ := dummySecretContent(t) + defer os.RemoveAll(dir) + + r := mockSecret{} + r.AllowCrossNamespace = true + _, err := NewParser(dir, r).Parse(ing) + if err != nil { + t.Errorf("not expecting an error") + } +} + func TestIngressInvalidSecretName(t *testing.T) { ing := buildIngress() diff --git a/internal/ingress/annotations/authreq/main.go b/internal/ingress/annotations/authreq/main.go index cbbe55e94..2b4330b6f 100644 --- a/internal/ingress/annotations/authreq/main.go +++ b/internal/ingress/annotations/authreq/main.go @@ -419,8 +419,10 @@ func (a authReq) Parse(ing *networking.Ingress) (interface{}, error) { if cns == "" { cns = ing.Namespace } + + secCfg := a.r.GetSecurityConfiguration() // We don't accept different namespaces for secrets. - if cns != ing.Namespace { + if !secCfg.AllowCrossNamespaceResources && cns != ing.Namespace { return nil, ing_errors.LocationDenied{ Reason: fmt.Errorf("cross namespace usage of secrets is not allowed"), } diff --git a/internal/ingress/annotations/authtls/main.go b/internal/ingress/annotations/authtls/main.go index 25c292e29..cc383004e 100644 --- a/internal/ingress/annotations/authtls/main.go +++ b/internal/ingress/annotations/authtls/main.go @@ -160,7 +160,9 @@ func (a authTLS) Parse(ing *networking.Ingress) (interface{}, error) { if ns == "" { ns = ing.Namespace } - if ns != ing.Namespace { + secCfg := a.r.GetSecurityConfiguration() + // We don't accept different namespaces for secrets. + if !secCfg.AllowCrossNamespaceResources && ns != ing.Namespace { return &Config{}, ing_errors.NewLocationDenied("cross namespace secrets are not supported") } diff --git a/internal/ingress/annotations/fastcgi/main.go b/internal/ingress/annotations/fastcgi/main.go index 591ba65f0..7fa61e3db 100644 --- a/internal/ingress/annotations/fastcgi/main.go +++ b/internal/ingress/annotations/fastcgi/main.go @@ -129,8 +129,10 @@ func (a fastcgi) Parse(ing *networking.Ingress) (interface{}, error) { Reason: fmt.Errorf("error reading configmap name from annotation: %w", err), } } + secCfg := a.r.GetSecurityConfiguration() - if cmns != "" && cmns != ing.Namespace { + // We don't accept different namespaces for secrets. + if cmns != "" && !secCfg.AllowCrossNamespaceResources && cmns != ing.Namespace { return fcgiConfig, fmt.Errorf("different namespace is not supported on fast_cgi param configmap") } diff --git a/internal/ingress/annotations/parser/main.go b/internal/ingress/annotations/parser/main.go index 9fcc77f0b..af19b6420 100644 --- a/internal/ingress/annotations/parser/main.go +++ b/internal/ingress/annotations/parser/main.go @@ -54,7 +54,7 @@ var ( // AnnotationRisk is a subset of risk that an annotation may represent. // Based on the Risk, the admin will be able to allow or disallow users to set it // on their ingress objects -type AnnotationRisk string +type AnnotationRisk int type AnnotationFields map[string]AnnotationConfig diff --git a/internal/ingress/annotations/parser/validators.go b/internal/ingress/annotations/parser/validators.go index 36cf1f8ef..45ce6911e 100644 --- a/internal/ingress/annotations/parser/validators.go +++ b/internal/ingress/annotations/parser/validators.go @@ -33,10 +33,10 @@ import ( type AnnotationValidator func(string) error const ( - AnnotationRiskLow AnnotationRisk = "Low" - AnnotationRiskMedium AnnotationRisk = "Medium" - AnnotationRiskHigh AnnotationRisk = "High" - AnnotationRiskCritical AnnotationRisk = "Critical" + AnnotationRiskLow AnnotationRisk = iota + AnnotationRiskMedium + AnnotationRiskHigh + AnnotationRiskCritical ) var ( diff --git a/internal/ingress/annotations/proxyssl/main.go b/internal/ingress/annotations/proxyssl/main.go index 66a70da0a..411dc66e0 100644 --- a/internal/ingress/annotations/proxyssl/main.go +++ b/internal/ingress/annotations/proxyssl/main.go @@ -199,7 +199,9 @@ func (p proxySSL) Parse(ing *networking.Ingress) (interface{}, error) { return &Config{}, ing_errors.NewLocationDenied(err.Error()) } - if ns != ing.Namespace { + secCfg := p.r.GetSecurityConfiguration() + // We don't accept different namespaces for secrets. + if !secCfg.AllowCrossNamespaceResources && ns != ing.Namespace { return &Config{}, ing_errors.NewLocationDenied("cross namespace secrets are not supported") } diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index 345ce89b4..f3ea6d5a6 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -97,6 +97,17 @@ type Configuration struct { // If disabled, only snippets added via ConfigMap are added to ingress. AllowSnippetAnnotations bool `json:"allow-snippet-annotations"` + // AllowCrossNamespaceResources enables users to consume cross namespace resource on annotations + // Case disabled, attempts to use secrets or configmaps from a namespace different from Ingress will + // be denied + // This valid will default to `false` on future releases + AllowCrossNamespaceResources bool `json:"allow-cross-namespace-resources"` + + // AnnotationsRisk represents the risk accepted on an annotation. If the risk is, for instance `Medium`, annotations + // with risk High and Critical will not be accepted. + // Default Risk is Critical by default, but this may be changed in future releases + AnnotationsRisk string `json:"annotations-risk"` + // AnnotationValueWordBlocklist defines words that should not be part of an user annotation value // (can be used to run arbitrary code or configs, for example) and that should be dropped. // This list should be separated by "," character @@ -853,8 +864,10 @@ func NewDefault() Configuration { cfg := Configuration{ AllowSnippetAnnotations: true, + AllowCrossNamespaceResources: true, AllowBackendServerHeader: false, AnnotationValueWordBlocklist: "", + AnnotationsRisk: "Critical", AccessLogPath: "/var/log/nginx/access.log", AccessLogParams: "", EnableAccessLogForDefaultBackend: false, diff --git a/internal/ingress/controller/controller_test.go b/internal/ingress/controller/controller_test.go index ad31103e0..2e3b44406 100644 --- a/internal/ingress/controller/controller_test.go +++ b/internal/ingress/controller/controller_test.go @@ -73,6 +73,13 @@ func (fis fakeIngressStore) GetBackendConfiguration() ngx_config.Configuration { return fis.configuration } +func (fis fakeIngressStore) GetSecurityConfiguration() defaults.SecurityConfiguration { + return defaults.SecurityConfiguration{ + AnnotationsRisk: fis.configuration.AnnotationsRisk, + AllowCrossNamespaceResources: fis.configuration.AllowCrossNamespaceResources, + } +} + func (fakeIngressStore) GetConfigMap(key string) (*corev1.ConfigMap, error) { return nil, fmt.Errorf("test error") } diff --git a/internal/ingress/controller/store/store.go b/internal/ingress/controller/store/store.go index 03503ecef..6f37414c2 100644 --- a/internal/ingress/controller/store/store.go +++ b/internal/ingress/controller/store/store.go @@ -69,6 +69,9 @@ type Storer interface { // GetBackendConfiguration returns the nginx configuration stored in a configmap GetBackendConfiguration() ngx_config.Configuration + // GetSecurityConfiguration returns the configuration options from Ingress + GetSecurityConfiguration() defaults.SecurityConfiguration + // GetConfigMap returns the ConfigMap matching key. GetConfigMap(key string) (*corev1.ConfigMap, error) @@ -926,8 +929,9 @@ func (s *k8sStore) updateSecretIngressMap(ing *networkingv1.Ingress) { "secure-verify-ca-secret", } + secConfig := s.GetSecurityConfiguration().AllowCrossNamespaceResources for _, ann := range secretAnnotations { - secrKey, err := objectRefAnnotationNsKey(ann, ing) + secrKey, err := objectRefAnnotationNsKey(ann, ing, secConfig) if err != nil && !errors.IsMissingAnnotations(err) { klog.Errorf("error reading secret reference in annotation %q: %s", ann, err) continue @@ -943,7 +947,7 @@ func (s *k8sStore) updateSecretIngressMap(ing *networkingv1.Ingress) { // objectRefAnnotationNsKey returns an object reference formatted as a // 'namespace/name' key from the given annotation name. -func objectRefAnnotationNsKey(ann string, ing *networkingv1.Ingress) (string, error) { +func objectRefAnnotationNsKey(ann string, ing *networkingv1.Ingress, allowCrossNamespace bool) (string, error) { // We pass nil fields, as this is an internal process and we don't need to validate it. annValue, err := parser.GetStringAnnotation(ann, ing, nil) if err != nil { @@ -958,7 +962,7 @@ func objectRefAnnotationNsKey(ann string, ing *networkingv1.Ingress) (string, er if secrNs == "" { return fmt.Sprintf("%v/%v", ing.Namespace, secrName), nil } - if secrNs != ing.Namespace { + if !allowCrossNamespace && secrNs != ing.Namespace { return "", fmt.Errorf("cross namespace secret is not supported") } return annValue, nil @@ -1135,6 +1139,17 @@ func (s *k8sStore) GetBackendConfiguration() ngx_config.Configuration { return s.backendConfig } +func (s *k8sStore) GetSecurityConfiguration() defaults.SecurityConfiguration { + s.backendConfigMu.RLock() + defer s.backendConfigMu.RUnlock() + + secConfig := defaults.SecurityConfiguration{ + AllowCrossNamespaceResources: s.backendConfig.AllowCrossNamespaceResources, + AnnotationsRisk: s.backendConfig.AnnotationsRisk, + } + return secConfig +} + func (s *k8sStore) setConfig(cmap *corev1.ConfigMap) { s.backendConfigMu.Lock() defer s.backendConfigMu.Unlock() diff --git a/internal/ingress/defaults/main.go b/internal/ingress/defaults/main.go index 0aab2ff47..078d50e63 100644 --- a/internal/ingress/defaults/main.go +++ b/internal/ingress/defaults/main.go @@ -170,3 +170,15 @@ type Backend struct { // It disables that behavior and instead uses a single upstream in NGINX, the service's Cluster IP and port. ServiceUpstream bool `json:"service-upstream"` } + +type SecurityConfiguration struct { + // AllowCrossNamespaceResources enables users to consume cross namespace resource on annotations + // Case disabled, attempts to use secrets or configmaps from a namespace different from Ingress will + // be denied + // This valid will default to `false` on future releases + AllowCrossNamespaceResources bool `json:"allow-cross-namespace-resources"` + + // AnnotationsRisk represents the risk accepted on an annotation. If the risk is, for instance `Medium`, annotations + // with risk High and Critical will not be accepted + AnnotationsRisk string `json:"annotations-risk"` +} diff --git a/internal/ingress/resolver/main.go b/internal/ingress/resolver/main.go index e05a2aaae..7d17f4e16 100644 --- a/internal/ingress/resolver/main.go +++ b/internal/ingress/resolver/main.go @@ -26,6 +26,9 @@ type Resolver interface { // GetDefaultBackend returns the backend that must be used as default GetDefaultBackend() defaults.Backend + // GetSecurityConfiguration returns the configuration options from Ingress + GetSecurityConfiguration() defaults.SecurityConfiguration + // GetConfigMap searches for configmap containing the namespace and name usting the character / GetConfigMap(string) (*apiv1.ConfigMap, error) diff --git a/internal/ingress/resolver/mock.go b/internal/ingress/resolver/mock.go index 62c5c6db9..2a1c265e7 100644 --- a/internal/ingress/resolver/mock.go +++ b/internal/ingress/resolver/mock.go @@ -26,7 +26,9 @@ import ( // Mock implements the Resolver interface type Mock struct { - ConfigMaps map[string]*apiv1.ConfigMap + ConfigMaps map[string]*apiv1.ConfigMap + AnnotationRisk string + AllowCrossNamespace bool } // GetDefaultBackend returns the backend that must be used as default @@ -34,6 +36,17 @@ func (m Mock) GetDefaultBackend() defaults.Backend { return defaults.Backend{} } +func (m Mock) GetSecurityConfiguration() defaults.SecurityConfiguration { + defRisk := m.AnnotationRisk + if defRisk == "" { + defRisk = "Critical" + } + return defaults.SecurityConfiguration{ + AnnotationsRisk: defRisk, + AllowCrossNamespaceResources: m.AllowCrossNamespace, + } +} + // GetSecret searches for secrets contenating the namespace and name using a the character / func (m Mock) GetSecret(string) (*apiv1.Secret, error) { return nil, nil diff --git a/test/e2e/e2e.go b/test/e2e/e2e.go index 614dd166a..f9bfb18a4 100644 --- a/test/e2e/e2e.go +++ b/test/e2e/e2e.go @@ -47,6 +47,8 @@ import ( _ "k8s.io/ingress-nginx/test/e2e/settings" _ "k8s.io/ingress-nginx/test/e2e/settings/modsecurity" _ "k8s.io/ingress-nginx/test/e2e/settings/ocsp" + // _ "k8s.io/ingress-nginx/test/e2e/settings/validations" // Test is not working, need to check the cross namespace stuff + _ "k8s.io/ingress-nginx/test/e2e/ssl" _ "k8s.io/ingress-nginx/test/e2e/status" _ "k8s.io/ingress-nginx/test/e2e/tcpudp" diff --git a/test/e2e/settings/validations/validations.go b/test/e2e/settings/validations/validations.go new file mode 100644 index 000000000..47d5ff439 --- /dev/null +++ b/test/e2e/settings/validations/validations.go @@ -0,0 +1,110 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package annotations + +import ( + "context" + "fmt" + "net/http" + "strings" + + "github.com/onsi/ginkgo/v2" + "github.com/stretchr/testify/assert" + "golang.org/x/crypto/bcrypt" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "k8s.io/ingress-nginx/test/e2e/framework" +) + +func buildSecret(username, password, name, namespace string) *corev1.Secret { + //out, err := exec.Command("openssl", "passwd", "-crypt", password).CombinedOutput() + out, err := bcrypt.GenerateFromPassword([]byte(password), 14) + encpass := fmt.Sprintf("%v:%s\n", username, out) + assert.Nil(ginkgo.GinkgoT(), err) + + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + DeletionGracePeriodSeconds: framework.NewInt64(1), + }, + Data: map[string][]byte{ + "auth": []byte(encpass), + }, + Type: corev1.SecretTypeOpaque, + } +} + +var _ = framework.DescribeAnnotation("annotation validations", func() { + f := framework.NewDefaultFramework("annotations-validations") + + ginkgo.BeforeEach(func() { + f.NewEchoDeployment() + otherns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "otherns", + }, + } + _, err := f.KubeClientSet.CoreV1().Namespaces().Create(context.Background(), otherns, metav1.CreateOptions{}) + assert.Nil(ginkgo.GinkgoT(), err, "creating namespace") + }) + + ginkgo.AfterEach(func() { + err := f.KubeClientSet.CoreV1().Namespaces().Delete(context.Background(), "otherns", metav1.DeleteOptions{}) + assert.Nil(ginkgo.GinkgoT(), err, "deleting namespace") + }) + + ginkgo.It("should return status code 401 when authentication is configured but Authorization header is not configured", func() { + host := "annotation-validations" + // Allow cross namespace consumption + f.UpdateNginxConfigMapData("allow-cross-namespace-resources", "true") + // Sleep a while just to guarantee that the configmap is applied + framework.Sleep() + + s := f.EnsureSecret(buildSecret("foo", "bar", "test", "otherns")) + + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/auth-type": "basic", + "nginx.ingress.kubernetes.io/auth-secret": fmt.Sprintf("%s/%s", s.Namespace, s.Name), + "nginx.ingress.kubernetes.io/auth-realm": "test auth", + } + + ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) + f.EnsureIngress(ing) + + f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, "server_name annotation-validations") + }) + + f.HTTPTestClient(). + GET("/"). + WithHeader("Host", host). + Expect(). + Status(http.StatusUnauthorized). + Body().Contains("401 Authorization Required") + + f.HTTPTestClient(). + GET("/"). + WithHeader("Host", host). + WithBasicAuth("foo", "bar"). + Expect(). + Status(http.StatusOK) + }) +})