Skip to content
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.

Sign up for GitHub

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

Jump to bottom

v2: add content digest module and verify blob integrity on download #105

Merged
merged 4 commits into from
Apr 29, 2019
Merged

v2: add content digest module and verify blob integrity on download #105

merged 4 commits into from
Apr 29, 2019

Conversation

steveej
Copy link
Contributor

@steveej steveej commented Apr 24, 2019 •
edited
Loading

Motivation for this is to detect if a registry sends a blob which doesn't match its layer digest.

TODO

  • Unit tests for digest
  • Mock tests for get_blobs?

Depends on #104.

lucab
lucab reviewed Apr 25, 2019
View reviewed changes
src/v2/blobs.rs Outdated Show resolved Hide resolved
src/v2/blobs.rs Outdated Show resolved Hide resolved
@steveej
Copy link
Contributor Author

steveej commented Apr 25, 2019

@lucab All your comments are addressed, PTAL. Also, do you think mock tests would make sense for get_blob? We only have them for has_blob at the moment.

@steveej steveej requested a review from lucab April 25, 2019 16:08
@steveej steveej force-pushed the pr/verify-layer-digests branch 2 times, most recently from a6cfb77 to e33db41 Compare April 25, 2019 16:36
@lucab
Copy link
Member

lucab commented Apr 25, 2019

@steveej I'm fine with either a mock-test or making sure travis runs a get_blob+verify against a live registry (I didn't check, I hope it does already). If you care about ensuring we catch the negative case (received blob with non-matching digest), then I think a mock-test is the only way.

Except for tests, are there any other WIP parts in here?

@steveej
Copy link
Contributor Author

steveej commented Apr 25, 2019

If you care about ensuring we catch the negative case (received blob with non-matching digest), then I think a mock-test is the only way.

I'll give mocking this a try, shouldn't be too complex.

Except for tests, are there any other WIP parts in here?

Except for the test and forming a proper git history I consider it complete.

@steveej
Copy link
Contributor Author

steveej commented Apr 25, 2019

@lucab I added the most tests for get_blob. I consider this ready for final review. That said, I'm open to discussing the stronger typing around the digest again.

lucab
lucab reviewed Apr 26, 2019
View reviewed changes
src/v2/blobs.rs Outdated Show resolved Hide resolved
src/v2/blobs.rs Outdated Show resolved Hide resolved
src/v2/content_digest.rs Outdated

/// DigestAlgorithm declares the supported algorithms
#[derive(Display, Clone, Debug, PartialEq, EnumString)]
pub enum DigestAlgorithm {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not exposed in the library API, I think you can remove the pub (or scope it down to pub(crate)).

src/v2/content_digest.rs Outdated Show resolved Hide resolved
src/v2/content_digest.rs Outdated Show resolved Hide resolved
src/v2/content_digest.rs Show resolved Hide resolved
@steveej steveej changed the title [WIP] v2/blobs: verify blob integrity on download v2: add content digest module and verify blob integrity on download Apr 26, 2019
@steveej
v2: add content_digest module
815e7e2 
This module implements content digest verification and will be used by
the blob and manifest modules.
@steveej
{tests/mock,srv/v2}/blob: verify blob consistency
a91af0b 
Use the content_digest module for consistency checks when downloading
blobs.
Also verify the format of the given digest.
@steveej steveej requested a review from lucab April 26, 2019 11:17
@steveej steveej mentioned this pull request Apr 26, 2019
Open
2 tasks
@lucab
Copy link
Member

lucab commented Apr 29, 2019

#104 landed in the meanwhile.

@steveej
Copy link
Contributor Author

steveej commented Apr 29, 2019 •
edited
Loading

#104 landed in the meanwhile.

I'm pretty sure the commit was already in this history, but I rebased again and now the merge commit is in here too.

I can see a lot of conversations which aren't marked as resolved. I think they're all settled though, please take a look if I missed anything.

lucab
lucab approved these changes Apr 29, 2019
View reviewed changes
Copy link
Member

@lucab lucab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lucab lucab merged commit eb6349f into camallo:master Apr 29, 2019
@steveej steveej deleted the pr/verify-layer-digests branch April 29, 2019 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucab lucab lucab approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@steveej @lucab