Add Kbs events - #360
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yairpod The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideThis PR adds a TLS-capable KBS reverse-proxy sidecar that inspects RCAR authentication, attestation, and resource responses, correlates sessions, and records Kubernetes events. It integrates the proxy into Trustee pod generation, image resolution, RBAC/runtime configuration, build and release automation, documentation, and end-to-end testing. Sequence diagram for RCAR attestation event recordingsequenceDiagram
participant Node
participant Proxy as KBS Event Proxy
participant KBS
participant Kubernetes
Node->>Proxy: POST /kbs/v0/auth
Proxy->>KBS: Forward auth request
KBS-->>Proxy: 200 + kbs-session-id
Proxy->>Proxy: Store session tee_type
Proxy-->>Node: Return auth response
Node->>Proxy: POST /kbs/v0/attest
Proxy->>KBS: Forward attest request
KBS-->>Proxy: Attestation response
alt non-200 response
Proxy->>Kubernetes: record_event AttestationFailed on TrustedExecutionCluster
end
Proxy-->>Node: Return attest response
Node->>Proxy: GET /kbs/v0/resource/default/{machine_id}/root
Proxy->>KBS: Forward resource request
KBS-->>Proxy: Resource response
Proxy->>Kubernetes: lookup_machine_ref
alt 200 response
Proxy->>Kubernetes: record_event AttestationSucceeded on Machine
else 401 response
Proxy->>Kubernetes: record_event AttestationFailed on Machine
else 403 response
Proxy->>Kubernetes: record_event ResourcePolicyDenied on Machine
end
Proxy-->>Node: Return resource response
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="kbs-event-proxy/src/main.rs" line_range="209" />
<code_context>
+ };
+ record_event(
+ &state.recorder,
+ &machine_ref,
+ event_type,
+ reason,
</code_context>
<issue_to_address>
**🚨 issue (security):** The proxy attempts to publish `events.k8s.io` events, but the diff adds no `events.k8s.io` permissions to the operator ClusterRole. Kubernetes denies these publishes with RBAC `forbidden` errors, so none of the new events are created.
**Triggers:** When the generated operator RBAC manifests are applied without an independently added events rule.
**Suggested fix:** Add `events.k8s.io` permissions for `events` with at least `create` and `patch` to the ClusterRole used by `trusted-cluster-operator`, and regenerate the bundled manifests.
</issue_to_address>
### Comment 2
<location path="tests/attestation.rs" line_range="364-372" />
<code_context>
+ assert!(has_encrypted_root, "VM {ENCRYPTED_ROOT_ASSERT}");
+ test_ctx.info("Attestation successful, verifying KBS proxy events");
+
+ let machines: Api<Machine> = Api::namespaced(client.clone(), namespace);
+ let machine_list = machines.list(&Default::default()).await?;
+ let machine_name = machine_list.items.first()
+ .expect("No Machine found in namespace")
+ .metadata
+ .name
+ .as_ref()
+ .expect("Machine should have a name");
+
+ wait_for_event(client, namespace, machine_name, "AttestationSucceeded", scaled_timeout(60)).await?;
+ test_ctx.info("Event AttestationSucceeded verified on Machine");
</code_context>
<issue_to_address>
**issue (testing):** The test selects the first Machine returned by an unordered namespace-wide list instead of the Machine created for `vm_name`. With more than one Machine, it waits for `AttestationSucceeded` on the wrong resource and fails or validates an unrelated event.
**Triggers:** When the test namespace already contains another Machine or the API returns a different item first.
**Suggested fix:** Derive the expected Machine name from `att_ctx`/`vm_name`, or filter the list by the VM identity before waiting for the event.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and this changes the KBS trust boundary and routes all attestation and secret-release traffic through a new TLS-terminating proxy, so a proxy or deployment mistake could affect every attestation or expose/release secrets. Reverting restores the old routing, but requests handled incorrectly while the proxy is live and any resulting access or disclosures cannot be undone.
Blocking findings: kbs-event-proxy/src/main.rs:209, tests/attestation.rs:372
| let machines: Api<Machine> = Api::namespaced(client.clone(), namespace); | ||
| let machine_list = machines.list(&Default::default()).await?; | ||
| let machine_name = machine_list.items.first() | ||
| .expect("No Machine found in namespace") | ||
| .metadata | ||
| .name | ||
| .as_ref() | ||
| .expect("Machine should have a name"); | ||
|
|
There was a problem hiding this comment.
issue (testing): The test selects the first Machine returned by an unordered namespace-wide list instead of the Machine created for vm_name. With more than one Machine, it waits for AttestationSucceeded on the wrong resource and fails or validates an unrelated event.
Triggers: When the test namespace already contains another Machine or the API returns a different item first.
Suggested fix: Derive the expected Machine name from att_ctx/vm_name, or filter the list by the VM identity before waiting for the event.
02abcc9 to
0c648ab
Compare
|
/test azure-integration-test |
1 similar comment
|
/test azure-integration-test |
| | `AttestationFailed` | Warning | Machine | Resource endpoint returns 401 for `default/{machine-id}/root` | | ||
| | `ResourcePolicyDenied` | Warning | Machine | Resource endpoint returns 403 for `default/{machine-id}/root` | | ||
|
|
||
| The proxy emits `AttestationFailed` on the TrustedExecutionCluster (not on a Machine) at the attest step because the RCAR protocol does not carry a machine identifier at that point. The session carries only the TEE type. |
There was a problem hiding this comment.
Right now we don't have it yet, but in the future the uuid of the machine should be provided as initdata as part of the attestation. In this way, it should be possible to extrapolate which machine failed the attestation
There was a problem hiding this comment.
So we can create a ticket to continue this work when the uuid is provided.
| ### Container image | ||
|
|
||
| The proxy binary is built in the unified `Containerfile` alongside the other operator components. It uses its own distribution stage based on `fedora:43` and exposes port 8080. |
There was a problem hiding this comment.
nit: Is this really necessary?
There was a problem hiding this comment.
I can remove this section
| } | ||
|
|
||
| fn machine_id_from_path(path: &str) -> Option<&str> { | ||
| let stripped = path.strip_prefix("/kbs/v0/resource/default/")?; |
There was a problem hiding this comment.
can we use a constant here?
There was a problem hiding this comment.
You mean instead of the API path? sure
There was a problem hiding this comment.
Yes, moving this to a constant
| let machines: Api<Machine> = Api::default_namespaced(client.clone()); | ||
| let machine_name = format!("machine-{machine_id}"); |
There was a problem hiding this comment.
Any chance to use the cache and store here?
| if path == "/kbs/v0/auth" && backend.status == StatusCode::OK { | ||
| handle_auth_response(&state, &body_bytes, &backend.headers).await; | ||
| } else if path == "/kbs/v0/attest" { | ||
| handle_attest_response(&state, &req_headers, backend.status).await; | ||
| } else if path.starts_with("/kbs/v0/resource/default/") { | ||
| handle_resource_response(&state, &path, backend.status).await; | ||
| } |
There was a problem hiding this comment.
Can we use some constant for the path here as well
| } | ||
| } | ||
|
|
||
| virt_test! { |
There was a problem hiding this comment.
Instead of adding a new test can we simply extend an existing one
|
@yairpod thanks for this work! It is already a huge step forward. What I'm a bit afraid is for the failed attestation, that we are firing an event without the possibility of saying at which machine it was corresponding. |
Signed-off-by: Roy Kaufman <rkaufman@redhat.com>
Move new_recorder, spawn_reflector, and sync_cache from the operator crate to trusted-cluster-operator-lib so kbs-event-proxy can use them without duplicating code. Signed-off-by: Yair Podemsky <ypodemsk@redhat.com> Assisted-by: AI
Add a reverse proxy sidecar container to the KBS pod that observes RCAR attestation HTTP traffic and emits Kubernetes events for attestation activity. The proxy sits between the Service and KBS, inspecting request/response pairs to emit AttestationSucceeded (per Machine), AttestationFailed (on TrustedExecutionCluster), and ResourcePolicyDenied (per Machine) events. Session cookies correlate auth, attest, and resource steps. - New crate: kbs-event-proxy (axum-based reverse proxy) - KBS binds to 127.0.0.1:8081, proxy serves on 0.0.0.0:8080 - TLS on both hops when a certificate is configured - KBS pod uses trusted-cluster-operator ServiceAccount - Add events.k8s.io RBAC to ClusterRole - Add kbs-event-proxy to unified Containerfile build Signed-off-by: Yair Podemsky <ypodemsk@redhat.com> Assisted-by: AI
Verify that the kbs-event-proxy sidecar emits an AttestationSucceeded event on the Machine after a successful attestation and key release. Signed-off-by: Yair Podemsky <ypodemsk@redhat.com> Assisted-by: AI
|
@yairpod: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Adding kubernetes events to KBS by creating an Event Proxy that will monitor the KBS and emit events as needed.
Summary by Sourcery
Add a KBS event proxy that surfaces attestation activity as Kubernetes events while integrating its binary, image, deployment, documentation, and end-to-end verification into the operator.
New Features:
Enhancements:
Build:
Deployment:
Documentation:
Tests:
Chores: