From 8dadeaf8ea4d3a3f06ab3283e353887039675ff2 Mon Sep 17 00:00:00 2001
From: Yong Tang <yong.tang.github@outlook.com>
Date: Sun, 8 Jan 2017 19:30:58 -0800
Subject: [PATCH] Return error when insecure registry contains scheme

While investigating 29936 I noticed one potential issue
in `LoadInsecureRegistries`.

The implementation of the func assumes that the format
of insecure registry should be `host:port` if not CIDR.
However, it is very common that user may incorrectly
provide a registry with a scheme (e.g, `http://myregistry.com:5000`)
Such a registry format with a scheme will cause docker pull to
always try https endpoint.

The reason is that the func of `isSecureIndex()` actually will
check for the map of the index server for `myregistry.com:5000`
while the insecure registry only has a record of `http://myregistry.com:5000`.
As a consequence, docker assumes that `myregistry.com:5000` is not
a insecure registry and will go ahead with https endpoint.

This fix addresses the issue by error out insecure registries with scheme.

A unit test has been added.

This fix is related to 29936.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
---
 registry/config.go      | 14 +++++++++++
 registry/config_test.go | 55 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/registry/config.go b/registry/config.go
index c839d87cf6..cc4111efaf 100644
--- a/registry/config.go
+++ b/registry/config.go
@@ -7,6 +7,7 @@ import (
 	"net/url"
 	"strings"
 
+	"github.com/Sirupsen/logrus"
 	registrytypes "github.com/docker/docker/api/types/registry"
 	"github.com/docker/docker/opts"
 	"github.com/docker/docker/reference"
@@ -150,6 +151,19 @@ skip:
 			config.ServiceConfig.IndexConfigs = originalIndexInfos
 			return err
 		}
+		if strings.HasPrefix(strings.ToLower(r), "http://") {
+			logrus.Warnf("insecure registry %s should not contain 'http://' and 'http://' has been removed from the insecure registry config", r)
+			r = r[7:]
+		} else if strings.HasPrefix(strings.ToLower(r), "https://") {
+			logrus.Warnf("insecure registry %s should not contain 'https://' and 'https://' has been removed from the insecure registry config", r)
+			r = r[8:]
+		} else if validateNoScheme(r) != nil {
+			// Insecure registry should not contain '://'
+			// before returning err, roll back to original data
+			config.ServiceConfig.InsecureRegistryCIDRs = originalCIDRs
+			config.ServiceConfig.IndexConfigs = originalIndexInfos
+			return fmt.Errorf("insecure registry %s should not contain '://'", r)
+		}
 		// Check if CIDR was passed to --insecure-registry
 		_, ipnet, err := net.ParseCIDR(r)
 		if err == nil {
diff --git a/registry/config_test.go b/registry/config_test.go
index 0382530776..0555af2329 100644
--- a/registry/config_test.go
+++ b/registry/config_test.go
@@ -1,6 +1,7 @@
 package registry
 
 import (
+	"strings"
 	"testing"
 )
 
@@ -48,3 +49,57 @@ func TestValidateMirror(t *testing.T) {
 		}
 	}
 }
+
+func TestLoadInsecureRegistries(t *testing.T) {
+	testCases := []struct {
+		registries []string
+		index      string
+		err        string
+	}{
+		{
+			registries: []string{"http://mytest.com"},
+			index:      "mytest.com",
+		},
+		{
+			registries: []string{"https://mytest.com"},
+			index:      "mytest.com",
+		},
+		{
+			registries: []string{"HTTP://mytest.com"},
+			index:      "mytest.com",
+		},
+		{
+			registries: []string{"svn://mytest.com"},
+			err:        "insecure registry svn://mytest.com should not contain '://'",
+		},
+		{
+			registries: []string{"-invalid-registry"},
+			err:        "Cannot begin or end with a hyphen",
+		},
+	}
+	for _, testCase := range testCases {
+		config := newServiceConfig(ServiceOptions{})
+		err := config.LoadInsecureRegistries(testCase.registries)
+		if testCase.err == "" {
+			if err != nil {
+				t.Fatalf("expect no error, got '%s'", err)
+			}
+			match := false
+			for index := range config.IndexConfigs {
+				if index == testCase.index {
+					match = true
+				}
+			}
+			if !match {
+				t.Fatalf("expect index configs to contain '%s', got %+v", testCase.index, config.IndexConfigs)
+			}
+		} else {
+			if err == nil {
+				t.Fatalf("expect error '%s', got no error", testCase.err)
+			}
+			if !strings.Contains(err.Error(), testCase.err) {
+				t.Fatalf("expect error '%s', got '%s'", testCase.err, err)
+			}
+		}
+	}
+}