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: centralize handling in CDH and AA #440

Conversation

mkulke
Copy link
Contributor

@mkulke mkulke commented Jan 18, 2024

In both AA and CDH there is duplicate logic to retrieve aa_kbc_params through various means. This change moves all related logic into a single place to guarantee consistency and make it easier to iterate on the way we retrieve aa_kbc_params (it is expected that the current approaches will be replaced by init_data in some fashion).

Besides kernel cmdline and kata-agent config a third option that takes precedence over the other approaches has been added. This will help with testing, so we don't need to rely on the peerpod detection heurstic for that.
Edit: will be replaced by an aa config-file, see discussion below

e.g. AA_KBC_PARAMS=cc_kbc::http://localhost:8080 ./attestation-agent -a unix://tmp/aa-test.sock

A dependency to attestation_agent has been added to the CDH/KMS project to gain access to AA's aa_kbc_params module. I think this dependency is warranted, since this code is already coupled to AA implicitly (by depending on aa_kbc_params).

@mkulke mkulke requested review from jialez0 and sameo as code owners January 18, 2024 13:52
@mkulke mkulke force-pushed the mkulke/centralize-aa_kbc_params-handling branch 2 times, most recently from 678883b to a791697 Compare January 18, 2024 15:24
@mkulke
Copy link
Contributor Author

mkulke commented Jan 18, 2024

The unit tests should pass once #438 has been merged and this is rebased. the failing image-rs tests seem unrelated, they seem to fail in other PRs in a similar fashion

@fitzthum
Copy link
Member

#438 is merged.

We might want to have a little sync about configuration. I think we are planning to introduce an AA config file in the near future. Also maybe there is some overlap with #439

@mkulke mkulke force-pushed the mkulke/centralize-aa_kbc_params-handling branch from a791697 to 90ceea3 Compare January 18, 2024 16:15
@mkulke
Copy link
Contributor Author

mkulke commented Jan 18, 2024

#438 is merged.

We might want to have a little sync about configuration. I think we are planning to introduce an AA config file in the near future. Also maybe there is some overlap with #439

this is mostly attempting to consolidate what exists at the moment (in a non-breaking fashion) so we can more easily iterate on a future solution.

In both AA and CDH there is logic to retrieve aa_kbc_params through
various means. This change moves all related logic into a single place
to guarantee consistency and make it easier to iterate on the way we
retrieve aa_kbc_params (it is expected that the current approaches will
be replaced by init_data in some fashion).

A dependency to `attestation_agent` has been added to the CDH/KMS
project to gain access to AA's `aa_kbc_params` module. I think this
dependency is warranted, since this code is already coupled to AA
implicitly (by depending on aa_kbc_params).

Signed-off-by: Magnus Kulke <[email protected]>
@Xynnn007
Copy link
Member

Overall, the deduplication is quite good. However, there are still some issues that need to be synchronized, which may or may not be related to this PR

  1. We may want to add a configuration file instead of env to pass parameters. The reason is that currently only aa_kbc_params is configurable, but in the future AA aims to support different attestation services. At that time, we might need to configure the URL, credentials, etc. of the specific service. It might be better if there is a configuration file. Although now without initdata we still can't leverage this configuration file.
  2. We will add configuration for both AA and CDH
  3. Once the initdata is ready, we may replace aa_kbc_params with kbs_url in AA because kbc will not be used in AA.

Overall, I recommend that we

  1. Temporarily delete env-way configuration
  2. @jialez0 agreed to merge this first, and then he will add AA configuration based on the deduplicated version.

Did I miss anything?

@mkulke
Copy link
Contributor Author

mkulke commented Jan 19, 2024

Overall, the deduplication is quite good. However, there are still some issues that need to be synchronized, which may or may not be related to this PR

  1. We may want to add a configuration file instead of env to pass parameters. The reason is that currently only aa_kbc_params is configurable, but in the future AA aims to support different attestation services.

Yes, a configuration file would also work for this purpose and possibly be more future proof. The motivation for introducing env was mostly to work around the current heuristic "if there is a peerpod file on the fs => parse kata-agent config for aa config" which couples AA to kata and peerpod in obscure ways.

I understand that you think we can introduce a configuration file now, even if the structure of the configuration is not completely settled yet. E.g. this configuration file could be just like this for now?

[kbc]
name = "cc_kbc"
uri = "https://some.kbs:8080"

Overall, I recommend that we

  1. Temporarily delete env-way configuration

Ok

@Xynnn007
Copy link
Member

btw, the image-rs ci has been fixed by #441. You can take a rebase to make CI happy then

@mkulke mkulke force-pushed the mkulke/centralize-aa_kbc_params-handling branch 3 times, most recently from 023ca36 to b0851d9 Compare January 19, 2024 07:35
Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks. cc @jialez0 @fitzthum

@fitzthum fitzthum merged commit 292ca7c into confidential-containers:main Jan 19, 2024
17 checks passed
@mkulke mkulke deleted the mkulke/centralize-aa_kbc_params-handling branch January 19, 2024 15:17
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.

3 participants