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

Jump to bottom

When a registry returns a blob or manifest, verify its contents by default #22558

Merged
merged 3 commits into from Apr 15, 2019
Merged

When a registry returns a blob or manifest, verify its contents by default #22558

merged 3 commits into from Apr 15, 2019

Conversation

smarterclayton
Copy link
Contributor

This guarantees integrity in scenarios where the registry is untrusted.

While not strictly necessary in all cases, it's better to verify rather than
allow someone to assume the server is trusted. Add a flag to Context to
allow disabling this behavior.

Tested before and after performance and didn't notice anything specific.

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 12, 2019
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 12, 2019
soltysh
soltysh approved these changes Apr 12, 2019
View reviewed changes
Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

lgtm

@dmage
Copy link
Member

dmage commented Apr 12, 2019

/retest

dmage
dmage approved these changes Apr 12, 2019
View reviewed changes
Copy link
Member

@dmage dmage left a comment

Choose a reason for hiding this comment

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

lgtm

adambkaplan
adambkaplan requested changes Apr 12, 2019
View reviewed changes
Copy link
Contributor

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

nits on godoc.

Per slack @dmage also thinks we should remove https://github.com/openshift/origin/blame/e004e6513ec755a7a106dc97516703847e470d0b/pkg/image/importer/image.go#L61-L62

pkg/image/registryclient/client.go Outdated
distribution.ManifestService
}

// Get retrieves the manifest identifievwd by the digest and guaranteezvs it matches the content it is retrieved by.
Copy link
Contributor

Choose a reason for hiding this comment

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

/s/identifievwd/identified/

/s/guaranteezvs/guarantees/

@smarterclayton
Copy link
Contributor Author 

oc image info registry.svc.ci.openshift.org/ocp/4.1-art-latest:cli
error: unable to read image registry.svc.ci.openshift.org/ocp/4.1-art-latest:cli: content integrity error: the manifest retrieved with digest sha256:65a5f1e0e9df1d29b778e325f73c7ca38d197c05455556ab39050fb9ce819ccf does not match the digest calculated from the content sha256:010af02040618c98474d73512091d673fc57ccca4eaabafdca10011726cc2677

I need to look into this

@smarterclayton
Copy link
Contributor Author

Ok, so the registry on pullthrough of a schema1 tag is continually recreating the manifest. Hit this testing against quay. That happens because we generate a unique key each time on each different registry.

@smarterclayton
Copy link
Contributor Author

I'm not sure there's any value in us not having a stable key. since schema1 signatures are going away, if we have to generate them it'd be better to do it with a hardcoded key checked into our code. it's not like we can trust the key, nor does it have any verifying power.

@smarterclayton
Copy link
Contributor Author

So we have a couple of options

  1. we can provide a consistent and static private key in the registry and in image tools for pull through (so it's locally stable when we convert)
  2. we can not verify schema1 at all (which kind of sucks)
  3. we add flags to a lot of cli tools to skip verify (which we can do)

@smarterclayton
Copy link
Contributor Author

I'm concerned that we can't strictly parse manifests from schema1 without regressing customers. So we can't verify it on import from v1, only warn, and we don't have a channel for that.

@smarterclayton
Copy link
Contributor Author

Re static key openshift/image-registry#174 is at least what it would look like. No one can use the keys we generate today and they are dead and deprecated. Anyone trusting our signature is no worse off today (since we don't publish those keys).

@smarterclayton smarterclayton mentioned this pull request Apr 12, 2019
Closed
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 13, 2019
@smarterclayton
Copy link
Contributor Author

Ok, I had a mistake in the verification process for schema1 manifests (I need to generate digest from the canonical manifest).

On top of the verification changes in the manifest, do the following:

  1. unify all the flags to image commands so that we can have a global "DisableContentVerification" flag that they all use
  2. have the image and release commands perform extra verification that the content hash matches the manifest so we can display accurate errors, and if anything goes wrong print an error
  3. fix a bug reported by sttts while I'm in there about only using the home dir to look up auth config

@smarterclayton
Copy link
Contributor Author

Removed the static private key changes, they aren't necessary

@smarterclayton
Copy link
Contributor Author

once this merges we'll need to copy the registry client changes to image-registry.

dmage
dmage reviewed Apr 15, 2019
View reviewed changes
pkg/oc/cli/admin/release/new.go
options.DryRun = o.DryRun
options.From = toImageBase
options.ConfigurationCallback = func(dgst digest.Digest, config *docker10.DockerImageConfig) error {
options.ConfigurationCallback = func(dgst, contentDigest digest.Digest, config *docker10.DockerImageConfig) error {
verifier.Verify(dgst, contentDigest)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need Verifier? Can we just return the error?

if dgst != contentDigest {
    err := fmt.Errorf("the base image failed content verification and may have been tampered with")
    if !o.SecurityOptions.SkipVerification {
        return err
    }
    fmt.Fprintf(o.ErrOut, "warning: %v\n", err) // klog?
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For many of these they can run in parallel, I'd rather use common code. I think over time I might move the logic and message into verifier but it's not clear today it cleans up the code enough. I don't want to have different checks everywhere.

We don't use klog for human facing messages in cli commands (since it prints output not geared for humans)

@adambkaplan
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, dmage, smarterclayton, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@smarterclayton
When a registry returns a blob or manifest, verify its contents by de…
c85784c 
…fault

This guarantees integrity in scenarios where the registry is untrusted. We provide
a helper that client libraries can use, and then ensure that tools properly
verify its use. Add a flag to context that allows disabling this behavior.
@smarterclayton
Unify flag behavior for image commands and support --skip-verify
6348a8b 
There are specific cases where users may need to bypass verification
of old content, possibly when dealing with broken registries. Allow
client code to bypass this logic with --skip-verification.

Also unify other flags (--registry-config, --insecure, --max-per-registry)
so they are consistent across tools.
@smarterclayton
Image commands should perform extra verification on input content
56e7736 
The underlying registry client should protect against invalid content,
but the client tools should perform an additional set of checks to
display warnings even if verification is overriden. Double check all
content accessed during release tooling and print better messages
when the image is displayed directly.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2019
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@smarterclayton smarterclayton added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2019
@smarterclayton
Copy link
Contributor Author

rebased

@openshift-merge-robot openshift-merge-robot merged commit fbfe4e9 into openshift:master Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmage dmage dmage approved these changes

@adambkaplan adambkaplan adambkaplan requested changes

@soltysh soltysh soltysh approved these changes

@mfojtik mfojtik Awaiting requested review from mfojtik

@miminar miminar Awaiting requested review from miminar

Assignees

@dmage dmage

@soltysh soltysh

@adambkaplan adambkaplan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@smarterclayton @dmage @adambkaplan @openshift-ci-robot @soltysh @openshift-merge-robot