istio / istio

Connect, secure, control, and observe services.
https://istio.io
Apache License 2.0
35.95k stars 7.76k forks source link

`portLevelMtls` in PeerAuthentication not enforcing strict mode #49098

Closed peterj closed 9 months ago

peterj commented 9 months ago

Is this the right place to submit this?

Bug Description

Using the httpbin (deployed to default ns and part of the mesh) and sleep (deployed to sleep ns, not in the mesh) workloads from the samples folder. Using sidecars.

Apply the following policy:

apiVersion: security.istio.io/v1beta1
kind: PeerAuthentication
metadata:
  name: httpbin
  namespace: default
spec:
  selector:
    matchLabels:
      app: httpbin
  portLevelMtls:
    8000:
      mode: STRICT

Expected Request sent from the sleep pod should not be let through (STRICT mode, calling from non-mesh workload),

Actual The call succeeds:

k exec -it deploy/sleep -n sleep -- curl -v httpbin.default.svc.cluster.local:8000/ip

Note that if I change the port in the peer authentication to 80 - the container port - the setting is applied correctly. The k8s service only declares port 8000.

Version

$ istioctl version
client version: 1.20.2
control plane version: 1.20.2
data plane version: 1.20.2 (4 proxies)

Additional Information

No response

keithmattix commented 9 months ago

This is for sidecar and not Ambient correct?

peterj commented 9 months ago

Yes, this is for sidecar (haven't tried with ambient though)

howardjohn commented 9 months ago

So you have

port: 8000
targetPort: 80

right?

If so, the API applies to targetPort/container ports, not service port, so this is working as expected.

PS: In the future when reporting things along the lines of "this security feature does not work" its best to disclose privately (https://istio.io/latest/docs/releases/security-vulnerabilities/#reporting-a-vulnerability) in the event it is a legitimate security issue.

peterj commented 9 months ago

Yep, that's it. I'll probably add a note to the docs to explicitly call this out. Thinking about it, it now makes sense, but it feels like a lot of people might expect the portLevelMtls to be a port on the service (not the workload).

Thanks for that reminder - I blindly clicked the checkmark on both - my bad.

howardjohn commented 9 months ago

Slightly related: https://istio.io/latest/news/security/istio-security-2021-002/. Not directly the same though

peterj commented 9 months ago

Good point -- I'll ensure the docs explicitly mention the ports in Authpolicies are workload ports, not k8s service ports.

bleggett commented 9 months ago

Just hit this in ambient testing https://github.com/istio/istio/issues/48272 - actually, I am seeing RBAC rejection even with the containerPort allowlisted:

apiVersion: security.istio.io/v1beta1
kind: PeerAuthentication
metadata:
  name: strict-and-permissive-mtls
spec:
  selector:
    matchLabels:
      app: httpbin
  mtls:
    mode: STRICT
  portLevelMtls:
    8080:
      mode: DISABLE
    8000:
      mode: DISABLE
    image: docker.io/kong/httpbin
    imagePullPolicy: IfNotPresent
    name: httpbin
    ports:
    - containerPort: 8080
      protocol: TCP
2024-01-31T20:01:26.687601Z  INFO proxy{uid=e79c5f6c-73b2-4211-bf1f-ce5af17f07fb}: ztunnel::proxy::inbound_passthrough: RBAC rejected conn=10.244.2.10(None)->10.244.2.8:8080

...I might be doing something wrong but that seems incorrect on the ambient side. If it is, will fix and add tests.

howardjohn commented 9 months ago

One minor comment: the literal containerPort field does nothing really. Its about the port the application actually listens on, the field is mostly documentation (exception is for service named ports)

bleggett commented 9 months ago

One minor comment: the literal containerPort field does nothing really. Its about the port the application actually listens on, the field is mostly documentation (exception is for service named ports)

good to know - even so, given the ZT log there, seems incorrect.

bleggett commented 9 months ago

This is still not working correctly in ambient, not until https://github.com/istio/istio/pull/49131 merges anyway