Don't sync targets to all namespaces by default

[SgtCoDFish]

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.

[erikgb]

@SgtCoDFish I think it could make sense to add this to the v1 milestone.

[SgtCoDFish]

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!

[erikgb]

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.

[hawksight]

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.

[erikgb]

@hawksight I think #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

[hawksight]

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 {}).

[erikgb]

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.

[cert-manager-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
/lifecycle stale

[hawksight]

/remove-lifecycle stale