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

feat: use vals for secret decryption #2408

Closed
wants to merge 2 commits into from

Conversation

rajiteh
Copy link
Contributor

@rajiteh rajiteh commented May 3, 2024

Refers to #2407

Introduces the ability for the fleet.yaml to specify vals secret strings, which then will be decrypted at bundle processing time by the fleet operator.

Fleet operator deployment is responsible for configuring the appropriate environment variables for vals to decrypt secrets using the correct backend.

See #2407 for more context.

@rajiteh rajiteh requested a review from a team as a code owner May 3, 2024 21:45
Copy link
Contributor

@weyfonk weyfonk left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution :)
Leaving a couple of comments. One of our main concerns here is having all Helm values sent to an external library, perhaps we could filter values to only pre-process those bearing the known vals pattern.
We are currently assessing possible security implications of this feature.

@@ -370,6 +373,13 @@ func preprocessHelmValues(opts *fleet.BundleDeploymentOptions, cluster *fleet.Cl
if err != nil {
return err
}
logrus.Debugf("templating completed for %v", opts.Helm.ReleaseName)
opts.Helm.Values.Data, err = processSecretValues(opts.Helm.Values.Data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should all Helm values be forwarded to vals? Or would we rather want to filter values based on a known pattern (eg. ref+<provider>://) and send only those to the library?

t.Fatalf("key %s not found", testCase.Key)
} else {
if field != testCase.ExpectedValue {
t.Fatalf("key %s was not the expected value. Expected: '%s' Actual: '%s'", testCase.Key, field, testCase.ExpectedValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this would also work:

Suggested change
t.Fatalf("key %s was not the expected value. Expected: '%s' Actual: '%s'", testCase.Key, field, testCase.ExpectedValue)
t.Fatalf("key %s does not have the expected value. Expected: %q Actual: %q", testCase.Key, field, testCase.ExpectedValue)

@@ -81,3 +80,6 @@ propagateDebugSettingsToAgents: true

migrations:
clusterRegistrationCleanup: true

## Additional environment variables to be injected to the fleet-controller container
additionalEnvs: {}
Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer an approach similiar to how nginx handles env vars from secrets:

## @param extraEnvVarsSecret Secret with extra environment variables
##
extraEnvVarsSecret: ""

However, the secret would have to be created manually in this case. Which is not as automatic/user friendly?

Additionally we're adding extraEnvs, like Rancher, to allow users to specify non-secret env vars as helm values.

@rajiteh
Copy link
Contributor Author

rajiteh commented May 29, 2024

@weyfonk I think you raise a valid concern. I could imagine we introduce a new gotpl helper that wraps the secret ref as an argument?

${ decodeSecretRef "ref+foo://my/secret" }

@weyfonk
Copy link
Contributor

weyfonk commented Jun 3, 2024

We have discussed this approach and will not be merging this in Fleet, because storing confidential values in etcd by means of custom resources (bundles in this case) is not secure by default.

An alternative to this could be using Kubernetes secrets, as @manno noted here.

@weyfonk weyfonk closed this Jun 3, 2024
@weyfonk
Copy link
Contributor

weyfonk commented Jun 4, 2024

@rajiteh Please feel free to reopen this PR to further discuss this.

@manno
Copy link
Member

manno commented Jun 4, 2024

An alternative to this could be using Kubernetes secrets, as @manno noted here.

That's the first part, to have the credentials used to access the vals provider in a secret, which the original PR did. Second part is to keep the response from vals out of the bundledeployment options and in a secret instead. We want to avoid storing sensitive material in our CRDs. secrets.
Doing so would make it more difficult to harden the cluster. E.g. https://kubernetes.io/docs/tasks/administer-cluster/encrypt-data/ would need to include bundledeployments.fleet.cattle.io. However, the RKE2 guide only encrypts secrets.

We are still discussing a possible solution in #2375. That involves creating a secret in the bundledeployment's namespace (see "cluster namespace" in docs) and allowing the downstream agent to fetch it:

When targeting, it also clones the secret containing all the OCI values, just changing the namespace to match the bundledeployment's namespace. (This way the agents will be able to obtain the OCI values) It will also flag the bundledeployment to state that the content is stored in the OCI registry instead to a content resource.

We could do the same for sensitive helm values?

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.

3 participants