add support for hw keys certs and attestations#2622
Conversation
5a3166c to
4a5d445
Compare
Dry-run check results |
4a5d445 to
0e05902
Compare
marcoieni
left a comment
There was a problem hiding this comment.
Thanks! Some initial comments 👍
AI disclosure
I used GPT5.6-Sol with the codex harness to find 6/7 of these findings
| last-name = "Doe" # Your last name or preferred surname | ||
| account-handle = "john.doe" # The handle to define a @rust-lang.org account (e.g. john.doe@rust-lang.org) | ||
|
|
||
| hardware-keys = ["key-id"] |
There was a problem hiding this comment.
this is unrelated to [google-workspace] so we should move it at the top, right?
There was a problem hiding this comment.
If we don't want to have a new top-folder directory, we could have hardware-keys under people, which also makes it more clear that it's related to that 🤔
Wdyt?
The con I can think of:
peoplenow has a subdirectory, which might break some scripts- the URL for retreaving hardware keys might be longer?
There was a problem hiding this comment.
I thought about this, and tbh I don't have a strong opinion. I think the top-level folder causes less harm.
| wrapper(data.people(), errors, |person, _| { | ||
| for key in person.hardware_keys() { | ||
| if !data.hardware_keys(key) { | ||
| dbg!(data); |
There was a problem hiding this comment.
is this a leftover or you want to have it here? It might be to spammy, right? 🤔
| } | ||
| Ok(()) | ||
| }); | ||
| } |
There was a problem hiding this comment.
we could also add the following validation: "Enforce that a key is only owned by exactly one person" to avoid duplicates and orphans
|
|
||
| let hardware_keys_dir = directory.join("hardware-keys"); | ||
| for entry in std::fs::read_dir(hardware_keys_dir)? { | ||
| let path = entry?.path(); |
There was a problem hiding this comment.
let's validate that path is a directory. Do we also validate that the directory contains the files we expect? (and only those files?)
There was a problem hiding this comment.
For now we don't validate the dir contents because I don't know about whether we should always expect the attestation files for both the 9a and 9c slots. For example, perhaps there is an agreement that only 9a attestation is required.
I can clarify with that with folks involved in TUF, but anyhow, my take would be introduce this additional validation as a follow-up. Let me know whether it makes sense to you!
|
|
||
| pub(crate) fn hardware_keys(&self, key_alias: &str) -> bool { | ||
| self.hardware_keys.contains(&key_alias.to_string()) | ||
| } |
There was a problem hiding this comment.
given that this is the only operation we do, should we use a BTreeSet instead of a Vec? Also, maybe there's a way to check that the element is contained without allocating a string (to_string) ? 🤔
There was a problem hiding this comment.
Added an equivalent check that don't allocate here
| #[serde(default)] | ||
| permissions: Permissions, | ||
| google_workspace: Option<GoogleWorkspace>, | ||
| #[serde(skip_serializing_if = "Vec::is_empty", default)] |
There was a problem hiding this comment.
| #[serde(skip_serializing_if = "Vec::is_empty", default)] | |
| #[serde(default)] |
no need since the type doesn't inherit Serialize.
Towards #2501. Context here.
This PR introduces a new
hardware-keysfolder and assumes a convention that each subfolder corresponds to attestation files and (f9) certificates derived from a single Yubikey.There is a minimal TOML schema addition + validation to link a team member with these folders and files. We don't expose any information from these files through
teamAPI for now.Documentation on how to generate these files to rollow on Forge.
In general we can expect
That means a few dozens of files a few Kbs each stored at this repo, which sounds doable.
I added two for myself related to PIV 9a (auth) as an example of these files.