carvel-dev/secretgen-controller

introduce SecretTemplate to allow to "shape" secrets

Closed this issue · 6 comments

cppforlife commented

Describe the problem/challenge you have
Use cases:

  • i want to merge multiple secrets into one
  • i want to rename keys within secrets
  • i want to hide pieces of the secret so that it can get exported (only exporting ca.crt vs private key)

Describe the solution you'd like
introduce SecretTemplate CR that is able to read one or more secrets and produce another secret.

for example:

---
kind: SecretTemplate
metadata:
  name: combined-secret # will create combined-secret Secret
spec:
  resources:
  - secretRef:
      name: signing-key
  - secretRef:
      name: another
  ytt:
    templateContractV1:
      data.yml: |
        #@ load("@ytt:data", "data")
        metadata:
           annotations:
              foo: bar
        data:
           public.pem: #@ data.values.secrets["signing-key"].data.public
           password-value: #@ data.values.secrets.another.data.value

TBD:

  • should we allow name glob on resource refs?
  • should we allow referencing of configmaps?
  • how does baes64 encoding play in here?

Anything else you would like to add:
[Additional information that will assist in solving the issue.]

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you would like to work on this issue.

Samze commented

We're interested in helping with this feature and putting together a PR.

However along with the use cases above we also have the following additional usecases:

  • I want to create a secret with data from arbitrary k8s resources, e.g. load ip information from a Service.
  • I want to load arbitrary k8s resources whose name is dynamic e.g. Load a RabbitmqCluster resource using the name defined in a RMQUser resource that can found in the field .spec.rabbitmqClusterReference.name
  • I want to create a secret with a string that is static and pre-defined.
  • For simple cases, I want to use jsonpath syntax to define my templated secret
  • For advanced cases, I want to use ytt syntax to define my templated secret

So this SecretTemplate would become more flexible to support a wider range of inputs.

Based on the combination of these requirements we have sketched out what a resource could look like. This Resource allows arbitrary input resources and allows either jsonpath or ytt style templating.

---
apiVersion: secretgen.k14s.io/v1alpha1
kind: SecretTemplate
metadata:
  name: generated-secret
spec:

  # ServiceAccount whose identity is used to `get` inputResources.  This would only 
  # require the RBAC to get a resource and its status.  Other RBAC necessary for this 
  # controller but not associated with the input resources will be supplied a-priori to 
  # the controller e.g. the RBAC necessary to create and update the templated Secret.
  serviceAccountName: my-resource-reader
  
  # An array of resources which have values on their API pertinent to creation of the 
  # final Secret.
  inputResources:
    
    # The name of the key used to refer to this input resource in other jsonpath syntax    
    # occurrences seen in a) other input resources in this array or b) in the Secret 
    # .spec.template.
  - name: rds
    ref:
      apiVersion: rds.services.k8s.aws/v1alpha1
      kind: DBInstance
      # Possibly optional if selector is present.
      name: my-rds-instance
      # Optional. Possible extension. Allows reading of resources across namespaces.
      # May have repercussions due to SecretTemplate creator being able to read arbitrary 
      # resources across all namespaces, but this is mitigated if we ensure that the
      # reading of input resources uses .spec.serviceAccountName’s permissions.
      namespace: rds-services
      # Optional.  Possible extension.  Allows resource selection via Labels (or 
      # fields), will fail if the number of matches != 1.
      ​​selector:
        matchLabels:
          type: ha

  - name: creds
    ref:
      apiVersion: v1
      kind: Secret
      # Possible extension.  The name of a input resource could be provided via the value 
      # of a path on another resource. Allowing acyclic graphs of resource dependencies 
      # to be expressed.  Cyclic references e.g. rds -> creds -> rds would be prevented 
      # or recognised and causing an error. A suggested way is to require that any    
      # referenced input resource must precede the current input resource. That way it's   
      # impossible to create cycles and the logical flow is also more clear.
      name: {.rds.masterPassword.name}
      namespace: rds-services

  # Secret template for the Secret this CR will create. Optional, either template OR ytt must be defined.
  template:
    # Possible extension.  Metadata exposed to allow the templating in of the name of the  
    # Secret.  This would also allow the exposure of label/annotation setting which could 
    # be useful when specifying Secrets as claimable.
    metadata:
      name: db-instance-secret
      labels:
        services-type: aws-rds

    # Use jsonpath syntax for referencing values on the input resources, otherwise it is 
    # static.
    stringData:
      type: postgresql
      port: {.rds.status.endpoint.port}
      database: {.rds.spec.dbName}
      host: {.rds.status.endpoint.address}
      username: {.rds.spec.masterUsername}
    # Data would be used to set base64 encoded data.
    data:
      password: {.creds.data.password}

  # Possible extension,  allows the templating      
  # of a Secret using ytt.  Optional either template OR ytt must be defined. Would enable more complex use cases such as: dynamic loading 
  # of keys from another resource (to access in a Secret), defaulting, and assertions.
  ytt: | 
    #@ load("@ytt:data", "data")
    
    # Store the input resource as data values with the key being 
    # the specified reference name.
    #@ rds = data.values.rds
    #@ creds = data.values.creds
    #@ endpoint = rds.status.endpoint
    metadata:
      name: db-instance-secret
    stringData:
      type: postgresql
      database: #@ rds.spec.dbName
      port: #@ endpoint.port
      host: #@ endpoint.address
      # Example of defaulting.
      username: #@ rds.spec.masterUsername if rds.spec.masterUsername != "" else "admin"

    data:
      # Example of dynamic key loading.
      password: #@ creds.data[rds.masterUserPassword.get("key")]

status: 
  createdSecret:
    name: db-instance-secret
  conditions:
    # All input resources exist
  - lastTransitionTime: 2019-10-22T16:29:24Z
    status: True
    type: InputResourcesFound
    # Templating has succeeded
  - lastTransitionTime: 2019-10-22T16:29:24Z
    status: True
    type: TemplateSucceeded
    # The Secret is created with templated values
  - lastTransitionTime: 2019-10-22T16:30:24Z
    status: True
    type: SecretCreated
  - lastTransitionTime: 2019-10-22T16:31:24Z
    status: True
    type: Ready

Some thoughts/questions we still have:

  1. The current jsonpath syntax defined would require using double quotes, we should investigate what syntax is most appropriate here. Tekton uses $( ), Cartographer uses $( )$.
  2. Should the template implicitly create a secret with the same name much like other carvel tooling, or should we allow it to be user defined or take advantage of GenerateName.
  3. Should the generated Secret be immutable?
  4. Should we start with the simple jsonpath templating case and add ytt in the future?
  5. If all input resources are secrets then serviceAccountName could be optional for simplicity of use? This would need to assume secrets are in the same namespace and if we ever supported x-namespace would need a serviceAccount to be defined.

Future enhancements:

  1. This resource could support cross namespace loading of resources, we have some usecases for this and access would be controlled by the defined service account, but this is something we could add later as a future enhancement.
  2. Allowing input resources to be specified by a selector.

If folks are generally happy with the approach we will start working on a PR.

  • 👀1
cppforlife commented

The current jsonpath syntax defined would require using double quotes, we should investigate what syntax is most appropriate here. Tekton uses $( ), Cartographer uses $( )$.

i would recommend going with more "familiar" syntax of $(). i believe tekton adopted it because k8s core uses this syntax within pod spec (inside env vars and args i believe).

Should the template implicitly create a secret with the same name much like other carvel tooling, or should we allow it to be user defined or take advantage of GenerateName.

for simplicitly sake i would favor name that matches name of the SecretTemplate. what are use cases where generated secret needs to have a different name from the template cr?

Should the generated Secret be immutable?

my gut feeling says no since it would be reconciled with changes over time.

Should we start with the simple jsonpath templating case and add ytt in the future?

adding ytt support should be fairly trivial (probably would want to do a quick follow). we have done this in a few places within carvel tools themselves. feel free to file a PR just with jsonpath support.

If all input resources are secrets then serviceAccountName could be optional for simplicity of use? This would need to assume secrets are in the same namespace and if we ever supported x-namespace would need a serviceAccount to be defined.

for general secretgen users majority of use cases would be covered by "without service account". for more advanced cases you have, we would definitely need "service account" support. i think we should follow the same methodology as with jsonpath (+ ytt for advanced). no-service-account for general use, with-service-account for advanced cases.

This resource could support cross namespace loading of resources, we have some usecases for this and access would be controlled by the defined service account, but this is something we could add later as a future enhancement.

sounds good.

Allowing input resources to be specified by a selector.

any example to share for this? i could see how it might be useful, but cant think of a concrete example ive seen out there.

st3v commented

@Samze - Knowing the more specific use case you/we have in mind for SecretTemplate in the context of Service Bindings...

  1. Would it make sense to allow the creator of the SecretTemplate resource to choose wether the template should be rendered as a mutable or series of immutable secrets? I was under the impression that we were favoring immutable secrets for our concrete use case. Is this something you were thinking could be added later? If so, what would it look like?
  2. I see your proposed API refers to the rendered secret in .status.createdSecret. That's great and particularly important if we decided to add support for immutable secrets which would probably be using generated names then. From the perspective of service bindings, it would be ideal to have a .status.binding.name that points at the secret, either in addition to or instead of .status.createdSecret. This would make SecretTemplate a Provisioned Service as defined by the service binding spec. Do you have any opinion on whether this should be handled here or by a separate API that sits on top of SecretTemplate?
Samze commented

Thanks for the input @cppforlife / @st3v .

"familiar" syntax of $()

Yeah makes sense. Will update. () are valid characters in jsonpath for expressions and https://github.com/vmware-tanzu/cartographer/ mentioned that they chose $( )$ to have a way to distinguish the closing bracket. I think we can still support $(.items[?(@.metadata.name == 'thing')]) but will have to be careful with the implementation. Seems like Tekton fell foul of this tektoncd/triggers#365 & tektoncd/triggers#376.

for simplicitly sake i would favor name that matches name of the SecretTemplate. what are use cases where generated secret needs to have a different name from the template cr?

I think this is linked with the decision on whether this should produce immutable secrets with .status.createdSecret being updated to point to the newly created immutable secret, or whether we should just update the secret in place.

For our use case we care about the secret being immutable so that when injected into a pod through something like https://github.com/k8s-service-bindings/spec and the secret is updated, the pod remounts a new secret to pick up the new details. If the secret is just updated in place then it is down to the application to "hot reload" the secrets contents.

Thinking about this more I'm not sure this is right place to handle immutability. In this case since we are reading dynamic input sources we must to handle updates. So there seem to be three strategies:

  1. Update an existing mutable secret.
  2. Create a new immutable secret with a new name and delete the old immutable secret.
  3. Delete the old immutable secret and create a new immutable secret with the same name.

Both options 2 and 3 seem a strange usecase for immutable secrets since we're not keeping the old secret around - we're effectively doing an update. So beyond our usecase that I described above I'm not sure there is any practical advantage of immutability here. For us, perhaps we should address the immutability issue elsewhere in the stack. Thoughts @st3v @gmrodgers ?

Would it make sense to allow the creator of the SecretTemplate resource to choose wether the template should be rendered as a mutable or series of immutable secrets? I was under the impression that we were favoring immutable secrets for our concrete use case. Is this something you were thinking could be added later? If so, what would it look like?

This could be an option, have an immutable field like regular Secrets do, but as mentioned above I wonder if this is the best place for it. Taking secret-gen controller as a whole it would make sense for Password SecretImport etc to all have this property. Has this ever come up @cppforlife ?

A Allowing input resources to be specified by a selector. any example to share for this?

We don't have any concrete usecases for this today but I think it makes sense to design the inputResource block with the possibility of defining a selector in the future.

I see your proposed API refers to the rendered secret in .status.createdSecret. That's great and particularly important if we decided to add support for immutable secrets which would probably be using generated names then. From the perspective of service bindings, it would be ideal to have a .status.binding.name that points at the secret, either in addition to or instead of .status.createdSecret. This would make SecretTemplate a Provisioned Service as defined by the service binding spec. Do you have any opinion on whether this should be handled here or by a separate API that sits on top of SecretTemplate?

Ah yeah, I knew I missed an open question! This is up for discussion. I think if we decide to make the secret name dynamic (e.g. it is immutable or allow the user to configure the name) then having SecretTemplate be Provisioned Service would make our lives much easier - it would be a question whether including that .status.binding.name field would be very usecase specific for this general tooling. @cppforlife may have opinions here.

However, if the name is known (e.g. the same as the SecretTemplate) and the secret is updated in place, then we can just refer to it via a Direct Secret Reference.

Samze commented

It seems like there are some outstanding details to be figured out but the agreement on the general approach so unless there are any strong objections we will start work on a PR.

gmrodgers commented

Thanks for the writeup @Samze!

Yeah makes sense. Will update. () are valid characters in jsonpath for expressions and https://github.com/vmware-tanzu/cartographer/ mentioned that they chose $( )$ to have a way to distinguish the closing bracket. I think we can still support $(.items[?(@.metadata.name == 'thing')]) but will have to be careful with the implementation. Seems like Tekton fell foul of this tektoncd/triggers#365 & tektoncd/triggers#376.

I think we should consider only supporting a subset of the jsonpath syntax. That subset may not include the () option and therefore the need for a closing )$ is mitigated. Service Bindings went down this path, see Fixed JSONPath.

Both options 2 and 3 seem a strange usecase for immutable secrets since we're not keeping the old secret around - we're effectively doing an update. So beyond our usecase that I described above I'm not sure there is any practical advantage of immutability here.

For me, I think the Secret immutability is a forcing function for us changing the Secret name on an update. Immutability is not necessary for us to do it! We could just delete a mutable Secret and create a new one with a different name.

For us, perhaps we should address the immutability issue elsewhere in the stack. Thoughts @st3v @gmrodgers ?

We could possibly enable rotation of Secret name here as well as a static name. If name is specified in .spec.template it is always used, else a name is generated and the status updated each time. From first glance, it would look like that either the Applications consuming Secrets would have to know to check for Secret contents updates or every link in the Secret chain (Password then this SecretTemplate then maybe SecretImportExport) would have to support transmitting updates to the Secret.

It would be a question whether including that .status.binding.name field would be very usecase specific for this general tooling. @cppforlife may have opinions here.

I think, as you implied, we can move ahead with an agnostic .status.createdSecret.name for now and then make an additive .status.binding.name later if we decide it is appropriate!

  • 👍1