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.

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

Verify manifest before accepting #12182

Merged
merged 2 commits into from
Dec 20, 2016

Conversation

miminar
Copy link

@miminar miminar commented Dec 8, 2016

Referenced blobs need to be checked for presence in a destination repository before accepting incoming manifest. Otherwise, user will be able to pull image without previous access to its blobs.

Resolves bz#1388018

Blocked on #11925

@miminar
Copy link
Author

miminar commented Dec 8, 2016

/cc @legionus, @mfojtik

if err != nil {
if err != distribution.ErrBlobUnknown {
errs = append(errs, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want to continue here otherwise you will append the error below too, right?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for the schema2 handler

@legionus
Copy link
Contributor

legionus commented Dec 8, 2016

@miminar Depends on what PR (this or #11925) would be merged first we get a conflict. In the #11925 I have changed your Registry: moved manifest schema operations to new files commit.

@miminar
Copy link
Author

miminar commented Dec 8, 2016

Depends on what PR (this or #11925) would be merged first we get a conflict. In the #11925 I have changed your Registry: moved manifest schema operations to new files commit.

@legionus Let's go with yours first.

@miminar miminar changed the title Verify manifest before accepting [DO NOT MERGE] Verify manifest before accepting Dec 8, 2016
@miminar miminar force-pushed the first-blobs-then-manifest branch from 70e9f00 to cbdcf19 Compare December 8, 2016 13:03
@miminar
Copy link
Author

miminar commented Dec 8, 2016

the yum flake beast went rampage

@miminar miminar changed the title [DO NOT MERGE] Verify manifest before accepting Verify manifest before accepting Dec 19, 2016
@miminar
Copy link
Author

miminar commented Dec 19, 2016

Yum flake #8571. re-[test]

Michal Minář added 2 commits December 19, 2016 22:16
Mostly copied from docker/distribution repo.

Signed-off-by: Michal Minář <[email protected]>
@miminar miminar force-pushed the first-blobs-then-manifest branch from cbdcf19 to 57f9cf6 Compare December 19, 2016 21:16
@miminar
Copy link
Author

miminar commented Dec 19, 2016

Updated 1st commit to match the one in #11925.

@soltysh
Copy link
Contributor

soltysh commented Dec 19, 2016

LGTM, tagging for merge after green tests.

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 57f9cf6

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12510/) (Base Commit: 07050d7)

@miminar
Copy link
Author

miminar commented Dec 20, 2016

@mfojtik, @pweil-, @soltysh someone please merge.

@soltysh
Copy link
Contributor

soltysh commented Dec 20, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 57f9cf6

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 20, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12510/) (Base Commit: 3c68360) (Image: devenv-rhel7_5573)

@openshift-bot openshift-bot merged commit 594df42 into openshift:master Dec 20, 2016
@miminar miminar deleted the first-blobs-then-manifest branch March 22, 2017 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants