fix: Refuse to overwrite foreign objects from TrustStore reconciler

[dervoeti]

Description

TLDR: Write access on TrustStore allows write access for ConfigMap or Secret in the same namespace

The TrustStore reconciler writes its output ConfigMap or Secret using the TrustStore CR's own name and applies it via SSA with force=true. Combined with the operator's cluster-wide write permissions on ConfigMaps and Secrets, this lets any principal who can create a TrustStore in a namespace cause the operator to overwrite the contents of any pre-existing same-named ConfigMap or Secret in that namespace, stealing ownership and attaching a controller OwnerReference that links the hijacked object to the TrustStore CR for cascade delete.

A good showcase is the auto-published kube-root-ca.crt ConfigMap:
A tenant with create on truststores.secrets.stackable.tech in their namespace can create a TrustStore named kube-root-ca.crt, overwrite the CA bundle every pod in the namespace uses to verify in-cluster TLS, and cascade-delete that ConfigMap by deleting the TrustStore. This allows a) MITM attacks (hard because you need to modify DNS resolution of the Pods) and b) potentially crashing existing Pods on restart because kube-root-ca.crt is corrupted.

The same applies to any ConfigMap or Secret in the same namespace whose name the attacker can guess.

Fix

Before applying, look up the existing target. If the object already exists and is not owned by the current TrustStore, refuse the reconciliation.

Definition of Done Checklist

• Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
• Please make sure all these things are done and tick the boxes

Author

• Changes are OpenShift compatible
• CRD changes approved
• CRD documentation for all fields, following the style guide.
• Helm chart can be installed and deployed operator works
• Integration tests passed (for non trivial changes)
• Changes need to be "offline" compatible
• Links to generated (nightly) docs added
• Release note snippet added

Reviewer

• Code contains useful comments
• Code contains useful logging statements
• (Integration-)Test cases added
• Documentation added or updated. Follows the style guide.
• Changelog updated
• Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

• Feature Tracker has been updated
• Proper release label has been added
• Links to generated (nightly) docs added
• Release note snippet added
• Add type/deprecation label & add to the deprecation schedule
• Add type/experimental label & add to the experimental features tracker

[sweb]

lgtm - but I primarily did this to dive into the code base, so grains of salt - I also tried out the review skill on this.

[NickLarsenNZ]

This looks ready to merge.

[NickLarsenNZ]

Merged cancelled due to a comment from @sbernauer

IIRC he wanted to do a little bit more digging around it to see what other places of the SDP are "affected". Or if this is all over the place and we are only fixing the tip of the iceberg. In any case we likely want some docs comparing it to cert-manager and trust-manger etc.

[dervoeti]

https://github.com/cert-manager/cert-manager has a similar problem but less severe (keys in Secrets/ConfigMaps are fixed and can't be chosen by an attacker), but https://github.com/cert-manager/trust-manager has the same problem. That does not mean we can't do it better though.

We have discovery ConfigMaps in other operators, but that problem is limited to ConfigMaps and the keys are fixed there as well. This problem in secret-operator is more severe since it allows overwriting ConfigMaps and Secrets and the keys can be chosen freely (at least not the content, but still a more severe problem, see the kube-root-ca.crt example in the initial comment).

For Listeners, a user can override/delete any Service in the namespace that he can create Listeners in. That is similar to the problem this PR fixes and could be fixed as well.

I did not find any similar problems.

We have three options:
a) merge this and create a PR for listener-operator as well (two local fixes)
b) put the guard into operator-rs so it does ownership checks in general when patching resources (one global fix)
c) leave everything as is and accept the risk

The real security benefit is probably not too high (having permissions to create TrustStores / Listeners is a requirement to exploit this) but it also should not break anything or make it much worse. I'm leaning towards option a) since it fixes the two worst problems and the blast radius is limited.

@sbernauer what's your opinion on this?

[sbernauer]

Sure, I let you decide, ship it :)

[lfrancke]

Should this have a release note?

[dervoeti]

Should this have a release note?

I don't think so, this should not affect customers in any way, it just strengthens the basic security of SDP a bit.