Repository: istio/istio (go)

Issue:
auto mtls: add support for workload selectors on PeerAuthentication
Currently, auto mtls only works for namespace or cluster scoped PeerAuthentications. This makes creating a DISABLE PeerAuthentication for a specific workload really hard, since now you need a DR which is confusing.

cc @incfly 

Pull request description:
Make auto mTLS works with workload level peer authentication policy.
Fixes #32788  #33809

- Creates the mTLS checker in the endpoint builder when the feature enabled.
- When policy enables mTLS plus the workload has the sidecar, we add metadata tlsMode: istio.
- Feature guarded via an env var. Integration test covered.

credits to @cpakulski for the work on envoy side

Patch:
diff --git a/operator/pkg/manifest/shared.go b/operator/pkg/manifest/shared.go
index 45b2e750a0..1bb122df70 100644
--- a/operator/pkg/manifest/shared.go
+++ b/operator/pkg/manifest/shared.go
@@ -212,6 +212,9 @@ func GenIOPFromProfile(profileOrPath, fileOverlayYAML string, setFlags []string,
 	if ns := GetValueForSetFlag(setFlags, "values.global.istioNamespace"); ns != "" {
 		finalIOP.Namespace = ns
 	}
+	if finalIOP.Spec.Profile == "" {
+		finalIOP.Spec.Profile = name.DefaultProfileName
+	}
 	return util.ToYAMLWithJSONPB(finalIOP), finalIOP, nil
 }
 
diff --git a/pilot/pkg/features/pilot.go b/pilot/pkg/features/pilot.go
index 7d5af0d500..dc233a0642 100644
--- a/pilot/pkg/features/pilot.go
+++ b/pilot/pkg/features/pilot.go
@@ -146,6 +146,11 @@ var (
 		false,
 		"Skip validating the peer is from the same trust domain when mTLS is enabled in authentication policy").Get()
 
+	EnableAutomTLSCheckPolicies = env.RegisterBoolVar(
+		"ENABLE_AUTO_MTLS_CHECK_POLICIES", true,
+		"Enable the auto mTLS EDS output to consult the PeerAuthentication Policy, only set the {tlsMode: istio} "+
+			" when server side policy enables mTLS PERMISSIVE or STRICT.").Get()
+
 	EnableProtocolSniffingForOutbound = env.RegisterBoolVar(
 		"PILOT_ENABLE_PROTOCOL_SNIFFING_FOR_OUTBOUND",
 		true,
diff --git a/pilot/pkg/model/context.go b/pilot/pkg/model/context.go
index 97c749a575..709d3e41b5 100644
--- a/pilot/pkg/model/context.go
+++ b/pilot/pkg/model/context.go
@@ -199,7 +199,7 @@ type XdsLogDetails struct {
 	AdditionalInfo string
 }
 
-var DefaultXdsLogDetails XdsLogDetails = XdsLogDetails{}
+var DefaultXdsLogDetails = XdsLogDetails{}
 
 // XdsResourceGenerator creates the response for a typeURL DiscoveryRequest. If no generator is associated
 // with a Proxy, the default (a networking.core.ConfigGenerator instance) will be used.
diff --git a/pilot/pkg/networking/util/util.go b/pilot/pkg/networking/util/util.go
index 839a2551e3..cbdd93f7ee 100644
--- a/pilot/pkg/networking/util/util.go
+++ b/pilot/pkg/networking/util/util.go
@@ -524,6 +524,32 @@ func BuildLbEndpointMetadata(networkID network.ID, tlsMode, workloadname, namesp
 	return metadata
 }
 
+// MaybeUpdateTLSModeLabel may or may not update the metadata for the Envoy transport socket matches for auto mTLS.
+func MaybeUpdateTLSModeLabel(metadata *core.Metadata, tlsMode string) bool {
+	if metadata == nil {
+		return false
+	}
+	epTLSMode := ""
+	if metadata.FilterMetadata != nil {
+		if v, ok := metadata.FilterMetadata[EnvoyTransportSocketMetadataKey]; ok {
+			epTLSMode = v.Fields[model.TLSModeLabelShortname].GetStringValue()
+		}
+	}
+	if epTLSMode == tlsMode {
+		return false
+	}
+	if tlsMode != "" && tlsMode != model.DisabledTLSModeLabel {
+		metadata.FilterMetadata[EnvoyTransportSocketMetadataKey] = &pstruct.Struct{
+			Fields: map[string]*pstruct.Value{
+				model.TLSModeLabelShortname: {Kind: &pstruct.Value_StringValue{StringValue: tlsMode}},
+			},
+		}
+	} else {
+		delete(metadata.FilterMetadata, EnvoyTransportSocketMetadataKey)
+	}
+	return true
+}
+
 func addIstioEndpointLabel(metadata *core.Metadata, key string, val *pstruct.Value) {
 	if _, ok := metadata.FilterMetadata[IstioMetadataKey]; !ok {
 		metadata.FilterMetadata[IstioMetadataKey] = &pstruct.Struct{
diff --git a/pilot/pkg/xds/endpoint_builder.go b/pilot/pkg/xds/endpoint_builder.go
index 71f1e1e6a9..0eb3e0a2f8 100644
--- a/pilot/pkg/xds/endpoint_builder.go
+++ b/pilot/pkg/xds/endpoint_builder.go
@@ -25,6 +25,7 @@ import (
 	"github.com/golang/protobuf/ptypes/wrappers"
 
 	networkingapi "istio.io/api/networking/v1alpha3"
+	"istio.io/istio/pilot/pkg/features"
 	"istio.io/istio/pilot/pkg/model"
 	"istio.io/istio/pilot/pkg/networking"
 	"istio.io/istio/pilot/pkg/networking/util"
@@ -100,9 +101,16 @@ func NewEndpointBuilder(clusterName string, proxy *model.Proxy, push *model.Push
 		hostname:   hostname,
 		port:       port,
 	}
+
+	enableMtlsChecker := false
+	// We need this for multi-network, or for clusters meant for use with AUTO_PASSTHROUGH.
 	if b.push.NetworkManager().IsMultiNetworkEnabled() || model.IsDNSSrvSubsetKey(clusterName) {
-		// We only need this for multi-network, or for clusters meant for use with AUTO_PASSTHROUGH
-		// As an optimization, we skip this logic entirely for everything else.
+		enableMtlsChecker = true
+	}
+	if features.EnableAutomTLSCheckPolicies {
+		enableMtlsChecker = true
+	}
+	if enableMtlsChecker {
 		b.mtlsChecker = newMtlsChecker(push, port, dr)
 	}
 	return b
@@ -318,6 +326,13 @@ func (b *EndpointBuilder) buildLocalityLbEndpointsFromShards(
 			if ep.EnvoyEndpoint == nil {
 				ep.EnvoyEndpoint = buildEnvoyLbEndpoint(ep)
 			}
+			if features.EnableAutomTLSCheckPolicies {
+				tlsMode := ep.TLSMode
+				if b.mtlsChecker != nil && b.mtlsChecker.mtlsDisabledByPeerAuthentication(ep) {
+					tlsMode = ""
+				}
+				util.MaybeUpdateTLSModeLabel(ep.EnvoyEndpoint.Metadata, tlsMode)
+			}
 			locLbEps.append(ep, ep.EnvoyEndpoint, ep.TunnelAbility)
 
 			// detect if mTLS is possible for this endpoint, used later during ep filtering
diff --git a/pkg/istio-agent/agent.go b/pkg/istio-agent/agent.go
index c979d9cc36..b557dcb66a 100644
--- a/pkg/istio-agent/agent.go
+++ b/pkg/istio-agent/agent.go
@@ -89,7 +89,6 @@ const (
 	MetadataClientCertKey   = "ISTIO_META_TLS_CLIENT_KEY"
 	MetadataClientCertChain = "ISTIO_META_TLS_CLIENT_CERT_CHAIN"
 	MetadataClientRootCert  = "ISTIO_META_TLS_CLIENT_ROOT_CERT"
-	MetadataCertSigner      = "ISTIO_META_CERT_SIGNER"
 )
 
 // Agent contains the configuration of the agent, based on the injected
diff --git a/pkg/istio-agent/xds_proxy.go b/pkg/istio-agent/xds_proxy.go
index 280cc71f31..e58550f1c8 100644
--- a/pkg/istio-agent/xds_proxy.go
+++ b/pkg/istio-agent/xds_proxy.go
@@ -800,7 +800,7 @@ func (p *XdsProxy) makeTapHandler() func(w http.ResponseWriter, req *http.Reques
 		var buf bytes.Buffer
 		err = jsonm.Marshal(&buf, response)
 		if err == nil {
-			_, _ = w.Write(buf.Bytes())
+			_, err = w.Write(buf.Bytes())
 			if err != nil {
 				log.Infof("fail to write debug response: %v", err)
 			}
diff --git a/releasenotes/notes/33864.yaml b/releasenotes/notes/33864.yaml
new file mode 100644
index 0000000000..059bd95dad
--- /dev/null
+++ b/releasenotes/notes/33864.yaml
@@ -0,0 +1,8 @@
+apiVersion: release-notes/v2
+kind: bug-fix
+area: istioctl
+issue:
+  - 33857
+releaseNotes:
+  - |
+    **Fixed** an issue where the `default` profile name was missing in the `istioctl install` confirmation prompt message.
diff --git a/releasenotes/notes/34118.yaml b/releasenotes/notes/34118.yaml
new file mode 100644
index 0000000000..0b22e38165
--- /dev/null
+++ b/releasenotes/notes/34118.yaml
@@ -0,0 +1,10 @@
+apiVersion: release-notes/v2
+kind: feature
+area: security
+issue:
+  - 33809
+releaseNotes:
+  - |
+    **Added** Auto mTLS support for workload level peer authentication. You no longer need to configure destination rule when servers are configured with workload level peer authentication policy. This can be disabled by setting ENABLE_AUTO_MTLS_CHECK_POLICIES to "false". 
+docs:
+- https://istio.io/latest/docs/tasks/security/authentication/authn-policy/#enable-mutual-tls-per-workload
\ No newline at end of file
diff --git a/samples/bookinfo/src/ratings/ratings.js b/samples/bookinfo/src/ratings/ratings.js
index 49929bd63b..cea32887b6 100644
--- a/samples/bookinfo/src/ratings/ratings.js
+++ b/samples/bookinfo/src/ratings/ratings.js
@@ -139,15 +139,18 @@ dispatcher.onGet(/^\/ratings\/[0-9]*/, function (req, res) {
           connection.end()
       })
     } else {
-      MongoClient.connect(url, function (err, db) {
+      MongoClient.connect(url, function (err, client) {
         if (err) {
           res.writeHead(500, {'Content-type': 'application/json'})
           res.end(JSON.stringify({error: 'could not connect to ratings database'}))
+          console.log(err)
         } else {
+          const db = client.db("test")
           db.collection('ratings').find({}).toArray(function (err, data) {
             if (err) {
               res.writeHead(500, {'Content-type': 'application/json'})
               res.end(JSON.stringify({error: 'could not load ratings from database'}))
+              console.log(err)
             } else {
               firstRating = data[0].rating
               secondRating = data[1].rating
@@ -161,8 +164,8 @@ dispatcher.onGet(/^\/ratings\/[0-9]*/, function (req, res) {
               res.writeHead(200, {'Content-type': 'application/json'})
               res.end(JSON.stringify(result))
             }
-            // close DB once done:
-            db.close()
+            // close client once done:
+            client.close()
           })
         }
       })


Test patch:
diff --git a/tests/fuzz/Dockerfile.fuzz b/tests/fuzz/Dockerfile.fuzz
new file mode 100644
index 0000000000..3a11fec121
--- /dev/null
+++ b/tests/fuzz/Dockerfile.fuzz
@@ -0,0 +1,32 @@
+# Copyright 2021 Istio Authors
+#
+#   Licensed under the Apache License, Version 2.0 (the "License");
+#   you may not use this file except in compliance with the License.
+#   You may obtain a copy of the License at
+#
+#       http://www.apache.org/licenses/LICENSE-2.0
+#
+#   Unless required by applicable law or agreed to in writing, software
+#   distributed under the License is distributed on an "AS IS" BASIS,
+#   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#   See the License for the specif
+
+FROM golang:1.15
+
+RUN apt-get update && \
+    apt-get install --no-install-recommends -y \
+    clang && \
+    apt-get clean && \
+    rm -rf  /var/log/*log \
+    /var/lib/apt/lists/* \
+    /var/log/apt/* \
+    /var/lib/dpkg/*-old \
+    /var/cache/debconf/*-old
+
+RUN go get -u github.com/dvyukov/go-fuzz/go-fuzz \
+    github.com/dvyukov/go-fuzz/go-fuzz-dep \
+    github.com/dvyukov/go-fuzz/go-fuzz-build
+
+COPY . /istio
+WORKDIR /istio
+
diff --git a/tests/fuzz/README.md b/tests/fuzz/README.md
new file mode 100644
index 0000000000..cd48b7cb2a
--- /dev/null
+++ b/tests/fuzz/README.md
@@ -0,0 +1,45 @@
+# Istio fuzzing
+
+Istio has a series of fuzzers that run continuously through OSS-fuzz.
+
+## Local testing
+
+To run the fuzzers using `Dockerfile.fuzz`, follow these steps:
+
+```bash
+git clone https://github.com/istio/istio
+```
+
+```bash
+cd istio
+```
+
+```bash
+mv tests/fuzz/Dockerfile.fuzz ./
+```
+
+```bash
+sudo docker build -t istio-fuzz -f Dockerfile.fuzz .
+```
+
+Next, get a shell in the container:
+
+```bash
+sudo docker run -it istio-fuzz
+```
+
+At this point, you can navigate to `tests/fuzz` and build any of the fuzzers:
+
+```bash
+cd $PATH_TO_FUZZER
+go-fuzz-build -libfuzzer -func=FUZZ_NAME && \
+clang -fsanitize=fuzzer PACKAGE_NAME.a -o fuzzer
+```
+
+If you encounter any errors when linking with `PACKAGE_NAME.a`, simply `ls` after running `go-fuzz-build...`, and you will see the archive to link with.
+
+If everything goes well until this point, you can run the fuzzer:
+
+```bash
+./fuzzer
+```
diff --git a/tests/fuzz/oss_fuzz_build.sh b/tests/fuzz/oss_fuzz_build.sh
index abb391b7cf..bbdbb2e895 100755
--- a/tests/fuzz/oss_fuzz_build.sh
+++ b/tests/fuzz/oss_fuzz_build.sh
@@ -27,6 +27,7 @@ compile_go_fuzzer istio.io/istio/tests/fuzz FuzzCompareDiff fuzz_compare_diff
 compile_go_fuzzer istio.io/istio/tests/fuzz FuzzHelmReconciler fuzz_helm_reconciler
 compile_go_fuzzer istio.io/istio/tests/fuzz FuzzIntoResourceFile fuzz_into_resource_file
 compile_go_fuzzer istio.io/istio/tests/fuzz FuzzTranslateFromValueToSpec fuzz_translate_from_value_to_spec
+compile_go_fuzzer istio.io/istio/tests/fuzz FuzzBNMUnmarshalJSON fuzz_bnm_unmarshal_json
 
 # Create seed corpora:
 zip "${OUT}"/fuzz_analyzer_seed_corpus.zip "${SRC}"/istio/galley/pkg/config/analysis/analyzers/testdata/*.yaml
diff --git a/tests/fuzz/pilot_model_fuzzer.go b/tests/fuzz/pilot_model_fuzzer.go
index 811f8d80c4..b7cf60a742 100644
--- a/tests/fuzz/pilot_model_fuzzer.go
+++ b/tests/fuzz/pilot_model_fuzzer.go
@@ -205,3 +205,9 @@ func FuzzInitContext(data []byte) int {
 	_ = pc.InitContext(env, nil, nil)
 	return 1
 }
+
+func FuzzBNMUnmarshalJSON(data []byte) int {
+	var bnm model.BootstrapNodeMetadata
+	_ = bnm.UnmarshalJSON(data)
+	return 1
+}
diff --git a/tests/fuzz/regression_test.go b/tests/fuzz/regression_test.go
index b7c3452150..ec11294779 100644
--- a/tests/fuzz/regression_test.go
+++ b/tests/fuzz/regression_test.go
@@ -126,6 +126,7 @@ func TestFuzzers(t *testing.T) {
 		{"FuzzIntoResourceFile", FuzzIntoResourceFile},
 		{"FuzzTranslateFromValueToSpec", FuzzTranslateFromValueToSpec},
 		{"FuzzConfigValidation2", FuzzConfigValidation2},
+		{"FuzzBNMUnmarshalJSON", FuzzBNMUnmarshalJSON},
 	}
 	for _, tt := range cases {
 		if testedFuzzers.Contains(tt.name) {
diff --git a/tests/integration/security/reachability_test.go b/tests/integration/security/reachability_test.go
index 4e19068686..deb0391bcf 100644
--- a/tests/integration/security/reachability_test.go
+++ b/tests/integration/security/reachability_test.go
@@ -89,6 +89,29 @@ func TestReachability(t *testing.T) {
 					ExpectMTLS:             Never,
 					SkippedForMulticluster: true,
 				},
+				{
+					ConfigFile: "beta-mtls-automtls-workload.yaml",
+					Namespace:  apps.Namespace1,
+					Include: func(src echo.Instance, opts echo.CallOptions) bool {
+						return (apps.B.Contains(src) || apps.IsNaked(src)) &&
+							(apps.A.Contains(opts.Target) || apps.B.Contains(opts.Target))
+					},
+					ExpectSuccess: func(src echo.Instance, opts echo.CallOptions) bool {
+						// Sidecar injected client always succeed.
+						if apps.B.Contains(src) {
+							return true
+						}
+						// For naked app as client, only requests targeted to mTLS disabled endpoints succeed:
+						// A are disabled by workload selector for entire service.
+						// B port 8090 http port are disabled.
+						return apps.A.Contains(opts.Target) || (apps.B.Contains(opts.Target) && opts.PortName == "http")
+					},
+					// Only when the source is B and the destination does not disable mTLS.
+					ExpectMTLS: func(src echo.Instance, opts echo.CallOptions) bool {
+						return apps.B.Contains(src) && opts.PortName != "http" && !apps.A.Contains(opts.Target)
+					},
+					SkippedForMulticluster: true,
+				},
 				{
 					ConfigFile:             "plaintext-to-permissive.yaml",
 					Namespace:              systemNM,
@@ -124,22 +147,6 @@ func TestReachability(t *testing.T) {
 					},
 					ExpectMTLS: mtlsOnExpect,
 				},
-				{
-					ConfigFile: "beta-mtls-partial-automtls.yaml",
-					Namespace:  apps.Namespace1,
-					Include:    Always,
-					ExpectSuccess: func(src echo.Instance, opts echo.CallOptions) bool {
-						// autoMtls doesn't work for client that doesn't have proxy, unless target doesn't
-						// have proxy or have mTLS disabled
-						if apps.IsNaked(src) {
-							return apps.IsNaked(opts.Target) || (apps.B.Contains(opts.Target) && opts.PortName != "http")
-						}
-						// PeerAuthentication disable mTLS for workload app:b, except http port. Thus, autoMTLS
-						// will fail on all ports on b, except http port.
-						return !apps.B.Contains(opts.Target) || opts.PortName == "http"
-					},
-					ExpectMTLS: mtlsOnExpect,
-				},
 				{
 					ConfigFile: "no-peer-authn.yaml",
 					Namespace:  systemNM,
diff --git a/tests/integration/security/testdata/beta-mtls-automtls-workload.yaml b/tests/integration/security/testdata/beta-mtls-automtls-workload.yaml
new file mode 100644
index 0000000000..274533f87f
--- /dev/null
+++ b/tests/integration/security/testdata/beta-mtls-automtls-workload.yaml
@@ -0,0 +1,30 @@
+apiVersion: security.istio.io/v1beta1
+kind: PeerAuthentication
+metadata:
+  name: "a-disable"
+  annotations:
+    test-suite: "beta-mtls-automtls-workload"
+spec:
+  selector:
+    matchLabels:
+      app: a
+  mtls:
+    mode: DISABLE
+---
+apiVersion: security.istio.io/v1beta1
+kind: PeerAuthentication
+metadata:
+  name: "b-strict"
+  annotations:
+    test-suite: "beta-mtls-automtls-workload"
+spec:
+  selector:
+    matchLabels:
+      app: b
+  mtls:
+    mode: STRICT
+  portLevelMtls:
+    # 8090 is the targetPort for service http port.
+    8090:
+      mode: DISABLE
+---
\ No newline at end of file
diff --git a/tests/integration/security/testdata/beta-mtls-partial-automtls.yaml b/tests/integration/security/testdata/beta-mtls-partial-automtls.yaml
deleted file mode 100644
index e2d94bf28c..0000000000
--- a/tests/integration/security/testdata/beta-mtls-partial-automtls.yaml
+++ /dev/null
@@ -1,28 +0,0 @@
-# No DR is added for this test. enableAutoMtls is expected on by default.
-# Workload-level PeerAuthentication will expectedly cause trouble to connect to the corresponding service
-apiVersion: security.istio.io/v1beta1
-kind: PeerAuthentication
-metadata:
-  name: "default"
-  annotations:
-    test-suite: "beta-mtls-partial-automtls"
-spec:
-  mtls:
-    mode: STRICT
----
-apiVersion: security.istio.io/v1beta1
-kind: PeerAuthentication
-metadata:
-  name: "per-port"
-  annotations:
-    test-suite: "beta-mtls-partial-automtls"
-spec:
-  selector:
-    matchLabels:
-      app: b
-  mtls:
-    mode: DISABLE
-  portLevelMtls:
-    # 8090 is the targetPort for service http(80)
-    8090:
-      mode: STRICT