confidential-containers / cloud-api-adaptor

Ability to create Kata pods using cloud provider APIs aka the peer-pods approach
Apache License 2.0
48 stars 88 forks source link

Kbs key release rework #2055

Closed stevenhorsman closed 5 hours ago

stevenhorsman commented 1 month ago

The KbsKeyRelease tests have had errors setting the secret key, which are revealed when trace is on:

=== RUN   TestLibvirtKbsKeyRelease
time="2024-09-24T13:53:12Z" level=info msg="set key resource: ../../kbs/config/kubernetes/overlays/x86_64/key.bin"
time="2024-09-24T13:53:12Z" level=trace msg="./kbs-client --url http://192.168.122.140:31680 config --auth-private-key ../../kbs/config/kubernetes/base/kbs.key set-resource --path reponame/workload_key/key.bin --resource-file ../../kbs/config/kubernetes/overlays/x86_64/key.bin, output: Error: Request Failed, Response: \"{\\\"type\\\":\\\"https://github.com/confidential-containers/kbs/errors/SetSecretFailed\\\",\\\"detail\\\":\\\"Set secret failed: write local fs\\\"}\"\n"

I believe this is base the reponame/workload_key directory is created by the KBS at start-up, so it owns the resource. It also leads to error is the KBS log:

[2024-09-24T11:11:40Z INFO  kbs] Using config file /etc/kbs/kbs-config.toml
[2024-09-24T11:11:40Z WARN  attestation_service::rvps] No RVPS address provided and will launch a built-in rvps
[2024-09-24T11:11:40Z INFO  attestation_service::token::simple] No Token Signer key in config file, create an ephemeral key and without CA pubkey cert
[2024-09-24T11:11:40Z INFO  kbs] Starting HTTP server at [0.0.0.0:8080]
[2024-09-24T11:11:40Z INFO  actix_server::builder] starting 4 workers
[2024-09-24T11:11:40Z INFO  actix_server::server] Tokio runtime found; starting in existing Tokio runtime
[2024-09-24T13:07:50Z ERROR kbs::http::error] Set secret failed: write local fs

which isn't helpful when debugging.

This PR changes the secret to be in a different directory and also does some refactoring so that a more dynamic secret can be set to avoid use finding errors late and reduce duplication.

wainersm commented 1 month ago

Hi @stevenhorsman !

Could you do me a favor? I need the following change to be able to install kbs on my environment (Fedora); according to uname man-page, -i is a non-portable option and in my env it returns unknown (rather than x86_64). I don't know if that change breaks on s390x (I hope note).

diff --git a/src/cloud-api-adaptor/test/provisioner/trustee_kbs.go b/src/cloud-api-adaptor/test/provisioner/trustee_kbs.go
index a628b1f..cf97351 100644
--- a/src/cloud-api-adaptor/test/provisioner/trustee_kbs.go
+++ b/src/cloud-api-adaptor/test/provisioner/trustee_kbs.go
@@ -29,7 +29,7 @@ import (
 const TRUSTEE_REPO_PATH = "../trustee"

 func getHardwarePlatform() (string, error) {
-       out, err := exec.Command("uname", "-i").Output()
+       out, err := exec.Command("uname", "-m").Output()
        return strings.TrimSuffix(string(out), "\n"), err
 }
stevenhorsman commented 1 month ago

Hi @stevenhorsman !

Could you do me a favor? I need the following change to be able to install kbs on my environment (Fedora); according to uname man-page, -i is a non-portable option and in my env it returns unknown (rather than x86_64). I don't know if that change breaks on s390x (I hope note).

diff --git a/src/cloud-api-adaptor/test/provisioner/trustee_kbs.go b/src/cloud-api-adaptor/test/provisioner/trustee_kbs.go
index a628b1f..cf97351 100644
--- a/src/cloud-api-adaptor/test/provisioner/trustee_kbs.go
+++ b/src/cloud-api-adaptor/test/provisioner/trustee_kbs.go
@@ -29,7 +29,7 @@ import (
 const TRUSTEE_REPO_PATH = "../trustee"

 func getHardwarePlatform() (string, error) {
-       out, err := exec.Command("uname", "-i").Output()
+       out, err := exec.Command("uname", "-m").Output()
        return strings.TrimSuffix(string(out), "\n"), err
 }

Sure - we use uname -m everywhere else, so I can swap it here

mkulke commented 1 month ago

I also attempted to reduce the brittleness of kbs-related tests and ended up with https://github.com/confidential-containers/cloud-api-adaptor/blob/a670a9ec5b841f0a9595e30ad4fcd66f2f7a73b8/src/cloud-api-adaptor/test/e2e/remote_attestation.go#L12

this test will only acquire a kbs token that could be used to retrieve secrets, but it actually doesn't retrieve any, so we don't have to bother about secret provisioning, but it'll still cover einitdata, cc_kbc configuration and remote attestation.

stevenhorsman commented 1 month ago

I also attempted to reduce the brittleness of kbs-related tests and ended up with

https://github.com/confidential-containers/cloud-api-adaptor/blob/a670a9ec5b841f0a9595e30ad4fcd66f2f7a73b8/src/cloud-api-adaptor/test/e2e/remote_attestation.go#L12

this test will only acquire a kbs token that could be used to retrieve secrets, but it actually doesn't retrieve any, so we don't have to bother about secret provisioning, but it'll still cover einitdata, cc_kbc configuration and remote attestation.

Cool - I knew I'd seen a PR from you updating this code recently, but couldn't find it. It feels like that test is mostly a subset of the testing of TestKbsKeyRelease, but doesn't require the KBS to be deployed in our set-up, so maybe there are overlaps with TestTrusteeOperatorKeyReleaseForSpecificKey which I think is trying to test a similar remote attestation set-up?

mkulke commented 1 month ago

Cool - I knew I'd seen a PR from you updating this code recently, but couldn't find it. It feels like that test is mostly a subset of the testing of TestKbsKeyRelease, but doesn't require the KBS to be deployed in our set-up, so maybe there are overlaps with TestTrusteeOperatorKeyReleaseForSpecificKey which I think is trying to test a similar remote attestation set-up?

well. it does require KBS to be deployed, since this is the remote party of the remote attestation. what it doesn't need is actual secrets to be copied around and asserted.

stevenhorsman commented 1 month ago

well. it does require KBS to be deployed, since this is the remote party of the remote attestation. what it doesn't need is actual secrets to be copied around and asserted.

But the KBS doesn't need to be deployed by our test code for this test, just the KBS endpoint supplied as an envvar?

mkulke commented 1 month ago

But the KBS doesn't need to be deployed by our test code for this test, just the KBS endpoint supplied as an envvar?

it does need to be deployed, otherwise it wouldn't be able to test remote attestation. what the test doesn't bother with is secrets.

kbs is actually 2 services (logically, since you can run them in a single instance). a token-kbs and a resource-kbs, the token-kbs does the remote attestation, hands out a token, and using this token you can go to resource-kbs to get the secret.

that's why the cdh config has seemingly redundant kbs url entries. in a real deployment you might want token-kbs and resource-kbs to be different systems.