A service configuration can make all the egress HTTPs service inaccessible

[iandyh]

Bug description

Define a service with following definition:

  ports:
  - name: http
    port: 443
    protocol: TCP
    targetPort: 80

This definition will be populated as a 443 route in the sidecar proxy and therefore all the outgoing https traffic will have protocol level errors.

Affected product area (please put an X in all that apply)

[X] Configuration Infrastructure
[ ] Docs
[ ] Installation
[X] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience

Expected behavior
One misconfiguration should not impact existing networking behaviour.

Steps to reproduce the bug

1. Deploy one service with sidecar injected
2. Deploy another service with above service configuration

The service deployed in step 1 cannot access to any external HTTPs service.

Version (include the output of istioctl version --remote and kubectl version)
Istio 1.1.7

How was Istio installed?
No egressgateway installed
mTLS in permissive mode

kubectl get configmap istio -n istio-system -o yaml | grep -o "mode: ALLOW_ANY"

mode: ALLOW_ANY
mode: ALLOW_ANY

Looking at the source code, the behaviour is expected since it will populate isHTTP service into the configuration. But we need to prevent misconfiguration as above. I cannot think of better solution other than hard coding the validation in Galley.

Thank you.

[irisdingbj]

If changed name as `https` like below, will it fix the problem?

  ports:
  - name: https
    port: 443
    protocol: TCP
    targetPort: 80

[iandyh]

@irisdingbj Yes, it will.

But the problem is, this kind of configuration will make all the external HTTPs service access in other namespaces immediately gone and nobody knows why. It actually took us quite a while to understand the problem.

[howardjohn]

I am not sure what we can really do here. We can't reject ij galley because galley doesn't handle deployments. We also probably don't even want to, because it is valid to have an http port on 443 (not a good idea but technically possible right?)

As far as the other namespaces problem you could isolate the scope of the problem with a Sidecar.

Is there anything else you think would be appropriate to mitigate this?

[irisdingbj]

I am not sure what we can really do here. We can't reject ij galley because galley doesn't handle deployments. We also probably don't even want to, because it is valid to have an http port on 443 (not a good idea but technically possible right?)

As far as the other namespaces problem you could isolate the scope of the problem with a Sidecar.

Is there anything else you think would be appropriate to mitigate this?

Wondering whether logging a warning on this case will help or not?

[iandyh]

I am not sure what we can really do here. We can't reject ij galley because galley doesn't handle deployments. We also probably don't even want to, because it is valid to have an http port on 443 (not a good idea but technically possible right?)

As far as the other namespaces problem you could isolate the scope of the problem with a Sidecar.

Is there anything else you think would be appropriate to mitigate this?

I am wondering if there is anything can be done at Envoy level. Because HTTPs traffic should not be handled by the HTTP clusters. As you said, it's entirely possible people can send HTTP traffic over port 443.

From Istio control plane point of view, I don't think it can be handled easily. We have a webhook developed in house so the problem can be avoided.

[iandyh]

@howardjohn I am thinking adding this kind of validation to Galley. Because the right now the named service port requirements actually make will make deployment/service failed. What do you think?

[howardjohn]

what would the validation be? 443 must be https?

[iandyh]

@howardjohn This will be one. Moreover forcing all the services to have named ports(This is the hard requirement for Istio atm anyway.) ref: https://istio.io/docs/setup/kubernetes/prepare/requirements/

[howardjohn]

I may be wrong here but I think the validation we do in galley needs to be universally applicable. There is nothing wrong with specifying http on port 443 (probably a bad idea though). You also don't need a named port, it will just use tcp otherwise.

The port naming stuff is hopefully going away in the near future anyways.

I think it's a great idea for an admission webhook but not sure if it would make sense in galley

[iandyh]

@howardjohn Right now Istio needs to understand the protocol from service.name and then apply relevant filters/plugins (https://github.com/istio/istio/blob/af876f2c754bce9103946ca1597f1c907b5a7f16////pilot/pkg/serviceregistry/kube/conversion.go#L219:6). Unless Kubernetes supports more protocols at service level, I doubt istio can remove the above code any time soon. (I personally don't like it neither)

We are doing this in our in-house webhook but we find it will have hard dependency on istio codebase and I am just thinking maybe it should be added to Istio(Galley) instead.

[howardjohn]

check out the doc "Better Default Networking <https://docs.google.com/document/d/12Z_oFCFn3_rTWbQsuR1GmjU5_WZVkBTJwFpFMx802Ns/edit#heading=h.qex63c29z2to> "

…

On Sun, Jun 16, 2019, 10:40 PM Andy Huang ***@***.***> wrote: @howardjohn <https://github.com/howardjohn> Right now Istio needs to understand the protocol from service.name and then apply relevant filters/plugins ( https://github.com/istio/istio/blob/af876f2c754bce9103946ca1597f1c907b5a7f16////pilot/pkg/serviceregistry/kube/conversion.go#L219:6). Unless Kubernetes supports more protocol level at service level, I doubt istio can remove the above code any time soon. (I personally don't like it neither) We are doing this in our in-house webhook but we find it will have hard dependency on istio codebase and I am just thinking maybe it should be added to Istio(Galley) instead. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14520?email_source=notifications&email_token=AAEYGXKWTM4BHLDQAAERMBTP24PU7A5CNFSM4HSE4CHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX2CZHQ#issuecomment-502541470>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEYGXPGDPVUNRUO7NQ2DXTP24PU7ANCNFSM4HSE4CHA> .

[iandyh]

@howardjohn oh great. Thanks for the link!

[andraxylia]

Closed, will be addressed with changes proposed by Better Default Networking.