feat: secret copy #741

[docandrew]

Description

• Adds workaround for Zarf issue dev deploy with ###ZARF_REGISTRY### values in helm chart zarf-dev/zarf#2713
• Adds secret copy functionality for issue Secrets manipulation using the UDS Operator #741

Related Issue

Fixes #741

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Other (security config, docs update, etc)

Checklist before merging

• Test, docs, adr added or updated as needed
• Contributor Guide followed

[bburky]

This appears to be unprivileged and effectively allows cluster-wide read of any Secrets if you have RBAC permissions of read/write to a Secret in a single namespace. Also, because the name is customizable, this is a bypass of resourceNames RBAC too.

It's reasonable for an app to have runtime read/write secrets in it's own namespace (possibly restricted by resourceNames) , but this would allow reading any secret in the cluster.

There's a few different ways you could address this, but I'd suggest driving it via a CR instead of the Secret resources alone. For example, editing Package resources is privileged (things shouldn't be giving away RBAC to this resource kind), which drives most of this operator and solves the permissions issue.

This is isn't a comment on the feature itself, I have no opinion on it.

[mjnagel]

Sorry for the delayed review on this - happy to have further convo on my comments. If you want to think through and propose other options here it might be good to write up a design doc for the team + security to review before we dive into another implementation.

[mjnagel]

Zarf 0.42.1 included this fix/feature to allow us to pass in --registry-url during the dev deploy (set to docker.io). Once uds-cli consumes that zarf version this should be switched to use that flag instead of yq.

[mjnagel]

Would be curious to get your thoughts on this design approach of mutating the "destination" secret. Initially I thought this was a mistake because I didn't read far enough in the PR 😅.

My inclination would be that it makes more sense to Watch (or Reconcile) secrets that have a label like uds.dev/copyMe (i.e. the "source" secret). That way you can guarantee you know when the secrets update at the source and ensure they are kept up to date in the destination. This current pattern would only watch changes/creations on the destination side, meaning you might miss changes if the source updates I think? There may be a few more design implementations to think through here, but overall I think it would make a bit more sense? (a few of those design decisions: do you need an overwrite: true/false annotation, do you support mutliple destinations and how, etc)

I think this might also resolve most of @bburky's security concerns since the only secret being read would be one that has been explicitly labelled to be copied. The only caveat would be on that overwrite piece - we would want to be careful implementing this to not overprivilege that label to write to any secret without some safeguards (maybe we only allow overwriting if pepr is already controlling the secret, use ownership metadata, etc).

The one obvious downside I could see to this is that you would need to have access to label/annotate the source secret which might be an issue with some upstream charts/operators? If that is a major sticking point we could move to a custom resource as @bburky recommended. If we want to go that route I think I'd lean slightly towards a new custom resource rather than using the Package CR.