Fetch targets by HASH.FILENAME.EXT #233
Conversation
|
Rebase to make review easier? |
|
Rebased! Thanks for merging the other CLs. |
tests/simple_example.rs
Outdated
| // FIXME(232): This is a helper function that, if `consistent_snapshot` is true, it prefixes target | ||
| // paths with their file hash. This is unfortunately necessary because we don't have a high level | ||
| // interface for creating TUF metadata. | ||
| async fn store_target( | ||
| remote: &EphemeralRepository<Json>, | ||
| target_file: &[u8], | ||
| target_path: &TargetPath, | ||
| consistent_snapshot: bool, | ||
| ) -> Result<()> { | ||
| if consistent_snapshot { | ||
| let target_path = hash_prefix_target_path(target_file, target_path)?; | ||
| remote.store_target(target_file, &target_path).await | ||
| } else { | ||
| remote.store_target(target_file, target_path).await | ||
| } | ||
| } | ||
|
|
||
| fn hash_prefix_target_path(target_file: &[u8], target: &TargetPath) -> Result<TargetPath> { | ||
| let target_description = TargetDescription::from_reader(target_file, &[HashAlgorithm::Sha256])?; | ||
| let (_, value) = crypto::hash_preference(target_description.hashes())?; | ||
|
|
||
| let mut components = target.components(); | ||
| let file_name = match components.pop() { | ||
| Some(file_name) => file_name, | ||
| None => { | ||
| return Err(Error::IllegalArgument("Path cannot be empty".into())); | ||
| } | ||
| }; | ||
| components.push(format!("{}.{}", value, file_name)); | ||
|
|
||
| TargetPath::new(components.join("/")) | ||
| } |
There was a problem hiding this comment.
Rather than do all this, could you just hardcode the path, since it's only used once? That'd probably make the test easier to understand, too.
There was a problem hiding this comment.
I cleaned this up, like it better?
src/client.rs
Outdated
| }; | ||
| components.push(format!("{}.{}", value, file_name)); | ||
|
|
||
| let target = TargetPath::new(components.join("/"))?; |
There was a problem hiding this comment.
It doesn't really feel like this is the right place to be mangling the TargetPath. It seems like this translation is supposed to happen when you translate a VirtualTargetPath into a TargetPath? Does that not work for some reason?
There was a problem hiding this comment.
Note, I factored this out a wee bit to make it a little more readable. Does it read better?
Regarding VirtualTargetPath, this is an area where rust-tuf is being a little inconsistent. The metadata is constructed with VirtualTargetPath, but Repository uses TargetPath. I actually think this is the wrong way to go about, the notion of mapping a target path to a real path should really only be done in FileSystemRepository, but clients really should just be using / separators everywhere (see #236 and theupdateframework/specification#63). I'd like to clean this up if / when we implement #236.
In section 5.5.2 of the [TUF Spec], it states that if consistent snapshots is enabled, targets are downloaded by fetching HASH.FILENAME.EXT. Closes theupdateframework#225 [TUF Spec]: https://github.com/theupdateframework/specification/blob/master/tuf-spec.md#5-detailed-workflows
|
I took a review pass in ComputerDruid's absence; LGTM. |
In section 5.5.2 of the TUF Spec, it states that if consistent snapshots is enabled, targets are downloaded by fetching HASH.FILENAME.EXT.
Note that this is layered on top of #231, so I recommend reviewing that first, then reviewing the second commit.
Closes #225