Better attestations #186
Better attestations #186
Conversation
|
This is perfect! Thank you so much! |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces long-term cryptographic key management and attestation signing capabilities to the rebuilderd daemon. The changes establish a persistent identity for the daemon and enhance the integrity of attestations through additional signatures.
- Introduces automatic generation and management of long-term Ed25519 signing keys for the daemon
- Adds cryptographic attestation signing functionality using in-toto standards
- Implements transparent re-signing of existing attestations with the daemon's key when fetched via API
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| daemon/src/attestation.rs | New module implementing cryptographic key management and attestation signing using in-toto |
| daemon/src/args.rs | New command-line argument parser with key generation and signing key options |
| daemon/src/main.rs | Refactored main function to support key generation commands and private key loading |
| daemon/src/lib.rs | Updated to integrate attestation module and pass private key to web handlers |
| daemon/src/api.rs | Enhanced with new public key endpoint and attestation signing in build reports |
| daemon/src/config.rs | Added configuration option for transparent attestation signing |
| common/src/utils.rs | New utility function for atomic file creation and key loading |
| tests/src/main.rs | Comprehensive test suite additions for attestation verification and key management |
| worker/src/auth.rs | Refactored to use shared utility function for key loading |
| Multiple config files | Updated configuration templates and dependency specifications |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -14,7 +14,7 @@ pub async fn download(url_str: &str, path: &Path) -> Result<PathBuf> { | |||
| let filename = url | |||
| .path_segments() | |||
| .ok_or_else(|| format_err!("Url doesn't seem to have a path"))? | |||
| .last() | |||
| .next_back() | |||
There was a problem hiding this comment.
[nitpick] Using next_back() instead of last() is unnecessary here. Both methods return the last element of the iterator, but last() is more idiomatic and clearly expresses the intent to get the final element.
| .next_back() | |
| .last() |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Clippy disagrees:
warning: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
| @@ -19,6 +19,7 @@ impl actix_web::error::ResponseError for Error {} | |||
|
|
|||
| impl From<errors::Error> for Error { | |||
| fn from(err: errors::Error) -> Error { | |||
| errors::error!("error occurred in http handler: {err:#}"); | |||
There was a problem hiding this comment.
[nitpick] This error logging may create duplicate log entries since the error is likely already logged at the source. Consider using debug! or trace! level instead of error! to avoid log noise, or remove this logging if errors are handled appropriately elsewhere.
| errors::error!("error occurred in http handler: {err:#}"); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is needed because the web handlers simply use ? on Result's. Matching on Err(err) and calling error! on every function that may fail would be unnecessarily verbose.
| @@ -91,6 +91,7 @@ pub fn setup_auth_cookie() -> Result<String> { | |||
| let mut file = OpenOptions::new() | |||
| .mode(0o640) | |||
| .write(true) | |||
| .truncate(true) | |||
There was a problem hiding this comment.
Adding .truncate(true) to the auth cookie file creation could be problematic if the file already exists and contains valid data. This will overwrite existing auth cookies, potentially breaking active sessions. Consider checking if the file exists first or using a different approach to ensure atomic updates.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
At this point we've already checked the file doesn't exist. This could be written differently to be race-free, but I think it's fine.
| "signatures": [ | ||
| { | ||
| "keyid": "c25d24c04760b6982de77736776edc6600d5f8e1e84d0bba2a7299959ce7d47f", | ||
| "sig": "8cd70318ea1b34c91bf7303e9c8811df43d1b4746aa9adf1d503ebb0241e0fbff9be28f36dac0318825782bf05dbbcea7171eb0ca9a89be3b02666f0f3c84301" |
There was a problem hiding this comment.
[nitpick] Using hardcoded cryptographic key IDs and signatures in tests makes the code brittle and harder to maintain. Consider generating test keys dynamically or using constants defined at the top of the file to make the relationship between test data clearer.
| "sig": "8cd70318ea1b34c91bf7303e9c8811df43d1b4746aa9adf1d503ebb0241e0fbff9be28f36dac0318825782bf05dbbcea7171eb0ca9a89be3b02666f0f3c84301" | |
| "keyid": TEST_KEY_ID, | |
| "sig": TEST_SIGNATURE |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I think this is acceptable as-is.
This introduces:
This is related (but not blocked by) my patch in in-toto/in-toto-rs#111