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

add -allow-insecure-registry flag to permit unsecured container registries #669

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

dekkagaijin
Copy link
Member

Fixes #311

Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

lgtm! you could write a quick test by starting an insecure registry in docker, pushing an image, signing & verifying it (that's basically why we needed this in chains)

@dekkagaijin
Copy link
Member Author

dekkagaijin commented Sep 15, 2021

@priyawadhwa

$ ./cosign sign -key cosign.key localhost:443/my-image
error: signing [localhost:443/my-image]: getting remote image: Get "https://localhost:443/v2/": x509: certificate is not valid for any names, but wanted to match localhost; Get "http://localhost:443/v2/": net/http: HTTP/1.x transport connection broken: malformed HTTP response "\x15\x03\x01\x00\x02\x02"
$ ./cosign sign -allow-insecure-registry -key cosign.key localhost:443/my-image
Enter password for private key: Pushing signature to: localhost:443/my-image:sha256-69704ef328d05a9f806b6b8502915e6a0a4faa4d72018dc42343f511490daf8a.sig
$ ./cosign verify -allow-insecure-registry -key cosign.pub localhost:443/my-image

Verification for localhost:443/my-image --
The following checks were performed on each of these signatures:
  - The cosign claims were validated
  - The signatures were verified against the specified public key
  - Any certificates were verified against the Fulcio roots.

[{"critical":{"identity":{"docker-reference":"localhost:443/my-image"},"image":{"docker-manifest-digest":"sha256:69704ef328d05a9f806b6b8502915e6a0a4faa4d72018dc42343f511490daf8a"},"type":"cosign container image signature"},"optional":null}]
jsand-macbookpro3:cosign jsand$ 

ref, err := name.ParseReference(imageRef)
if err != nil {
return err
}

get, err := remote.Get(ref, remote.WithAuthFromKeychain(authn.DefaultKeychain))
get, err := remote.Get(ref, regOpts.GetRegistryClientOpts(ctx)...)
Copy link
Member

Choose a reason for hiding this comment

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

Is dropping the LHS intentional, since IIRC it's the default behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

?

The effect of the change on this line specifically should be to a) propagate ctx, b) allow for broken TLS when the flag is set

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 what confused me was that the LHS wasn't using DefaultRegistryClientOpts and the implementation of this function delegates to that which hides the fact that it is a superset. I left a comment on that below, let's kill DefaultRegistryClientOpts completely if we can.

Copy link
Member Author

Choose a reason for hiding this comment

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

roger

@cpanato cpanato added this to the v1.3.0 milestone Sep 15, 2021
}

func (co *RegistryOpts) GetRegistryClientOpts(ctx context.Context) []remote.Option {
opts := DefaultRegistryClientOpts(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether it's worth getting rid of this helper? Just inline it so that folks use the appropriate new one?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would gently nudge people in the right direction...

…istries

Signed-off-by: Jake Sanders <jsand@google.com>
@dekkagaijin dekkagaijin merged commit cd781b5 into sigstore:main Sep 16, 2021
@dekkagaijin dekkagaijin deleted the insecure branch September 16, 2021 20:43
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.

Allow specifying an insecure repo for sign/verify
4 participants