confidential-containers / confidential-containers

Confidential Containers Community
https://confidentialcontainers.org/
Apache License 2.0
199 stars 48 forks source link

Combine AA/ocicrypt-rs/image-rs to one repo #89

Closed Xynnn007 closed 1 year ago

Xynnn007 commented 1 year ago

Background

Now some repos under CoCo have a dependency relationship, s.t.

The underlying reason is enclave-cc depends on image-rs and now uses function call rather than interprocess communication for confidential resource injection. Thus we need to chain these dependencies.

This leaves developers (like me) great trouble when we want to make a change to attestation-agent. We need to do the following things:

  1. Make an PR that brings some new code in attestation-agent
  2. Make an PR in ocicrypt-rs, which only changes the rev of attestation-agent
  3. Make an PR in image-rs, which only changes the rev of both attestation-agent and ocicrypt-rs

Step 2 and 3 are great waste of time. Similiar things will happen when cutting releases, also making great trouble.

Suggestion

We can combine these three repos into one, with different workspace like what we do in td-shim

This will also support publishing single crates image-rs, ocicrypt-rs and attestation-agent to crates.io. See the example of block-cipher in RustCrypto

fitzthum commented 1 year ago

I think this is a good idea. There might be some outside consumers of ocicrypt-rs who wouldn't be interested in the other pieces. We can keep the crates separate, but it still might be slightly confusing for people who just want to contribute to one project. That said, I'm not aware of any people who actually fall into this category, and I think the synchronization benefits far outweigh any potential issues.

What do we want to call the combined repo? Do we want to create a new repo for all three or move two projects into an existing one?

Xynnn007 commented 1 year ago

An naive idea: for these are all software stack inside an "guest"/"pod", we might directly name that "pod-stack"? Potentially, AS and KBS can also be combined for the same reason with name "tenant-stack"?

fitzthum commented 1 year ago

Yeah maybe "guest-tools" or "guest-components" although that might imply that the Kata Agent would be there too which it won't be.

stevenhorsman commented 1 year ago

From my experience of doing some releases and trying to keep versions of these projects in sync, I think this sounds like a good idea to pull the repos together, but still keep the crates separate. We'll probably have to have some good doc on the front page to direct people to the 'sub-projects' as appropriate. I'm not going to propose a name though as I'm terrible at that!

dcmiddle commented 1 year ago

Sounds like a good idea to combine these 3. I think there's a little error in the top of the issue describing the dependencies...

ocicrypt-rs depends on attestation-agent image-rs depends on both:

I'd suggest merging all 3 into image-rs and keeping the image-rs name unchanged. This will save many hours of angst to find the perfect name :-D

fitzthum commented 1 year ago

I'd suggest merging all 3 into image-rs and keeping the image-rs name unchanged.

Putting the AA under image-rs is confusing to me.

sameo commented 1 year ago

The image-rs dependency on ocicrypt-rs makes sense to me, it's logical. The direct 'attestation-agent` one, maybe less so and things would be cleaner with only a gRPC interface dependency.

@Xynnn007 Do you need to go through steps 2 and 3 because the AA API/ABI is changing?

sameo commented 1 year ago

Also, if we follow that logic we will put any future component that depends on the AA crate under that repo. So I'd rather call it guest-stack or guest-components rather than regrouping under image-rs.

Xynnn007 commented 1 year ago

@sameo

Thanks for the question. Current implementation in enclave-cc a process named enclave-agent who plays the role similar as kata-agent and it is responsible for pulling encrypted images. So it is needed to do attestation itself. This is determined by SGX abstraction (a process, not a VM like TDX & SNP)

If we do the same thing as kata-agent using gRPC/ttRPC, s.t. put attestation-agent in another process, the attested TCB will be the process of AA rather than enclave-agent. That's why the only choice is function call.

mythi commented 1 year ago

Following to @Xynnn007's comments, the functionality is useful not only for cVMs so guest-* is misleading.

I'm also wondering what the future will be wrt: moving to a snapshotter and making AA a generic secrets provider for workloads. If all the three repos won't fit together easily, we can keep image-rs as is and have key/secrets/resource provisioning including, ocicrypt-rs as the KeyProvider, attesters, kbcs, and AA in one repo (be it tee-secrets-provisioning).

Having ocicrypt-rs and attestation-agent developed and validated together and updated to image-rs in one go should already simplify the flow.

dcmiddle commented 1 year ago

I guess my point was to use the image-rs repo and then have 3 workspaces / crates within it. Not to change any dependency structure. Like the discussion about merging the documentation and community repo it probably makes sense to reuse one of these repos (image-rs) and then rename it if we like to something else.

For example, merge attestation-agent and ocicrypt-rs into image-rs *repo*. Create a workspace and shift image-rs *code*. Rename image-rs repo to guest-components.

guest-components/
  Cargo.toml #workspace
  attestation-agent/
  image-rs/
  ocicrypt-rs/
Xynnn007 commented 1 year ago

Let's close this issue because https://github.com/confidential-containers/image-rs/pull/158 is merged. I will open an issue under image-rs to track the fixes for CI.