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

Make --insecure-registry work with TLS registries whose certs we can't verify. #398

Merged
merged 2 commits into from
Jul 28, 2021

Conversation

evankanderson
Copy link
Contributor

Fixes #396

Note that this is technically breaking if people are using --insecure-registry and replacing the RoundTripper with something that's not an http.Transport. I decided to blow up loudly, rather than let it pass, but I could be convinced to go the other way.

Verified on harbor 2.2.3 with a self-signed cert with a SAN of 192.168.10.12.

@google-cla google-cla bot added the cla: yes label Jul 26, 2021
@evankanderson evankanderson force-pushed the in-secure branch 9 times, most recently from cf4ed82 to 4d61bbc Compare July 27, 2021 00:06
@@ -105,6 +107,17 @@ func WithTagOnly(tagOnly bool) Option {
func Insecure(b bool) Option {
return func(i *defaultOpener) error {
i.insecure = b
t, ok := i.t.(*http.Transport)
if !ok {
return fmt.Errorf("unable to configure insecure roundtripper (not HTTP)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it makes sense to error here.

If we can't override the TLSClientConfig, then in the case where we needed to, it will fail anyway, and in the case where we didn't need to, it fails even when it shouldn't.

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've switched this -- do you think I should log anything here for the debugging case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Logging sounds reasonable, but also the only time anyone is going to hit this is if they are using ko as a library and supplying their own transport, so it might not really matter.

@codecov-commenter
Copy link

Codecov Report

Merging #398 (1ea8fb9) into main (c014ec1) will decrease coverage by 0.81%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #398      +/-   ##
==========================================
- Coverage   44.15%   43.33%   -0.82%     
==========================================
  Files          34       34              
  Lines        1701     1740      +39     
==========================================
+ Hits          751      754       +3     
- Misses        817      847      +30     
- Partials      133      139       +6     
Impacted Files Coverage Δ
pkg/publish/options.go 22.50% <0.00%> (-5.63%) ⬇️
pkg/build/gobuild.go 55.90% <0.00%> (-3.29%) ⬇️
pkg/publish/daemon.go 44.77% <0.00%> (-0.68%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c014ec1...1ea8fb9. Read the comment docs.

@jonjohnsonjr jonjohnsonjr merged commit f04730d into ko-build:main Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't figure out flags for insecure registry
3 participants