Merge pull request #14114 from miminar/secure-images-prune-by-default

Merged by openshift-bot

Author: OpenShift Bot

contrib/completions/bash/oadm

@@ -4546,6 +4546,8 @@ _oadm_prune_images()
     local_nonpersistent_flags+=("--certificate-authority=")
     flags+=("--confirm")
     local_nonpersistent_flags+=("--confirm")
+    flags+=("--force-insecure")
+    local_nonpersistent_flags+=("--force-insecure")
     flags+=("--keep-tag-revisions=")
     local_nonpersistent_flags+=("--keep-tag-revisions=")
     flags+=("--keep-younger-than=")

contrib/completions/bash/oc

@@ -4555,6 +4555,8 @@ _oc_adm_prune_images()
     local_nonpersistent_flags+=("--certificate-authority=")
     flags+=("--confirm")
     local_nonpersistent_flags+=("--confirm")
+    flags+=("--force-insecure")
+    local_nonpersistent_flags+=("--force-insecure")
     flags+=("--keep-tag-revisions=")
     local_nonpersistent_flags+=("--keep-tag-revisions=")
     flags+=("--keep-younger-than=")

contrib/completions/bash/openshift

@@ -4546,6 +4546,8 @@ _openshift_admin_prune_images()
     local_nonpersistent_flags+=("--certificate-authority=")
     flags+=("--confirm")
     local_nonpersistent_flags+=("--confirm")
+    flags+=("--force-insecure")
+    local_nonpersistent_flags+=("--force-insecure")
     flags+=("--keep-tag-revisions=")
     local_nonpersistent_flags+=("--keep-tag-revisions=")
     flags+=("--keep-younger-than=")
@@ -9669,6 +9671,8 @@ _openshift_cli_adm_prune_images()
     local_nonpersistent_flags+=("--certificate-authority=")
     flags+=("--confirm")
     local_nonpersistent_flags+=("--confirm")
+    flags+=("--force-insecure")
+    local_nonpersistent_flags+=("--force-insecure")
     flags+=("--keep-tag-revisions=")
     local_nonpersistent_flags+=("--keep-tag-revisions=")
     flags+=("--keep-younger-than=")

contrib/completions/zsh/oadm

@@ -4695,6 +4695,8 @@ _oadm_prune_images()
     local_nonpersistent_flags+=("--certificate-authority=")
     flags+=("--confirm")
     local_nonpersistent_flags+=("--confirm")
+    flags+=("--force-insecure")
+    local_nonpersistent_flags+=("--force-insecure")
     flags+=("--keep-tag-revisions=")
     local_nonpersistent_flags+=("--keep-tag-revisions=")
     flags+=("--keep-younger-than=")

contrib/completions/zsh/oc

@@ -4704,6 +4704,8 @@ _oc_adm_prune_images()
     local_nonpersistent_flags+=("--certificate-authority=")
     flags+=("--confirm")
     local_nonpersistent_flags+=("--confirm")
+    flags+=("--force-insecure")
+    local_nonpersistent_flags+=("--force-insecure")
     flags+=("--keep-tag-revisions=")
     local_nonpersistent_flags+=("--keep-tag-revisions=")
     flags+=("--keep-younger-than=")

contrib/completions/zsh/openshift

@@ -4695,6 +4695,8 @@ _openshift_admin_prune_images()
     local_nonpersistent_flags+=("--certificate-authority=")
     flags+=("--confirm")
     local_nonpersistent_flags+=("--confirm")
+    flags+=("--force-insecure")
+    local_nonpersistent_flags+=("--force-insecure")
     flags+=("--keep-tag-revisions=")
     local_nonpersistent_flags+=("--keep-tag-revisions=")
     flags+=("--keep-younger-than=")
@@ -9818,6 +9820,8 @@ _openshift_cli_adm_prune_images()
     local_nonpersistent_flags+=("--certificate-authority=")
     flags+=("--confirm")
     local_nonpersistent_flags+=("--confirm")
+    flags+=("--force-insecure")
+    local_nonpersistent_flags+=("--force-insecure")
     flags+=("--keep-tag-revisions=")
     local_nonpersistent_flags+=("--keep-tag-revisions=")
     flags+=("--keep-younger-than=")

pkg/cmd/admin/prune/images.go

@@ -28,6 +28,7 @@ import (
 	imageapi "github.com/openshift/origin/pkg/image/api"
 	"github.com/openshift/origin/pkg/image/prune"
 	oserrors "github.com/openshift/origin/pkg/util/errors"
+	"github.com/openshift/origin/pkg/util/netutils"
 )
 
 // PruneImagesRecommendedName is the recommended command name
@@ -46,7 +47,17 @@ var (
 		--confirm flag is needed for changes to be effective.
 
 		Only a user with a cluster role %s or higher who is logged-in will be able to actually
-		delete the images.`)
+		delete the images.
+
+		If the registry is secured with a certificate signed by a self-signed root certificate
+		authority other than the one present in current user's config, you may need to specify it
+		using --certificate-authority flag.
+
+		Insecure connection is allowed in following cases unless certificate-authority is specified:
+		 1. --force-insecure is given
+		 2. user's config allows for insecure connection (the user logged in to the cluster with
+			--insecure-skip-tls-verify or allowed for insecure connection)
+		 3. registry url is not given or it's a private/link-local address`)
 
 	imagesExample = templates.Examples(`
 	  # See, what the prune command would delete if only images more than an hour old and obsoleted
@@ -80,11 +91,13 @@ type PruneImagesOptions struct {
 	CABundle            string
 	RegistryUrlOverride string
 	Namespace           string
+	ForceInsecure       bool
 
 	OSClient       client.Interface
 	KClient        kclientset.Interface
 	RegistryClient *http.Client
 	Out            io.Writer
+	Insecure       bool
 }
 
 // NewCmdPruneImages implements the OpenShift cli prune images command.
@@ -117,8 +130,9 @@ func NewCmdPruneImages(f *clientcmd.Factory, parentName, name string, out io.Wri
 	cmd.Flags().DurationVar(opts.KeepYoungerThan, "keep-younger-than", *opts.KeepYoungerThan, "Specify the minimum age of an image for it to be considered a candidate for pruning.")
 	cmd.Flags().IntVar(opts.KeepTagRevisions, "keep-tag-revisions", *opts.KeepTagRevisions, "Specify the number of image revisions for a tag in an image stream that will be preserved.")
 	cmd.Flags().BoolVar(opts.PruneOverSizeLimit, "prune-over-size-limit", *opts.PruneOverSizeLimit, "Specify if images which are exceeding LimitRanges (see 'openshift.io/Image'), specified in the same namespace, should be considered for pruning. This flag cannot be combined with --keep-younger-than nor --keep-tag-revisions.")
-	cmd.Flags().StringVar(&opts.CABundle, "certificate-authority", opts.CABundle, "The path to a certificate authority bundle to use when communicating with the managed Docker registries. Defaults to the certificate authority data from the current user's config file.")
+	cmd.Flags().StringVar(&opts.CABundle, "certificate-authority", opts.CABundle, "The path to a certificate authority bundle to use when communicating with the managed Docker registries. Defaults to the certificate authority data from the current user's config file. It cannot be used together with --force-insecure.")
 	cmd.Flags().StringVar(&opts.RegistryUrlOverride, "registry-url", opts.RegistryUrlOverride, "The address to use when contacting the registry, instead of using the default value. This is useful if you can't resolve or reach the registry (e.g.; the default is a cluster-internal URL) but you do have an alternative route that works.")
+	cmd.Flags().BoolVar(&opts.ForceInsecure, "force-insecure", opts.ForceInsecure, "If true, allow an insecure connection to the docker registry that is hosted via HTTP or has an invalid HTTPS certificate. Whenever possible, use --certificate-authority instead of this dangerous option.")
 
 	return cmd
 }
@@ -151,7 +165,16 @@ func (o *PruneImagesOptions) Complete(f *clientcmd.Factory, cmd *cobra.Command,
 	}
 	o.Out = out
 
-	osClient, kClient, registryClient, err := getClients(f, o.CABundle)
+	clientConfig, err := f.ClientConfig()
+	if err != nil {
+		return err
+	}
+
+	o.Insecure = o.ForceInsecure
+	if !o.Insecure && len(o.CABundle) == 0 {
+		o.Insecure = clientConfig.TLSClientConfig.Insecure || len(o.RegistryUrlOverride) == 0 || netutils.IsPrivateAddress(o.RegistryUrlOverride)
+	}
+	osClient, kClient, registryClient, err := getClients(f, o.CABundle, o.Insecure)
 	if err != nil {
 		return err
 	}
@@ -176,6 +199,9 @@ func (o PruneImagesOptions) Validate() error {
 	if _, err := url.Parse(o.RegistryUrlOverride); err != nil {
 		return fmt.Errorf("invalid --registry-url flag: %v", err)
 	}
+	if o.ForceInsecure && len(o.CABundle) > 0 {
+		return fmt.Errorf("--certificate-authority cannot be specified with --force-insecure")
+	}
 	return nil
 }
 
@@ -251,6 +277,7 @@ func (o PruneImagesOptions) Run() error {
 		DryRun:             o.Confirm == false,
 		RegistryClient:     o.RegistryClient,
 		RegistryURL:        o.RegistryUrlOverride,
+		Insecure:           o.Insecure,
 	}
 	if o.Namespace != metav1.NamespaceAll {
 		options.Namespace = o.Namespace
@@ -435,8 +462,10 @@ func (p *describingManifestDeleter) DeleteManifest(registryClient *http.Client,
 	return err
 }
 
-// getClients returns a Kube client, OpenShift client, and registry client.
-func getClients(f *clientcmd.Factory, caBundle string) (*client.Client, kclientset.Interface, *http.Client, error) {
+// getClients returns a Kube client, OpenShift client, and registry client. Note that
+// registryCABundle and registryInsecure=true are mutually exclusive. If registryInsecure=true is
+// specified, the ca bundle is ignored.
+func getClients(f *clientcmd.Factory, registryCABundle string, registryInsecure bool) (*client.Client, kclientset.Interface, *http.Client, error) {
 	clientConfig, err := f.ClientConfig()
 	if err != nil {
 		return nil, nil, nil, err
@@ -461,8 +490,18 @@ func getClients(f *clientcmd.Factory, caBundle string) (*client.Client, kclients
 		return nil, nil, nil, err
 	}
 
+	cadata := []byte{}
+	registryCABundleIncluded := false
+	if len(registryCABundle) > 0 {
+		cadata, err = ioutil.ReadFile(registryCABundle)
+		if err != nil {
+			return nil, nil, nil, fmt.Errorf("failed to read registry ca bundle: %v", err)
+		}
+	}
+
 	// copy the config
 	registryClientConfig := *clientConfig
+	registryClientConfig.TLSClientConfig.Insecure = registryInsecure
 
 	// zero out everything we don't want to use
 	registryClientConfig.BearerToken = ""
@@ -471,8 +510,20 @@ func getClients(f *clientcmd.Factory, caBundle string) (*client.Client, kclients
 	registryClientConfig.KeyFile = ""
 	registryClientConfig.KeyData = []byte{}
 
-	// we have to set a username to something for the Docker login
-	// but it's not actually used
+	if registryInsecure {
+		// it's not allowed to specify insecure flag together with CAs
+		registryClientConfig.CAFile = ""
+		registryClientConfig.CAData = []byte{}
+
+	} else if len(cadata) > 0 && len(registryClientConfig.CAData) == 0 {
+		// If given, we want to append cabundle to the resulting tlsConfig.RootCAs. However, if we
+		// leave CAData unset, tlsConfig may not be created. We could append the caBundle to the
+		// CAData here directly if we were ok doing a binary magic, which is not the case.
+		registryClientConfig.CAData = cadata
+		registryCABundleIncluded = true
+	}
+
+	// we have to set a username to something for the Docker login but it's not actually used
 	registryClientConfig.Username = "unused"
 
 	// set the "password" to be the token
@@ -483,19 +534,13 @@ func getClients(f *clientcmd.Factory, caBundle string) (*client.Client, kclients
 		return nil, nil, nil, err
 	}
 
-	// if the user specified a CA on the command line, add it to the
-	// client config's CA roots
-	if tlsConfig != nil && len(caBundle) > 0 {
-		data, err := ioutil.ReadFile(caBundle)
-		if err != nil {
-			return nil, nil, nil, err
-		}
-
+	// Add the CA bundle to the client config's CA roots if provided and we haven't done that already.
+	// FIXME: handle registryCABundle on one place
+	if tlsConfig != nil && len(cadata) > 0 && !registryCABundleIncluded && !registryInsecure {
 		if tlsConfig.RootCAs == nil {
 			tlsConfig.RootCAs = x509.NewCertPool()
 		}
-
-		tlsConfig.RootCAs.AppendCertsFromPEM(data)
+		tlsConfig.RootCAs.AppendCertsFromPEM(cadata)
 	}
 
 	transport := knet.SetTransportDefaults(&http.Transport{

pkg/image/prune/prune.go

@@ -29,6 +29,7 @@ import (
 	deploygraph "github.com/openshift/origin/pkg/deploy/graph/nodes"
 	imageapi "github.com/openshift/origin/pkg/image/api"
 	imagegraph "github.com/openshift/origin/pkg/image/graph/nodes"
+	"github.com/openshift/origin/pkg/util/netutils"
 )
 
 // TODO these edges should probably have an `Add***Edges` method in images/graph and be moved there
@@ -140,6 +141,8 @@ type PrunerOptions struct {
 	RegistryClient *http.Client
 	// RegistryURL is the URL for the registry.
 	RegistryURL string
+	// Allow a fallback to insecure transport when contacting the registry.
+	Insecure bool
 }
 
 // Pruner knows how to prune istags, images, layers and image configs.
@@ -170,7 +173,8 @@ type registryPinger interface {
 
 // defaultRegistryPinger implements registryPinger.
 type defaultRegistryPinger struct {
-	client *http.Client
+	client   *http.Client
+	insecure bool
 }
 
 func (drp *defaultRegistryPinger) ping(registry string) error {
@@ -183,23 +187,29 @@ func (drp *defaultRegistryPinger) ping(registry string) error {
 		defer healthResponse.Body.Close()
 
 		if healthResponse.StatusCode != http.StatusOK {
-			return fmt.Errorf("unexpected status code %d", healthResponse.StatusCode)
+			return fmt.Errorf("unexpected status: %s", healthResponse.Status)
 		}
 
 		return nil
 	}
 
-	var err error
-	for _, proto := range []string{"https", "http"} {
+	var errs []error
+	protos := make([]string, 0, 2)
+	protos = append(protos, "https")
+	if drp.insecure || netutils.IsPrivateAddress(registry) {
+		protos = append(protos, "http")
+	}
+	for _, proto := range protos {
 		glog.V(4).Infof("Trying %s for %s", proto, registry)
-		err = healthCheck(proto, registry)
+		err := healthCheck(proto, registry)
 		if err == nil {
-			break
+			return nil
 		}
+		errs = append(errs, err)
 		glog.V(4).Infof("Error with %s for %s: %v", proto, registry, err)
 	}
 
-	return err
+	return kerrors.NewAggregate(errs)
 }
 
 // dryRunRegistryPinger implements registryPinger.
@@ -287,7 +297,10 @@ func NewPruner(options PrunerOptions) Pruner {
 	if options.DryRun {
 		rp = &dryRunRegistryPinger{}
 	} else {
-		rp = &defaultRegistryPinger{options.RegistryClient}
+		rp = &defaultRegistryPinger{
+			client:   options.RegistryClient,
+			insecure: options.Insecure,
+		}
 	}
 
 	return &pruner{

pkg/util/netutils/common.go

@@ -10,6 +10,9 @@ import (
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 )
 
+var localHosts []string = []string{"127.0.0.1", "::1", "localhost"}
+var localSubnets []string = []string{"10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16", "fc00::/7", "fe80::/10"}
+
 func IPToUint32(ip net.IP) uint32 {
 	return binary.BigEndian.Uint32(ip.To4())
 }
@@ -114,3 +117,34 @@ func ParseCIDRMask(cidr string) (*net.IPNet, error) {
 	}
 	return net, nil
 }
+
+// IsPrivateAddress returns true if given address in format "<host>[:<port>]" is a localhost or an ip from
+// private network range (e.g. 172.30.0.1, 192.168.0.1).
+func IsPrivateAddress(addr string) bool {
+	host, _, err := net.SplitHostPort(addr)
+	if err != nil {
+		// assume indexName is of the form `host` without the port and go on.
+		host = addr
+	}
+	for _, localHost := range localHosts {
+		if host == localHost {
+			return true
+		}
+	}
+
+	ip := net.ParseIP(host)
+	if ip == nil {
+		return false
+	}
+
+	for _, subnet := range localSubnets {
+		ipnet, err := ParseCIDRMask(subnet)
+		if err != nil {
+			continue // should not happen
+		}
+		if ipnet.Contains(ip) {
+			return true
+		}
+	}
+	return false
+}