-
Notifications
You must be signed in to change notification settings - Fork 6
Initial commit #1
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
Conversation
src/main.rs
Outdated
| resource_tag: &str, | ||
| ) -> Result<String> { | ||
| //let output = StdCommand::new("trustee-attester") | ||
| let output = StdCommand::new("/home/afrosi/src/trustee/target/debug/kbs-client") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to fix this
travier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not tested but looking good. Comments can be addressed as a follow up PR as well. Let's not forget to add a LICENSE to this repo immediately as well.
data.json
Outdated
| "resource_repository": "conf-cluster", | ||
| "resource_type": "root" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably have only one "path" variable here and have the logic of figuring out what is in there be in another tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split it because of the tag/id, this needs to be part of the path as well, so would you prefer to have a partial path here?
src/main.rs
Outdated
| } | ||
|
|
||
| fn main() -> Result<()> { | ||
| let matches = Command::new("clevis-pin-trustee") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the derive pattern with structures to make this easier to read. See https://github.com/confidential-clusters/compute-pcrs/blob/main/cli/src/main.rs#L10 for an example.
sarroutbi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, changes LGTM. The only comment is that the logic for fetching the key, decoding it, and creating a Jwk is nearly identical in both the encrypt and decrypt functions.
You might create a helper function to handle this logic
f14f8cc to
121a2c6
Compare
121a2c6 to
0dd1152
Compare
Containerize the build of the pin. Signed-off-by: Alice Frosi <[email protected]>
|
@travier @uril as far as I understand we need to pass to the trustee-agent the certificate for the trustee sever, am I right? If so, don't we need here an array of certificates like for the urls? If so, then I would merge the urls together with the certificate for the pin configuration. Like: {
"servers": [
{
"url": "http://trustee1:8080",
"cert": "",
},
{
"url": "http://trustee2:8080",
"cert": "",
},
],
"resource_path": "conf-cluster/<id>/root",
} |
|
The pin configuration depends on the discussion in coreos/ignition#2099 |
|
@alicefr Yes, we do want a list of (KBSURL, https-certificate). |
|
Nice ! I added some minor comments. |
53671e6 to
5f76990
Compare
Signed-off-by: Alice Frosi <[email protected]>
5f76990 to
49591e2
Compare
travier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Some nits but we can also fix them later
| @@ -0,0 +1,10 @@ | |||
| FROM docker.io/library/rust:trixie as build | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use a Fedora image here to do the build. We can do what we did in compure-pcrs.
| @@ -0,0 +1,3 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, why do we need that? Let's place the binaries in the right place directly?
| const MAX_ATTEMPTS: u32 = 3; | ||
| const DELAY: Duration = Duration::from_secs(5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine for now but we'll need to make this either bigger or configurable.
travier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start with that and we'll iterate
Create first version of the pin.
The
test.shscript tries to encrypt and unlock a device and can be used for some manual testing.In order to perform the encryption, you need to have trustee running and set its url in the
data.json. Additionally, you also need to upload the secret, and you can use thetest-secret-trusteeas example.I have uploaded the secret using the
kbs-clientfrom trustee:Clevis expect to have the encrypt and decrypt scripts, so I have created those scripts in my local enviroment:
and
Clevis looks for the scripts automatically based on the name of the pin