ClusterGenerator ignores namespace in serviceAccountRef

[jmalloc]

trafficstars

Describe the bug

When using a ClusterGenerator of kind ECRAuthorizationToken with JWT (IRSA) authentication, the controller attempts to lookup the service account in the namespace of the ExternalSecret, instead of the namespace specified in the serviceAccountRef of the ClusterGenerator.

The ExternalSecret then fails to resolve with the following error:

error processing spec.dataFrom[0].sourceRef.generatorRef, err: error using generator: unable to create aws session: ServiceAccount "external-secrets.ecr" not found

To Reproduce

Define a ClusterGenerator that refers to a service account in one namespace, and an ExternalSecret in a different namespace.

apiVersion: v1
kind: ServiceAccount
automountServiceAccountToken: true
metadata:
  namespace: kube-addons # SERVICE ACCOUNT NAMESPACE
  name: external-secrets.ecr
  annotations:
    eks.amazonaws.com/role-arn: arn:aws:iam::<REDACTED>:role/external-secrets.ecr
---  
apiVersion: generators.external-secrets.io/v1alpha1
kind: ClusterGenerator
metadata:
  name: aws-elastic-container-registry
spec:
  kind: ECRAuthorizationToken
  generator:
    ecrAuthorizationTokenSpec:
      auth:
        jwt:
          serviceAccountRef:
            namespace: kube-addons # CORRECTLY REFERS TO SERVICE ACCOUNT NAMESPACE
            name: external-secrets.ecr
      region: us-east-1
---
apiVersion: external-secrets.io/v1beta1
kind: ExternalSecret
metadata:
  namespace: argocd # EXTERNAL SECRET NAMESPACE
  name: ecr-helm
spec:
  dataFrom:
  - sourceRef:
      generatorRef:
        apiVersion: generators.external-secrets.io/v1alpha1
        kind: ClusterGenerator
        name: aws-elastic-container-registry
  refreshInterval: 6h
  target:
    creationPolicy: Owner
    deletionPolicy: Delete
    name: ecr-helm
    template:
      data:
        enableOCI: "true"
        name: ecr-helm
        password: '{{ .password }}'
        type: helm
        url: <REDACTED>.dkr.ecr.us-east-1.amazonaws.com
        username: '{{ .username }}'
      engineVersion: v2
      mergePolicy: Replace
      metadata:
        labels:
          argocd.argoproj.io/secret-type: repository

ESO version: 0.14.0 K8s version: v1.31.5-eks-8cce635

Expected behavior

The resolver/generator should use look in the namespace specified in the serviceAccountRef instead of in the namespace of the ExternalSecret.

Additional context

I'm not familiar with the codebase, but in an attempt to provide some more context, I've found that the call to credsFromServiceAccount() hardcodes false for the isClusterKind parameter, but this should probably be set based on whether or not the generator is a "virtual" generator created by a ClusterGenerator resource.

Perhaps an IsVirtual field could be added to ecr.Generator and a different instance returned based on whether or not the generator was obtained via a ClusterGenerator. I'm not sure if this is possible or logical given the existing registry of generators. Otherwise, I think the Generate() method of the v1alpha1.Generator interface would need to accept such a parameter.

Thank you!

[Skarlso]

Hello.

Yes, the ClusterGenerator is basically there so people don't have to define a generator in all namespaces. It's currently not there to provide cross namespace access to secrets.

So this isn't a bug, but a feature. :D I'm not saying lets not do it, I'm saying that it's an incremental approach to features a cluster generator can do.

[jmalloc]

Cool, good to know! What does the current usage of a ClusterGenerator look like? Would the user typically create a service account in each namespace, or is it likely that this just isn't something that's been encountered before (for example, using a password generator, etc)?

[Skarlso]

It is something that has been encountered I believe. Currently, the ClusterGenerator just simply is there so you don't have to define a generator in each namespace. Nothing more really. It's still expecting things to be at the location from which it was referred to. I.e. either the PushSecret or the ExternalSecret object's namespace.

[tstjerne-arg]

I'm having the same issue but for ACR. It would be nice if generators under a ClusterGenerator could work similarly to ClusterSecretStore and honor the namespace in the ServiceAccountRef.

I'm not very familiar with this code base either. Was curious about how it works on the store side and noticed this check for Azure keyvault whether the store is of the cluster variety (and uses the ServiceAccountRef namespace if it is): https://github.com/external-secrets/external-secrets/blob/9a1096dee3f5c60bf8b41ce21c0af55ab3b76241/pkg/provider/azure/keyvault/keyvault.go#L872-L874

[Skarlso]

So. ClusterGenerator is more akin to ClusterExternalSecret. Meaning, it will still not allow cross-namespace access. It's not a store. It's an object generator basically where the end result will be referenced by another object and used as a "secret provider". Meaning it cannot be allowed to access items outside the generated object's namespace. That will still be the focus. If this would be allowed to do so, it would mean cross-namespace secret access. That is a security vulnerability.

ClusterSecretStore is allowed cross-namespace. Hence, why we discourage using it. :) And encourage actually disabling ClusterSecretStore resource here https://external-secrets.io/latest/guides/security-best-practices/#3-selectively-disable-reconciliation-of-cluster-wide-resources.

[tstjerne-arg]

What would be your recommendation for someone who is trying to separate infra concerns (using ESO and ServiceAccount(s) with workload identity federation for retrieving secret values, and images) from application concerns (application deployment containing ExternalSecret objects to be reconciled) ?

[Skarlso]

Create a ClusterSecretStore that then developers can reference. From there, the application layer just creates ExternalSecret objects. If you have dynamic secrets that need to be created in a new Namespace, you can use a regex or a namespace prefix to select all those namespaces and use a ClusterExternalSecret to replicate secrets into that new namespace whenever the namespace is created. Call that a Bootstrapping of a namespace.

Once this is done, the Generators will now work as expected. These objects can be created by the admin or something like that and people outside of the namespace don't need to be aware of them.

[tstjerne-arg]

Create a ClusterSecretStore that then developers can reference. From there, the application layer just creates ExternalSecret objects. If you have dynamic secrets that need to be created in a new Namespace, you can use a regex or a namespace prefix to select all those namespaces and use a ClusterExternalSecret to replicate secrets into that new namespace whenever the namespace is created. Call that a Bootstrapping of a namespace.

This is what I had already ended up doing for values from an external kv (in my case Azure kv). Will consider previous comment about security, but maybe not quite as bad for a single-tenant case?

Once this is done, the Generators will now work as expected. These objects can be created by the admin or something like that and people outside of the namespace don't need to be aware of them.

This I'm not following. Manual applications after app deployment is exactly what I'm trying to get rid of. Especially the prospect of having to reconfigure workload identity federation if for some reason the application namespace {needs to be / ends up being} recreated.

Regarding expectations, my expectation for generators would be that "you can reference service accounts across namespaces" like how the documentation says "# note: you can reference service accounts across namespaces.".

In other words that there would be a path for generated credentials where a secret could be deployed in the application namespace while the workload identity federated ServiceAccount used for reconciliation exists in another namespace, similar to how one can accomplish this for stored credentials using a ClusterSecretStore, ergo my initial comment about ClusterGenerator.

[Skarlso]

Yeah, I understand what you are saying. The problem is two-fold.

1. We already have cluster scoped resources that blur the line between being a security risk or not. In fact, in most cases, the cluster resources are being disabled by our users.
2. The entire way of how generators work would need to be rewritten because right now, they just get the namespace and that's it. There is no notion of where the request came from, it's a virtual entity. Meaning, if we allow accessing cross namespace services we allow it for all generator types not just the cluster generator.

The second part means a massive refactor. Which we just don't have the capacity for right now. But the real concern is the first case. We could break the generators, they are alpha after all. It's just not how they were intended to be used at first. I'm sorry. :/ I know that this isn't satisfactory outcome of this, and I'm not saying outright no to this. Eventually, we might overhaul the generators, but right now, I can't give you a good timeline.

[gusfcarvalho]

FYI - regarding cross-namespace cluster generator access, (as can be read on #4532) - if you want to take a jab at implementing it - please do provide a design documentation first. This isn't as easy as it looks due to the dynamic aspect of the generators CRDs versus the static aspect of their pair SecretStore/ClusterSecretStores.

(and Thanks for the interest in the first place!)

[tstjerne-arg]

Thank you both for the feedback and your work on this project.

I'll take a look at the code, if nothing else I'll probably learn something. Good info on the dynamic/static difference, makes sense.

My off-the-top-of-my-head hypothesis would be to use some inversion-of-control-like scheme for namespaced objects (foregoing the cluster varieties) where the serviceAccountRef is just a name, which doesn't have to resolve to an actual object, and then it would be up to the ESO controller (for namespaces it has access to and adhering to some configuration on what to reconcile) which decides whether to honor the "wish" for reconciliation. This probably makes no sense, would break everything and isn't even how K8s works : ) ...but if I can refine it to anything that does make sense I'll send in a design document. : ) Cheers!

[Skarlso]

Thanks, that sounds good! :) Looking forward to reading it! :)

[gusfcarvalho]

Updating title to reflect the real nature of the bug - a ClusterGenerator limitation.