[Defect] Need ability to support SNI

[sandy-adi]

Currently when zuul routes to origins over https, it creates an SSLHandler without setting the SNI. This works for most cases, but for cases where the service may require SNI the SSL handshake fails. Here's the snippet from the initChannel method of DefaultOriginChannelInitializer

if (connectionPoolConfig.isSecure()) {
   pipeline.addLast("ssl", sslContext.newHandler(ch.alloc()));
}

if this can be updated to something along the lines below, it will help route to origins that need the SNI

SslHandler sslHandler = null;
if (connectionPoolConfig.isSecure()) {
    String sni = connectionPoolConfig.getSNI();
    if (sni != null) {
        int securePort = connectionPoolConfig.getSecurePort();
        sslHandler = sslContext.newHandler(ch.alloc(), sni, securePort);
    }else {
        sslHandler = sslContext.newHandler(ch.alloc());
    }
    pipeline.addLast("ssl", sslHandler);
}

Please let me know if this looks like the right approach, We've an issue right now because of this and would like to address it as soon as possible.

[artgon]

What's the use case for having SNI on the client side?

[sandy-adi]

What's the use case for having SNI on the client side?

oh no the client doesn't need it. The service that is being proxied via zuul needs it. So we need the ability to set the peer host when creating the ssl handler.

[artgon]

I'm still not clear on the use case. You want to proxy the SNI bytes from the incoming request to the outbound request? Why does your backend need SNI?

[sandy-adi]

The backend servers are behind a load balancer. This is a common use case where the client has to send in the SNI to the server.
In this specific case let's say the request would come in at ourproxy.com the origin servers are behind a load balancer originservers.com on port 8443. The SSL handshake from our zuul instances is failing because of this. In order to overcome this we will have to set the peer host when creating the ssl handler as shown in the example above.

[sandy-adi]

@artgon did that make sense?

[artgon]

It's interesting that your backends are also behind load balancers. Is that in lieu of a discovery mechanism? I can see what you're trying to do but it's not a use that we're planning to support.

[artgon]

We do, however, plan to support SNI on the front end. This will allow you to terminate multiple TLS certs on the same port.

[sandy-adi]

It's interesting that your backends are also behind load balancers. Is that in lieu of a discovery mechanism? I can see what you're trying to do but it's not a use that we're planning to support.

most of our backends do not use eureka for service discovery, they are either existing services or new services on Kubernetes. We are basically running zuul in stand alone mode as our edge gateway. All east west traffic is handled within kubernetes or other solutions. I understand you don't have plans of supporting it. Could you please take a look at #739 this doesn't change the existing behavior but gives us the ability to extend and customize the Default behavior.

[artgon]

I think you can solve this without having to send us a PR. You're already on the right track, but essentially this is what you need to do in your app (which is what we do in closed-source Zuul as well):

1. Create your own OriginManager (e.g. BasicNettyOriginManager) and bind it in your app
2. Create your own NettyOrigin (e.g. BasicNettyOrigin) that gets initialized in the manager above
3. Create your own DefaultClientChannelManager that gets initialized in the Origin class above
4. Create your own DefaultOriginChannelInitializer that sets SNI bytes on the sslContext

Once you have these 4 pieces in place, you can customize any piece of the origin side of the request. We do this internally for all kinds of custom features that make sense for Netflix but wouldn't necessarily make sense for the OSS project.

[sandy-adi]

I think you can solve this without having to send us a PR. You're already on the right track, but essentially this is what you need to do in your app (which is what we do in closed-source Zuul as well):

1. Create your own OriginManager (e.g. BasicNettyOriginManager) and bind it in your app
2. Create your own NettyOrigin (e.g. BasicNettyOrigin) that gets initialized in the manager above
3. Create your own DefaultClientChannelManager that gets initialized in the Origin class above
4. Create your own DefaultOriginChannelInitializer that sets SNI bytes on the sslContext

Once you have these 4 pieces in place, you can customize any piece of the origin side of the request. We do this internally for all kinds of custom features that make sense for Netflix but wouldn't necessarily make sense for the OSS project.

we are doing that right now actually. The problem is our pieces are pretty much a copy of the default classes except for the small piece that creates the sslHandler. We would like to make sure we get any default behavior changes in the future without having to copy over the code to our new components. Not sure if you looked at the PR, but it would be greatly appreciated and very helpful to us from a maintenance point of view if you can accept the changes. All we are doing is creating accessor methods to enable overrides where needed.

[sandy-adi]

@artgon hope all is well...

Any thoughts regarding

I think you can solve this without having to send us a PR. You're already on the right track, but essentially this is what you need to do in your app (which is what we do in closed-source Zuul as well):

1. Create your own OriginManager (e.g. BasicNettyOriginManager) and bind it in your app
2. Create your own NettyOrigin (e.g. BasicNettyOrigin) that gets initialized in the manager above
3. Create your own DefaultClientChannelManager that gets initialized in the Origin class above
4. Create your own DefaultOriginChannelInitializer that sets SNI bytes on the sslContext

Once you have these 4 pieces in place, you can customize any piece of the origin side of the request. We do this internally for all kinds of custom features that make sense for Netflix but wouldn't necessarily make sense for the OSS project.

we are doing that right now actually. The problem is our pieces are pretty much a copy of the default classes except for the small piece that creates the sslHandler. We would like to make sure we get any default behavior changes in the future without having to copy over the code to our new components. Not sure if you looked at the PR, but it would be greatly appreciated and very helpful to us from a maintenance point of view if you can accept the changes. All we are doing is creating accessor methods to enable overrides where needed.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.