diff --git a/internal/controller/consumer_controller.go b/internal/controller/consumer_controller.go index 230255a2..a50eb50a 100644 --- a/internal/controller/consumer_controller.go +++ b/internal/controller/consumer_controller.go @@ -20,6 +20,7 @@ package controller import ( "context" "fmt" + "slices" "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" @@ -47,6 +48,8 @@ import ( pkgutils "github.com/apache/apisix-ingress-controller/pkg/utils" ) +const keyAuthType = "key-auth" + // ConsumerReconciler reconciles a Gateway object. type ConsumerReconciler struct { //nolint:revive client.Client @@ -103,6 +106,9 @@ func (r *ConsumerReconciler) SetupWithManager(mgr ctrl.Manager) error { }, ), ). + Watches(&v1alpha1.Consumer{}, + handler.EnqueueRequestsFromMapFunc(r.listConsumersSharingKeyAuthKeys), + ). Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.listConsumersForSecret), ). @@ -118,7 +124,7 @@ func (r *ConsumerReconciler) listConsumersForSecret(ctx context.Context, obj cli r.Log.Error(nil, "failed to convert to Secret", "object", obj) return nil } - return ListRequests( + requests := ListRequests( ctx, r.Client, r.Log, @@ -127,6 +133,51 @@ func (r *ConsumerReconciler) listConsumersForSecret(ctx context.Context, obj cli indexer.SecretIndexRef: indexer.GenIndexKey(secret.GetNamespace(), secret.GetName()), }, ) + return append(requests, r.listKeyAuthKeyOwners(ctx, string(secret.Data["key"]))...) +} + +// listConsumersSharingKeyAuthKeys requeues Consumers sharing a key-auth key with +// obj, so a credential skipped as a duplicate returns once the key is free. +func (r *ConsumerReconciler) listConsumersSharingKeyAuthKeys(ctx context.Context, obj client.Object) []reconcile.Request { + consumer, ok := obj.(*v1alpha1.Consumer) + if !ok { + r.Log.Error(nil, "failed to convert to Consumer", "object", obj) + return nil + } + keys := make([]string, 0, len(consumer.Spec.Credentials)) + for _, credential := range consumer.Spec.Credentials { + if credential.Type != keyAuthType { + continue + } + if credential.SecretRef == nil { + keys = append(keys, indexer.InlineKeyAuthKey(credential)) + continue + } + var secret corev1.Secret + if err := r.Get(ctx, credentialSecretNN(consumer, credential), &secret); err != nil { + continue + } + keys = append(keys, string(secret.Data["key"])) + } + return r.listKeyAuthKeyOwners(ctx, keys...) +} + +func (r *ConsumerReconciler) listKeyAuthKeyOwners(ctx context.Context, keys ...string) []reconcile.Request { + var requests []reconcile.Request + for _, key := range keys { + if key == "" { + continue + } + owners, err := r.keyAuthKeyOwners(ctx, key) + if err != nil { + r.Log.Error(err, "failed to list consumers sharing a key-auth key") + continue + } + for i := range owners { + requests = append(requests, reconcile.Request{NamespacedName: utils.NamespacedName(&owners[i])}) + } + } + return requests } func (r *ConsumerReconciler) listConsumersForGateway(ctx context.Context, obj client.Object) []reconcile.Request { @@ -241,7 +292,15 @@ func (r *ConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c statusErr = err } - if err := r.Provider.Update(ctx, tctx, consumer); err != nil { + published, skipped, err := r.skipDuplicateKeyAuthCredentials(ctx, tctx, consumer) + if err != nil { + return ctrl.Result{}, err + } + if skipped > 0 { + statusErr = fmt.Errorf("%d key-auth credential(s) skipped: key already used by another Consumer", skipped) + } + + if err := r.Provider.Update(ctx, tctx, published); err != nil { r.Log.Error(err, "failed to update consumer", "consumer", utils.NamespacedName(consumer)) statusErr = err } @@ -293,6 +352,132 @@ func (r *ConsumerReconciler) processSpec(ctx context.Context, tctx *provider.Tra return loadPluginSecrets(ctx, r.Client, tctx, consumer.GetNamespace(), consumer.Spec.Plugins) } +// skipDuplicateKeyAuthCredentials drops key-auth credentials whose key an older +// Consumer on the same Gateway already uses. Admission alone misses a duplicate +// that arrives later, e.g. via a Secret created after its Consumer. +func (r *ConsumerReconciler) skipDuplicateKeyAuthCredentials(ctx context.Context, tctx *provider.TranslateContext, consumer *v1alpha1.Consumer) (*v1alpha1.Consumer, int, error) { + kept := make([]v1alpha1.Credential, 0, len(consumer.Spec.Credentials)) + for _, credential := range consumer.Spec.Credentials { + key := publishedKeyAuthKey(tctx, consumer, credential) + if key != "" { + taken, err := r.keyAuthKeyTaken(ctx, consumer, key) + if err != nil { + return nil, 0, err + } + if taken { + continue + } + } + kept = append(kept, credential) + } + skipped := len(consumer.Spec.Credentials) - len(kept) + if skipped == 0 { + return consumer, 0, nil + } + published := consumer.DeepCopy() + published.Spec.Credentials = kept + return published, skipped, nil +} + +// keyAuthKeyTaken reports whether an older Consumer on the same Gateway uses key. +func (r *ConsumerReconciler) keyAuthKeyTaken(ctx context.Context, consumer *v1alpha1.Consumer, key string) (bool, error) { + owners, err := r.keyAuthKeyOwners(ctx, key) + if err != nil { + return false, err + } + gatewayRef := indexer.ConsumerGatewayRefIndexFunc(consumer) + for i := range owners { + owner := &owners[i] + if owner.Namespace == consumer.Namespace && owner.Name == consumer.Name { + continue + } + if !slices.Equal(indexer.ConsumerGatewayRefIndexFunc(owner), gatewayRef) { + continue + } + if olderConsumer(owner, consumer) { + return true, nil + } + } + return false, nil +} + +// keyAuthKeyOwners lists the Consumers whose key-auth credentials use key. +func (r *ConsumerReconciler) keyAuthKeyOwners(ctx context.Context, key string) ([]v1alpha1.Consumer, error) { + index := client.MatchingFields{indexer.KeyAuthKey: indexer.GenKeyAuthKeyIndex(key)} + + var owners v1alpha1.ConsumerList + if err := r.List(ctx, &owners, index); err != nil { + return nil, err + } + var secrets corev1.SecretList + if err := r.List(ctx, &secrets, index); err != nil { + return nil, err + } + for _, secret := range secrets.Items { + var refs v1alpha1.ConsumerList + if err := r.List(ctx, &refs, client.MatchingFields{ + indexer.SecretIndexRef: indexer.GenIndexKey(secret.Namespace, secret.Name), + }); err != nil { + return nil, err + } + for i := range refs.Items { + uses, err := usesKeyAuthSecret(ctx, r.Client, &refs.Items[i], utils.NamespacedName(&secret)) + if err != nil { + return nil, err + } + if uses { + owners.Items = append(owners.Items, refs.Items[i]) + } + } + } + return owners.Items, nil +} + +// usesKeyAuthSecret reports whether consumer loads secretNN as a key-auth credential. +func usesKeyAuthSecret(ctx context.Context, c client.Client, consumer *v1alpha1.Consumer, secretNN types.NamespacedName) (bool, error) { + for _, credential := range consumer.Spec.Credentials { + if credential.Type != keyAuthType || credential.SecretRef == nil || credentialSecretNN(consumer, credential) != secretNN { + continue + } + if secretNN.Namespace == consumer.Namespace { + return true, nil + } + return CheckConsumerSecretRef(ctx, c, consumer.Namespace, secretNN) + } + return false, nil +} + +// publishedKeyAuthKey returns the key a key-auth credential publishes, or "". +func publishedKeyAuthKey(tctx *provider.TranslateContext, consumer *v1alpha1.Consumer, credential v1alpha1.Credential) string { + if credential.Type != keyAuthType { + return "" + } + if credential.SecretRef == nil { + return indexer.InlineKeyAuthKey(credential) + } + secret := tctx.Secrets[credentialSecretNN(consumer, credential)] + if secret == nil { + return "" + } + return string(secret.Data["key"]) +} + +func credentialSecretNN(consumer *v1alpha1.Consumer, credential v1alpha1.Credential) types.NamespacedName { + ns := consumer.Namespace + if credential.SecretRef.Namespace != nil { + ns = *credential.SecretRef.Namespace + } + return types.NamespacedName{Namespace: ns, Name: credential.SecretRef.Name} +} + +// olderConsumer orders by creation time, then by namespace/name for a stable tie-break. +func olderConsumer(a, b *v1alpha1.Consumer) bool { + if !a.CreationTimestamp.Equal(&b.CreationTimestamp) { + return a.CreationTimestamp.Before(&b.CreationTimestamp) + } + return utils.NamespacedName(a).String() < utils.NamespacedName(b).String() +} + func (r *ConsumerReconciler) updateStatus(consumer *v1alpha1.Consumer, err error) { condition := NewCondition(consumer.Generation, true, "Successfully") if err != nil { diff --git a/internal/controller/consumer_controller_test.go b/internal/controller/consumer_controller_test.go index 92278611..5290f21b 100644 --- a/internal/controller/consumer_controller_test.go +++ b/internal/controller/consumer_controller_test.go @@ -20,18 +20,22 @@ package controller import ( "context" "testing" + "time" "github.com/go-logr/logr" "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" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" clientgoscheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" "github.com/apache/apisix-ingress-controller/api/v1alpha1" + "github.com/apache/apisix-ingress-controller/internal/controller/indexer" "github.com/apache/apisix-ingress-controller/internal/provider" ) @@ -48,7 +52,11 @@ func buildConsumerReconciler(t *testing.T, objs ...runtime.Object) *ConsumerReco require.NoError(t, v1alpha1.AddToScheme(scheme)) require.NoError(t, gatewayv1.Install(scheme)) - cli := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objs...).Build() + cli := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objs...). + WithIndex(&v1alpha1.Consumer{}, indexer.SecretIndexRef, indexer.ConsumerSecretIndexFunc). + WithIndex(&v1alpha1.Consumer{}, indexer.KeyAuthKey, indexer.ConsumerKeyAuthKeyIndexFunc). + WithIndex(&corev1.Secret{}, indexer.KeyAuthKey, indexer.SecretKeyAuthKeyIndexFunc). + Build() return &ConsumerReconciler{Client: cli, Log: logr.Discard()} } @@ -144,3 +152,134 @@ func TestProcessSpec_SameNamespaceSecretRef_Allowed(t *testing.T) { require.NoError(t, err) require.Contains(t, tctx.Secrets, types.NamespacedName{Namespace: consumerNS, Name: "local-secret"}) } + +const sharedKey = "shared-key" + +func keyAuthConsumer(name, gateway string, created time.Time, credentials ...v1alpha1.Credential) *v1alpha1.Consumer { + return &v1alpha1.Consumer{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: consumerNS, + CreationTimestamp: metav1.NewTime(created.Truncate(time.Second)), + }, + Spec: v1alpha1.ConsumerSpec{ + GatewayRef: v1alpha1.GatewayRef{Name: gateway}, + Credentials: credentials, + }, + } +} + +func inlineKeyAuth(name, key string) v1alpha1.Credential { + return v1alpha1.Credential{ + Name: name, + Type: "key-auth", + Config: apiextensionsv1.JSON{Raw: []byte(`{"key":"` + key + `"}`)}, + } +} + +func secretKeyAuth(name, secret string, namespace *string) v1alpha1.Credential { + return v1alpha1.Credential{ + Name: name, + Type: "key-auth", + SecretRef: &v1alpha1.SecretReference{Name: secret, Namespace: namespace}, + } +} + +func keyAuthSecret(name, namespace string) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + Data: map[string][]byte{"key": []byte(sharedKey)}, + } +} + +func skipDuplicates(t *testing.T, r *ConsumerReconciler, consumer *v1alpha1.Consumer) (*v1alpha1.Consumer, int) { + t.Helper() + tctx := provider.NewDefaultTranslateContext(context.Background()) + require.NoError(t, r.processSpec(context.Background(), tctx, consumer)) + published, skipped, err := r.skipDuplicateKeyAuthCredentials(context.Background(), tctx, consumer) + require.NoError(t, err) + return published, skipped +} + +// A Secret created after its Consumer must not publish a key an older Consumer already uses. +func TestSkipDuplicateKeyAuth_SecretCreatedLater(t *testing.T) { + now := time.Now() + victim := keyAuthConsumer("victim", "gw", now.Add(-time.Hour), inlineKeyAuth("cred", sharedKey)) + attacker := keyAuthConsumer("attacker", "gw", now, + secretKeyAuth("dup", "late-secret", nil), + inlineKeyAuth("own", "attacker-key"), + ) + r := buildConsumerReconciler(t, victim, attacker, keyAuthSecret("late-secret", consumerNS)) + + published, skipped := skipDuplicates(t, r, attacker) + require.Equal(t, 1, skipped) + require.Len(t, published.Spec.Credentials, 1) + require.Equal(t, "own", published.Spec.Credentials[0].Name) + require.Len(t, attacker.Spec.Credentials, 2, "the cached Consumer must stay untouched") +} + +// The older Consumer keeps its key. +func TestSkipDuplicateKeyAuth_OlderConsumerKeepsKey(t *testing.T) { + now := time.Now() + victim := keyAuthConsumer("victim", "gw", now.Add(-time.Hour), inlineKeyAuth("cred", sharedKey)) + attacker := keyAuthConsumer("attacker", "gw", now, secretKeyAuth("dup", "late-secret", nil)) + r := buildConsumerReconciler(t, victim, attacker, keyAuthSecret("late-secret", consumerNS)) + + published, skipped := skipDuplicates(t, r, victim) + require.Zero(t, skipped) + require.Same(t, victim, published) +} + +// Equal creation times fall back to namespace/name, so exactly one side wins. +func TestSkipDuplicateKeyAuth_TieBreak(t *testing.T) { + now := time.Now() + a := keyAuthConsumer("a", "gw", now, inlineKeyAuth("cred", sharedKey)) + b := keyAuthConsumer("b", "gw", now, inlineKeyAuth("cred", sharedKey)) + r := buildConsumerReconciler(t, a, b) + + _, skippedA := skipDuplicates(t, r, a) + _, skippedB := skipDuplicates(t, r, b) + require.Zero(t, skippedA) + require.Equal(t, 1, skippedB) +} + +// Consumers on different Gateways may share a key. +func TestSkipDuplicateKeyAuth_OtherGatewayIgnored(t *testing.T) { + now := time.Now() + other := keyAuthConsumer("other", "other-gw", now.Add(-time.Hour), inlineKeyAuth("cred", sharedKey)) + consumer := keyAuthConsumer("consumer", "gw", now, inlineKeyAuth("cred", sharedKey)) + r := buildConsumerReconciler(t, other, consumer) + + _, skipped := skipDuplicates(t, r, consumer) + require.Zero(t, skipped) +} + +// A cross-namespace Secret that no ReferenceGrant permits is never published, so it holds no key. +func TestSkipDuplicateKeyAuth_UngrantedSecretOwnsNoKey(t *testing.T) { + SetEnableReferenceGrant(true) + defer SetEnableReferenceGrant(false) + + now := time.Now() + target := secretNS + ungranted := keyAuthConsumer("ungranted", "gw", now.Add(-time.Hour), secretKeyAuth("cred", "foreign", &target)) + consumer := keyAuthConsumer("consumer", "gw", now, inlineKeyAuth("cred", sharedKey)) + r := buildConsumerReconciler(t, ungranted, consumer, keyAuthSecret("foreign", secretNS)) + + _, skipped := skipDuplicates(t, r, consumer) + require.Zero(t, skipped) +} + +// Changes that free a key requeue the Consumer that was skipped for it. +func TestKeyAuthKeyOwnersRequeued(t *testing.T) { + now := time.Now() + victim := keyAuthConsumer("victim", "gw", now.Add(-time.Hour), inlineKeyAuth("cred", sharedKey)) + attacker := keyAuthConsumer("attacker", "gw", now, secretKeyAuth("dup", "late-secret", nil)) + secret := keyAuthSecret("late-secret", consumerNS) + r := buildConsumerReconciler(t, victim, attacker, secret) + + attackerReq := reconcile.Request{NamespacedName: types.NamespacedName{Namespace: consumerNS, Name: "attacker"}} + victimReq := reconcile.Request{NamespacedName: types.NamespacedName{Namespace: consumerNS, Name: "victim"}} + + require.Contains(t, r.listConsumersSharingKeyAuthKeys(context.Background(), victim), attackerReq) + require.Contains(t, r.listConsumersForSecret(context.Background(), secret), victimReq) +} diff --git a/internal/controller/indexer/indexer.go b/internal/controller/indexer/indexer.go index 0e3480cf..ab4944d1 100644 --- a/internal/controller/indexer/indexer.go +++ b/internal/controller/indexer/indexer.go @@ -20,6 +20,9 @@ package indexer import ( "cmp" "context" + "crypto/sha256" + "encoding/hex" + "encoding/json" corev1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" @@ -56,6 +59,7 @@ const ( ApisixUpstreamRef = "apisixUpstreamRef" PluginConfigIndexRef = "pluginConfigRefs" ControllerName = "controllerName" + KeyAuthKey = "keyAuthKey" ) func SetupIndexer(mgr ctrl.Manager) error { @@ -193,6 +197,22 @@ func setupConsumerIndexer(mgr ctrl.Manager) error { ); err != nil { return err } + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), + &v1alpha1.Consumer{}, + KeyAuthKey, + ConsumerKeyAuthKeyIndexFunc, + ); err != nil { + return err + } + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), + &corev1.Secret{}, + KeyAuthKey, + SecretKeyAuthKeyIndexFunc, + ); err != nil { + return err + } return nil } @@ -255,6 +275,48 @@ func ConsumerSecretIndexFunc(rawObj client.Object) []string { return secretKeys } +// GenKeyAuthKeyIndex hashes a key-auth key so the index never holds plaintext. +func GenKeyAuthKeyIndex(key string) string { + sum := sha256.Sum256([]byte(key)) + return hex.EncodeToString(sum[:]) +} + +// InlineKeyAuthKey returns the key of an inline key-auth credential, or "" if none. +func InlineKeyAuthKey(credential v1alpha1.Credential) string { + if credential.Type != "key-auth" || credential.SecretRef != nil || len(credential.Config.Raw) == 0 { + return "" + } + var cfg struct { + Key string `json:"key"` + } + if err := json.Unmarshal(credential.Config.Raw, &cfg); err != nil { + return "" + } + return cfg.Key +} + +// ConsumerKeyAuthKeyIndexFunc indexes Consumers by their inline key-auth keys. +func ConsumerKeyAuthKeyIndexFunc(rawObj client.Object) []string { + consumer := rawObj.(*v1alpha1.Consumer) + var keys []string + for _, credential := range consumer.Spec.Credentials { + if key := InlineKeyAuthKey(credential); key != "" { + keys = append(keys, GenKeyAuthKeyIndex(key)) + } + } + return keys +} + +// SecretKeyAuthKeyIndexFunc indexes Secrets by the key-auth key they hold. +func SecretKeyAuthKeyIndexFunc(rawObj client.Object) []string { + secret := rawObj.(*corev1.Secret) + key := secret.Data["key"] + if len(key) == 0 { + return nil + } + return []string{GenKeyAuthKeyIndex(string(key))} +} + func ConsumerGatewayRefIndexFunc(rawObj client.Object) []string { consumer := rawObj.(*v1alpha1.Consumer)