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

aa_kbc_params: parse from kernel cmdlind first #511

Merged
merged 3 commits into from
Mar 19, 2024

Conversation

Xynnn007
Copy link
Member

In CoCo, the aa_kbc_params is still passed from the kernel cmdline and will be read in Confidential Data Hub. If we do not provide a default configuration file for CDH, a default one will set the aa_kbc_params env to offline-fs-kbc. This would make the kernel cmdline not work.

As a workaround, this commit bring the kernel cmdline parsing first. The disadvantage is that we will not support to use any other aa_kbc_params if we already set the kernel cmdline. This disadvantage will be covered once initdata is implemented.

following up to kata-containers/kata-containers#8870 (comment)

cc @fitzthum @mkulke @wainersm

@Xynnn007 Xynnn007 requested a review from jialez0 as a code owner March 19, 2024 02:31
Copy link
Contributor

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

as an alternative approach, could we not set the ENV at CDH's launch if get_value() returns something?

In CoCo, the aa_kbc_params is still passed from the kernel cmdline and
will be read in Confidential Data Hub. If we do not provide a default
configuration file for CDH, a default one will set the aa_kbc_params
env to offline-fs-kbc. This would make the kernel cmdline not work.

This fix checks if aa_kbc_params is given. If so, skip the logic of
setting env to allow the original aa_kbc_params to work.

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007 Xynnn007 force-pushed the fix-aa-kbc-params branch from 172fe9f to f1d3123 Compare March 19, 2024 10:56
@Xynnn007 Xynnn007 requested a review from sameo as a code owner March 19, 2024 10:56
Copy link
Contributor

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

LGTM, but there are some test failures. One minor check thing and one await that needs to be removed.

"AA_KBC_PARAMS",
format!("{}::{}", self.kbc.name, self.kbc.url),
);
if let Err(_) = attestation_agent::config::aa_kbc_params::get_value() {
Copy link
Member

@wainersm wainersm Mar 19, 2024

Choose a reason for hiding this comment

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

Hi @Xynnn007 !

attestation_agent::config::aa_kbc_params::get_value() is a very simple function. I wonder if it wouldn't make sense to simply duplicate it to CDH to avoid creating this dependency CDH -> AA as the build of CDH can get overly complicated.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind. I just noticed cdh -> AA dependency is already established.

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed, but they communicate via API and a clear interface, so a direct dep like this is unfortunate. I think we want to establish what configuration options we want to provide and then build configuration accordingly. At the moment this is more or less spec' ed in the implementation (e.g. "if aa_kbc_params kernel param is unset and there is no CDH config file, set aa_kbc_params as environment parameter to configure the CDH KMS - but without KBS cert").

So, there are a few assumptions, and it would be a good idea to simplify, even if that means duplication, e.g. the introduction of a cdh_kbs_params kernel param if we want to support passport on platforms without dynamic initdata.

Removes one await that we don't need and simplifies one
conditional.

Signed-off-by: Tobin Feldman-Fitzthum <[email protected]>
@fitzthum fitzthum merged commit adca2f9 into confidential-containers:main Mar 19, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants