From 6a484706d74964de85b569c26bf86b636ec9fe01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Thu, 19 Oct 2023 12:40:59 +0200 Subject: [PATCH] fix: run basic credentials validation despite relation with consumers (#4887) --- CHANGELOG.md | 24 ++--- hack/deploy-admission-controller.sh | 1 + internal/admission/handler.go | 7 +- internal/admission/validator.go | 75 +++++++++------- internal/admission/validator_test.go | 59 +++++++++++++ test/integration/webhook_test.go | 127 ++++++--------------------- 6 files changed, 143 insertions(+), 150 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c54e2380c4..63fec9dd86 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -124,12 +124,12 @@ Adding a new version? You'll need three changes: [#4608](https://github.com/Kong/kubernetes-ingress-controller/pull/4608) - Do not parse error body when failed to get response from reloading declarative configurations to produce proper error log in such situations, - [#4666](https://github.com/Kong/kubernetes-ingress-controller/pull/4666) + [#4666](https://github.com/Kong/kubernetes-ingress-controller/pull/4666) - Set type meta of objects when adding them to caches and reference indexers to ensure that indexes of objects in reference indexers have correct object - kind. This ensures referece relations of objects are stored and indexed + kind. This ensures referece relations of objects are stored and indexed correctly. - [#4663](https://github.com/Kong/kubernetes-ingress-controller/pull/4663) + [#4663](https://github.com/Kong/kubernetes-ingress-controller/pull/4663) - Display Service ports on generated Kong services, instead of a static default value. This change is cosmetic only. [#4503](https://github.com/Kong/kubernetes-ingress-controller/pull/4503) @@ -150,7 +150,7 @@ Adding a new version? You'll need three changes: [#4641](https://github.com/Kong/kubernetes-ingress-controller/issues/4641) [#4643](https://github.com/Kong/kubernetes-ingress-controller/issues/4643) - Fix `Licenses` and `ConsumerGroups` missing in sanitized copies of Kong configuration. - [#4710](https://github.com/Kong/kubernetes-ingress-controller/pull/4710 + [#4710](https://github.com/Kong/kubernetes-ingress-controller/pull/4710) ## [2.11.1] @@ -203,7 +203,7 @@ Adding a new version? You'll need three changes: [#4211](https://github.com/Kong/kubernetes-ingress-controller/pull/4211) - Assign priorities to routes translated from Ingresses when parser translate them to expression based Kong routes. The assigning method is basically the - same as in Kong gateway's `traditional_compatible` router, except that + same as in Kong gateway's `traditional_compatible` router, except that `regex_priority` field in Kong traditional route is not supported. This method is adopted to keep the compatibility with traditional router on maximum effort. @@ -213,7 +213,7 @@ Adding a new version? You'll need three changes: [specification on priorities of matches in `HTTPRoute`][httproute-specification]. [#4296](https://github.com/Kong/kubernetes-ingress-controller/pull/4296) [#4434](https://github.com/Kong/kubernetes-ingress-controller/pull/4434) -- Assign priorities to routes translated from GRPCRoutes when the parser translates +- Assign priorities to routes translated from GRPCRoutes when the parser translates them to expression based Kong routes. The priority order follows the [specification on match priorities in GRPCRoute][grpcroute-specification]. [#4364](https://github.com/Kong/kubernetes-ingress-controller/pull/4364) @@ -230,7 +230,7 @@ Adding a new version? You'll need three changes: in terms of accepting data-plane traffic, but are ready to accept configuration updates. The controller will now send configuration to such Gateways and will actively monitor their readiness for accepting configuration updates. - [#4368](https://github.com/Kong/kubernetes-ingress-controller/pull/4368 + [#4368](https://github.com/Kong/kubernetes-ingress-controller/pull/4368) - `KongConsumer`, `KongConsumerGroup` `KongPlugin`, and `KongClusterPlugin` CRDs were extended with `Status.Conditions` field. It will contain the `Programmed` condition describing whether an object was successfully translated into Kong entities and sent to Kong. @@ -265,11 +265,11 @@ Adding a new version? You'll need three changes: - Changed the Gateway's readiness probe in all-in-one manifests from `/status` to `/status/ready`. Gateways will be considered ready only after an initial configuration is applied by the controller. - [#4368](https://github.com/Kong/kubernetes-ingress-controller/pull/4368 -- When translating to expression based Kong routes, annotations to specify + [#4368](https://github.com/Kong/kubernetes-ingress-controller/pull/4368) +- When translating to expression based Kong routes, annotations to specify protocols are translated to `protocols` field of the result Kong route, - instead of putting the conditions to match protocols inside expressions. - [#4422](https://github.com/Kong/kubernetes-ingress-controller/pull/4422) + instead of putting the conditions to match protocols inside expressions. + [#4422](https://github.com/Kong/kubernetes-ingress-controller/pull/4422) ### Fixed @@ -285,7 +285,7 @@ Adding a new version? You'll need three changes: - `Gateway` can now correctly update `AttachedRoutes` even if there are more than 100 `HttpRoute`s. [#4458](https://github.com/Kong/kubernetes-ingress-controller/pull/4458) - + [gojson]: https://github.com/goccy/go-json [httproute-specification]: https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRoute [grpcroute-specification]: https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1alpha2.GRPCRouteRule diff --git a/hack/deploy-admission-controller.sh b/hack/deploy-admission-controller.sh index 069a186b8c..2d4970033c 100755 --- a/hack/deploy-admission-controller.sh +++ b/hack/deploy-admission-controller.sh @@ -58,6 +58,7 @@ webhooks: apiVersions: - 'v1' operations: + - CREATE - UPDATE resources: - secrets diff --git a/internal/admission/handler.go b/internal/admission/handler.go index 1d90da6dba..169bf41af1 100644 --- a/internal/admission/handler.go +++ b/internal/admission/handler.go @@ -246,13 +246,8 @@ func (h RequestHandler) handleSecret( return responseBuilder.Allowed(true).Build(), nil } - // secrets are only validated on update because they must be referenced by a - // managed consumer in order for us to validate them, and because credentials - // validation also happens at the consumer side of the reference so a - // credentials secret can not be referenced without being validated. - switch request.Operation { //nolint:exhaustive - case admissionv1.Update: + case admissionv1.Update, admissionv1.Create: ok, message, err := h.Validator.ValidateCredential(ctx, secret) if err != nil { return nil, err diff --git a/internal/admission/validator.go b/internal/admission/validator.go index 155c3fbe8a..ae1a0f9b59 100644 --- a/internal/admission/validator.go +++ b/internal/admission/validator.go @@ -48,11 +48,17 @@ type AdminAPIServicesProvider interface { GetRoutesService() (kong.AbstractRouteService, bool) } +// ConsumerGetter is an interface for retrieving KongConsumers. +type ConsumerGetter interface { + ListAllConsumers(ctx context.Context) ([]kongv1.KongConsumer, error) +} + // KongHTTPValidator implements KongValidator interface to validate Kong // entities using the Admin API of Kong. type KongHTTPValidator struct { Logger logrus.FieldLogger SecretGetter kongstate.SecretGetter + ConsumerGetter ConsumerGetter ManagerClient client.Client AdminAPIServicesProvider AdminAPIServicesProvider ParserFeatures parser.FeatureFlags @@ -77,6 +83,7 @@ func NewKongHTTPValidator( return KongHTTPValidator{ Logger: logger, SecretGetter: &managerClientSecretGetter{managerClient: managerClient}, + ConsumerGetter: &managerClientConsumerGetter{managerClient: managerClient}, ManagerClient: managerClient, AdminAPIServicesProvider: servicesProvider, ParserFeatures: parserFeatures, @@ -234,48 +241,46 @@ func (validator KongHTTPValidator) ValidateCredential( ctx context.Context, secret corev1.Secret, ) (bool, string, error) { - // if the secret doesn't contain a type key it's not a credentials secret + // If the secret doesn't contain a type key it's not a credentials secret. _, ok := secret.Data[credsvalidation.TypeKey] if !ok { return true, "", nil } - // credentials are only validated if they are referenced by a managed consumer - // in the namespace, as such we pull a list of all consumers from the cached - // client to determine if the credentials are referenced. + // If we know it's a credentials secret, we can ensure its base-level validity. + if err := credsvalidation.ValidateCredentials(&secret); err != nil { + return false, fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, err), nil + } + + // Credentials are validated further for unique key constraints only if they are referenced by a managed consumer + // in the namespace, as such we pull a list of all consumers from the cached client to determine + // if the credentials are referenced. managedConsumers, err := validator.listManagedConsumers(ctx) if err != nil { return false, ErrTextConsumerUnretrievable, err } - // verify whether this secret is referenced by any managed consumer + // Verify whether this secret is referenced by any managed consumer. managedConsumersWithReferences := listManagedConsumersReferencingCredentialsSecret(secret, managedConsumers) if len(managedConsumersWithReferences) == 0 { - // if no managed consumers reference this secret, its considered - // unmanaged and we don't validate it unless it becomes referenced - // by a managed consumer at a later time. + // If no managed consumers reference this secret, its considered unmanaged, and we don't validate it + // unless it becomes referenced by a managed consumer at a later time. return true, "", nil } - // now that we know at least one managed consumer is referencing this - // secret we perform the base-level credentials secret validation. - if err := credsvalidation.ValidateCredentials(&secret); err != nil { - return false, ErrTextConsumerCredentialValidationFailed, err - } - - // if base-level validation passes we move on to create an index of - // all managed credentials so that we can verify that the updates to - // this secret are not in violation of any unique key constraints. + // If base-level validation passed and the credential is referenced by a consumer, + // we move on to create an index of all managed credentials so that we can verify that + // the updates to this secret are not in violation of any unique key constraints. ignoreSecrets := map[string]map[string]struct{}{secret.Namespace: {secret.Name: {}}} credentialsIndex, err := globalValidationIndexForCredentials(ctx, validator.ManagerClient, managedConsumers, ignoreSecrets) if err != nil { return false, ErrTextConsumerCredentialValidationFailed, err } - // the index is built, now validate that the newly updated secret + // The index is built, now validate that the newly updated secret // is not in violation of any constraints. if err := credentialsIndex.ValidateCredentialsForUniqueKeyConstraints(&secret); err != nil { - return false, ErrTextConsumerCredentialValidationFailed, err + return false, fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, err), nil } return true, "", nil @@ -467,17 +472,15 @@ func (noOpRoutesValidator) Validate(_ context.Context, _ *kong.Route) (bool, str // ----------------------------------------------------------------------------- func (validator KongHTTPValidator) listManagedConsumers(ctx context.Context) ([]*kongv1.KongConsumer, error) { - // gather a list of all consumers from the cached client - consumers := &kongv1.KongConsumerList{} - if err := validator.ManagerClient.List(ctx, consumers, &client.ListOptions{ - Namespace: corev1.NamespaceAll, - }); err != nil { - return nil, err + // Gather a list of all consumers from the cached client. + consumers, err := validator.ConsumerGetter.ListAllConsumers(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list consumers: %w", err) } - // reduce the consumer set to consumers managed by this controller + // Reduce the consumer set to consumers managed by this controller. managedConsumers := make([]*kongv1.KongConsumer, 0) - for _, consumer := range consumers.Items { + for _, consumer := range consumers { consumer := consumer if !validator.ingressClassMatcher(&consumer.ObjectMeta, annotations.IngressClassKey, annotations.ExactClassMatch) { @@ -526,10 +529,6 @@ func (validator KongHTTPValidator) validatePluginAgainstGatewaySchema(ctx contex return "", nil } -// ----------------------------------------------------------------------------- -// Private - Manager Client Secret Getter -// ----------------------------------------------------------------------------- - type managerClientSecretGetter struct { managerClient client.Client } @@ -541,3 +540,17 @@ func (m *managerClientSecretGetter) GetSecret(namespace, name string) (*corev1.S Name: name, }, secret) } + +type managerClientConsumerGetter struct { + managerClient client.Client +} + +func (m *managerClientConsumerGetter) ListAllConsumers(ctx context.Context) ([]kongv1.KongConsumer, error) { + consumers := &kongv1.KongConsumerList{} + if err := m.managerClient.List(ctx, consumers, &client.ListOptions{ + Namespace: corev1.NamespaceAll, + }); err != nil { + return nil, err + } + return consumers.Items, nil +} diff --git a/internal/admission/validator_test.go b/internal/admission/validator_test.go index 71dc5c7666..52b389535b 100644 --- a/internal/admission/validator_test.go +++ b/internal/admission/validator_test.go @@ -9,7 +9,9 @@ import ( "github.com/kong/go-kong/kong" "github.com/samber/lo" "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -565,3 +567,60 @@ func TestKongHTTPValidator_ValidateConsumerGroup(t *testing.T) { } func fakeClassMatcher(*metav1.ObjectMeta, string, annotations.ClassMatching) bool { return true } + +func TestKongHTTPValidator_ValidateCredential(t *testing.T) { + testCases := []struct { + name string + secret corev1.Secret + wantOK bool + wantMessage string + wantErrContains string + }{ + { + name: "valid key-auth credential with no consumers gets accepted", + secret: corev1.Secret{ + Data: map[string][]byte{ + "kongCredType": []byte("key-auth"), + "key": []byte("my-key"), + }, + }, + wantOK: true, + }, + { + name: "invalid key-auth credential with no consumers gets rejected", + secret: corev1.Secret{ + Data: map[string][]byte{ + "kongCredType": []byte("key-auth"), + // missing key + }, + }, + wantOK: false, + wantMessage: fmt.Sprintf("%s: %s", ErrTextConsumerCredentialValidationFailed, "invalid credentials secret, no data present"), + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + validator := KongHTTPValidator{ + ConsumerGetter: fakeConsumerGetter{}, + AdminAPIServicesProvider: fakeServicesProvider{}, + ingressClassMatcher: fakeClassMatcher, + Logger: logrus.New(), + } + + ok, msg, err := validator.ValidateCredential(context.Background(), tc.secret) + require.NoError(t, err) + assert.Equal(t, tc.wantOK, ok) + assert.Equal(t, tc.wantMessage, msg) + }) + } +} + +type fakeConsumerGetter struct { + consumers []kongv1.KongConsumer +} + +func (f fakeConsumerGetter) ListAllConsumers(context.Context) ([]kongv1.KongConsumer, error) { + return f.consumers, nil +} diff --git a/test/integration/webhook_test.go b/test/integration/webhook_test.go index 160b79a9dc..cc7906fb32 100644 --- a/test/integration/webhook_test.go +++ b/test/integration/webhook_test.go @@ -75,7 +75,7 @@ func TestValidationWebhook(t *testing.T) { APIVersions: []string{"v1"}, Resources: []string{"secrets"}, }, - Operations: []admregv1.OperationType{admregv1.Update}, + Operations: []admregv1.OperationType{admregv1.Create, admregv1.Update}, }, { Rule: admregv1.Rule{ @@ -303,36 +303,6 @@ func TestValidationWebhook(t *testing.T) { }, wantErr: false, }, - { - name: "a consumer with an invalid credential type should fail validation", - consumer: &kongv1.KongConsumer{ - ObjectMeta: metav1.ObjectMeta{ - Name: uuid.NewString(), - Annotations: map[string]string{ - annotations.IngressClassKey: consts.IngressClass, - }, - }, - Username: "junklawnmower", - CustomID: uuid.NewString(), - Credentials: []string{ - "junklawnmowercreds", - }, - }, - credentials: []*corev1.Secret{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "junklawnmowercreds", - }, - StringData: map[string]string{ - "kongCredType": "invalid-auth", - "username": "junklawnmower", - "password": "testpass", - }, - }, - }, - wantErr: true, - wantPartialErr: "invalid credential type", - }, { name: "a consumer referencing credentials secrets which do not yet exist should fail validation", consumer: &kongv1.KongConsumer{ @@ -451,31 +421,6 @@ func TestValidationWebhook(t *testing.T) { wantErr: true, wantPartialErr: "unique key constraint violated for username", }, - { - name: "secret with missing fields", - consumer: &kongv1.KongConsumer{ - ObjectMeta: metav1.ObjectMeta{ - Name: uuid.NewString(), - Annotations: map[string]string{ - annotations.IngressClassKey: consts.IngressClass, - }, - }, - Username: "missingpassword", - CustomID: uuid.NewString(), - Credentials: []string{ - "basic-auth-with-missing-fields", - }, - }, - credentials: []*corev1.Secret{ - { - TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Secret"}, - ObjectMeta: metav1.ObjectMeta{Name: "basic-auth-with-missing-fields"}, - StringData: map[string]string{"kongCredType": "basic-auth", "username": "foo"}, - }, - }, - wantErr: true, - wantPartialErr: "missing required field(s): password", - }, } { t.Run(tt.name, func(t *testing.T) { for _, credential := range tt.credentials { @@ -510,29 +455,35 @@ func TestValidationWebhook(t *testing.T) { }) } - t.Log("verifying that an invalid credential secret not yet referenced by a KongConsumer is not validated") + t.Log("verifying that an invalid credential secret not yet referenced by a KongConsumer fails validation") invalidCredential := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "brokenfence", }, StringData: map[string]string{ - "kongCredType": "invalid-auth", // not a valid credential type, but wont be validated until referenced by consumer + "kongCredType": "invalid-auth", // not a valid credential type "username": "brokenfence", "password": "testpass", }, } - invalidCredential, err = env.Cluster().Client().CoreV1().Secrets(ns.Name).Create(ctx, invalidCredential, metav1.CreateOptions{}) + _, err = env.Cluster().Client().CoreV1().Secrets(ns.Name).Create(ctx, invalidCredential, metav1.CreateOptions{}) + require.ErrorContains(t, err, "invalid credential type") + + t.Log("creating a valid credential secret to be referenced by a KongConsumer") + validCredential, err := env.Cluster().Client().CoreV1().Secrets(ns.Name).Create(ctx, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "brokenfence", + }, + StringData: map[string]string{ + "kongCredType": "basic-auth", + "username": "brokenfence", + "password": "testpass", + }, + }, metav1.CreateOptions{}) require.NoError(t, err) - defer func() { - if err := env.Cluster().Client().CoreV1().Secrets(ns.Name).Delete(ctx, invalidCredential.Name, metav1.DeleteOptions{}); err != nil { - if !apierrors.IsNotFound(err) { - assert.NoError(t, err) - } - } - }() - t.Log("an existing invalid credential that becomes referenced by a consumer fails consumer validation") - validConsumerLinkedToInvalidCredentials := &kongv1.KongConsumer{ + t.Log("verifying that valid credentials assigned to a consumer pass validation") + validConsumerLinkedToValidCredentials := &kongv1.KongConsumer{ ObjectMeta: metav1.ObjectMeta{ Name: uuid.NewString(), Annotations: map[string]string{ @@ -545,26 +496,16 @@ func TestValidationWebhook(t *testing.T) { "brokenfence", }, } - _, err = kongClient.ConfigurationV1().KongConsumers(ns.Name).Create(ctx, validConsumerLinkedToInvalidCredentials, metav1.CreateOptions{}) - require.Error(t, err, "a consumer that references an invalid credential can not be created") - require.Contains(t, err.Error(), "invalid credential type") + validConsumerLinkedToValidCredentials, err = kongClient.ConfigurationV1().KongConsumers(ns.Name).Create(ctx, validConsumerLinkedToValidCredentials, metav1.CreateOptions{}) + require.NoError(t, err) defer func() { - if err := kongClient.ConfigurationV1().KongConsumers(ns.Name).Delete(ctx, validConsumerLinkedToInvalidCredentials.Name, metav1.DeleteOptions{}); err != nil { + if err := kongClient.ConfigurationV1().KongConsumers(ns.Name).Delete(ctx, validConsumerLinkedToValidCredentials.Name, metav1.DeleteOptions{}); err != nil { if !apierrors.IsNotFound(err) { assert.NoError(t, err) } } }() - t.Log("fixing the invalid credentials") - invalidCredential.Data["kongCredType"] = []byte("basic-auth") - validCredential, err := env.Cluster().Client().CoreV1().Secrets(ns.Name).Update(ctx, invalidCredential, metav1.UpdateOptions{}) - require.NoError(t, err) - - t.Log("verifying that now that the credentials are fixed the consumer passes validation") - _, err = kongClient.ConfigurationV1().KongConsumers(ns.Name).Create(ctx, validConsumerLinkedToInvalidCredentials, metav1.CreateOptions{}) - require.NoError(t, err) - t.Log("verifying that the valid credentials which include a unique-constrained key can be updated in place") validCredential.Data["value"] = []byte("newpassword") validCredential, err = env.Cluster().Client().CoreV1().Secrets(ns.Name).Update(ctx, validCredential, metav1.UpdateOptions{}) @@ -576,10 +517,10 @@ func TestValidationWebhook(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "invalid credential type") - t.Log("verifying that if the referent consumer goes away the validation passes for updates that would make the credential invalid") - require.NoError(t, kongClient.ConfigurationV1().KongConsumers(ns.Name).Delete(ctx, validConsumerLinkedToInvalidCredentials.Name, metav1.DeleteOptions{})) + t.Log("verifying that if the referent consumer goes away the validation fails for updates that make the credential invalid") + require.NoError(t, kongClient.ConfigurationV1().KongConsumers(ns.Name).Delete(ctx, validConsumerLinkedToValidCredentials.Name, metav1.DeleteOptions{})) _, err = env.Cluster().Client().CoreV1().Secrets(ns.Name).Update(ctx, validCredential, metav1.UpdateOptions{}) - require.NoError(t, err) + require.ErrorContains(t, err, "invalid credential type") t.Log("verifying that a JWT credential which has keys with missing values fails validation") invalidJWTName := uuid.NewString() @@ -596,23 +537,7 @@ func TestValidationWebhook(t *testing.T) { }, } _, err = env.Cluster().Client().CoreV1().Secrets(ns.Name).Create(ctx, invalidJWT, metav1.CreateOptions{}) - require.NoError(t, err) - jwtConsumer := &kongv1.KongConsumer{ - ObjectMeta: metav1.ObjectMeta{ - Name: uuid.NewString(), - Annotations: map[string]string{ - annotations.IngressClassKey: consts.IngressClass, - }, - }, - Username: "bad-jwt-consumer", - CustomID: uuid.NewString(), - Credentials: []string{ - invalidJWTName, - }, - } - _, err = kongClient.ConfigurationV1().KongConsumers(ns.Name).Create(ctx, jwtConsumer, metav1.CreateOptions{}) - require.Error(t, err) - require.Contains(t, err.Error(), "some fields were invalid due to missing data: rsa_public_key, key, secret") + require.ErrorContains(t, err, "some fields were invalid due to missing data: rsa_public_key, key, secret") } func ensureWebhookService(ctx context.Context, t *testing.T, name string) {