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-agent - a simple tool to fetch secrets from Trustee #791

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

uril
Copy link
Contributor

@uril uril commented Nov 8, 2024

The trustee attester is a tool to attest and fetch secrets from Trustee.

Trustee attester is using attestation-agent's kbs_protocol client and attesters to gather hardware-based confidential-computing evidence and send it over to Trustee.

This PR replaces #755. Binary renamed and moved under kbs_protocol.
Also Trustee URL and optional certificate are given in the command line and not in a configuration file.

@uril uril requested a review from a team as a code owner November 8, 2024 01:10
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.

Thanks @uril ! The code looks pretty good and I like it! Some last comments from my side

attestation-agent/kbs_protocol/Cargo.toml Outdated Show resolved Hide resolved
@Xynnn007
Copy link
Member

Xynnn007 commented Nov 8, 2024

cargo fmt needs to be performed under the codebase to make the CI happy.

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.

some nits

is "agent" the appropriate name here? i'd associate agent with a long-running process that is doing something on behalf of me, like attestation-agent or ssh-agent

@uril uril force-pushed the trustee-agent branch 2 times, most recently from ddac172 to 5c2b909 Compare November 10, 2024 18:19
@uril
Copy link
Contributor Author

uril commented Nov 10, 2024

cargo fmt needs to be performed under the codebase to make the CI happy.

Thanks @Xynnn007, it found two issues.

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.

Thanks. Some more nits about the readme document. The final rendering shape can be seen on the page https://github.com/uril/coco-guest-components/tree/trustee-agent/attestation-agent/kbs_protocol/src/bin/trustee-agent. You can take a look if it meets your ideal shape when debugging

@uril uril force-pushed the trustee-agent branch 5 times, most recently from 069aa99 to d4d01be Compare November 13, 2024 22:00
@uril
Copy link
Contributor Author

uril commented Nov 13, 2024

Thanks. Some more nits about the readme document. The final rendering shape can be seen on the page https://github.com/uril/coco-guest-components/tree/trustee-agent/attestation-agent/kbs_protocol/src/bin/trustee-agent. You can take a look if it meets your ideal shape when debugging

Thanks, that was a good tip. I fixed the markdown syntax.
Note that the rename broke this link.

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.

Last comments, otherwise looks good to me. Please pay attention to @mkulke 's comments

@@ -0,0 +1,33 @@
# Trustee attester #
Copy link
Member

Choose a reason for hiding this comment

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

struct Cli {
/// Trustee URL of format <protocol>://<host>:<port>
#[clap(long, value_parser)]
// url: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

Is this something left for debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, removed.

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.

a small nitpick, other lgtm

Comment on lines 78 to 82
let resource_path = format!("/{}", path); // path must start with a '/'
let resource = ResourceUri::new("", &resource_path)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this will bail with an obscure error message if the user-specified path already starts with a /, so I would suggest to either:

a) validate the path format with a regex, so we can safely prepend the /

b) prepend the / only if it's not present: format!("/{}", path.trim_start_matches('/')) (even if it looks a bit ridiculous)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a bit strange when a user asks for /repository/type/tag and gets an error message saying that the format should be /repository/type/tag.

I replaced it with:
let resource_path = match path.starts_with('/') {
false => format!("/{}", path),
true => path,
};

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for booleans we can stick to if/else statements though, they also are expressions in rust, i.e. you can return from a branch: let x = if 1 != 2 { "foo" } else { "bar" }

The trustee-attester is a simple program that gather Confidential Computing
HW "evidence", send it to Trustee and upon successful attestation get
resources.

As first implementation it directly uses attestation-agent's kbs_protocol
and attesters.

Signed-off-by: Uri Lublin <[email protected]>
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.

sorry I just noticed this: we shouldn't merge the PR without CI for the tool.

we don't need to publish binaries, but there should at least be compile/lint/format checks for the code.

you can take a look at the AA build for reference: https://github.com/confidential-containers/guest-components/blob/main/.github/workflows/aa_basic.yml

@uril
Copy link
Contributor Author

uril commented Nov 18, 2024

Added a commit with CI. Thanks for recommending aa_basic.yml.

.github/workflows/trustee-attester.yml Outdated Show resolved Hide resolved
Comment on lines 32 to 33
strategy:
fail-fast: false
matrix:
rust:
- stable
instance:
- ubuntu-24.04
include:
- instance: ubuntu-24.04
cargo_test_opts: "-p kbs_protocol --bin trustee-attester --no-default-features --features background_check,passport,openssl,all-attesters,bin"
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think this is a matrix. we only run the suite on ubuntu 24.04 w/ rust stable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the s390x entry of the list, for now, so only ubuntu is left there.
Possibly someone will re-add it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, maybe add this as a comment above the matrix statement. otherwise, it's unclear to the reader of that workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-added s390x. It was added to many other .yml files

.github/workflows/trustee-attester.yml Show resolved Hide resolved
.github/workflows/trustee-attester.yml Show resolved Hide resolved
.github/workflows/trustee-attester.yml Show resolved Hide resolved
.github/workflows/trustee-attester.yml Show resolved Hide resolved
cargo build/fmt/clippy --bin trustee-attester

Signed-off-by: Uri Lublin <[email protected]>
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

@fitzthum
Copy link
Member

@uril please keep an eye on the new workflow after we merge this. We may need a follow-up with some fixes.

@fitzthum fitzthum merged commit ed4356d into confidential-containers:main Nov 19, 2024
11 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.

5 participants