cert-manager
Don't sync targets to all namespaces by default
PR #37 introduced the ability for users to specify a subset of namespaces to which a target should be synced. We propose that the namespaceSelector becomes a required field, replacing the current behaviour of a target being synced everywhere by default.
This would be a breaking change. We agreed in the 2022-07-21 standup that breaking changes are something we're willing to accept for trust given it's pre-v1.0.
Justification
Imagine a multi tenanted cluster.
In namespace foo, FooCorp has their root certificate with subject CN=FooCorp. This is added to FooCorp's bundle.
In namespace bar, BarCorp is a different company and does totally different things.
By default, FooCorp's bundle will by synced to BarCorp's namespace. BarCorp employees will see a ConfigMap with FooCorp's root certificate in it. They won't be able to use the root to sign certificates, but they will be able to see that they're tenanted on the same cluster as FooCorp.
Likewise, if someone at BarCorp erroneously used the FooCorp bundle it would enable anyone with access to the FooCorp root to MitM any affected BarCorp service.
Syncing to all namespaces by default introduces an absolutely critical security requirement for some users; they must ensure they always set a namespaceSelector. Effectively they need to manage a denylist from a default position of "allow all".
Making namespaceSelector required would change this to an allowlist. It's a little more work for the average user but it will present a significant security win.
Yeah I guess that'd make sense! This is something I assume we'd change in v1alpha2 or some other new API version. I'll add to the milestone!
Yeah I guess that'd make sense! This is something I assume we'd change in v1alpha2 or some other new API version. I'll add to the milestone!
I have a couple of other suggestions for a new API version, so if you are ready for it, I can start on the job. I just finished introducing a new API version in https://github.com/cybozu-go/accurate, so my mind is full of experiences.
The use case certainly swayed me here, but I would like to think about the usability element. Thinking of cert-manager Certificate spec, there are "default" values that often people want to change but cannot (easily). It might well be easier in the long run to have a configurable default that allows for easier roll out in many use cases.
So in the example above, what if you are not a multi tenant cluster? What if you just want the current default behaviour to sync everywhere? Currently that would involve:
spec:
target:
namespaceSelector:
matchLabels:
key: value
Plus labelling all the applicable namespaces. (Which could be many) k label ns test key=value.
That seems like a potential chore for people, so I would like to try and make it easier for non multi-tenant environments.
Perhaps a good start would be to have a namespace list option along with labels? Similar to CertificateRequestPolicies
FIELDS:
matchLabels <map[string]string>
MatchLabels is the set of Namespace labels that select on
CertificateRequests which have been created in a Namespace matching the
selector.
matchNames <[]string>
MatchNames are the set of Namespace names that select on CertificateRequests
that have been created in a matching Namespace. Accepts wildcards "*".
Something like:
spec:
target:
namespaceSelector:
matchNames: [*]
This negates the need for namespace labelling so it is less work to implement at least.
A follow on might be that a default matchLabels or matchNames value could be supplied at installation of trust-manager? (not sure how this looks) So the idea being you can choose a default value to apply when no field is supplied on each resource. (Yes this sounds like Kyverno or OPA equivalent thing). Also unsure how that plays with the required part.
I'm also wondering if the mutli-tenant scenario actually plays better to the future Bundle (namespaced) resource as opposed to the ClusterBundle (cluster wide) resource we currently have?
- Bundle -> has to be scoped upwards to go beyond namespace?
- ClusterBundle -> Has to be scoped downwards to be applied successfully.
I do agree that with such an important role to play we do need to think of the implications of CA's being presented across a cluster.
@hawksight I think https://github.com/cert-manager/trust-manager/issues/58 would solve the multi-tenant use cases much better. It is a feature I would love to see implemented. We already use the referenced Openshift feature a lot to inject the cluster CA into configmaps. It would represent a more pull-based workflow, as opposed to the push-based approach supported by trust-manager.
I don't think we should support wildcards in the namespace selector as it would increase the complexity of controller code. To select all namespaces, a user should use the empty selector IMO:
spec:
target:
namespaceSelector: {}
Similar to how you configure networkpolicies, ref. https://kubernetes.io/docs/concepts/services-networking/network-policies/#allow-all-ingress-traffic
Also Kubernetes injects a label on all namespaces with the namespace name. So if we are not going for wildcard support, the matchNames is not really required.
kind: Namespace
apiVersion: v1
metadata:
name: egb-test-cert
labels:
kubernetes.io/metadata.name: egb-test-cert
This is probably a case I should have tried before I commented. @erikgb {} works just as well, good example.
So currently this works:
apiVersion: trust.cert-manager.io/v1alpha1
kind: Bundle
metadata:
name: debian-ca-bundle
spec:
sources:
- useDefaultCAs: true
target:
configMap:
key: "ca-certificates.crt"
namespaceSelector: {}
Example output to all namesapces:
> k get configmap -A -l trust.cert-manager.io/bundle=debian-ca-bundle
NAMESPACE NAME DATA AGE
app-appset-dev-15 debian-ca-bundle 1 99s
app-team-1 debian-ca-bundle 1 100s
app-team-2 debian-ca-bundle 1 100s
app-team-3 debian-ca-bundle 1 99s
default debian-ca-bundle 1 100s
demos debian-ca-bundle 1 100s
development debian-ca-bundle 1 99s
httpbin debian-ca-bundle 1 99s
ingress-nginx debian-ca-bundle 1 100s
istio-system debian-ca-bundle 1 100s
jetstack-secure debian-ca-bundle 1 100s
kube-node-lease debian-ca-bundle 1 100s
kube-public debian-ca-bundle 1 100s
kube-system debian-ca-bundle 1 100s
staging debian-ca-bundle 1 100s
test debian-ca-bundle 1 100s
venafi debian-ca-bundle 1 100s
Which works just as well as not specifying the key currently:
apiVersion: trust.cert-manager.io/v1alpha1
kind: Bundle
metadata:
name: debian-ca-bundle
spec:
sources:
- useDefaultCAs: true
target:
configMap:
key: "ca-certificates.crt"
So this change basically only means, a user would need to specify the spec.target.namespaceSelector field with {}, rather than omitting the key which can be done at the moment? if it remains this way then I can probably retract all my previous words.
I guess the objective here is that the trust-manager user of the Bundle / ClusterBundle resource is forced to make this choice at creation time that it will in fact target all namespaces (when using {}).
So this change basically only means, a user would need to specify the spec.target.namespaceSelector field with {}, rather than omitting the key which can be done at the moment?
Yes, I consider this change to be just making the namespaceSelector a required field. So a user who wants to sync to all namespaces has to configure this explicitly.