Skip to content
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

cloud-config: add support for CDH config #1748

Merged

Conversation

mkulke
Copy link
Contributor

@mkulke mkulke commented Mar 15, 2024

fixes #1720

This change will add a write_files entry to the cloud-config file that is produced by CAA. aa-kbc-params are converted into a config file with kbc name and kbc url.

process-user-data has been made more flexible to also support this entry.

guest-components in versions.yaml has been updated to a new revision that requires a cdh config file.

the kata-agent service unit has been extended to have the env CDH_CONFIG_FILE=/run/confidential-containers/cdh.toml set, which is the path that we add as a cloud-config directive.

Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

Just to confirm I've understood correctly, you are currently leaving the agent-config.toml alone and not removing the AA_KBC param from there? I'm not completely sure of how the interaction between agent_config AA_KBC and the cdh config will be in CCv0, but you will understand better than I do the precedence of the new CDH configuration, so I guess that's okay for this PR, and we can re-visit when we switch to main

@mkulke mkulke force-pushed the mkulke/support-cdh-config-file branch from 0c60023 to fc7a00a Compare March 15, 2024 10:03
@mkulke
Copy link
Contributor Author

mkulke commented Mar 15, 2024

This looks good to me.

Just to confirm I've understood correctly, you are currently leaving the agent-config.toml alone and not removing the AA_KBC param from there? I'm not completely sure of how the interaction between agent_config AA_KBC and the cdh config will be in CCv0, but you will understand better than I do the precedence of the new CDH configuration, so I guess that's okay for this PR, and we can re-visit when we switch to main

yes, we should remove that logic. I just pushed the draft PR in this state to get a libvirt test run since this is rather intrusive and I'd expect some breakage.

@mkulke mkulke force-pushed the mkulke/support-cdh-config-file branch from fc7a00a to 39cdd02 Compare March 15, 2024 11:17
@mkulke
Copy link
Contributor Author

mkulke commented Mar 15, 2024

Compilation target (architecture, OS) tuple (s390x, linux) is not part of the supported tuples. Please compile with the "generate-bindings" feature or add support for your platform :)', /root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tss-esapi-sys-0.5.0/build.rs:35:17

Ok, that error is unrelated to the change but it's another breaking change that prevents us from updating guest-components.. current revisions of guest-components will compile all attester modules and that doesn't always make sense e.g. the TDX bindings for the IBM archs.

I wonder why this didn't pop up on the guest-components CI, do we have s390x builds there?

@stevenhorsman
Copy link
Member

I wonder why this didn't pop up on the guest-components CI, do we have s390x builds there?

I added some basic s390x workflow for CDH:
https://github.com/confidential-containers/guest-components/blob/18d8062899127302f691621e1c70bdcb4814a244/.github/workflows/cdh_basic.yml#L59-L61,

but maybe that isn't running the TEE_PLATFORM="all", or whatever the issue that Wainer also found meant there were libc incompatibilities. Maybe Ding might understand it better?

@mkulke
Copy link
Contributor Author

mkulke commented Mar 15, 2024

I wonder why this didn't pop up on the guest-components CI, do we have s390x builds there?

I added some basic s390x workflow for CDH: https://github.com/confidential-containers/guest-components/blob/18d8062899127302f691621e1c70bdcb4814a244/.github/workflows/cdh_basic.yml#L59-L61,

but maybe that isn't running the TEE_PLATFORM="all", or whatever the issue that Wainer also found meant there were libc incompatibilities. Maybe Ding might understand it better?

I see. we probably need something similar for attestation agent (which has the TEE specific attester code)

It should be possible to set the ATTESTER env explicitly to overwrite the all-attesters feature, let's see...

@mkulke mkulke force-pushed the mkulke/support-cdh-config-file branch 2 times, most recently from d170d0a to 92e50eb Compare March 16, 2024 15:37
@mkulke
Copy link
Contributor Author

mkulke commented Mar 18, 2024

I fixed guest-components to a revision with CDH config file, but not the latest, which will not run anyway, due to problems with CCv0 (see #1750), so we don't run into the build issues either.

@mkulke
Copy link
Contributor Author

mkulke commented Mar 18, 2024

Just to confirm I've understood correctly, you are currently leaving the agent-config.toml alone and not removing the AA_KBC param from there? I'm not completely sure of how the interaction between agent_config AA_KBC and the cdh config will be in CCv0, but you will understand better than I do the precedence of the new CDH configuration, so I guess that's okay for this PR, and we can re-visit when we switch to main

After testing it became clear that we need both:

  • AA_KBC_PARAMS in kata-agent to configure attestation-agent
  • cdh.toml to configure confidential-data-hub

It has to do with passport model that was introduced. AA will perform remote-attestation w/ the configured KBS to get a token. CDH will use that token to retrieve a secret from KBS. In theory we could have different kbs'es for CDH and AA (token-kbs vs resource-kbs) but we don't have any config options for this.

@stevenhorsman
Copy link
Member

After testing it became clear that we need both:

  • AA_KBC_PARAMS in kata-agent to configure attestation-agent
  • cdh.toml to configure confidential-data-hub

Thanks for the info.

Are there any plans to use an aa.toml type approach to avoid going through the kata-agent?

I guess that shows that we still need:
https://github.com/kata-containers/kata-containers/blob/c25983202cc7a45a03582edae2945c8807c1f9de/tests/integration/kubernetes/k8s-confidential-attestation.bats#L56-L59:

        # TODO - do we still need AA_KBC set and checked - do we have any other options?
	if [ "${AA_KBC}" = "cc_kbc" ]; then
		kernel_params_value+=" agent.aa_kbc_params=cc_kbc::${CC_KBS_ADDR}"
	fi

in the current merge-to-main attestation resource testing as well? Though I don't think we have AA_KBC set in out CI anywhere anymore, so maybe that is why kata-containers/kata-containers#8870 is failing?

fixes confidential-containers#1720

This change will add a write_files entry to the cloud-config file that
is produced by CAA. aa-kbc-params are converted into a config file with
kbc name and kbc url.

process-user-data has been made more flexible to also support this
entry.

guest-components in versions.yaml has been updated to a new revision
that requires a cdh config file.

the kata-agent service unit has been extended to have the env
CDH_CONFIG_FILE=/run/confidential-containers/cdh.toml set, which is the
path that we add as a cloud-config directive.

Signed-off-by: Magnus Kulke <magnuskulke@microsoft.com>
@mkulke mkulke force-pushed the mkulke/support-cdh-config-file branch from 92e50eb to 2d5b755 Compare March 18, 2024 09:37
@mkulke
Copy link
Contributor Author

mkulke commented Mar 18, 2024

Thanks for the info.

Are there any plans to use an aa.toml type approach to avoid going through the kata-agent?

I guess that shows that we still need: https://github.com/kata-containers/kata-containers/blob/c25983202cc7a45a03582edae2945c8807c1f9de/tests/integration/kubernetes/k8s-confidential-attestation.bats#L56-L59:

        # TODO - do we still need AA_KBC set and checked - do we have any other options?
	if [ "${AA_KBC}" = "cc_kbc" ]; then
		kernel_params_value+=" agent.aa_kbc_params=cc_kbc::${CC_KBS_ADDR}"
	fi

in the current merge-to-main attestation resource testing as well? Though I don't think we have AA_KBC set in out CI anywhere anymore, so maybe that is why kata-containers/kata-containers#8870 is failing?

there is some config file support for AA (see this PR), but I think it's not a full replacement for aa_kbc_params yet, since it only covers the uri part. So yes, I'd assume you still need to set aa_kbc_params and hence AA_KBC (or tweak the build script).

@mkulke mkulke marked this pull request as ready for review March 18, 2024 11:19
@mkulke mkulke mentioned this pull request Mar 18, 2024
6 tasks
@mkulke mkulke requested a review from wainersm March 20, 2024 14:16
Copy link
Member

@liudalibj liudalibj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't try it by hand,
but the codes LGTM.

@mkulke mkulke merged commit 4d03250 into confidential-containers:main Mar 21, 2024
27 checks passed
@mkulke mkulke deleted the mkulke/support-cdh-config-file branch March 21, 2024 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_e2e_libvirt Run Libvirt e2e tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podvm: cannot launch the latest CDH any more
3 participants