-
Notifications
You must be signed in to change notification settings - Fork 528
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
skip tls verification if default transport is used with insecure option #1559
Conversation
339f268
to
6b9e7e9
Compare
/cc @jonjohnsonjr |
pkg/crane/options.go
Outdated
} | ||
} | ||
|
||
// Insecure is an Option that allows image references to be fetched without TLS. | ||
// This will also allow for untrusted (e.g. self-signed) certificates in cases where | ||
// the default transport is used (i.e. when WithTransport is not defined). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// the default transport is used (i.e. when WithTransport is not defined). | |
// the default transport is used (i.e. when WithTransport is not used). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggestion should be incorporated in the latest force-push. 223f982
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM overall. Do you mind adding a test to cover this? I'd probably be lazy and add a private transport
to Options
when you call Insecure
and WithTransport
, then just check that InsecureSkipVerify
is set for that.
6b9e7e9
to
223f982
Compare
Codecov Report
@@ Coverage Diff @@
## main #1559 +/- ##
==========================================
+ Coverage 72.75% 72.95% +0.20%
==========================================
Files 118 118
Lines 9236 9386 +150
==========================================
+ Hits 6720 6848 +128
- Misses 1831 1845 +14
- Partials 685 693 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@jonjohnsonjr Sure thing. I went ahead and added tests to make sure we're setting the internal boolean values (that trigger the logic we want), and then a test to make sure we're adding the transport how we expect. As you suggested, I added a private transport value to facilitate the test, and rewired the logic to set that value. Made the test much simpler. |
pkg/crane/options.go
Outdated
func WithTransport(t http.RoundTripper) Option { | ||
return func(o *Options) { | ||
o.Remote = append(o.Remote, remote.WithTransport(t)) | ||
o.transportSet = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about o.transport = t
instead of the bool, then have makeOptions
check for opt.transport == nil
rather than having both the bool and the transport?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we'd still want to track the insecure bool, but otherwise that could work and removes a bit of a redundancy. I'll put it together and send it here momentarily (along with the Boilerplate check fix).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in 5112fe3
223f982
to
5112fe3
Compare
Signed-off-by: Jose R. Gonzalez <jose@flutes.dev>
5112fe3
to
b8a70b6
Compare
Thanks! |
Fixes #1553
This PR addresses an inconsistency between the Crane library and the Crane CLI (which works correctly) with regards to interactions with image registries behind untrusted (read: self-signed) certificates.
This implementation internally tracks the Insecure/WithTransport options being set to allow
makeOptions
to decide whether or not a slightly modified default transport needs to be added to the remote options. If the right combination is not set, this does not add any default transport, relying on whatever default was to be used (same behavior as before).This PR does not change the Crane CLI because the
http.RoundTripper
that it builds out ultimately takes into account docker config values that a library caller should invoke manually. In doing so, a caller would be defining their own transport, and the logic here would not take effect. https://github.com/google/go-containerregistry/blob/main/cmd/crane/cmd/root.go#L88Signed-off-by: Jose R. Gonzalez jose@flutes.dev