Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow connection secrets to be backed up and restored #140

Closed
negz opened this issue Apr 3, 2020 · 3 comments · Fixed by #142
Closed

Allow connection secrets to be backed up and restored #140

negz opened this issue Apr 3, 2020 · 3 comments · Fixed by #142
Assignees
Labels
enhancement New feature or request

Comments

@negz
Copy link
Member

negz commented Apr 3, 2020

What problem are you facing?

When a connection secret is backed up and subsequently restored using a tool like https://velero.io it will no longer be constantly propagated by the secret reconciler. This is because the propagating and propagated connection secrets rely on UID based annotations to configure and consent to propagation, and a secret's UID changes when it is backed up and restored.

How could Crossplane help solve your problem?

Crossplane could remove the use of UIDs in the propagation annotations. Resources are uniquely identified at a particular point in time by their group, kind, namespace, and name. UIDs allow a resource to be uniquely identified over time. We included the UID in the propagation annotations to cover the case in which:

  1. Secret from consents to propagate to Secret to.
  2. Secret to is deleted.
  3. A malicious party creates a new instance of Secret to.
    1 Propagation continues, propagating from to to, which is now owned by the malicious party.

In retrospect this concern doesn't really fit with how RBAC works in Kubernetes; if the malicious party had access to create and read Secret to at step 3, they probably had access to read the previous iteration of Secret to at step 1. Including the UID in the propagation configuration thus only really protects against the case in which:

  1. Propagation is configured from Secret from to Secret to.
  2. Secret to is deleted, but from is still configured to propagate.
  3. Much time passes, everyone forgets about the propagation...
  4. RBAC permissions are updated, allowing a malicious party who cannot read from to create to
  5. The malicious party can now read the content of from by creating to and relying on propagation

I don't think we need to be too concerned about this case.

@negz negz added the enhancement New feature or request label Apr 3, 2020
@negz negz self-assigned this Apr 3, 2020
@negz
Copy link
Member Author

negz commented Apr 3, 2020

One tricky issue here is that we have a one-to-many from-secret-to-secret relationship. i.e. One source secret can be propagated to many destination secrets. We configure this using annotations.

On the "from" secret:

metadata:
  annotations:
    to.propagation.crossplane.io/some-uid: namespace/name
    to.propagation.crossplane.io/some-other-uid: namespace/another-name

On the "to" secret:

metadata:
  annotations:
    from.propagation.crossplane.io/namespace: somenamespace
    from.propagation.crossplane.io/name: somename
    from.propagation.crossplane.io/uid: some-uid

Updating the latter is trivial; we can just remove from.propagation.crossplane.io/uid. The former is a little harder, as we currently use the UID as part of looking up which secrets may be propagated to.

Some ideas I'm considering are...

Just use the namespace and name as the key:

metadata:
  annotations:
    to.propagation.crossplane.io/namespace-name: "true"
    to.propagation.crossplane.io/namespace-different-name: "true"

Hash the namespace and name as a key:

metadata:
  annotations:
    to.propagation.crossplane.io/A83D2: namespace/name
    to.propagation.crossplane.io/E2E48: "true"

@negz
Copy link
Member Author

negz commented Apr 3, 2020

Another option would be to encode the set of "to" secrets in a single annotation:

metadata:
  annotations:
    to.propagation.crossplane.io: namespace/name,othernamespace/anothername

This has a few implications; primarily that updating the set of secrets becomes a little trickier to handle.

Some other things to consider:

  • The secret reconciler watches both the propagating and propagated secrets in case either change.
  • The actual reconcile request is passed the propagated secret, then looks up the propagating secret. It looks up the one secret in its from.propagation.crossplane.io annotation.
  • The EnqueueRequestForPropagated event handler iterates a propagating secret's to.propagation.crossplane.io annotations in order to determine which secrets to enqueue reconciles for.
  • We instantiate the secret reconciler for connection secrets that are owned by a particular kind of managed resource; i.e. only for secrets that are controlled by an EKSCluster. This allows us to keep the secret propagation controllers running in the infrastructure provider code, thus ensuring propagation always works even if the core Crossplane controller is offline. This may be less important as we move toward a design in which claim controllers live in Crossplane core.

@negz
Copy link
Member Author

negz commented Apr 3, 2020

I also considered introducing a new custom resource, something like:

apiVersion: propagation.crossplane.io/v1alpha1
kind: ConnectionSecretPropagation
metadata:
  namespace: crossplane-system
  name: some-managed-secret
spec:
  secretRef:
    name: some-managed-secret
  propagateTo:
  - secretRef:
       namespace: default
       name: some-claim-secret
  - secretRef:
       namespace: default
       name: some-target-secret

There are some nice properties of this approach; we could avoid having our claim and target controllers write connection secrets, and instead just create a ConnectionSecretPropagation. There's a few more things to unwind here than I'd like to address right now though, i.e.:

  • What owns the claim secret? We want to couple its lifecycle with the claim, but it would be created by the ConnectionSecretPropagation.
  • Where does the ConnectionSecretPropagation controller live? Are we okay with consolidating all secret propagation into one controller?
  • How do we know when the propagated or propagating secrets are updated? They don't have a reference back to the ConnectionSecretPropagation. Would we need to watch all secrets in the cluster to implement this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant