skip tls verification if default transport is used with insecure option by komish · Pull Request #1559 · google/go-containerregistry

komish · 2023-02-07T16:32:29Z

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#L88

Signed-off-by: Jose R. Gonzalez jose@flutes.dev

komish · 2023-02-07T16:38:14Z

/cc @jonjohnsonjr

jonjohnsonjr · 2023-02-07T18:40:52Z

pkg/crane/options.go


 // 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).


Suggested change

// the default transport is used (i.e. when WithTransport is not defined).

// the default transport is used (i.e. when WithTransport is not used).

This suggestion should be incorporated in the latest force-push. 223f982

jonjohnsonjr

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.

codecov-commenter · 2023-02-07T20:58:47Z

Codecov Report

Merging #1559 (223f982) into main (e04520b) will increase coverage by 0.20%.
The diff coverage is 86.20%.

❗ Current head 223f982 differs from pull request most recent head b8a70b6. Consider uploading reports for the commit b8a70b6 to get more accurate results

@@            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

Impacted Files	Coverage Δ
pkg/v1/remote/write.go	`62.43% <62.50%> (-0.27%)`	⬇️
pkg/v1/remote/descriptor.go	`72.34% <67.56%> (-1.51%)`	⬇️
internal/cmd/edit.go	`54.12% <85.00%> (+0.64%)`	⬆️
pkg/crane/options.go	`86.44% <100.00%> (+7.27%)`	⬆️
pkg/registry/manifest.go	`94.04% <100.00%> (+1.83%)`	⬆️
pkg/registry/registry.go	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

komish · 2023-02-07T21:00:41Z

@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.

jonjohnsonjr · 2023-02-07T23:44:08Z

pkg/crane/options.go

 func WithTransport(t http.RoundTripper) Option {
 	return func(o *Options) {
 		o.Remote = append(o.Remote, remote.WithTransport(t))
+		o.transportSet = true


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?

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).

Implemented in 5112fe3

Signed-off-by: Jose R. Gonzalez <jose@flutes.dev>

jonjohnsonjr · 2023-02-08T19:43:06Z

Thanks!

This change enables Skaffold to push to container image registries that use self-signed or other untrusted certificates, when Skaffold runs on macOS or Windows. It also removes the need for the `SSL_CERT_FILE` environment variable workaround on Linux. Prior to this fix, Skaffold would fail to retrieve the digest from the registry after the image was built, even if the registry was configured as an insecure registry in Skaffold configuration: ``` getting image: Get "https://localhost:8443/v2/": tls: failed to verify certificate: x509: certificate signed by unknown authority; GET http://localhost:8443/v2/: unexpected status code 400 Bad Request: Client sent an HTTP request to an HTTPS server. ``` On Linux environments only, a possible workaround was to set the `SSL_CERT_FILE` environment variable. However, this workaround cannot be used on macOS or Windows. This change updates `getRemoteIndex()` and `getRemoteImage()` in `pkg/skaffold/docker/remote.go`, adding the `InsecureSkipVerify` TLS config field to the transport if the registry from the image name matches one of the insecure registries configured in Skaffold. Fixes: GoogleContainerTools#3039 GoogleContainerTools#3116 Related: google/go-containerregistry#1559

This change enables Skaffold to push to container image registries that use self-signed or other untrusted certificates, when Skaffold runs on macOS or Windows. It also removes the need for the `SSL_CERT_FILE` environment variable workaround on Linux. Prior to this fix, Skaffold would fail to retrieve the digest from the registry after the image was built, even if the registry was configured as an insecure registry in Skaffold configuration: ``` getting image: Get "https://localhost:8443/v2/": tls: failed to verify certificate: x509: certificate signed by unknown authority; GET http://localhost:8443/v2/: unexpected status code 400 Bad Request: Client sent an HTTP request to an HTTPS server. ``` On Linux environments only, a possible workaround was to set the `SSL_CERT_FILE` environment variable. However, this workaround cannot be used on macOS or Windows. This change updates `getRemoteIndex()` and `getRemoteImage()` in `pkg/skaffold/docker/remote.go`, adding the `InsecureSkipVerify` TLS config field to the transport if the registry from the image name matches one of the insecure registries configured in Skaffold. Fixes: #3039 #3116 Related: google/go-containerregistry#1559

komish force-pushed the crane-insecure-skip-verify branch 2 times, most recently from 339f268 to 6b9e7e9 Compare February 7, 2023 16:35

jonjohnsonjr reviewed Feb 7, 2023

View reviewed changes

komish force-pushed the crane-insecure-skip-verify branch from 6b9e7e9 to 223f982 Compare February 7, 2023 20:58

jonjohnsonjr reviewed Feb 7, 2023

View reviewed changes

komish force-pushed the crane-insecure-skip-verify branch from 223f982 to 5112fe3 Compare February 8, 2023 15:34

skip tls verification if default transport is used with insecure option

b8a70b6

Signed-off-by: Jose R. Gonzalez <jose@flutes.dev>

komish force-pushed the crane-insecure-skip-verify branch from 5112fe3 to b8a70b6 Compare February 8, 2023 19:05

jonjohnsonjr approved these changes Feb 8, 2023

View reviewed changes

jonjohnsonjr merged commit 9cd098e into google:main Feb 8, 2023

halvards mentioned this pull request Sep 1, 2025

fix: Enable registries with self-signed certificates GoogleContainerTools/skaffold#9863

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skip tls verification if default transport is used with insecure option#1559

skip tls verification if default transport is used with insecure option#1559
jonjohnsonjr merged 1 commit intogoogle:mainfrom
komish:crane-insecure-skip-verify

komish commented Feb 7, 2023 •

edited

Loading

Uh oh!

komish commented Feb 7, 2023

Uh oh!

jonjohnsonjr Feb 7, 2023

Uh oh!

komish Feb 7, 2023 •

edited

Loading

Uh oh!

jonjohnsonjr left a comment

Uh oh!

codecov-commenter commented Feb 7, 2023 •

edited

Loading

Uh oh!

komish commented Feb 7, 2023 •

edited

Loading

Uh oh!

jonjohnsonjr Feb 7, 2023

Uh oh!

komish Feb 8, 2023

Uh oh!

komish Feb 8, 2023

Uh oh!

jonjohnsonjr commented Feb 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// the default transport is used (i.e. when WithTransport is not defined).
	// the default transport is used (i.e. when WithTransport is not used).

Conversation

komish commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

komish commented Feb 7, 2023

Uh oh!

jonjohnsonjr Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

komish Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonjohnsonjr left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

komish commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonjohnsonjr Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

komish Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

komish Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

jonjohnsonjr commented Feb 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

komish commented Feb 7, 2023 •

edited

Loading

komish Feb 7, 2023 •

edited

Loading

codecov-commenter commented Feb 7, 2023 •

edited

Loading

komish commented Feb 7, 2023 •

edited

Loading