Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plaintext Endpoints via PeerAuthentication aren't filtered out in multi-network #28798

Closed
stevenctl opened this issue Nov 11, 2020 · 7 comments · Fixed by #30705
Closed

Plaintext Endpoints via PeerAuthentication aren't filtered out in multi-network #28798

stevenctl opened this issue Nov 11, 2020 · 7 comments · Fixed by #30705
Labels
area/security feature/Multi-cluster issues related with multi-cluster support lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed

Comments

@stevenctl
Copy link
Contributor

stevenctl commented Nov 11, 2020

We filter out non-tls endpoints from cross-network LoadBalancing. Without TLS we don't have SNI, required to do routing at the gateway.

if tlsMode := envoytransportSocketMetadata(lbEp, "tlsMode"); tlsMode == model.DisabledTLSModeLabel {
// dont allow cross-network endpoints for uninjected traffic
continue
}

As part of #28621 and #28720 we're seeing the test apply the following PeerAuthentications, but the unreachable endpoints aren't being removed from the LoadBalancing rotation.

Global mtls disable
# mTLS is disabled without destination rule.
apiVersion: "security.istio.io/v1beta1"
kind: "PeerAuthentication"
metadata:
  name: "default"
  annotations:
    test-suite: "beta-mtls-off"
spec:
  mtls:
    mode: DISABLE
Port level mtls disable
apiVersion: "security.istio.io/v1beta1"
kind: "PeerAuthentication"
metadata:
  name: "mtls"
spec:
  selector:
    matchLabels:
      app: server
  mtls:
    mode: STRICT
  portLevelMtls:
    8090:
      mode: DISABLE
    8092:
      mode: DISABLE
---
apiVersion: networking.istio.io/v1alpha3
kind: DestinationRule
metadata:
  name: server
spec:
  host: server.%s.svc.cluster.local
  trafficPolicy:
    tls:
      mode: ISTIO_MUTUAL
    portLevelSettings:
    - port:
        number: 8090
      tls:
        mode: DISABLE
    - port:
        number: 8092
      tls:
        mode: DISABLE

Does the PeerAuthentication set the tlsMode early enough in our EDS code to be filtered out properly?

@stevenctl
Copy link
Contributor Author

cc @jtrbs

@stevenctl stevenctl added area/security feature/Multi-cluster issues related with multi-cluster support labels Nov 11, 2020
@jtrbs
Copy link
Contributor

jtrbs commented Nov 11, 2020

Related issue #28766

@yangminzhu
Copy link
Contributor

cc @incfly

@incfly
Copy link

incfly commented Dec 17, 2020

I clicked through the link and having a bit trouble to connect the dots.

 if tlsMode := envoytransportSocketMetadata(lbEp, "tlsMode"); tlsMode == model.DisabledTLSModeLabel { 

This line is only looking at the endpoint label. The label is the result of the injection. And it's not taking PeerAuthenticatoin into account.

but the unreachable endpoints aren't being removed from the LoadBalancing rotation.

What are the characteristics of those enpdoints? With sidecar injected but configured as plaintext, therefore cross cluster traffic fails?

@stevenctl
Copy link
Contributor Author

Yes, we require TLS for SNI routing through the gateway. This doesn't fail for cross-cluster, single-network. Only cross-network traffic fails.

@incfly
Copy link

incfly commented Dec 17, 2020

I see. To answer the question, yes, I think we gather the PeerAuthentication config at the time when push context is initiated. If we consult that, it should be able to tell whether endpoints are plaintext or not.

if ps.AuthnBetaPolicies, initBetaPolicyErro = initAuthenticationPolicies(env); initBetaPolicyErro != nil {

Though I would look into how the client side auto mTLS decision is made, in cluster.go,

tls, mtlsCtxType = buildAutoMtlsSettings(tls, opts.serviceAccounts, opts.istioMtlsSni, opts.proxy,

trying to reuse as much as we can.

@stevenctl
Copy link
Contributor Author

So if i understand correctly this is mostly related to the TLSContext on the cluster, not info directly on the lb endpoints. I'll have to look into a way to reference that in the eds flow with minimal recomputation.

I did notice that we surround the buildAutoMtlsSettings call with a check that we're not in sni-dnat mode.

if opts.clusterMode != SniDnatClusterMode && opts.direction != model.TrafficDirectionInbound {

We want to filter these non-tls endpoints from the logic that converts endpoint address to gateway address, but we want to filter the endpoints entirely when building eds for the gateways themselves which have ISTIO_META_ROUTER_MODE: sni-dnat

- name: ISTIO_META_ROUTER_MODE
value: "sni-dnat"

Not sure how to handle this without recomputing some of the traffic policy logic inside of eds

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Mar 17, 2021
@stevenctl stevenctl added lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed and removed lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while labels Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security feature/Multi-cluster issues related with multi-cluster support lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants