Implement SPIFFE Broker Endpoint & API

[matheuscscp]

Reference implementation for spiffe/spiffe#340 arndt-s/spiffe#1

go-spiffe PR: spiffe/go-spiffe#388 plugin-sdk PR: spiffe/spire-plugin-sdk#74

Follow-up issues: #7074 #7075
Follow-up PRs: matheuscscp#1 (waiting for this one to merge)

Pull Request check list

• Commit conforms to CONTRIBUTING.md?
• Proper tests/regressions included? Added here
• Documentation updated? Added here

Affected functionality

Description of change

Which issue this PR fixes

Supersedes: #6594

[matheuscscp]

@arndt-s @amartinezfayo @MarcosDY @sorindumitru I spiked through the implementation, will add tests and docs after an initial review. PTAL 🙏

[matheuscscp]

@sorindumitru I updated the PR according to the spire-plugin-sdk changes your requested (remove workloadattestorv2), PTAL 🙏

https://github.com/spiffe/spire/compare/1162ed0003f12d4b39bce24229b12fe2432a1101..66717590424544388b3ccd6baa08745d3f5df61f

[arndt-s]

Structural-wise I believe this is looking good.

[matheuscscp]

@arndt-s Addressed your first three comments here: https://github.com/spiffe/spire/compare/4973fcf5ee282081a3025b9c1faf68a06bd79368..cfe8a847be3432652de75e7aa386f5a68aa51700

[amartinezfayo]

Something I realized after talking with @sorindumitru is that the lever we were really after is controlling the remote, cluster-wide lookup, and allowed_reference_types doesn't quite get there. Enabling KubernetesObjectReference also enables the API-server path, so the cache and the cluster-wide Pod watch start as soon as a reference can't be resolved locally (a cross-node pod, a fresh pod not yet in the kubelet list, or any non-pod object). There's no way to allow object references while keeping resolution local-only, and a broker usually doesn't have the workload PID, so it can't fall back to PID for local pods the way a sidecar would.

I think we should add an explicit remote-lookup control, default to off. Off would resolve object references through the kubelet only, and anything needing the API server would return NotFound without starting the cache.

[matheuscscp]

Something I realized after talking with @sorindumitru is that the lever we were really after is controlling the remote, cluster-wide lookup, and allowed_reference_types doesn't quite get there. Enabling KubernetesObjectReference also enables the API-server path, so the cache and the cluster-wide Pod watch start as soon as a reference can't be resolved locally (a cross-node pod, a fresh pod not yet in the kubelet list, or any non-pod object). There's no way to allow object references while keeping resolution local-only, and a broker usually doesn't have the workload PID, so it can't fall back to PID for local pods the way a sidecar would.

I think we should add an explicit remote-lookup control, default to off. Off would resolve object references through the kubelet only, and anything needing the API server would return NotFound without starting the cache.

I think your concern really is with caching / watching. I think the right gate we need is for the cache, not for the API server. In my follow-up PR I want to introduce API server calls for more things, including when attesting PID references, but only via Broker API. If it's a Broker API call, which is an entirely new feature, I think the k8s attestor plugin should always perform a SubjectAccessReview, regardless of the reference type. I think it's really important to release this feature with this check. @amartinezfayo @sorindumitru What do you guys think?

[matheuscscp]

Something I realized after talking with @sorindumitru is that the lever we were really after is controlling the remote, cluster-wide lookup, and allowed_reference_types doesn't quite get there. Enabling KubernetesObjectReference also enables the API-server path, so the cache and the cluster-wide Pod watch start as soon as a reference can't be resolved locally (a cross-node pod, a fresh pod not yet in the kubelet list, or any non-pod object). There's no way to allow object references while keeping resolution local-only, and a broker usually doesn't have the workload PID, so it can't fall back to PID for local pods the way a sidecar would.
I think we should add an explicit remote-lookup control, default to off. Off would resolve object references through the kubelet only, and anything needing the API server would return NotFound without starting the cache.

I think your concern really is with caching / watching. I think the right gate we need is for the cache, not for the API server. In my follow-up PR I want to introduce API server calls for more things, including when attesting PID references, but only via Broker API. If it's a Broker API call, which is an entirely new feature, I think the k8s attestor plugin should always perform a SubjectAccessReview, regardless of the reference type. I think it's really important to release this feature with this check. @amartinezfayo @sorindumitru What do you guys think?

I replaced allowed_reference_types with api_server_cache.enabled defaulting to false here: https://github.com/spiffe/spire/compare/cbb222f77cf59f101edb5febd97d470ff866ae5a..a60a835a801a677394ba643f422b22b04ff1fc1c

[sorindumitru]

This was mentioned in a separate comment, since the extra checks are not specified by the specification we can't enable it by default. The SubjectAccessReview checks should be optional and default to disabled.

Additionally, we still want to limit the k8s plugin to just the kubelet data by default. I think the current configuration is ok as is, it just needs to change what is checked.

Thank you for looking into using PartialObjectMetadata, that seems to be a good optimisation to have in general. We still think this can add a lot of memory spiking due to parsing large List responses, since the metadata can contain labels and annotations, which is why we would like to limit requests to the kubelet.

[matheuscscp]

since the extra checks are not specified by the specification we can't enable it by default.

Agreed.

The SubjectAccessReview checks should be optional and default to disabled.

Since SAR will be optional, I strongly disagree that this feature should have a default. Users should be forced to make that choice, as it is a very serious one. Defaulting to less safe does not sound like SPIRE at all. Furthermore, we should choose the config knob and values in a way that encourages SAR:

broker {
  access_policy = "enforced" # or "permissive". no default. the plugin initialization will fail if a value is not specified.
}

---

About using only kubelet for pods by default, sure, we can do that. We can branch the behavior based on the already-existing pod_reference_scope (values: agent_node and cluster). The default is already agent_node. So this one is easy to implement: just don't fallback to API server on agent_node.

Note: This will work only for pods. There's nothing for non-pod objects in kubelet.

[matheuscscp]

Both implemented:

• SAR is now not the default, but also there's no default either. Field is mandatory, user has to make a choice.
• agent_node now limits the info to kubelet.
• Non-pod object references still use the API server because kubelet is useless for them, and PartialObjectMetadata is the best we can do in this case.

[sorindumitru]

• SAR is now not the default, but also there's no default either. Field is mandatory, user has to make a choice.

That would be a breaking change for previous users of the plugin. We really need to default to not doing the check.

[matheuscscp]

• SAR is now not the default, but also there's no default either. Field is mandatory, user has to make a choice.

That would be a breaking change for previous users of the plugin. We really need to default to not doing the check.

No, it would not be a breaking change at all. The field lives under broker, which can perfectly be unset. The field broker.access_policy is only required if the broker field is set. Nobody is setting broker at the moment because it does not exist, so this is not a breaking change. We really don't need to have a default.

[matheuscscp]

(If it's not clear, SAR is a broker-only check. There's no SPIFFE ID to check if there's no broker.)

[amartinezfayo]

Thanks for working through all the rounds on this, @matheuscscp