Repository: linkerd/linkerd2
Pull request #7940 (2022-02-22 18:13:53)

cli: Only allow HTTPS URLs with `inject`
The CLI may access manifests over insecure channels, potentially
allowing MITM-attacks to run arbitrary code.

This change modifies `inject` to only support `https` URLs to mitigate
this risk.

This change addresses a security review finding (`TOB-LKDTM-4`).

Signed-off-by: Oliver Gould <ver@buoyant.io>

<!--  Thanks for sending a pull request!

If you already have a well-structured git commit message, chances are GitHub
set the title and description of this PR to the git commit message subject and
body, respectively. If so, you may delete these instructions and submit your PR.

If this is your first time, please read our contributor guide:
https://github.com/linkerd/linkerd2/blob/main/CONTRIBUTING.md

The title and description of your Pull Request should match the git commit
subject and body, respectively. Git commit messages are structured as follows:

```
Subject

Problem

Solution

Validation

Fixes #[GitHub issue ID]

DCO Sign off
```

Example git commit message:

```
Introduce Pull Request Template

GitHub's community guidelines recommend a pull request template, the repo was
lacking one.

Introduce a `PULL_REQUEST_TEMPLATE.md` file.

Once merged, the
[Community profile checklist](https://github.com/linkerd/linkerd2/community)
should indicate the repo now provides a pull request template.

Fixes #3321

Signed-off-by: Jane Smith <jane.smith@example.com>
```

Note the git commit message subject becomes the pull request title.

For more details around git commits, see the section on Committing in our
contributor guide:
https://github.com/linkerd/linkerd2/blob/main/CONTRIBUTING.md#committing
-->



Patch:
diff --git a/Cargo.lock b/Cargo.lock
index bd3f022e9..f1711703c 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -401,9 +401,9 @@ dependencies = [
 
 [[package]]
 name = "getrandom"
-version = "0.2.4"
+version = "0.2.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "418d37c8b1d42553c93648be529cb70f920d3baf8ef469b74b9638df426e0b4c"
+checksum = "d39cd93900197114fa1fcb7ae84ca742095eed9442088988ae74fa744e930e77"
 dependencies = [
  "cfg-if",
  "libc",
diff --git a/charts/linkerd-crds/templates/policy/server.yaml b/charts/linkerd-crds/templates/policy/server.yaml
index ba8530f74..d9fb155f6 100644
--- a/charts/linkerd-crds/templates/policy/server.yaml
+++ b/charts/linkerd-crds/templates/policy/server.yaml
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/inject.go b/cli/cmd/inject.go
index a3a05cf3c..10dadbe70 100644
--- a/cli/cmd/inject.go
+++ b/cli/cmd/inject.go
@@ -70,7 +70,7 @@ sub-folders, or coming from stdin.`,
   kubectl get deploy -o yaml | linkerd inject - | kubectl apply -f -
 
   # Injecting a file from a remote URL
-  linkerd inject http://url.to/yml | kubectl apply -f -
+  linkerd inject https://url.to/yml | kubectl apply -f -
 
   # Inject all the resources inside a folder and its sub-folders.
   linkerd inject <folder> | kubectl apply -f -`,
diff --git a/cli/cmd/inject_util.go b/cli/cmd/inject_util.go
index 60175f80e..2c4508a3f 100644
--- a/cli/cmd/inject_util.go
+++ b/cli/cmd/inject_util.go
@@ -10,6 +10,7 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/linkerd/linkerd2/pkg/inject"
 	corev1 "k8s.io/api/core/v1"
@@ -142,17 +143,19 @@ func processList(bytes []byte, rt resourceTransformer) ([]byte, []inject.Report,
 // Read all the resource files found in path into a slice of readers.
 // path can be either a file, directory or stdin.
 func read(path string) ([]io.Reader, error) {
-	var (
-		in  []io.Reader
-		err error
-	)
 	if path == "-" {
-		in = append(in, os.Stdin)
-	} else if isValidURL(path) {
-		resp, err := http.Get(path)
+		return []io.Reader{os.Stdin}, nil
+	}
+
+	if url, ok := toURL(path); ok {
+		if strings.ToLower(url.Scheme) != "https" {
+			return nil, fmt.Errorf("only HTTPS URLs are allowed")
+		}
+		resp, err := http.Get(url.String())
 		if err != nil {
 			return nil, err
 		}
+		defer resp.Body.Close()
 
 		if resp.StatusCode != http.StatusOK {
 			return nil, fmt.Errorf("unable to read URL %q, server reported %s, status code=%d", path, resp.Status, resp.StatusCode)
@@ -164,26 +167,21 @@ func read(path string) ([]io.Reader, error) {
 		if err != nil {
 			return nil, err
 		}
-		resp.Body.Close()
-		in = append(in, buf)
-	} else {
-		in, err = walk(path)
-		if err != nil {
-			return nil, err
-		}
+
+		return []io.Reader{buf}, nil
 	}
 
-	return in, nil
+	return walk(path)
 }
 
 // checks if the given string is a valid URL
-func isValidURL(path string) bool {
+func toURL(path string) (*url.URL, bool) {
 	u, err := url.ParseRequestURI(path)
-	if err != nil {
-		return false
+	if err == nil && u.Host != "" && u.Scheme != "" {
+		return u, true
 	}
 
-	return u.Host != "" && u.Scheme != ""
+	return nil, false
 }
 
 // walk walks the file tree rooted at path. path may be a file or a directory.
diff --git a/controller/gen/apis/server/v1beta1/types.go b/controller/gen/apis/server/v1beta1/types.go
index 099fa929c..5b5211deb 100644
--- a/controller/gen/apis/server/v1beta1/types.go
+++ b/controller/gen/apis/server/v1beta1/types.go
@@ -29,7 +29,7 @@ type Server struct {
 
 // ServerSpec specifies a Server resource.
 type ServerSpec struct {
-	PodSelector   *metav1.LabelSelector `json:"podSelector,omitempty"`
+	PodSelector   *metav1.LabelSelector `json:"podSelector"`
 	Port          intstr.IntOrString    `json:"port,omitempty"`
 	ProxyProtocol string                `json:"proxyProtocol,omitempty"`
 }
diff --git a/policy-controller/k8s/api/src/labels.rs b/policy-controller/k8s/api/src/labels.rs
index cfdd17321..3c5bbb433 100644
--- a/policy-controller/k8s/api/src/labels.rs
+++ b/policy-controller/k8s/api/src/labels.rs
@@ -27,7 +27,8 @@ pub enum Operator {
     DoesNotExist,
 }
 
-/// Selects a set of pods that expose a server.
+/// Selects a set of pods that expose a server. The result of `match_labels` and
+/// `match_expressions` are ANDed.
 #[derive(Clone, Debug, Eq, PartialEq, Default, Deserialize, Serialize, JsonSchema)]
 #[serde(rename_all = "camelCase")]
 pub struct Selector {
@@ -38,14 +39,22 @@ pub struct Selector {
 // === Selector ===
 
 impl Selector {
-    pub fn from_expressions(exprs: Expressions) -> Self {
+    #[cfg(test)]
+    fn new(labels: Map, exprs: Expressions) -> Self {
+        Self {
+            match_labels: Some(labels),
+            match_expressions: Some(exprs),
+        }
+    }
+
+    fn from_expressions(exprs: Expressions) -> Self {
         Self {
             match_labels: None,
             match_expressions: Some(exprs),
         }
     }
 
-    pub fn from_map(map: Map) -> Self {
+    fn from_map(map: Map) -> Self {
         Self {
             match_labels: Some(map),
             match_expressions: None,
@@ -201,6 +210,32 @@ mod tests {
                 true,
                 "expression match",
             ),
+            (
+                Selector::new(
+                    Map::from([("foo".to_string(), "bar".to_string())]),
+                    vec![Expression {
+                        key: "bah".into(),
+                        operator: Operator::In,
+                        values: Some(Some("bar".to_string()).into_iter().collect()),
+                    }],
+                ),
+                Labels::from_iter(vec![("foo", "bar"), ("bah", "baz")]),
+                false,
+                "matches labels but not expressions",
+            ),
+            (
+                Selector::new(
+                    Map::from([("foo".to_string(), "bar".to_string())]),
+                    vec![Expression {
+                        key: "bah".into(),
+                        operator: Operator::In,
+                        values: Some(Some("bar".to_string()).into_iter().collect()),
+                    }],
+                ),
+                Labels::from_iter(vec![("foo", "bar"), ("bah", "bar")]),
+                true,
+                "matches both labels and expressions",
+            ),
         ] {
             assert_eq!(selector.matches(labels), *matches, "{}", msg);
         }


Test patch:
diff --git a/cli/cmd/inject_test.go b/cli/cmd/inject_test.go
index 997d53c85..f3b56372d 100644
--- a/cli/cmd/inject_test.go
+++ b/cli/cmd/inject_test.go
@@ -577,7 +577,7 @@ func TestInjectFilePath(t *testing.T) {
 	})
 }
 
-func TestValidURL(t *testing.T) {
+func TestToURL(t *testing.T) {
 	// if the string follows a URL pattern, true has to be returned
 	// if not false is returned
 
@@ -593,9 +593,9 @@ func TestValidURL(t *testing.T) {
 	}
 
 	for url, expectedValue := range tests {
-		value := isValidURL(url)
-		if value != expectedValue {
-			t.Errorf("Result mismatch for %s. expected %v, but got %v", url, expectedValue, value)
+		_, ok := toURL(url)
+		if ok != expectedValue {
+			t.Errorf("Result mismatch for %s. expected %v, but got %v", url, expectedValue, ok)
 		}
 	}
 
diff --git a/cli/cmd/testdata/install_controlplane_tracing_output.golden b/cli/cmd/testdata/install_controlplane_tracing_output.golden
index 8e0409c21..fa5e09083 100644
--- a/cli/cmd/testdata/install_controlplane_tracing_output.golden
+++ b/cli/cmd/testdata/install_controlplane_tracing_output.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_custom_domain.golden b/cli/cmd/testdata/install_custom_domain.golden
index 8bc44caef..378cc2d95 100644
--- a/cli/cmd/testdata/install_custom_domain.golden
+++ b/cli/cmd/testdata/install_custom_domain.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_custom_registry.golden b/cli/cmd/testdata/install_custom_registry.golden
index 8e431d0ef..785183255 100644
--- a/cli/cmd/testdata/install_custom_registry.golden
+++ b/cli/cmd/testdata/install_custom_registry.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_default.golden b/cli/cmd/testdata/install_default.golden
index 8bc44caef..378cc2d95 100644
--- a/cli/cmd/testdata/install_default.golden
+++ b/cli/cmd/testdata/install_default.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_default_override_dst_get_nets.golden b/cli/cmd/testdata/install_default_override_dst_get_nets.golden
index e992de988..c7cd89d51 100644
--- a/cli/cmd/testdata/install_default_override_dst_get_nets.golden
+++ b/cli/cmd/testdata/install_default_override_dst_get_nets.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_default_token.golden b/cli/cmd/testdata/install_default_token.golden
index 24639c539..91ee17444 100644
--- a/cli/cmd/testdata/install_default_token.golden
+++ b/cli/cmd/testdata/install_default_token.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_ha_output.golden b/cli/cmd/testdata/install_ha_output.golden
index a82a5f56a..f6d1d1254 100644
--- a/cli/cmd/testdata/install_ha_output.golden
+++ b/cli/cmd/testdata/install_ha_output.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_ha_with_overrides_output.golden b/cli/cmd/testdata/install_ha_with_overrides_output.golden
index 2ff1506d9..489edcf46 100644
--- a/cli/cmd/testdata/install_ha_with_overrides_output.golden
+++ b/cli/cmd/testdata/install_ha_with_overrides_output.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_heartbeat_disabled_output.golden b/cli/cmd/testdata/install_heartbeat_disabled_output.golden
index f20376f76..fe7526249 100644
--- a/cli/cmd/testdata/install_heartbeat_disabled_output.golden
+++ b/cli/cmd/testdata/install_heartbeat_disabled_output.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_helm_crds_output.golden b/cli/cmd/testdata/install_helm_crds_output.golden
index aea4795f0..986154154 100644
--- a/cli/cmd/testdata/install_helm_crds_output.golden
+++ b/cli/cmd/testdata/install_helm_crds_output.golden
@@ -97,9 +97,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_helm_crds_output_ha.golden b/cli/cmd/testdata/install_helm_crds_output_ha.golden
index aea4795f0..986154154 100644
--- a/cli/cmd/testdata/install_helm_crds_output_ha.golden
+++ b/cli/cmd/testdata/install_helm_crds_output_ha.golden
@@ -97,9 +97,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_no_init_container.golden b/cli/cmd/testdata/install_no_init_container.golden
index 8c61af4e0..3cedeadf8 100644
--- a/cli/cmd/testdata/install_no_init_container.golden
+++ b/cli/cmd/testdata/install_no_init_container.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_output.golden b/cli/cmd/testdata/install_output.golden
index 4c6749ec8..3037eb2b7 100644
--- a/cli/cmd/testdata/install_output.golden
+++ b/cli/cmd/testdata/install_output.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_proxy_ignores.golden b/cli/cmd/testdata/install_proxy_ignores.golden
index e3c65b3f2..5e50687d1 100644
--- a/cli/cmd/testdata/install_proxy_ignores.golden
+++ b/cli/cmd/testdata/install_proxy_ignores.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object
diff --git a/cli/cmd/testdata/install_values_file.golden b/cli/cmd/testdata/install_values_file.golden
index bc04879c6..2e30c0d2c 100644
--- a/cli/cmd/testdata/install_values_file.golden
+++ b/cli/cmd/testdata/install_values_file.golden
@@ -95,9 +95,9 @@ spec:
                   type: object
                   description: >-
                     Selects pods in the same namespace.
-                  oneOf:
-                    - required: [matchExpressions]
-                    - required: [matchLabels]
+
+                    The result of matchLabels and matchExpressions are ANDed.
+                    Selects all if empty.
                   properties:
                     matchLabels:
                       type: object