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 providing cluster-wide age identity via controller env var #918

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mraerino
Copy link

@mraerino mraerino commented Jul 4, 2023

SOPS has the handy feature of allowing to read the identity for decryption from SOPS_AGE_KEY. Because of the keyservice implementation detail of Flux you can't use that env var to set a cluster-wide identity on the controller.

This change implements that ability using the env var FLUX_SOPS_AGE_KEY. When you set that variable on the Kustomize Controller it will be used as an additional key in decryption and allows omitting the spec.decryption.secretRef property on the Flux Kustomization (it is marked as optional in the API).

@mraerino mraerino force-pushed the feat/age-global-key branch 2 times, most recently from adfa781 to c088f51 Compare July 4, 2023 22:07
Signed-off-by: Marcus Weiner <marcus.weiner@gmail.com>
Signed-off-by: Marcus Weiner <marcus.weiner@gmail.com>
@mraerino mraerino marked this pull request as ready for review July 4, 2023 22:26
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a convenient feature. However, we need to think about the security implications of this in multi-tenant environments. If we agree to add this feature, we must document it in https://fluxcd.io/flux/components/kustomize/kustomization/, https://fluxcd.io/flux/guides/mozilla-sops/ and https://fluxcd.io/flux/security/secrets-management/. Likely also in https://fluxcd.io/flux/security/best-practices/.

}

if d.kustomization.Spec.Decryption.SecretRef == nil {
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the case where the env var is not set and no secret is provided in the Kustomization, this just bails out but it should actually return an error because providing a decryption provider without any keys is a user error.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that behaviour was there before, i did not introduce it?

@mraerino
Copy link
Author

mraerino commented Jul 6, 2023

However, we need to think about the security implications of this in multi-tenant environments.

i'm happy to discuss them. the hope was that since this is an env var for the controller, people setting this would need to know what they're doing? it's also prefixed with FLUX so it's unlikely to get there by mistake?

@stefanprodan
Copy link
Member

stefanprodan commented Jul 6, 2023

We have a dedicated section in the API docs for Controller global decryption, this the place where this feature should be documented.

@stefanprodan stefanprodan requested a review from hiddeco July 6, 2023 16:01
@mraerino
Copy link
Author

mraerino commented Jul 6, 2023

so it seems like we have precedent for secret-less decryption?

@stefanprodan
Copy link
Member

so it seems like we have precedent for secret-less decryption?

No, not with some hardcoded private key, all the global options are for KMS & OIDC.

@stefanprodan
Copy link
Member

stefanprodan commented Jul 6, 2023

we need to think about the security implications of this in multi-tenant environments.

No one should use this on multi-tenant clusters. Given that it requires a cluster admin to add the env var to kustomize-controller, it can't be enabled by mistake and it's opt-in, I think it's safe to add this feature. @hiddeco what's your opinion on this?

@hiddeco
Copy link
Member

hiddeco commented Jul 6, 2023

Think it would be okay to include.

However, I am not really sure about the naming of the FLUX_SOPS_AGE_KEY environment variable, and think it may be wise to wait a tiny bit until we have the next SOPS release out which will remove all forked keysource implementations.

@mraerino
Copy link
Author

mraerino commented Jul 6, 2023

I am not really sure about the naming of the FLUX_SOPS_AGE_KEY environment variable

What are your doubts about it? Any kind of conventions or existing patterns you'd like it to follow instead?

@hiddeco
Copy link
Member

hiddeco commented Jul 6, 2023

SOPS itself already has default places where it looks for things, and it may thus be better to inherit these for environment defaults: https://github.com/getsops/sops/blob/02a866f27d8545fdc3de3b44a563e46f8e9c9df9/age/keysource.go#L19-L27

@mraerino
Copy link
Author

mraerino commented Jul 6, 2023

if you prefer omitting the FLUX_ prefix, that's totally possible, i was just thinking it might be nicer to scope it. would that help? i can also implement the SOPS_AGE_KEY_FILE if that makes it easier to accept this change

@mraerino
Copy link
Author

mraerino commented Sep 3, 2023

@hiddeco @stefanprodan do you think it makes sense to get this implemented?

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

Successfully merging this pull request may close these issues.

4 participants