Skip to content
26 changes: 23 additions & 3 deletions image/docker/docker_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ type dockerClient struct {
// tlsClientConfig is setup by newDockerClient and will be used and updated
// by detectProperties(). Callers can edit tlsClientConfig.InsecureSkipVerify in the meantime.
tlsClientConfig *tls.Config
// registryProxy is the proxy URL from the registry configuration, if any.
// It has the lowest priority and can be overridden by either DockerProxyURL or environment variables.
// When pulling, this value could be overwritten by a mirror-specific proxy. See docker_client_src.go.
Comment thread
cyqsimon marked this conversation as resolved.
Outdated
registryProxy *url.URL
// The following members are not set by newDockerClient and must be set by callers if needed.
auth types.DockerAuthConfig
registryToken string
Expand Down Expand Up @@ -262,18 +266,24 @@ func newDockerClient(sys *types.SystemContext, registry, reference string) (*doc
return nil, err
}

// Check if TLS verification shall be skipped (default=false) which can
// be specified in the sysregistriesv2 configuration.
skipVerify := false
// Fetch and load sysregistriesv2 configurations.
Comment thread
cyqsimon marked this conversation as resolved.
Outdated
reg, err := sysregistriesv2.FindRegistry(sys, reference)
if err != nil {
return nil, fmt.Errorf("loading registries: %w", err)
}
skipVerify := false
var registryProxy *url.URL
if reg != nil {
if reg.Blocked {
return nil, fmt.Errorf("registry %s is blocked in %s or %s", reg.Prefix, sysregistriesv2.ConfigPath(sys), sysregistriesv2.ConfigDirPath(sys))
}
// Check if TLS verification shall be skipped (default=false).
skipVerify = reg.Insecure
// Set registry proxy.
registryProxy, err = sysregistriesv2.ParseProxy(reg.Proxy)
if err != nil {
return nil, err
}
}
tlsClientConfig.InsecureSkipVerify = skipVerify

Expand All @@ -287,6 +297,7 @@ func newDockerClient(sys *types.SystemContext, registry, reference string) (*doc
registry: registry,
userAgent: userAgent,
tlsClientConfig: tlsClientConfig,
registryProxy: registryProxy,
tokenCache: map[string]*bearerToken{},
reportedWarnings: set.New[string](),
}, nil
Expand Down Expand Up @@ -968,6 +979,15 @@ func (c *dockerClient) detectPropertiesHelper(ctx context.Context) error {
}
tr := tlsclientconfig.NewTransport()
tr.TLSClientConfig = c.tlsClientConfig
// Set registry-specific proxy with lowest priority, which can be overridden by environment variables.
if c.registryProxy != nil {
tr.Proxy = func(req *http.Request) (*url.URL, error) {
if envProxy, err := http.ProxyFromEnvironment(req); err != nil || envProxy != nil {
return envProxy, err
}
Comment thread
cyqsimon marked this conversation as resolved.
Outdated
return c.registryProxy, nil
}
}
// if set DockerProxyURL explicitly, use the DockerProxyURL instead of system proxy
if c.sys != nil && c.sys.DockerProxyURL != nil {
tr.Proxy = http.ProxyURL(c.sys.DockerProxyURL)
Expand Down
46 changes: 46 additions & 0 deletions image/docker/docker_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -445,3 +445,49 @@ func TestIsManifestUnknownError(t *testing.T) {
assert.True(t, res, "%s: %#v", c.name, err)
}
}

// Helper function to test that the selected proxy for a registry matches expected.
func testProxyForRegistry(t *testing.T, ctx context.Context, sys *types.SystemContext, registry string, expectedProxy string) {
Comment thread
cyqsimon marked this conversation as resolved.
Outdated
t.Run(fmt.Sprintf(`Expecting proxy "%s" for registry "%s"`, expectedProxy, registry), func(t *testing.T) {
req, err := http.NewRequest("GET", fmt.Sprintf("http://%s/v2/", registry), nil)
require.NoError(t, err)

// Proxy configured using environment variables have priority, so we skip if it's set.
envProxy, _ := http.ProxyFromEnvironment(req)
if envProxy != nil {
t.Skip("Skipping registry proxy test: proxy configured using environment variables")
}

client, err := newDockerClient(sys, registry, registry)
require.NoError(t, err)

// Ping will fail, but we only care about the side effect of setting the proxy.
_ = client.detectProperties(ctx)

transport, ok := client.client.Transport.(*http.Transport)
require.True(t, ok)
require.NotNil(t, transport.Proxy)

proxyURL, err := transport.Proxy(req)
require.NoError(t, err)

if expectedProxy == "" {
require.Nil(t, proxyURL)
} else {
require.NotNil(t, proxyURL)
assert.Equal(t, expectedProxy, proxyURL.String())
}
})
}

func TestRegistrySpecificProxy(t *testing.T) {
ctx := context.Background()
sys := &types.SystemContext{
SystemRegistriesConfPath: "../pkg/sysregistriesv2/testdata/proxy.conf",
SystemRegistriesConfDirPath: "../pkg/sysregistriesv2/testdata/this-does-not-exist",
DockerInsecureSkipTLSVerify: types.OptionalBoolTrue,
}

testProxyForRegistry(t, ctx, sys, "registry-1.com", "")
testProxyForRegistry(t, ctx, sys, "registry-2.com", "https://proxy-2.example.com")
Comment thread
cyqsimon marked this conversation as resolved.
Outdated
}
5 changes: 5 additions & 0 deletions image/docker/docker_image_src.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,11 @@ func newImageSourceAttempt(ctx context.Context, sys *types.SystemContext, logica
return nil, err
}
client.tlsClientConfig.InsecureSkipVerify = pullSource.Endpoint.Insecure
registryProxy, err := sysregistriesv2.ParseProxy(pullSource.Endpoint.Proxy)
if err != nil {
return nil, err
}
client.registryProxy = registryProxy

s := &dockerImageSource{
PropertyMethodsInitialize: impl.PropertyMethods(impl.Properties{
Expand Down
42 changes: 42 additions & 0 deletions image/pkg/sysregistriesv2/system_registries_v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"io/fs"
"maps"
"net/url"
"os"
"path/filepath"
"reflect"
Expand Down Expand Up @@ -59,6 +60,8 @@ type Endpoint struct {
// If true, certs verification will be skipped and HTTP (non-TLS)
// connections will be allowed.
Insecure bool `toml:"insecure,omitempty"`
// The forwarding proxy to be used for accessing this endpoint.
Proxy string `toml:"proxy,omitempty"`
Comment thread
cyqsimon marked this conversation as resolved.
Outdated
// PullFromMirror is used for adding restrictions to image pull through the mirror.
// Set to "all", "digest-only", or "tag-only".
// If "digest-only", mirrors will only be used for digest pulls. Pulling images by
Expand Down Expand Up @@ -341,6 +344,32 @@ func parseLocation(input string) (string, error) {
return trimmed, nil
}

// ParseProxy parses the input string for a proxy configuration.
// Errors if a scheme is unsupported or unspecified, or if the input is not a valid URL.
func ParseProxy(input string) (*url.URL, error) {
Comment thread
cyqsimon marked this conversation as resolved.
Outdated
if input == "" {
return nil, nil
}

var hasSupportedScheme bool
for _, scheme := range []string{"http://", "https://", "socks5://", "socks5h://"} {
if strings.HasPrefix(input, scheme) {
Comment thread
cyqsimon marked this conversation as resolved.
Outdated
hasSupportedScheme = true
break
}
}
if !hasSupportedScheme {
return nil, &InvalidRegistries{s: "invalid proxy: proxy URL must specify one of the supported schemes: http://, https://, socks5://, socks5h://"}
}

parsed, err := url.Parse(input)
if err != nil {
return nil, fmt.Errorf("parsing proxy URL %q: %w", input, err)
}

return parsed, nil
}

// ConvertToV2 returns a v2 config corresponding to a v1 one.
func (config *V1RegistriesConf) ConvertToV2() (*V2RegistriesConf, error) {
regMap := make(map[string]*Registry)
Expand Down Expand Up @@ -409,6 +438,10 @@ func (config *V2RegistriesConf) postProcessRegistries() error {
return err
}

if _, err = ParseProxy(reg.Proxy); err != nil {
Comment thread
cyqsimon marked this conversation as resolved.
Outdated
Comment thread
cyqsimon marked this conversation as resolved.
Outdated
return err
}

if reg.Prefix == "" {
if reg.Location == "" {
return &InvalidRegistries{s: "invalid condition: both location and prefix are unset"}
Expand Down Expand Up @@ -438,6 +471,10 @@ func (config *V2RegistriesConf) postProcessRegistries() error {
return err
}

if _, err = ParseProxy(mir.Proxy); err != nil {
return err
}

// FIXME: unqualifiedSearchRegistries now also accepts empty values
// and shouldn't
// https://github.com/containers/image/pull/1191#discussion_r610623216
Expand Down Expand Up @@ -483,6 +520,11 @@ func (config *V2RegistriesConf) postProcessRegistries() error {
return &InvalidRegistries{s: msg}
}

if reg.Proxy != other.Proxy {
msg := fmt.Sprintf("registry '%s' is defined multiple times with conflicting 'proxy' setting", reg.Location)
return &InvalidRegistries{s: msg}
}

if reg.Blocked != other.Blocked {
msg := fmt.Sprintf("registry '%s' is defined multiple times with conflicting 'blocked' setting", reg.Location)
return &InvalidRegistries{s: msg}
Expand Down
50 changes: 50 additions & 0 deletions image/pkg/sysregistriesv2/system_registries_v2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,28 @@ func TestParseLocation(t *testing.T) {
assert.Equal(t, "example.com:5000/with/path", location)
}

func TestParseProxy(t *testing.T) {
for _, valid := range []string{
"",
"http://proxy.example.com",
"https://proxy.example.com",
"socks5://proxy.example.com",
"socks5h://proxy.example.com:1080",
} {
_, err := ParseProxy(valid)
assert.Nil(t, err, valid)
}

for _, invalid := range []string{
"no-scheme.example.com",
"ftp://bad-scheme.example.com",
"ssh://bad-scheme.example.com:2222",
} {
_, err := ParseProxy(invalid)
assert.NotNil(t, err)
}
}

func TestEmptyConfig(t *testing.T) {
registries, err := GetRegistries(&types.SystemContext{
SystemRegistriesConfPath: "testdata/empty.conf",
Expand Down Expand Up @@ -983,3 +1005,31 @@ func TestCredentialHelpers(t *testing.T) {
require.Equal(t, test.helpers, helpers, "%v", test)
}
}

func TestProxyConfiguration(t *testing.T) {
sys := &types.SystemContext{
SystemRegistriesConfPath: "testdata/proxy.conf",
SystemRegistriesConfDirPath: "testdata/this-does-not-exist",
}

registries, err := GetRegistries(sys)
require.NoError(t, err)
require.Equal(t, 2, len(registries))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(Non-blocking: I suspect right now all of this could be a single assert.Equal against a literal, instead of 10 individual checks. But add the *net.URL field first before trying that transformation.)


reg1 := registries[0]
assert.Equal(t, "registry-1.com", reg1.Location)
assert.Equal(t, "", reg1.Proxy)
require.Equal(t, 2, len(reg1.Mirrors))

mirror1 := reg1.Mirrors[0]
assert.Equal(t, "mirror-1.registry-1.com", mirror1.Location)
assert.Equal(t, "", mirror1.Proxy)

mirror2 := reg1.Mirrors[1]
assert.Equal(t, "mirror-2.registry-1.com", mirror2.Location)
assert.Equal(t, "http://proxy-1.example.com", mirror2.Proxy)

reg2 := registries[1]
assert.Equal(t, "registry-2.com", reg2.Location)
assert.Equal(t, "https://proxy-2.example.com", reg2.Proxy)
}
13 changes: 13 additions & 0 deletions image/pkg/sysregistriesv2/testdata/proxy.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
[[registry]]
location = "registry-1.com"

[[registry.mirror]]
location = "mirror-1.registry-1.com"

[[registry.mirror]]
location = "mirror-2.registry-1.com"
proxy = "http://proxy-1.example.com"

[[registry]]
location = "registry-2.com"
proxy = "https://proxy-2.example.com"