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

[RFC] Trust Store for TLS and SSH #3366

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

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented Dec 2, 2022

Fixes #3078.

@pjbgf pjbgf added area/rfc Feature request proposals in the RFC format area/security Security related issues and pull requests labels Dec 2, 2022
Comment on lines +209 to +223
Object-level trust store expansion is disabled by default. To enable it start
the controller with:
Copy link
Member

Choose a reason for hiding this comment

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

I would love to see this expanded a little to provide reasoning for this default setting. With the current Flux version 0.37 the trust store can always be expanded for objects such as HelmRepositories or GitRepositories by defining a caFile field in the referenced Secret. Given many admins will unlikely deviate from the defaults, users would no longer be able to do that. Please correct me if I'm wrong here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is certainly a point up for debate. The security invariant here was "Flux should only connect to trusted servers" and the criteria used was that most users would expect Flux to be secure by default.

Given many admins will unlikely deviate from the defaults

This is exactly the point. If something works, users won't look up the documentation and try understand the impact of security sensitive settings. Hence why ideally we should always prioritise security by design and by default.

The idea is that, by pre-populating the controller level SSH trust store with the top SaaS Git providers, a considerable amount of users would not have to worry about this. It will simply securely work - just like for TLS.
Then enterprise customers should configure this once, at controller level and be done with it, for both single and multi-tenancy. The only use case I see for Object-level trust stores to be used in production, would be in a Namespace as a Service multi-tenancy model, which would further weaken an already fragile security posture.

Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
@pjbgf pjbgf marked this pull request as ready for review December 5, 2022 12:14
@pjbgf pjbgf changed the title RFC-NNNN: Trust Store for TLS and SSH [RFC] Trust Store for TLS and SSH Dec 7, 2022
the main Git SaaS providers. As a result, users will only need to update their SSH
Trust Store for custom or less well known servers.

#### TLS
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#### TLS
### TLS

ecosystem (e.g. OPA) to enable users to achieve the same outcome.

Due to the importance that Flux has in the bootstrapping of clusters, such an
important requirement (enforce trust at controller level) should be inherit to
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe

Suggested change
important requirement (enforce trust at controller level) should be inherit to
important requirement (enforce trust at controller level) should be inherent to

The new fields `spec.trustStore.tls` and `spec.trustStore.ssh` analogous
to Kubernetes `EnvFromSource`, in which it can be used to define either a
`configMapRef` or a `secretRef`, but not both.

Copy link
Contributor

@darkowlzz darkowlzz Dec 7, 2022

Choose a reason for hiding this comment

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

Maybe we can also have some details about how the controllers would behave for different values of --insecure-object-trust-store={tls-only,ssh-only,both,disabled}.
When it's disabled but a source definition specifies a trust store, should the object reconciliation stall? Because the configuration is invalid based on the current controller level configuration and can only be fixed by restarting the controller with new config or updating the source definition with different config.
Also, maybe some description about the error messaging when it's completely disabled and partially disabled and how it'd be communicated to the user.

Comment on lines +113 to +115
A new field is to be introduced into the existing kinds `ImageUpdateAutomation` and
`GitRepository`, to allow users to expand on the controller-level known hosts for
SSH operations:
Copy link
Member

Choose a reason for hiding this comment

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

I find this very problematic, why would we allow for a namespaced object own by a tenant to alter the trust store for all tenants? This also makes Flux bootstrap break, since you can't control which CR is reconciled first, if people add a trust store in one CR we can't reconcile that first to make it available to others.

Copy link
Member Author

@pjbgf pjbgf Dec 9, 2022

Choose a reason for hiding this comment

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

It does not alter the trust store for all tenants, but rather only expands the trust store for that object only.

Copy link
Member

Choose a reason for hiding this comment

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

How does this cope with the know_hosts from the secretRef? Can you explain the differences in the doc please?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will rephrase to make that clearer. But basically that's what we already do at present based on the CA bundles provided by secrets, however this would be based on a field instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rfc Feature request proposals in the RFC format area/security Security related issues and pull requests
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

RFC - Consolidate validation of remote servers
5 participants