-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
guest-components: Bump guest-components dependency #1865
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -595,13 +595,14 @@ func DoTestKbsKeyRelease(t *testing.T, e env.Environment, assert CloudAssert) { | |
func DoTestKbsKeyReleaseForFailure(t *testing.T, e env.Environment, assert CloudAssert) { | ||
|
||
log.Info("Do test kbs key release failure case") | ||
pod := NewBusyboxPodWithName(E2eNamespace, "busybox-wget-failure") | ||
pod := NewCurlPodWithName(E2eNamespace, "curl-failure") | ||
testCommands := []TestCommand{ | ||
{ | ||
Command: []string{"wget", "-q", "-O-", "http://127.0.0.1:8006/cdh/resource/reponame/workload_key/key.bin"}, | ||
Command: []string{"curl", "-s", "http://127.0.0.1:8006/cdh/resource/reponame/workload_key/key.bin"}, | ||
ContainerName: pod.Spec.Containers[0].Name, | ||
TestCommandStdoutFn: func(stdout bytes.Buffer) bool { | ||
if strings.Contains(stdout.String(), "request unautorized") { | ||
body := stdout.String() | ||
if strings.Contains(strings.ToLower(body), "error") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be good to have a more specific error, but I think that depends on confidential-containers/guest-components#587, so this is okay for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess what we want to do is assert on an http status code (401 in this case once we have this). instead of relying on error strings |
||
log.Infof("Pass failure case as: %s", stdout.String()) | ||
return true | ||
} else { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Magnus, just to check I understand - the attestation-agent doesn't build in the KBCs supported now, but uses attesters that communicate specifics with the matching verifier that runs on the KBS? What is the behaviour for
aa_kbc_params=offline_fs_kbc::null
? is the offline_fs_kbc always included as well as cc_kbc?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this part well either. it's probably good that we upgrade. cc_kbc doesn't exist anymore as a specific flag. AA now exposes only this API:
While it used to be like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, I assume, the kbc is picked in the CDH config now:
that means for
offline_fs_kbc
to work, the CDH would in theory need to be populated. we currently do this via CAA + Cloud-Config, if the --aa-kbc-params is set. However, if it isn't set, there will be no CDH config file. However² CDH falls back tooffline_fs_kbc
if there's no config file afaik ...so it should work ootb 🍀There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - I guess we don't have any upstream testing of offline_fs_kbc any more (and no downstream testing since we switched to
main
?), so we might have found a(nother) hole, but that's not a reason to not try and stay current.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure. things might also just miraculously keep working... let's see what the e2e tests will report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory everything should continue working. The startup-up code is really convoluted tho. Hopefully we will clean it up when we finally switch to the init-data approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkulke does it really make sense to keep AA_KBC option to populate agent-config.toml anymore?
Or should we use AA_KBC to create a static CDH config during pod vm image building ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes indeed, that makes sense, we would always create a CDH config file, defaulting to offline_fs_kbc if —aa-kbc-params is not provided.
it would need more work though, since CDH depends on the AA sock atm, and AA also needs the templated kata agent-config.toml
So, I suggest we defer this to another PR, introducing an AA config file.