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 image signature #13585

Merged
merged 9 commits into from
May 5, 2017
Merged

Conversation

mfojtik
Copy link
Contributor

@mfojtik mfojtik commented Mar 30, 2017

Fresh version of: #12616 based on @mtrmac branch (rebased and polished to complain with the origin commit format).

This version is based on the containers/image and not just bundling the opengpg support for origin.
Trello: https://trello.com/c/BIJBu7qi/788-5-images-fill-in-the-metadata-signature-fields-when-a-signature-is-valid

@mfojtik
Copy link
Contributor Author

mfojtik commented Mar 30, 2017

[test]

@mtrmac I think the db3a491 commit is controversial and I don't think it will pass our validation. Can you explain why we need that there? (it does not exists in the upstream yet?). In that case we might need to add that as a "pick" or "carry" patch.

@mfojtik
Copy link
Contributor Author

mfojtik commented Mar 30, 2017

@smarterclayton @miminar can you guys review this pls?

@mtrmac
Copy link
Contributor

mtrmac commented Mar 30, 2017

@mtrmac I think the db3a491 commit is controversial and I don't think it will pass our validation

Which one is that? github can’t find it.

FWIW some points to consider WRT the c-i-verify-image-signature branch:

  • It would be nice to get rid of 5d42163 (“Eliminate dependency on containers/image/docker … to minimize version skew (on strslice, tlsconfig)”, by using a compatible version of both. I didn’t look into what that would take.

  • ed7bc58 (“Use signature.PolicyContext for image verification … instead of manual usage of SignatureMechanism.”) may not be what you want, notably it causes VerifyImageSignatureOptions.PublicKey* in this PR to be accepted and ignored, and instead the system-wide policy.json is used.

    What to do really depends on what kind of CLI you want.

    A plausible alternative would be to replace the signature.DefaultPolicy(nil) with manually constructing a policy with

     pr, err := signature.NewPRSignedByKeyPath(signature.SBKeyTypeGPGKeys, options.PublicKeyFilename, signature.NewPRMMatchRepoDigestOrExact())
     if err != nil {…}
     policy := signature.Policy{Default:[]signature.PolicyRequirement{pr}}

    or so.

@mtrmac
Copy link
Contributor

mtrmac commented Mar 30, 2017

@mtrmac I think the db3a491 commit is controversial and I don't think it will pass our validation

Which one is that? github can’t find it.

Well, now I see it: “Manually add vendor/github.com/opencontainers/image-spec/specs-go/v1/ : I have no idea how to make Godeps copy this in it seems.”

That commit is not “controversial”, it’s flat out wrong; I just couldn’t find a working solution quickly when preparing the prototype. Hopefully someone familiar with the OpenShift use of Godeps can find it trivial to do the right thing; if not, I’d have to do some digging.

@mfojtik
Copy link
Contributor Author

mfojtik commented Mar 30, 2017

@mtrmac i made it so our commit checker won't scream, but I want to ask why you need to manually add those

@mtrmac
Copy link
Contributor

mtrmac commented Mar 30, 2017

It’s not that I want to add them manually. github.com/opencontainers/image-spec/specs-go/v1 is a package required by some subpackages of containers/image, so it is needed and it should be somehow pulled in by Godeps. It’s just that I couldn’t, in the limited time I spent on this, persuade Godeps to actually do so.

This definitely should be done by using Godeps correctly instead of this manual patch.

@mfojtik
Copy link
Contributor Author

mfojtik commented Mar 31, 2017

@mtrmac i think my question was more like if the github.com/opencontainers/image-spec/specs-go/v1 mean we need newer version of this image-spec.

@mfojtik mfojtik added this to the 1.6.0 milestone Apr 3, 2017
@mfojtik
Copy link
Contributor Author

mfojtik commented Apr 3, 2017

@mtrmac I think I realized why this will not work:

$ go get -u github.com/mtrmac/gpgme
# github.com/mtrmac/gpgme
../../mtrmac/gpgme/data.go:4:20: fatal error: gpgme.h: No such file or directory
 // #include <gpgme.h>

This is what basically godep restore calls (go get -u ...). I know that we have build tag that prevents origin from building with gpgme support, but since we have to go get it during the godep restoration process, it will always fail if we don't install gpgme-devel and libassuan-devel.

@smarterclayton would adding two extra packages to the build process acceptable? Note that those are disabled by a build tag that force it to build with Go openpgp instead... But we need to get them because godep sux :-(

@mtrmac
Copy link
Contributor

mtrmac commented Apr 3, 2017

@mtrmac i think my question was more like if the github.com/opencontainers/image-spec/specs-go/v1 mean we need newer version of this image-spec.

N/A? OpenShift does not currently, AFAICS, depend on the above-quoted package at all. This PR is adding the dependency; so I don’t know what newer/older is referring to.

@mtrmac
Copy link
Contributor

mtrmac commented Apr 3, 2017

@mtrmac I think I realized why this will not work:

$ go get -u github.com/mtrmac/gpgme
# github.com/mtrmac/gpgme
../../mtrmac/gpgme/data.go:4:20: fatal error: gpgme.h: No such file or directory
 // #include <gpgme.h>

This is what basically godep restore calls (go get -u ...).

That does not quite explain why I wasn’t able to use godep for that, because I do have all the necessary headers to use gpgme and the above go get does not fail for me. But you are quite right that it may be very relevant for the OpenShift build process.

@mfojtik
Copy link
Contributor Author

mfojtik commented Apr 4, 2017

(btw. I am waiting here for the 1.6 rebase to land and then do the godep restore one more time...)

@mfojtik
Copy link
Contributor Author

mfojtik commented Apr 4, 2017

@mtrmac

N/A? OpenShift does not currently, AFAICS, depend on the above-quoted package at all. This PR is adding the dependency; so I don’t know what newer/older is referring to.

If it is adding new dependency, why you then added the v1/ folder in extra commmit instead of just adding it together with the new package? It is because the package version you copied to godeps does not include the v1/ folder? If yes, what I'm asking is to give me the version that does include it, or an explanation why you added it as a separate commit :-)

I figured it out, it can be squashed to the initial addition as the revision you used in Godeps.json includes v1/ folder.

That does not quite explain why I wasn’t able to use godep for that, because I do have all the necessary headers to use gpgme and the above go get does not fail for me. But you are quite right that it may be very relevant for the OpenShift build process.

You do, but if our Jenkins or any Origin developer will do godep restore after this merge, they don't necessary have those headers/libs installed. What I am saying is the Origin does not require those headers/libs (because we use build tag to opt-them-out), but godeps does not respect the build tags at all and will fail without having them (because it will try to build the containers/image)...
I don't think it is a show-stopper here, just annoying for other maintainers and reason why I hate to
use build tags.

At this point, I will just wait for the rebase to land (maybe I will start rebasing this on top of kubernetes 1.6 rebase when it gets stable) and we target to merge this for origin 3.6 (after 1.6 rebase lands, so we don't mess the guys doing the rebase...) Meanwhile, are there any review comments on the current PR/approach that should be addressed (besides godeps), or is this good to go after we are unblocked on the rebase?

@smarterclayton @soltysh @miminar PTAL as well, this is required from PM in 3.6

hack/common.sh Outdated
@@ -187,6 +187,8 @@ os::build::internal::build_binaries() {
version_ldflags=$(os::build::ldflags)

# Use eval to preserve embedded quoted strings.
Copy link

Choose a reason for hiding this comment

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

The eval comment should be followed by the eval statement.

// verifySignature verifies the image signature and returns the identity when the signature
// is valid.
func (o *VerifyImageSignatureOptions) verifySignature(pc *signature.PolicyContext, img *imageapi.Image, sigBlob []byte) (string, error) {
// Pretend that this is the only signature of img, and see what the policy says.
Copy link

Choose a reason for hiding this comment

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

I thought, this doesn't take into account any policy file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mtrmac ^ i thought we are not going to require policy.json here at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nvmd. this is referring to PolicyContext which we create in caller of this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

i thought we are not going to require policy.json here at all.

This is up to you.

The code as is right now includes ed7bc58 , which uses policy.json.

#13585 (comment) instead proposes another alternative, asking the user to provide a keyring and using only that one.

(The original PR was using the user’s default keyring by default; that’s plausible but not obviously the right thing to do: Habitual GPG users tend to have keyserver-options auto-key-retrieve in ~/.gnupg/gpg.conf, i.e. the default keyring contains various keys with varying levels of trust for email and such, which tends to have very little intersection with trust for running containers. The current containers/image API does not make it trivial to use the signature verification code with “all of ~/.gnupg” as the set of trusted keys, hence the two proposed alternatives above.)

return "", errors.New("internal error: signature rejected but no error set")
}
if err != nil {
return "", fmt.Errorf("signsture rejected: %v", err)
Copy link

Choose a reason for hiding this comment

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

typo

return "", fmt.Errorf("signsture rejected: %v", err)
}

// Because s.Content was the only signature used above, we now know that s.Content is acceptable, so untrustedInfo is good enough.
Copy link

Choose a reason for hiding this comment

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

This is outdated and doesn't make sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT this comment still applies: It was trying to say that we are calling an UntrustedSignatureInformation function but presenting the key ID as a trusted value, and the comment justifies why this is a safe thing to do. (Feel free to leave it out if you think this does not need pointing out.)

// fakeDockerReference is containers/image/docker.Reference, except that only provides identity information.
type fakeDockerReference struct{ ref reference.Named }

// parseReference converts a string, which should not start with the ImageTransport.Name prefix, into an Docker ImageReference.
Copy link

Choose a reason for hiding this comment

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

parseDockerReference

@@ -115,9 +112,6 @@ func (s imageStrategy) PrepareForUpdate(ctx kapi.Context, obj, old runtime.Objec
if err = api.ImageWithMetadata(newImage); err != nil {
utilruntime.HandleError(fmt.Errorf("Unable to update image metadata for %q: %v", newImage.Name, err))
}

// clear signature fields that will be later set by server once it's able to parse the content
s.clearSignatureDetails(newImage)
Copy link

Choose a reason for hiding this comment

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

This method doesn't seem to be used anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nuked.

func NewCmdVerifyImageSignature(name, fullName string, f *clientcmd.Factory, out, errOut io.Writer) *cobra.Command {
opts := &VerifyImageSignatureOptions{ErrOut: errOut, Out: out}
cmd := &cobra.Command{
Use: fmt.Sprintf("%s IMAGE [--confirm]", name),
Copy link

Choose a reason for hiding this comment

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

This should mention --expected-identity=… since it's mandatory argument.

I'd prefer just %s IMAGE EXPECTED_IDENTITY [--confirm], but I'm repeating myself.

By default, this command will not record a signature condition back to the Image object but only
print the verification status to the console.

To record a new condition, you have to pass the "--confirm" flag.
Copy link

Choose a reason for hiding this comment

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

The help should mention that verifying single signature using different --public-key will clear verification status from all the other image signatures not signed by this key.

And the same for expected-identity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Dump question, but can I verify just single image with this key? Is it possible to specify multiple public keys when validating multiple images?

Copy link
Member

Choose a reason for hiding this comment

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

I read further, when only single image is verified at the time, how can I clear status from other images. Seems odd reading this.

Copy link

Choose a reason for hiding this comment

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

Dump question, but can I verify just single image with this key?

The command works on single image. The problem is that the command can verify at most one signature at the time. The image can have multiple signatures though.

Is it possible to specify multiple public keys when validating multiple images?

Currently not but may be nice. I wouldn't make it a requirement for this PR though.

how can I clear status from other images

I think a script or oc observe are your best options.

Seems odd reading this.

Agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to specify multiple public keys when validating multiple images?

That depends on what the verification mechanism will end up being (see #13585 (comment) ), but so far all of the alternatives (policy.json, manually-created single-rule policy, manual SignatureMechanism invocation) accept a GPG keyring, which can contain several public keys; any of the keys is equally acceptable. (When that keyring is specified for a command, all images verified using that command would use the same keyring and the same complete set of public keys; it is not a 1-1 image-key mapping.)

@mfojtik
Copy link
Contributor Author

mfojtik commented Apr 5, 2017

[test]

interesting... it seems like just flakes.

@mfojtik
Copy link
Contributor Author

mfojtik commented Apr 6, 2017

[test]

@mfojtik
Copy link
Contributor Author

mfojtik commented Apr 6, 2017

[test]

func NewCmdVerifyImageSignature(name, fullName string, f *clientcmd.Factory, out, errOut io.Writer) *cobra.Command {
opts := &VerifyImageSignatureOptions{ErrOut: errOut, Out: out}
cmd := &cobra.Command{
Use: fmt.Sprintf("%s IMAGE EXPECTED_IDENTITY [--confirm]", name),
Copy link

Choose a reason for hiding this comment

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

The EXPECTED_IDENTITY suggests that the identity is a positional argument (something passed without leading --option on a fixed position). I like that, but the code below still defines --expected-identity and expects len(args) == 1 which is wrong. To keep the help message as is, the --expected-identity needs to go away and the identity needs to be fetched from args[1].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will "%s IMAGE --expected-identity=EXPECTED_IDENTITY [--confirm]" do?

Copy link

Choose a reason for hiding this comment

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

@mfojtik sure

Copy link

Choose a reason for hiding this comment

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

Why do you hate positional args? 😄 I don't get it. 😕

Copy link
Contributor Author

@mfojtik mfojtik Apr 10, 2017

Choose a reason for hiding this comment

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

@miminar if we ever want to add additional args with expected something, then we will have to keep adding to positional args which is ugly ;-) I want admins to know what they are doing.

Copy link

@miminar miminar Apr 10, 2017

Choose a reason for hiding this comment

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

I just find --expected-identity= too long to be given every time. This limits the usage of the tool really just to scripts. But it's just a subjective observation. I don't push you to any further changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it makes it long, but on the other hand it makes it really explicit on what are you expecting to be in the signature and it also helps understand what that flag is for. Otherwise it might look really strange to provide the name of the image (sha256:xxxxxxx) and then, again a name of the image...

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.

I left you some comments, the only part I didn't inspect carefully is the code verification. I guess there are more knowledgeable people to verify it.

"Rev": "c07f8fdceeda1517556602778a61ba94894e7c02"
},
{
"ImportPath": "github.com/containers/storage/pkg/homedir",
Copy link
Member

Choose a reason for hiding this comment

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

Can we get this part a separate commit, for clear history?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for hating me :-) sure it can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

Choose a reason for hiding this comment

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

It's not about hating you, I ❤️ you man! It's about having clear history when and what was added 😉

@@ -1964,6 +1964,10 @@
"Rev": "5cbbc6bafb45bd7ef10486b673deb3b81bb3b787"
},
{
"ImportPath": "github.com/mtrmac/gpgme",
Copy link
Member

Choose a reason for hiding this comment

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

If we are planning on using fork, create one under openshift org.

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 still not addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not an OpenShift-specific fork; containers/image and projectatomic/skopeo use the same one. See proglottis/gpgme#6 for the full history behind creating it. Hopefully it will eventually go away.

If OpenShift wants to take over that fork and commit to maintaining it, sure, why not. I don’t see how OpenShift benefits (especially because OpenShift opts out of actually using anything in this package, using the containers_image_openpgp build tag), but I’d be only too happy to hand over this responsibility :)

Copy link
Member

Choose a reason for hiding this comment

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

OK, I wasn't aware of the full story behind it.


var (
verifyImageSignatureLongDesc = templates.LongDesc(`
Verifies the imported image signature using the local public key.
Copy link
Member

Choose a reason for hiding this comment

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

Imported where? Is this only against images from the internal registry, if so be explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clarified.

By default, this command will not record a signature condition back to the Image object but only
print the verification status to the console.

To record a new condition, you have to pass the "--confirm" flag.
Copy link
Member

Choose a reason for hiding this comment

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

Dump question, but can I verify just single image with this key? Is it possible to specify multiple public keys when validating multiple images?

By default, this command will not record a signature condition back to the Image object but only
print the verification status to the console.

To record a new condition, you have to pass the "--confirm" flag.
Copy link
Member

Choose a reason for hiding this comment

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

I read further, when only single image is verified at the time, how can I clear status from other images. Seems odd reading this.

},
}

cmd.Flags().BoolVar(&opts.Confirm, "confirm", opts.Confirm, "If true, the result of the verification will be recorded to an image object.")
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest using record here, rather than confirm. The former suggests that something has to be recorded from the operation, see --record in oc apply. Whereas confirm, suggests confirmation, that's not what happens here, you're not doing any dry run which needs to be then confirmed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soltysh --record records the command into an annotation. it will be confusing if it has different behavior for verification?

Copy link
Member

Choose a reason for hiding this comment

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

For me --confirm holds this type of confusion right now. Whereas --record seems more reasonable. @openshift/cli-review wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--record=false: Record current kubectl command in the resource annotation.

This is clearely not this case, since --record will do more than recording an annotation. We also have --confirm for pruning and importing image stream, so I will want to keep it --confirm just for the sake of consistency.... you spend too much time in kubectl world @soltysh :P

Copy link
Member

Choose a reason for hiding this comment

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

For me --confirm + verify does not sound good enough. --confirm for pruning is because the initial operation is dry-run, and you need to explicitly confirm it. Not sure how this applies to signature verification. I'm not saying --record is the best option, I'm just arguing against using --confirm. Maybe something else should be used here, that's why I asked @openshift/cli-review, we developers are soooo bad at naming things 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

"record", "log", "post", "confirm"... No perfect option, IMO.

As a user I assume this is the default. Why do we need this option? I would rather add a "do not record status" flag later based on user feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

One reason to include this option is if the user does not have privilege to write the verification status. I'm okay with not writing the status as long as we report that to the user.

Another use case is if someone has an incorrect public key. They would be annotating incorrect status.

Copy link
Contributor

Choose a reason for hiding this comment

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

Record is very specific to apply today.

Confirm is when we ask the user a question. Force is when we override a safety default. We're not doing either here. --save is probably the closest to what you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with --save. Thinking through this, there are probably plenty of use cases to justify a --save=false default. Ignore my noise above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm taking --save as our best option. thanks!

LastProbeTime: now,
LastTransitionTime: now,
Reason: "verified manually",
Message: fmt.Sprintf("verified by user %s", o.CurrentUser),
Copy link
Member

Choose a reason for hiding this comment

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

verified by %q, would give use nice verified by 'userA'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also records quotes into API object ;-) i don't mind honestly, but I found no quotes nicer as they don't have to be escaped.

}

// fakeDockerReference is containers/image/docker.Reference, except that only provides identity information.
type fakeDockerReference struct{ ref reference.Named }
Copy link
Member

Choose a reason for hiding this comment

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

fake suggests it's used for testing, pick a different name, eg. identityOnlyDockerReference, dummyDockerReference. I think, I'm leaning towards the former.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you must be looking at outdating commit, this was removed already.

formatString(out, "\tType", s.Type)
if s.IssuedBy == nil {
// FIXME: Make this constant
formatString(out, "\tStatus", "Unverified")
Copy link
Member

Choose a reason for hiding this comment

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

I think <none would make it more explicit and stand out more.

Copy link
Contributor Author

@mfojtik mfojtik Apr 10, 2017

Choose a reason for hiding this comment

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

No, it should be either "verified" or "unverified". Any image that was not verified manually is "unverified"... "<none>" might be misleading.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like using the negation - generally we try to have status constants that are significantly different from each other. The constants are what are important - why not simply output the conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarterclayton I thought we want to have clear message about "verified vs. unverified" image. Returning only conditions gives a person who is familiar with them clue about the verification status but I don't think it will make it obvious.

`)
)

type VerifyImageSignatureOptions struct {
Copy link
Member

Choose a reason for hiding this comment

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

Tests are needed, preferably unit and cmd, but at least one is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will have to be a follow up and we have to rely on QE manual testing of this for now.
having a test for this will require infra changes (need a gpg key, skopeo installed, copying signed image, genering public key, etc..)

Copy link
Member

Choose a reason for hiding this comment

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

Sigh.... I'm ok with a followup (at least partially) + QE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue created...

@mfojtik mfojtik force-pushed the verify-signature branch 2 times, most recently from 2967181 to 479caf0 Compare April 10, 2017 11:37
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.

One godep I'd suggest to turn into a openshift fork + the flag naming is my only objections. All the rest lgtm.

@@ -1964,6 +1964,10 @@
"Rev": "5cbbc6bafb45bd7ef10486b673deb3b81bb3b787"
},
{
"ImportPath": "github.com/mtrmac/gpgme",
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 still not addressed.

cmd.Flags().BoolVar(&opts.Confirm, "confirm", opts.Confirm, "If true, the result of the verification will be recorded to an image object.")
cmd.Flags().BoolVar(&opts.Remove, "remove", opts.Remove, "If set, all signature verifications will be removed from the given image.")
cmd.Flags().StringVar(&opts.PublicKeyFilename, "public-key", opts.PublicKeyFilename, "A path to a public GPG key to be used for verification.")
cmd.Flags().StringVar(&opts.ExpectedIdentity, "expected-identity", opts.ExpectedIdentity, "An expected image docker reference to verify.")
Copy link
Contributor

Choose a reason for hiding this comment

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

"--expected-identity" isn't clear enough to me.

"--image-name", "--image-reference" or just "--name" works better IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

--expect-name would be correct, if the thing being compared is the "name" of the image.

Copy link
Contributor

Choose a reason for hiding this comment

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

From the perspective of the end user, they know project/imagestream==name. I believe that's the value we expect here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aweiteka @mtrmac thought this should match with the skopeo terminology where this thing is called "image identity". Also this does not match to "image name" (as openshift image name is in sha256:foo format), but it matches to the docker pull spec (x.x.x.x:5000/foo/bar@...).

Copy link
Contributor

Choose a reason for hiding this comment

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

As for the format, of the value, this is ordinarily a hostname/ns…/repo:tag, but it also may be a hostname/ns…/repo@algo:digest.

For internally created images the value would ordinarily semantically match project/imagestream, ± details of the syntax.

For externally imported images, the expected identity and the imagestream name would, in general, not match; the expected identity of a RHEL image imported into the imagestream, when verifying the Red Hat signature, is registry.access.redhat.com/…/rhel:$something, not mycluster.example.com/myproject/baseimage:$somethingelse.

hack/common.sh Outdated
@@ -234,7 +236,7 @@ os::build::internal::build_binaries() {
if [[ ${#nonstatics[@]} -gt 0 ]]; then
GOOS=${platform%/*} GOARCH=${platform##*/} go install \
-pkgdir "${OS_OUTPUT_PKGDIR}/${platform}" \
-tags "${OS_GOFLAGS_TAGS-} ${!platform_gotags_envvar:-}" \
-tags "${common_gotags} ${OS_GOFLAGS_TAGS-} ${!platform_gotags_envvar:-}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

The code at this level shouldn't have to know about specific tags. You need to set this in build-cross / build-go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@mfojtik mfojtik force-pushed the verify-signature branch 2 times, most recently from ace87fc to f64d637 Compare May 2, 2017 12:15
@mfojtik
Copy link
Contributor Author

mfojtik commented May 2, 2017

@mtrmac @miminar ok, I wrote a simple helper that will retrieve the manifest from the registry. It will use the bearer token/username from the user running the command (which makes using --as=system:admin impossible, but that is a good thing because you don't want to hide the identity of the user that is verifying the image anyway...).

The latest version is also not using the DefaultPolicy(nil), so I believe it should really do some level of verification of the image identity/signature...

I really hesistate to make this command more complex by using containers/image docker transport/etc. For the current scope of this command, this is only intended for local (managed) images only (iow. only images managed by openshift registry)...


// GetImageManifestFromRegistry retrieves the image manifest from the registry using the basic
// authentication using the image ID.
func GetImageManifestFromRegistry(registry *url.URL, repositoryName, imageID, username, password string) ([]byte, error) {
Copy link

Choose a reason for hiding this comment

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

Can you name it like GetImageManifestByIDFromRegistry() since it doesn't support tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@mfojtik mfojtik force-pushed the verify-signature branch 2 times, most recently from 285f8b8 to f247cc0 Compare May 2, 2017 14:24
@mfojtik
Copy link
Contributor Author

mfojtik commented May 2, 2017

[test]

@mfojtik
Copy link
Contributor Author

mfojtik commented May 3, 2017

as a follow ups:

  • make an extended test using the origin image + gpg + skopeo to actually push signed image into registry and use this command to verify it
  • human readable GPG identity instead of GPG key ID
  • fix error messages in containers/image (when GPG keyring is not found for example or GNUPGHOME is not set)
  • add support for external registries (so I can verify image imported from an non-openshift registry - trelloed)
  • remove imageapi.SignatureForImage crazyness from validation (or at least make it useful and contain the actual identity of the image)

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 8d19047

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1110/) (Base Commit: 13b90e2)

@mfojtik
Copy link
Contributor Author

mfojtik commented May 3, 2017

lgtmed by @miminar per IRC

[merge]

@mfojtik
Copy link
Contributor Author

mfojtik commented May 4, 2017

flake: #13942

[merge]

@mfojtik
Copy link
Contributor Author

mfojtik commented May 4, 2017

[merge] flake: #14044

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 8d19047

@openshift-bot
Copy link
Contributor

openshift-bot commented May 5, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/561/) (Base Commit: 8508081) (Image: devenv-rhel7_6212)

@openshift-bot openshift-bot merged commit e7e8ae1 into openshift:master May 5, 2017
@miminar
Copy link

miminar commented May 9, 2017

Congrats

@mfojtik
Copy link
Contributor Author

mfojtik commented May 9, 2017

@aweiteka FYI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants