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

Move non-attestation functions out of AA #468

Merged
merged 7 commits into from
Mar 5, 2024

Conversation

Xynnn007
Copy link
Member

@Xynnn007 Xynnn007 commented Feb 5, 2024

The other half work for #412

cc @jialez0

@Xynnn007 Xynnn007 marked this pull request as ready for review February 5, 2024 07:03
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.

Few minor comments but I think this looks pretty good.

attestation-agent/README.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be some duplication between the gRPC and ttRPC implementations, but I guess that's ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. Before this PR, we put mod grpc and mod ttrpc inside one same file attestation/mod.rs with different mod blocks to implement like attestation proto APIs. Also, main() functions were put in src/grpc.rs and src/ttrpc.rs. In this PR, I bring the two mod blocks out and put into different files. And also bring grpc and ttrpc binaries into different rust binary targets rather than a same binary controlled by grpc/ttrpc feature. I think it would be more clear from viewpoint of file structure personally. wdyt? cc @jialez0

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.

Seems good. A couple other minor comments.

attestation-agent/attestation-agent/Cargo.toml Outdated Show resolved Hide resolved

- name: Run cargo test
uses: actions-rs/cargo@v1
with:
command: test
args: --features openssl,rust-crypto,offline_fs_kbc,kbs,coco_as -p attestation-agent -p attester -p coco_keyprovider -p kbc -p kbs_protocol -p attestation_agent -p crypto -p resource_uri -p sev@0.1.0
args: --features openssl,rust-crypto,all-attesters,kbs,coco_as -p attestation-agent -p attester -p coco_keyprovider -p kbc -p kbs_protocol -p crypto -p resource_uri
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the offline_fs_kbc or offline_sev_kbc being tested in the CDH workflow. Do we need to add that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add some unit tests to offline_fs_kbc, which will be enabled by default for CDH without specifying features explicitly.

offline_sev_kbc will not be used so it will not be test.

The original code of online_sev_kbc does not have any tests. The test will enable sev feature, which will cover the tests of online_sev_kbc.

@Xynnn007 Xynnn007 force-pushed the refactor-aa branch 2 times, most recently from c78fd35 to 43da2a7 Compare February 21, 2024 02:12
@Xynnn007
Copy link
Member Author

cc @jialez0

@fitzthum
Copy link
Member

Anyone else want to take a look at this? @mkulke @arronwy ?

Copy link
Member

@jialez0 jialez0 left a comment

Choose a reason for hiding this comment

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

LGTM.

After moving non-attestation related functionalities from AA to CDH, it
is time to delete these functionalities from AA, including the following

1. KBCs
2. abilities to access KBS

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
@Xynnn007 Xynnn007 force-pushed the refactor-aa branch 2 times, most recently from ce39eba to 6f98b56 Compare March 5, 2024 02:02
AA binary was a separate crate named `app`. For rust we can use binary
section
https://doc.rust-lang.org/cargo/reference/cargo-targets.html#binaries to
specify the binary built based on the crate.

This commit will combine `app` and `lib` crate into `attestation-agent`
lib. This will help users use a unambiguous crate name.

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
Now AA does not support KBCs and only have attestation related
functions. Thus we define a new flag named `ATTESTER` in AA's Makefile
to specify the attester plugin.

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
After moving non-attestation abilities from AA to CDH, we do not need
offline-fs-kbc in AA. Instead, it is in CDH now.

Also, test will not cover sev crate as it is now not used.

Fixes the related crate names.

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
Currently, not all TEEs support initdata mechanism. Thus we add a result
enum named `InitdataResult` that has three results
- Result::Ok(InitdataResult::Ok): initdata mechanism is supported and
check ok.
- Result::Ok(InitdataResult::Unsupported): initdata mechanism is not
supported on this platform
- Result::Err(_): error occurs when checking initdata

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
@fitzthum fitzthum merged commit 917097b into confidential-containers:main Mar 5, 2024
15 checks passed
@Xynnn007 Xynnn007 deleted the refactor-aa branch March 6, 2024 00: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.

4 participants