SecretStore prefix incompatibility with dataFrom

[netguino]

Describe the bug When adding a prefix to the SecretStore, said prefix is not applied when making queries based on a dataFrom spec.

To Reproduce Steps to reproduce the behavior:

If one specifies the following SecretStore:

---
apiVersion: external-secrets.io/v1beta1
kind: SecretStore
metadata:
  name: ssm
spec:
  provider:
    aws:
      prefix: /foo
      service: ParameterStore
      region: us-west-2
      auth:
        jwt:
          serviceAccountRef:
            name: serviceaccount

followed by an ExternalSecret like:

---
apiVersion: external-secrets.io/v1beta1
kind: ExternalSecret
metadata:
  name: test
  namespace: default
spec:
  secretStoreRef:
    name: ssm
    kind: SecretStore
  target:
    name: secrets
  dataFrom:
  - find:
      path: /mysecrets
      name:
        regexp: .*

Upon applying this, you can see that the prefix isn't added properly:

permissions","path":"/mysecrets"}

{"level":"info","ts":1745508481.2321577,"logger":"provider.parameterstore","msg":"GetParametersByPath: access denied. using fallback to describe parameters. It is recommended to add ssm:GetParametersByPath permissions","path":"/mysecrets"}

Versions: Server Version: v1.32.3-eks-d0fe756 + helm-chart-0.16.1

Expected behavior Since the prefix is defined at SecretStore and not external secret, I would expect the prefix to be added resulting in: /foo/mysecrets.

[gusfcarvalho]

Hi @netguino . IIRC - the use case for prefix is exactly when you can't use path for whatever the reason ('path' is actually better as it reduces the permissions scope needed for your SecretStore). Did you try using either one of the two (path or prefix) instead of them combined? Why do you need them combined? IMO, it seems you can do things just fine with standard 'path' logic

[netguino]

hi @gusfcarvalho !

When I read the original discussion and implementation of prefix: issue + pr, it seems that the scenario was exactly what I had in mind.

Operators would set SecretStores for /dev/, /prod, /test etc, ( in different clusters, or different namespaces):

---
apiVersion: external-secrets.io/v1beta1
kind: SecretStore
metadata:
  name: ssm
spec:
  provider:
    aws:
      prefix: /dev
      service: ParameterStore
      region: us-west-2
      auth:
        jwt:
          serviceAccountRef:
            name: serviceaccount

Then the ExternalSecret manifest is exactly the same alongside all environments, and can technically be distributed alongside the app and applied by the developer, where they don't care where the secret comes from, except that it belong to their app:

---
apiVersion: external-secrets.io/v1beta1
kind: ExternalSecret
metadata:
  name: test
  namespace: default
spec:
  secretStoreRef:
    name: ssm
    kind: SecretStore
  target:
    name: mysecrets
  dataFrom:
  - find:
      path: /myapp
      name:
        regexp: .*

At least that's what I was thinking.

[gusfcarvalho]

this incompatibility is for sure not a bug - as it wasn't part of the feature request. Plus, it is way more nuanced than you might think. Path triggers a completely different API set to be called (iirc more succeptible to rate limiting). IMO It should not be used as an alternative for users as a replacement for passing the prefixes in the regexp.

apiVersion: external-secrets.io/v1beta1
kind: ExternalSecret
metadata:
  name: test
  namespace: default
spec:
  secretStoreRef:
    name: ssm
    kind: SecretStore
  target:
    name: mysecrets
  dataFrom:
  - find:
      name:
        regexp: "/myapp/.*"

should work as you intend it to.

[gusfcarvalho]

If you still believe your use case needs to prepend prefixes into paths 😄 just beware of the good old prefix: /foo and path: /foo/bar problem. Does the user want foo/foo/bar or foo/bar? Can the user really tell if they cannot see the SecretStore configuration, but can see the Parameterstore layout?

And of course - PRs are welcome 😄

[netguino]

this incompatibility is for sure not a bug - as it wasn't part of the feature request. Plus, it is way more nuanced than you might think. Path triggers a completely different API set to be called (iirc more succeptible to rate limiting). IMO It should not be used as an alternative for users as a replacement for passing the prefixes in the regexp.

apiVersion: external-secrets.io/v1beta1
kind: ExternalSecret
metadata:
  name: test
  namespace: default
spec:
  secretStoreRef:
    name: ssm
    kind: SecretStore
  target:
    name: mysecrets
  dataFrom:
  - find:
      name:
        regexp: "/myapp/.*"

should work as you intend it to.

It actually doesn't because doing it that way, requires access to the full ssm, since regexp gets all of them.

My service account is specifically limited to /dev/ so, doing the regexp way, it tries to list things from /prod.

I understand that it is a different API, but I guess the way it is designed, in this specific scenario, developers cannot be the ones in charge of writing their ExternalSecrets and instead must consume the Secret, that is provisioned by the externalsecrets objects an operator sets for them.

[netguino]

Which is ok btw. I'm fine by it, I guess I misunderstood the way prefix is supposed to work.

[gusfcarvalho]

this incompatibility is for sure not a bug - as it wasn't part of the feature request. Plus, it is way more nuanced than you might think. Path triggers a completely different API set to be called (iirc more succeptible to rate limiting). IMO It should not be used as an alternative for users as a replacement for passing the prefixes in the regexp.

apiVersion: external-secrets.io/v1beta1
kind: ExternalSecret
metadata:
  name: test
  namespace: default
spec:
  secretStoreRef:
    name: ssm
    kind: SecretStore
  target:
    name: mysecrets
  dataFrom:
  - find:
      name:
        regexp: "/myapp/.*"

should work as you intend it to.

It actually doesn't because doing it that way, requires access to the full ssm, since regexp gets all of them.

My service account is specifically limited to /dev/ so, doing the regexp way, it tries to list things from /prod.

I understand that it is a different API, but I guess the way it is designed, in this specific scenario, developers cannot be the ones in charge of writing their ExternalSecrets and instead must consume the Secret, that is provisioned by the externalsecrets objects an operator sets for them.

Yeah, I did understand your use case. I just believe what you're trying to achieve here is a bit of duct-tape for a system that didn't evolve well when requirements started to appear (for instance, .It is bad practice to leave both prod and dev credentials in the same ParameterStore of the same AWS Account. If not, dev can rate limit 'prod' access at any point in time and you don't want that).

If I were you, I'd redesign it 😄 as your issues are not going to be fixed just by leveraging eso in the cluster. As devs will likely still have direct or indirect access to ParameterStore's creds anyways.

Re: to this feature request - PRs for your feature request are welcome, but it needs to be done very thoughtfully on all of these possible conflicts I mentioned. As if someone is using it the way it works today, your change will now introduce breaking changes. And making it non-breaking-change means a weird (IMO) experience 😄. Since we are already v1 we cannot introduce breaking changes anymore.

[netguino]

Can you expand on:

As devs will likely still have direct or indirect access to ParameterStore's creds anyways.

The way I think about it, (pretending the whole path thing works ), it's the eso controller that has access to ssm via the service account creds. The developer simply consumes secrets. Are you saying that they have indirect access because they can read the secrets handed over to the app?

[gusfcarvalho]

well, there are so many possibilities 😄 If you are using a SecretStore (like in your example) and if your developers can create Pod Specs (after all, their apps must be something in Kubernetes), they can just create a pod that fetches everything from parameterstore by attaching the SA for ESO to that Deployment (they are all on the same namespace). The only way to make this secure is by implementing (a lot of) admission control logics (with either Kyverno or Gatekeeper), plus a lot of care and thought. Or, you know, fixing the root cause of this RBAC mess 😛

edit: you can also do pod service account permission as opposed to one single central SA for ESO. This means one SecretStore per pod, but would allow you to make it as granular to /env/team-namespace/myapp level. So they would be able to get other info from other apps within their namespace (which they probably should anyways)

[netguino]

This is perfect, it's really the discussion I was looking for.

It explains quite well the reasons why it shouldn't probably work the way I thought it would, and be used only for fetching from Data, instead of Datafrom.

Thank you for going through the though process with me, I really appreciate it.

[netguino]

I'll see if I can maybe update the docs to point this out.

[netguino]

@gusfcarvalho After a bit of thought, how is this any different from the usecase in the initial feature request?

Copying from the original:

What is the added value?
This would allow easier portability of manifests, which look for secrets in paths, between environments using the same AWS account. For example, one SecretStore could be configured to provide secrets from parameters with the prefix /dev/my-app and another could be configured to provide them from /test/my-app and then ExternalSecrets could be configured to look for db-password and just change the SecretStore in which the value is being looked up.

[gusfcarvalho]

as I said in my comment - the two sources of truth to handle the path input. if both prefix and path are set, you need to create a logic that figures out when there is a conflict, when there is not, and when to merge them.

I can tell you by making this happen on other providers this isn't easy :)

The initial feature request didn't really care about using that on dataFrom.find. Keys were known by developers (which IMO makes more sense)

[netguino]

as I said in my comment - the two sources of truth to handle the path input. if both prefix and path are set, you need to create a logic that figures out when there is a conflict, when there is not, and when to merge them.

I can tell you by making this happen on other providers this isn't easy :)

The initial feature request didn't really care about using that on dataFrom.find. Keys were known by developers (which IMO makes more sense)

Ok, I see your point, it's all about that last part: data vs dataFrom where in data it's a specific key, and in dataFrom it's a request for all keys under a specific expression. This is a problem due to how the code works, because in the dataFrom it's all together a different API that makes completely different calls to AWS, making the whole adding a prefix quite tricky.

Did I get that right?

[gusfcarvalho]

it is specific even to dataFrom.find. dataFrom.extract should not have that same issue.

But yes, your general understanding is correct.

Just to be clear, I think this is a feature request indeed - your use case makes sense, some companies will need to have that sort of separation that they cannot change on the source (AWS accounts) for whatever the reason. It's just that implementing it is tricky.

And - because this could be achieved with a different design setup (which enforces better security posture), I see no rush on developing this feature as a maintainer 😅. So this could be something added by the community , as long as the tricky cases are covered.