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

Add trustee-client - a simple client for Trustee #755

Closed
wants to merge 1 commit into from

Conversation

uril
Copy link

@uril uril commented Oct 15, 2024

Gather evidence, attest and get secrets from Trustee

Currently it simply uses AA's kbs_protocol client and attesters. If needed, it can be enhanced later.

Gather evidence, attest and get secrets from Trustee

Currently it simply uses AA's kbs_protocol client and attesters.
If needed, it can be enhanced later.

Signed-off-by: Uri Lublin <[email protected]>
@uril uril requested a review from a team as a code owner October 15, 2024 12:00
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.

Looks good. One comment.

Also, I am not sure the config file is really necessary. it seems a little cumbersome to have to specify the KBS URL in the config file rather than just putting it in the command line, but it's not a big deal.

Btw, it's interesting that we're in the guest-components repo. I think that's fine and maybe having some kind of client in both guest-components and trustee repos will help us decouple the tests a little bit.

@@ -17,6 +17,7 @@ members = [
"confidential-data-hub/storage",
"image-rs",
"ocicrypt-rs",
"trustee-client",
Copy link
Member

Choose a reason for hiding this comment

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

I think this name might be too general. This tool only exercises a subset of the Trustee APIs. Btw see this proposal which might end up with a similar name.

Maybe this could be called cc-kbc-resource-getter or trustee-resource-getter or trustee-resource-client or something else

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I'll take a look at that proposal and think of names.

@uril
Copy link
Author

uril commented Oct 15, 2024

Thanks @fitzthum
I can put the Trustee URL + Certificate in the command line.
If needed a configuration file can be added later.
I noticed that AA and CDH each use a configuration file so I added it here too.

@ChengyuZhu6
Copy link
Member

Good job. While this PR functions similarly to a kbs-client. I agree with Tobin's comment that it would be better to move it to the trustee repo if the client is focused on expanding the kbs-client

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.

Nice work. I noticed that the sub-crate aims to implement a binary for dep kbs_protocol. Probably we could make it a binary of kbs_protocol instead of a separate crate. Example for this

@Xynnn007
Copy link
Member

Btw, it's interesting that we're in the guest-components repo. I think that's fine and maybe having some kind of client in both guest-components and trustee repos will help us decouple the tests a little bit.

The server (trustee) and client (guest-components) codes are in two repos. If we want to have some test for compability of the two repo, it would always require us to pull the other's code/image/... in another repo

@uril
Copy link
Author

uril commented Oct 16, 2024

@ChengyuZhu6 @Xynnn007 thanks for looking.

The idea is to have a simple program in guest-components repository that can get resources from Trustee.

I thought of adding it as a binary under kbs_protocol, but if more features are added later would it stay under kbs_protocol or move ?

@Xynnn007
Copy link
Member

I thought of adding it as a binary under kbs_protocol, but if more features are added later would it stay under kbs_protocol or move ?

We can see the concrete situation then. Up to now, it seems that we can keep the feature aligning with kbs_protocol's.

@fitzthum
Copy link
Member

Yeah seems fine to keep it in this repo and I think having a binary under kbs protocol makes sense. It should be relatively easy to move this there. We can move it again if the scope increases, but it seems like the goal of this really is to have an extremely narrow scope.

@uril
Copy link
Author

uril commented Oct 22, 2024

Thanks. I'll send another PR under kbs_protocol and close this one.
Is trustee-agent an acceptable name ? If not I'll go with the suggested trustee-resource-getter/trustee-resource-client.

@Xynnn007
Copy link
Member

All names sound not bad to me, as now trustee, kbs protocol server, (cc) kbs are referring to a same thing. I think we can know what this tool is at the first glance at the name

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