-
Notifications
You must be signed in to change notification settings - Fork 94
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
attestation-agent-config: generate attestation-agent config when createVM instance #1868
attestation-agent-config: generate attestation-agent config when createVM instance #1868
Conversation
@mkulke @stevenhorsman @liudalibj @bpradipt This is still in draft, I need some time to verify the code. Idea is to generate the agent-config.toml similar as cdh.toml. described in #1852 |
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.
looks good in general. however, we were discussing in slack that we probably don't need to template the agent-config file anymore, after we have a config file for AA, since all the fields but AAKBCParams
(which we wouldn't need anymore) are static and can be set at buildtime. what do you think?
ae922b5
to
321b2a1
Compare
@mkulke because |
My reasoning was:
|
@mkulke which is reasonable to me, I like this approach. How does the CDH and AA use |
we already do that for |
OK, my understanding is we need generate an |
yup, exactly note: there is a nuisance when using a aa.toml file for CAA currently: an aa config file is only valid if it has a /usr/bin/attestation-agent "${AA_CONFIG_FILE+--config $AA_CONFIG_FILE}" |
fe61999
to
09fae96
Compare
@mkulke I found we still need set
While it's OK if files |
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.
looks good! couple of nits
Sounds like CDH lost a function - Never use |
does CDH use the config file? In CAA we've been using the config file for CDH for a while now and it shouldn't rely on the agent-config.toml any more. This is the respective logic in CDH. |
where do you see that CDH is falling back to kata-agent-config? |
I did not find the code yet, it's confusion because I saw CDH logs like this and the function does not work if
|
Sounds like https://github.com/confidential-containers/guest-components/blob/main/confidential-data-hub/hub/src/bin/config/mod.rs#L130 |
Thanks, for searching, I didn't spot it myself. I suspect this function is dead code, but I'm not 100% sure. this fn sets AA_KBC_PARAMS + KBS_CERT envs as a side-effect and will do nothing else: pub fn set_configuration_envs(&self) {...} I was suspecting that image-rs is using it for encrypted images, but it doesn't seem to be used in guest-components: /dev/guest-components (main)$ git grep AA_KBC_PARAMS
attestation-agent/attestation-agent/src/config/aa_kbc_params.rs: if let Ok(params) = env::var("AA_KBC_PARAMS") {
confidential-data-hub/README.md:* **AA_KBC_PARAMS** environment variable
confidential-data-hub/hub/src/bin/config/mod.rs: "AA_KBC_PARAMS", |
Note: |
Yeah, I think which comes from the CDH config if agent-config.toml does not provide it, https://github.com/confidential-containers/guest-components/blob/main/confidential-data-hub/hub/src/bin/config/mod.rs#L133-L136, that's why I dropped it in this commit fc0a04e. @mkulke |
I tried it on s390x by building the image following as below:
|
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.
process-user-data
will not start on mkosi x86 images currently (and hence there is no daemon.json
provisioned, because this overlay issues an update-agent-config
command, that is not available any more:
mkosi.skeleton/usr/lib/systemd/system/process-user-data.service.d/10-override.conf
My fault, |
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.
this is what we provision when we specify --aa-kbc-params offline_fs_kbc::null
cat /run/peerpod/aa.toml
[token_configs]
[token_configs.coco_as]
url = ''
[token_configs.kbs]
url = 'null'
not sure if that makes sense, it probably doesn't, but it might also not hurt at the moment. I think what we could do is just provision that file if aa-kbc-params
is cc_kbc::*
from the CAA side, because the config file doesn't cover anything else yet.
Indeed, aa.toml, not sure cdh.toml should follow same pattern? |
With the fix in confidential-containers/guest-components#599, I think we can leave the code as is, I will ad a log for s.aaKBCParams and pick the guest-components new commit |
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.
lgtm. thanks!
Tested the following combinations on azure SNP & TDX cvms:
- empty aa_kbc_params
- aa_kbc_params=offline_fs_kbc
- aa_kbc_params=cc_kbc::$some_kbs
logged into the podvm to verify that AA, CDH and ASR services run properly and retrieved a confidential resource from KBS.
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.
The code is general fine, but I'd like to see the lbivirt e2e tests run before merging.
attestation-agent-config: generate and use attestation-agent config toml - Generate the attestation-agent toml file aa.toml when aaKBCParams provided - Use the cfg file to start attestation agent service when it exists - Start attestation agent service directly when no cfg file exists - remove aa_kbc_params in agent-config so that cdh won't read from it - rename package agent to aa to reflect the real config Signed-off-by: Qi Feng Huo <[email protected]>
- remove process-user-data.service.d in mkosi because update-agent-config is not needed after we generated aa.toml and cdh.toml dynamically - kata-agent-config.toml is static config file now Signed-off-by: Qi Feng Huo <[email protected]>
- start cdh using config conditionally in cdh service Signed-off-by: Qi Feng Huo <[email protected]>
- add aa.toml in in process-user-data provision list Signed-off-by: Magnus Kulke <[email protected]> Signed-off-by: Qi Feng Huo <[email protected]>
- Use same config.TokenCfg.CocoAs.URL as config.TokenCfg.Kbs.URL - Added log for aaKBCParams - Updated guest-components to df60725afe0ba452a25a740cf460c2855442c49a to pick cc_kbs cfg fix Signed-off-by: Qi Feng Huo <[email protected]>
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.
LGTM, thanks @huoqifeng
Tested this pr with s390x fedora image.
Both sample
and ibm.se
attestation drivers work as expected.
I tried to build podvm ubuntu amd64 image, from this PR.
with the built out podvm-image I can create a peerpod succeed. The related log from journalctl
@huoqifeng it seems that there are cycle decency on |
oh, I only tested mkosi on x86_64 (i.e. without cloud-config) So assuming you tested mkosi + cloud-config, and that worked, we probably should look into the delta of packer vs mkosi + cloud-config |
src/cloud-api-adaptor/podvm/files/etc/systemd/system/attestation-agent.service
Outdated
Show resolved
Hide resolved
src/cloud-api-adaptor/podvm/files/etc/systemd/system/confidential-data-hub.service
Outdated
Show resolved
Hide resolved
src/cloud-api-adaptor/podvm/files/etc/systemd/system/kata-agent.service
Outdated
Show resolved
Hide resolved
Verified: replace the With this replace fix, packer ubuntu amd64 peerpod + cloud-init + libvirt + sample kbc, works fine. |
fix cycle dependnecy between systemd services by use cloud-init instead cloud-final to avoid cycle dependency in service files Signed-off-by: Qi Feng Huo <[email protected]>
Fixes: #1852
Generate the attestation-agent toml file aa.toml when aaKBCParams provided
Use the cfg file to start attestation agent service when it exists
Start attestation agent service directly when no cfg file exists
remove aa_kbc_params in agent-config so that cdh won't read from it (this is tricky issue in CDH)
rename package
agent
toaa
to reflect it's for attestation agent configcerts field will be added in aa config in enable kbs cert for cdh #1875 after we tried it out
Signed-off-by: Qi Feng Huo [email protected]