feat: update patch manager to check signature on boot #171
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
==========================================
+ Coverage 96.27% 96.50% +0.23%
==========================================
Files 28 29 +1
Lines 3191 3402 +211
==========================================
+ Hits 3072 3283 +211
Misses 119 119
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
library/src/cache/patch_manager.rs
Outdated
| if let Some(public_key) = &self.patch_public_key { | ||
| // If we have a public key, verify that the patch has a signature | ||
| let patch_signature = patch | ||
| .signature | ||
| .clone() | ||
| .context("Patch signature is missing")?; | ||
|
|
||
| // Check that the signature is valid. | ||
| let patch_hash = signing::hash_file(&artifact_path)?; | ||
| signing::check_signature(&patch_hash, &patch_signature, public_key)?; | ||
| } else { | ||
| info!("No public key provided, skipping signature verification"); | ||
| } |
There was a problem hiding this comment.
Not sure where the hash check is.
I would expect the process to be:
- If we have a public key, check the signature on the hash matches.
- Hash the patch
- Check the hash matches.
Presumably that's already being done, just spread across other methods?
There was a problem hiding this comment.
Not sure where the hash check is.
We don't currently check the hash for equality, and I'm not sure we need to—any change to the hash would cause the signature to become invalid.
The process is:
- If we have a public key, hash the patch file
- Check that the signature matches the hash
There was a problem hiding this comment.
I think I/we need to write out exactly what we're singing in plain language. Maybe I'll add that to my in-progress blog post tomorrow.
library/src/cache/signing.rs
Outdated
| let decoded_sig = match base64::prelude::BASE64_STANDARD.decode(signature) { | ||
| Ok(sig) => sig, | ||
| Err(e) => { | ||
| bail!("Failed to decode signature: {:?}", e); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Possibly could be, let me see if there's a way to include the error message in a .context
There was a problem hiding this comment.
map_err works, just had to use anyhow's Error type, which is what is "thrown" by bail!:
let decoded_sig = base64::prelude::BASE64_STANDARD
.decode(signature)
.map_err(|e| anyhow::Error::msg(format!("Failed to decode signature: {:?}", e)))?;
Description
Update PatchManager to, if a public key is present, verify the signature of an inflated patch's hash before booting the patch.
Type of Change