Skip to content

Commit

Permalink
Do not enable auto mtls when cluster type is Cluster_ORIGINAL_DST (#…
Browse files Browse the repository at this point in the history 
…24319) (#25829)

* disable tls for headless service when mtls is auto detected

* move it to conditionallyConvertToIstioMtls

* cleanup

* fix e2e
  • Loading branch information
@hzxuzhonghu
hzxuzhonghu committed Jul 24, 2020
1 parent d1a6a17 commit 6779bb6
Show file tree
Hide file tree
Showing 7 changed files with 97 additions and 55 deletions.
10 changes: 8 additions & 2 deletions pilot/pkg/networking/core/v1alpha3/cluster.go
View file
Expand Up @@ -538,12 +538,17 @@ func conditionallyConvertToIstioMtls(
autoMTLSEnabled bool,
meshExternal bool,
serviceMTLSMode model.MutualTLSMode,
) (*networking.ClientTLSSettings, mtlsContextType) {
clusterDiscoveryType apiv2.Cluster_DiscoveryType) (*networking.ClientTLSSettings, mtlsContextType) {
mtlsCtx := userSupplied
if tls == nil {
if meshExternal || !autoMTLSEnabled || serviceMTLSMode == model.MTLSUnknown || serviceMTLSMode == model.MTLSDisable {
return nil, mtlsCtx
}
// Do not enable auto mtls when cluster type is `Cluster_ORIGINAL_DST`
// We don't know whether headless service instance has sidecar injected or not.
if clusterDiscoveryType == apiv2.Cluster_ORIGINAL_DST {
return nil, mtlsCtx
}

mtlsCtx = autoDetected
// we will setup transport sockets later
Expand Down Expand Up @@ -719,7 +724,7 @@ func applyTrafficPolicy(opts buildClusterOpts) {
autoMTLSEnabled := opts.push.Mesh.GetEnableAutoMtls().Value
var mtlsCtxType mtlsContextType
tls, mtlsCtxType = conditionallyConvertToIstioMtls(tls, opts.serviceAccounts, opts.istioMtlsSni, opts.proxy,
autoMTLSEnabled, opts.meshExternal, opts.serviceMTLSMode)
autoMTLSEnabled, opts.meshExternal, opts.serviceMTLSMode, opts.cluster.GetType())
applyUpstreamTLSSettings(&opts, tls, mtlsCtxType, opts.proxy)
}
}
Expand Down Expand Up @@ -966,6 +971,7 @@ func applyUpstreamTLSSettings(opts *buildClusterOpts, tls *networking.ClientTLSS

cluster := opts.cluster
proxy := opts.proxy

certValidationContext := &auth.CertificateValidationContext{}
var trustedCa *core.DataSource
if len(tls.CaCertificates) != 0 {
Expand Down
52 changes: 32 additions & 20 deletions pilot/pkg/networking/core/v1alpha3/cluster_test.go
View file
Expand Up @@ -857,24 +857,25 @@ func TestConditionallyConvertToIstioMtls(t *testing.T) {
Sni: "custom.foo.com",
}
tests := []struct {
name string
tls *networking.ClientTLSSettings
sans []string
sni string
proxy *model.Proxy
autoMTLSEnabled bool
meshExternal bool
serviceMTLSMode model.MutualTLSMode
want *networking.ClientTLSSettings
wantCtxType mtlsContextType
name string
tls *networking.ClientTLSSettings
sans []string
sni string
proxy *model.Proxy
autoMTLSEnabled bool
meshExternal bool
serviceMTLSMode model.MutualTLSMode
clusterDiscoveryType apiv2.Cluster_DiscoveryType
want *networking.ClientTLSSettings
wantCtxType mtlsContextType
}{
{
"Destination rule TLS sni and SAN override",
tlsSettings,
[]string{"spiffe://foo/serviceaccount/1"},
"foo.com",
&model.Proxy{Metadata: &model.NodeMetadata{}},
false, false, model.MTLSUnknown,
false, false, model.MTLSUnknown, apiv2.Cluster_EDS,
tlsSettings,
userSupplied,
},
Expand All @@ -891,7 +892,7 @@ func TestConditionallyConvertToIstioMtls(t *testing.T) {
[]string{"spiffe://foo/serviceaccount/1"},
"foo.com",
&model.Proxy{Metadata: &model.NodeMetadata{}},
false, false, model.MTLSUnknown,
false, false, model.MTLSUnknown, apiv2.Cluster_EDS,
&networking.ClientTLSSettings{
Mode: networking.ClientTLSSettings_ISTIO_MUTUAL,
CaCertificates: constants.DefaultRootCert,
Expand All @@ -912,7 +913,7 @@ func TestConditionallyConvertToIstioMtls(t *testing.T) {
TLSClientKey: "/custom/key.pem",
TLSClientRootCert: "/custom/root.pem",
}},
false, false, model.MTLSUnknown,
false, false, model.MTLSUnknown, apiv2.Cluster_EDS,
&networking.ClientTLSSettings{
Mode: networking.ClientTLSSettings_ISTIO_MUTUAL,
CaCertificates: "/custom/root.pem",
Expand All @@ -929,7 +930,7 @@ func TestConditionallyConvertToIstioMtls(t *testing.T) {
[]string{"spiffe://foo/serviceaccount/1"},
"foo.com",
&model.Proxy{Metadata: &model.NodeMetadata{}},
true, false, model.MTLSStrict,
true, false, model.MTLSStrict, apiv2.Cluster_EDS,
&networking.ClientTLSSettings{
Mode: networking.ClientTLSSettings_ISTIO_MUTUAL,
CaCertificates: constants.DefaultRootCert,
Expand All @@ -946,7 +947,7 @@ func TestConditionallyConvertToIstioMtls(t *testing.T) {
[]string{"spiffe://foo/serviceaccount/1"},
"foo.com",
&model.Proxy{Metadata: &model.NodeMetadata{}},
true, false, model.MTLSPermissive,
true, false, model.MTLSPermissive, apiv2.Cluster_EDS,
&networking.ClientTLSSettings{
Mode: networking.ClientTLSSettings_ISTIO_MUTUAL,
CaCertificates: constants.DefaultRootCert,
Expand All @@ -963,7 +964,7 @@ func TestConditionallyConvertToIstioMtls(t *testing.T) {
[]string{"spiffe://foo/serviceaccount/1"},
"foo.com",
&model.Proxy{Metadata: &model.NodeMetadata{}},
true, false, model.MTLSDisable,
true, false, model.MTLSDisable, apiv2.Cluster_EDS,
nil,
userSupplied,
},
Expand All @@ -973,7 +974,7 @@ func TestConditionallyConvertToIstioMtls(t *testing.T) {
[]string{"spiffe://foo/serviceaccount/1"},
"foo.com",
&model.Proxy{Metadata: &model.NodeMetadata{}},
true, false, model.MTLSUnknown,
true, false, model.MTLSUnknown, apiv2.Cluster_EDS,
nil,
userSupplied,
},
Expand All @@ -983,7 +984,7 @@ func TestConditionallyConvertToIstioMtls(t *testing.T) {
[]string{"spiffe://foo/serviceaccount/1"},
"foo.com",
&model.Proxy{Metadata: &model.NodeMetadata{}},
true, true, model.MTLSUnknown,
true, true, model.MTLSUnknown, apiv2.Cluster_EDS,
nil,
userSupplied,
},
Expand All @@ -993,15 +994,26 @@ func TestConditionallyConvertToIstioMtls(t *testing.T) {
[]string{"spiffe://foo/serviceaccount/1"},
"foo.com",
&model.Proxy{Metadata: &model.NodeMetadata{}},
false, false, model.MTLSDisable,
false, false, model.MTLSDisable, apiv2.Cluster_EDS,
nil,
userSupplied,
},
{
"Do not enable auto mtls when cluster type is `Cluster_ORIGINAL_DST`",
nil,
[]string{"spiffe://foo/serviceaccount/1"},
"foo.com",
&model.Proxy{Metadata: &model.NodeMetadata{}},
true, false, model.MTLSPermissive, apiv2.Cluster_ORIGINAL_DST,
nil,
userSupplied,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
gotTLS, gotCtxType := conditionallyConvertToIstioMtls(tt.tls, tt.sans, tt.sni, tt.proxy, tt.autoMTLSEnabled, tt.meshExternal, tt.serviceMTLSMode)
gotTLS, gotCtxType := conditionallyConvertToIstioMtls(tt.tls, tt.sans, tt.sni, tt.proxy,
tt.autoMTLSEnabled, tt.meshExternal, tt.serviceMTLSMode, tt.clusterDiscoveryType)
if !reflect.DeepEqual(gotTLS, tt.want) {
t.Errorf("cluster TLS does not match exppected result want %#v, got %#v", tt.want, gotTLS)
}
Expand Down
4 changes: 4 additions & 0 deletions tests/integration/pilot/cni/cni_test.go
View file
Expand Up @@ -80,6 +80,10 @@ func TestCNIReachability(t *testing.T) {
Namespace: systemNM,
RequiredEnvironment: environment.Kube,
Include: func(src echo.Instance, opts echo.CallOptions) bool {
// Exclude headless naked service, because it is no sidecar
if src == rctx.HeadlessNaked || opts.Target == rctx.HeadlessNaked {
return false
}
// Exclude calls to the headless TCP port.
if opts.Target == rctx.Headless && opts.PortName == "tcp" {
return false
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/security/mtls_first_party_jwt/strict_test.go
View file
Expand Up @@ -47,7 +47,7 @@ func TestMtlsStrictK8sCA(t *testing.T) {
// Exclude calls to the headless service.
// Auto mtls does not apply to headless service, because for headless service
// the cluster discovery type is ORIGINAL_DST, and it will not apply upstream tls setting
return opts.Target != rctx.Headless
return !rctx.IsHeadless(opts.Target)
},
ExpectSuccess: func(src echo.Instance, opts echo.CallOptions) bool {
// When mTLS is in STRICT mode, DR's TLS settings are default to mTLS so the result would
Expand All @@ -58,7 +58,7 @@ func TestMtlsStrictK8sCA(t *testing.T) {
}

// If source is naked, and destination is not, expect failure.
return !(src == rctx.Naked && opts.Target != rctx.Naked)
return !(rctx.IsNaked(src) && !rctx.IsNaked(opts.Target))
},
},
{
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/security/mtlsk8sca/strict_test.go
View file
Expand Up @@ -47,7 +47,7 @@ func TestMtlsStrictK8sCA(t *testing.T) {
// Exclude calls to the headless service.
// Auto mtls does not apply to headless service, because for headless service
// the cluster discovery type is ORIGINAL_DST, and it will not apply upstream tls setting
return opts.Target != rctx.Headless
return !rctx.IsHeadless(opts.Target)
},
ExpectSuccess: func(src echo.Instance, opts echo.CallOptions) bool {
// When mTLS is in STRICT mode, DR's TLS settings are default to mTLS so the result would
Expand All @@ -58,7 +58,7 @@ func TestMtlsStrictK8sCA(t *testing.T) {
}

// If source is naked, and destination is not, expect failure.
return !(src == rctx.Naked && opts.Target != rctx.Naked)
return !(rctx.IsNaked(src) && !rctx.IsNaked(opts.Target))
},
},
{
Expand Down
26 changes: 17 additions & 9 deletions tests/integration/security/reachability_test.go
View file
Expand Up @@ -49,13 +49,13 @@ func TestReachability(t *testing.T) {
return true
},
ExpectSuccess: func(src echo.Instance, opts echo.CallOptions) bool {
if src == rctx.Naked && opts.Target == rctx.Naked {
if rctx.IsNaked(src) && rctx.IsNaked(opts.Target) {
// naked->naked should always succeed.
return true
}

// If one of the two endpoints is naked, expect failure.
return src != rctx.Naked && opts.Target != rctx.Naked
return !rctx.IsNaked(src) && !rctx.IsNaked(opts.Target)
},
},
{
Expand All @@ -64,7 +64,7 @@ func TestReachability(t *testing.T) {
RequiredEnvironment: environment.Kube,
Include: func(src echo.Instance, opts echo.CallOptions) bool {
// Exclude calls to the naked app.
return opts.Target != rctx.Naked
return !rctx.IsNaked(opts.Target)
},
ExpectSuccess: func(src echo.Instance, opts echo.CallOptions) bool {
return true
Expand Down Expand Up @@ -103,8 +103,13 @@ func TestReachability(t *testing.T) {
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 neither.
if src == rctx.Naked {
return opts.Target == rctx.Naked
if rctx.IsNaked(src) {
return rctx.IsNaked(opts.Target)
}
// headless service with sidecar injected, global mTLS enabled,
// no client side transport socket or transport_socket_matches since it's headless service.
if src != rctx.Headless && opts.Target == rctx.Headless {
return false
}
return true
},
Expand All @@ -119,10 +124,14 @@ func TestReachability(t *testing.T) {
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 src == rctx.Naked {
return opts.Target == rctx.Naked || (opts.Target == rctx.B && opts.PortName != "http")
if rctx.IsNaked(src) {
return rctx.IsNaked(opts.Target) || (opts.Target == rctx.B && opts.PortName != "http")

}
// headless with sidecar injected, global mTLS enabled, no client side transport socket or transport_socket_matches since it's headless service.
if src != rctx.Headless && opts.Target == rctx.Headless {
return false
}
// PeerAuthentication disable mTLS for workload app:b, except http port. Thus, autoMTLS
// will fail on all ports on b, except http port.
return opts.Target != rctx.B || opts.PortName == "http"
Expand All @@ -134,10 +143,9 @@ func TestReachability(t *testing.T) {
RequiredEnvironment: environment.Kube,
Include: func(src echo.Instance, opts echo.CallOptions) bool {
// Exclude calls to the headless TCP port.
if opts.Target == rctx.Headless && opts.PortName == "tcp" {
if rctx.IsHeadless(opts.Target) && opts.PortName == "tcp" {
return false
}

return true
},
ExpectSuccess: func(src echo.Instance, opts echo.CallOptions) bool {
Expand Down
52 changes: 32 additions & 20 deletions tests/integration/security/util/reachability/context.go
View file
Expand Up @@ -61,14 +61,15 @@ type TestCase struct {

// Context is a context for reachability tests.
type Context struct {
ctx framework.TestContext
g galley.Instance
p pilot.Instance
Namespace namespace.Instance
A, B echo.Instance
Multiversion echo.Instance
Headless echo.Instance
Naked echo.Instance
ctx framework.TestContext
g galley.Instance
p pilot.Instance
Namespace namespace.Instance
A, B echo.Instance
Multiversion echo.Instance
Headless echo.Instance
Naked echo.Instance
HeadlessNaked echo.Instance
}

// CreateContext creates and initializes reachability context.
Expand All @@ -78,7 +79,7 @@ func CreateContext(ctx framework.TestContext, g galley.Instance, p pilot.Instanc
Inject: true,
})

var a, b, multiVersion, headless, naked echo.Instance
var a, b, multiVersion, headless, naked, headlessNaked echo.Instance
cfg := util.EchoConfig("multiversion", ns, false, nil, g, p)
cfg.Subsets = []echo.SubsetConfig{
// Istio deployment, with sidecar.
Expand All @@ -98,18 +99,21 @@ func CreateContext(ctx framework.TestContext, g galley.Instance, p pilot.Instanc
With(&headless, util.EchoConfig("headless", ns, true, nil, g, p)).
With(&naked, util.EchoConfig("naked", ns, false, echo.NewAnnotations().
SetBool(echo.SidecarInject, false), g, p)).
With(&headlessNaked, util.EchoConfig("headless-naked", ns, true, echo.NewAnnotations().
SetBool(echo.SidecarInject, false), g, p)).
BuildOrFail(ctx)

return Context{
ctx: ctx,
g: g,
p: p,
Namespace: ns,
A: a,
B: b,
Multiversion: multiVersion,
Headless: headless,
Naked: naked,
ctx: ctx,
g: g,
p: p,
Namespace: ns,
A: a,
B: b,
Multiversion: multiVersion,
Headless: headless,
Naked: naked,
HeadlessNaked: headlessNaked,
}
}

Expand Down Expand Up @@ -162,8 +166,8 @@ func (rc *Context) Run(testCases []TestCase) {
time.Sleep(10 * time.Second)
ctx.Logf("[%s] [%v] Finish waiting. Continue testing.", testName, time.Now())

for _, src := range []echo.Instance{rc.A, rc.B, rc.Headless, rc.Naked} {
for _, dest := range []echo.Instance{rc.A, rc.B, rc.Headless, rc.Multiversion, rc.Naked} {
for _, src := range []echo.Instance{rc.A, rc.B, rc.Headless, rc.Naked, rc.HeadlessNaked} {
for _, dest := range []echo.Instance{rc.A, rc.B, rc.Headless, rc.Multiversion, rc.Naked, rc.HeadlessNaked} {
copts := &callOptions
// If test case specified service call options, use that instead.
if c.CallOpts != nil {
Expand Down Expand Up @@ -209,3 +213,11 @@ func (rc *Context) Run(testCases []TestCase) {
})
}
}

func (rc *Context) IsNaked(i echo.Instance) bool {
return i == rc.HeadlessNaked || i == rc.Naked
}

func (rc *Context) IsHeadless(i echo.Instance) bool {
return i == rc.HeadlessNaked || i == rc.Headless
}

0 comments on commit 6779bb6

Please sign in to comment.