Enforcing naming conventions for AWS Secrets Manager (and HashiCorp Vault) entries

[muenchhausen]

1. "As cluster operations, I want to force developers to follow namespace based naming conventions on AWS Secrets Manager,
because secrets are just presented as a single list in the AWS console without any grouping possibilty. There is no other way than naming convention to tidy up and to prevent naming clashes."

2. "As an IT security manager, I want to enforce in a simple way, that Team A cannot access secrets of Team B, because AssumeRole is for some setups far too complicated."

The proposed solution is an enforced naming convention defined as annotation within the namespaces in addition to #143.

Given:

As an example AWS Secrets Manager naming convention could look like folder names starting with
deployment stage, followed by a namespace and ending with a secret name:

 dev/team-a-namespace/order-service
 dev/team-b-namespace/delivery-service

kind: Namespace
metadata:
  name: team-a-namespace
  annotations:
    # this annotation checks naming convention for AWS Secrets Manager entries
    secretsmanager.amazonaws.com/naming: dev/team-a-namespace/.*

---
kind: Namespace
metadata:
  name: team-b-namespace
  annotations:
    # this annotation checks naming convention for AWS Secrets Manager entries
    secretsmanager.amazonaws.com/naming: dev/team-b-namespace/.*

# Team A - valid naming
apiVersion: 'kubernetes-client.io/v1'
kind: ExternalSecret
metadata:
  namespace: team-a-namespace
  name: order-service
secretDescriptor:
  backendType: secretsManager
  data:
    # valid naming:
    - key: dev/team-a-namespace/order-service
      name: password
      property: password
      backendType: secretsManager

---
# Team A - invalid naming
apiVersion: 'kubernetes-client.io/v1'
kind: ExternalSecret
metadata:
  namespace: team-a-namespace
  name: order-service
secretDescriptor:
  backendType: secretsManager
  data:
    # invalid naming:
    - key: order-service
      name: password
      property: password
      backendType: secretsManager

---
# Team B - no access
apiVersion: 'kubernetes-client.io/v1'
kind: ExternalSecret
metadata:
  namespace: team-b-namespace
  name: order-service
secretDescriptor:
  backendType: secretsManager
  data:
    # invalid naming:
    - key: dev/team-a-namespace/order-service
      name: password
      property: password
      backendType: secretsManager

When
Checking naming convention in the poller module

Then
Team A should be able to retrieve the first secret. Team B should be rejected, because
it does not follow the naming convention of the namespace.

A possible implementation
... could be the following (file poller.js):

const annotationNamingKey = 'secretsmanager.amazonaws.com/naming'
...
_isPermitted (namespace, descriptor) {
  const role = descriptor.roleArn
  let allowed = true
  let reason = ''

  if (!namespace.metadata.annotations) {
    return {
      allowed, reason
    }
  }

  // an empty annotation value allows access to all roles
  const re = new RegExp(namespace.metadata.annotations[annotationPermittedKey])

  if (!re.test(role)) {
    allowed = false
    reason = `namespace does not allow to assume role ${role}`
  }

  # PATCH start
  const externalData = descriptor.data || descriptor.properties
  const namingConvention = namespace.metadata.annotations[annotationNamingKey]

  if (namingConvention) {
    externalData.forEach((secretProperty, index) => {    
      const reNaming = new RegExp(namingConvention)
      if (!reNaming.test(secretProperty.key)) {
        allowed = false
        reason = `secret key ${secretProperty.key} does not match naming convention ${namingConvention} of the namespace`
      }
    })
  }
  # PATCH end

  return {
    allowed,
    reason
  }
}

What is your opinion? If this proposal is fine, I will work on a pull request including some tests.

[lokino]

I have the same problem. AWS Secrets Manager names get quickly a mess. Good idea! +1

[moolen]

Having a way to limit access absolutely makes sense to me. Two points from my perspective:

(1) Enforcing naming conventions (IMHO) should be done using the validation webhook.
If we validate the naming conventions on the controller side like you suggested, users (CI/CD, devs) can actually create those resources. But they are invalid or may fail silently due to misconfiguration.

(2) That brings me to the next point: We desperately need a status field on the resource to keep track of the state in sync cycle and eventual errors. If something fails dev will not be able to see if something went wrong. (I'll create an issue about that)

[muenchhausen]

Good points!

I have the feeling, that the naming convention is the responsibility of externalsecret controller, because

1. the controller needs to map AWS Secrets to Kubernetes secrets.
2. naming conventions should also work for other secret management systems like HashiCorp Vault

The AWS config should not need to be aware of godaddy externalsecrets. In future we could even continue to create an generic externalsecret resource, that
is mapping simply all AWS secrets in a special naming convention directly to secrets, so we do not need to to create so many externalsecret resources.
Let me know if we should move more in this direction.

I was already trying to use validation webhook for "usecase 1" and the new AssumeRole functionality #143 to achieve "usecase 2". I really like the new AssumeRole functionality - good work!

The problem I had is the complexity on AWS side. My AssumeRole configuration was too complex and error-prone. Adding validation hooks will add even more complexity.
I wasn't able to explain my colleagues what is going on here - "just to make sure, that Team A is not able to create externalsecrets referring to Team B". I have the feeling that all necessary AssumeRole roles, permissions and resource changes are too complex for some basic usecases.

Yes the status on the externalsecrets resource would help to debug and to reduce error log - I appreciate if you create an issue here :)

Let me know if you have further ideas to simplify the configuration.