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

SSL Passthrough #15

Closed
rusenask opened this issue Oct 31, 2017 · 22 comments
Closed

SSL Passthrough #15

rusenask opened this issue Oct 31, 2017 · 22 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone

Comments

@rusenask
Copy link

Hello, it would be good to know whether Contour supports ssl passthrough and if it doesn't - whether it would be possible/reasonable to add it.

NGINX ingress controller has this ingress.kubernetes.io/ssl-passthrough: "true" annotation, it might make sense to keep the format the same to make it easier to switch between ingresses.

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: contour
    ingress.kubernetes.io/ssl-passthrough: "true"
  name: relay-ingress
  namespace: default
@rothgar
Copy link
Contributor

rothgar commented Oct 31, 2017

related to #14

@davecheney davecheney added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 1, 2017
@davecheney
Copy link
Contributor

@rusenask Thanks for raising this issue. I need to ask a few clarifying questions to check I understand what a successful outcome of this issue would look like.

q. In this scenario who is doing TLS processing? Is it the load balancer in front of Contour? Is it Contour? Is it the service the ingress points to? Is it two or maybe all three?

q. If TLS processing is being done by the final k8s service, it feels like you'd want to configure Envoy as a TCP proxy, not a HTTPS proxy. Did I get that right?

@rusenask
Copy link
Author

rusenask commented Nov 1, 2017

@rothgar that issue should probably be configured through another annotation:
ingress.kubernetes.io/secure-backend: "false/true"

@davecheney

q. In this scenario who is doing TLS processing? Is it the load balancer in front of Contour? Is it Contour? Is it the service the ingress points to? Is it two or maybe all three?

Service that ingress points to. LB and Contour would just pass through traffic to the destination, no termination should happen.

q. If TLS processing is being done by the final k8s service, it feels like you'd want to configure Envoy as a TCP proxy, not a HTTPS proxy. Did I get that right?

Not sure about Envoy TCP proxy but what I disliked in nginx-ingress was that if you set it to TCP proxy then it can't match hostnames anymore, it's more like 8080:namespace/serviceName:port:

apiVersion: v1
kind: ConfigMap
metadata:
  name: tcp-configmap
  namespace: kube-system
data:
  8080: "default/whr:8080"  

Desired behaviour in this case would be that you could still route based on hostnames to different services and ports.

I hope this makes sense.

@davecheney
Copy link
Contributor

Service that ingress points to. LB and Contour would just pass through traffic to the destination, no termination should happen.

Ingress objects are strongly coupled to HTTP. This sounds like a good use case for using a service load balancer directly. Have you tried that? What are the reasons why you decided not to use this approach?

Not sure about Envoy TCP proxy but what I disliked in nginx-ingress was that if you set it to TCP proxy then it can't match hostnames anymore, it's more like 8080:namespace/serviceName:port:

If you're in TCP mode, then i'm guessing there is no HTTP request to inspect to route the traffic. It sounds like you have to dedicate the public IP to forwarding the traffic to only one service, which is very similar to the service load balancer described above.

While I think Envoy can do this, I have some doubts about how to express this in the limitations of the Ingress object.

@davecheney
Copy link
Contributor

@rusenask ping, i'd like to keep this conversation going if you have the time.

@rusenask
Copy link
Author

rusenask commented Nov 8, 2017

@davecheney hey, apologies.

TLS functionality would be nice so you could mix HTTP and HTTPS (passthrough) services in one ingress.

I know that you could achieve the same thing with multiple service load balancers but having it in on ingress saves you one IP (atleast in GKE) and having external load balancers in GKE costs a bit.

At least in nginx-ingress it have this and I thought it's quite nice as you can have HTTP+HTTPS+GRPC services defined in one ingress. The only downside is that nginx keeps killing long connections. I don't need contour to provision TLS certs as I request them from my backend.

@davecheney
Copy link
Contributor

Thanks for your reply. I'll be honest, I'm not sure I understand what you're asking for, but I do understand supporting the

ingress.kubernetes.io/ssl-passthrough: "true"

annotation. So I'll work on that and we can see if there is anything left to do afterwards.

@davecheney
Copy link
Contributor

The way I think this can be accomplished is to use a tcp proxy filter on the tls listener.

There are a number of caveats with this approach:

  • It relies on TLS/SNI for host name routing. You can't do this on normal port 80 without binding to a new port.
  • Envoy must do TLS negotiation before making the backend connection. I don't know how to forward the TLS negotiation to the cluster backend, that seems to defeat the SSL spec. The backend service will have a TLS connection, but that TLS connection will be between envoy and itself, not the remote client.

If someone wants to attempt this, I'll mark it for 0.5, but I think the limitations are severe enough that you'd be better off using a service loadbalancer in TCP mode, you're not getting much value by adding Envoy into the mix.

@davecheney davecheney added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Mar 13, 2018
@davecheney davecheney added this to the 0.5.0 milestone Mar 13, 2018
@davecheney
Copy link
Contributor

I'm going to remove the milestone from this as i'm not sure how to implement it within the framework that the k8s Ingress document provides.

@davecheney davecheney removed this from the 0.5.0 milestone Apr 17, 2018
@davecheney davecheney added p3 - Wouldn't It Be Nice If... and removed priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Apr 17, 2018
@roadrunner
Copy link

this is only supported by Haproxy with L4 TCP SNI inspection.

frontend fc_tls
  bind *:443
  mode tcp
  tcp-request inspect-delay 3s
  tcp-request content accept if { req_ssl_hello_type 1 }
  use_backend abc-tls if { req_ssl_sni -i abc.example.org }
  use_backend xyz-tls if { req_ssl_sni -i xyz.example.org }
  default_backend https-back

currently Envoy doesn't support this feature but there is an old issue still active and @PiotrSikora working on a PR..

@davecheney
Copy link
Contributor

Blocked on envoyproxy/envoy#1843

@davecheney davecheney removed the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label May 10, 2018
@glerchundi
Copy link
Contributor

glerchundi commented Jun 26, 2018

Now that envoyproxy/envoy#1843 was already addressed and released on Envoy 1.7.0, it can be done once #443 gets fixed. Before doing anything and knowing that Contour is taking the use IngressRoute CRD approach (thus not polluting standard ingress with a myriad of annotations), what about something like this?

spec:
  virtualhost:
    fqdn: "www.google.com"
    tls:
      passthrough: true

I don't know if it's even possible but appears to be easily understandable.

@rosskukulinski rosskukulinski added this to the 0.7.0 milestone Aug 17, 2018
@rosskukulinski rosskukulinski added the blocked/needs-product Categorizes the issue or PR as blocked because it needs a decision from product management. label Aug 17, 2018
@glerchundi
Copy link
Contributor

@davecheney, @rosskukulinski can we push this forward knowing that IngressRoute (project) is already more or less well-established?

@davecheney
Copy link
Contributor

Thanks for the ping. I recently finished the upgrade of Envoy to 1.7.0 for the 0.7 milestone so I think we have all the raw materials needed to do this.

However,

spec:
  virtualhost:
    fqdn: "www.google.com"
    tls:
      passthrough: true

This isn't the only config that would change. K8s services, what envoy calls clusters, are tired implicitly to routes in the ingressroute spec. Some thought about how to change the routes... section of the spect will probably be needed.

@davecheney davecheney modified the milestones: 0.7.0, 0.8.0 Oct 23, 2018
@davecheney davecheney added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed blocked/needs-product Categorizes the issue or PR as blocked because it needs a decision from product management. labels Oct 29, 2018
@glerchundi
Copy link
Contributor

As it seems to be declared as out-of-scope in #787, when do you think would be the best release/time window to implement this? I'm open to collaborate on this.

@davecheney
Copy link
Contributor

I know you've been waiting a long time for this so I want to try to give you the most complete explanation of the current status.

#787 will add TCP port forwarding for TLS connections. They will be decrypted at the Envoy edge and the SNI handshake used to direct the unencrypted traffic to the target pods.

This ticket is a request to add nginx's ssl-passthrough option. Having read the docs for this feature, https://kubernetes.github.io/ingress-nginx/user-guide/tls/#ssl-passthrough, I believe that #787 will add the support to Contour to enable this.

However, envoyproxy/envoy#1843, suggests that it is possible to do the SNI handshake, but somehow forward the encrypted traffic to the backend service. From my understanding of the nginx documentation this is not now ssl-passthrough works. But for completeness the nginx docs don't specify if the traffic inside the cluster is decrypted or not.

Given the ambiguity, I forwarding encrypted traffic through Envoy, what I believe this ticket is asking for, is not in scope for Contour 0.8, but obviously doing most of this plumbing will make it possible to add it in the future.

@PiotrSikora
Copy link

@davecheney ssl-passthrough is implemented by peeking at the TLS ClientHello message without consuming it (i.e. without performing TLS handshake at the proxy), and simply forwarding encrypted TCP packets between TLS endpoints, in both: Envoy (using TLS Inspector) and NGINX (using ngx_stream_ssl_preread_module).

The only difference between this and #787 (assuming you're using filter chain matching in Envoy) is whether or not tls_context is included in the config.

@davecheney
Copy link
Contributor

@PiotrSikora thanks for confirming.

@glerchundi
Copy link
Contributor

Cannot explain better than how @PiotrSikora did, thanks! And also for the tip that the absence of tls_context makes envoy work as ssl passthrough.

@davecheney davecheney modified the milestones: 0.8.0, 0.9.0 Nov 13, 2018
@glerchundi
Copy link
Contributor

glerchundi commented Dec 19, 2018

@davecheney just to let you know that we're going to start testing this is the next days.

I came across with a patch that should work. Obviously this wouldn't work without the invaluable hint from @PiotrSikora and with your fantastic work on making TCP proxying / forwarding a reality.

Here it is:

From 865201dacdea553f30d1d8bc0f114275c3392bfa Mon Sep 17 00:00:00 2001
From: Gorka Lerchundi Osa <g.lerchundi@saltosystems.com>
Date: Wed, 19 Dec 2018 22:02:27 +0100
Subject: [PATCH] tcpproxy: add tls passthrough

---
 internal/contour/listener.go | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/internal/contour/listener.go b/internal/contour/listener.go
index a13caca..1ae9b85 100644
--- a/internal/contour/listener.go
+++ b/internal/contour/listener.go
@@ -16,12 +16,13 @@ package contour
 import (
 	"sync"
 
-	"github.com/envoyproxy/go-control-plane/envoy/api/v2"
+	v2 "github.com/envoyproxy/go-control-plane/envoy/api/v2"
+	"github.com/envoyproxy/go-control-plane/envoy/api/v2/auth"
 	"github.com/envoyproxy/go-control-plane/envoy/api/v2/listener"
 	"github.com/gogo/protobuf/proto"
 	"github.com/heptio/contour/internal/dag"
 	"github.com/heptio/contour/internal/envoy"
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 )
 
 const (
@@ -230,11 +231,6 @@ func (v *listenerVisitor) visit(vertex dag.Vertex) {
 		// the listener properly.
 		v.http = true
 	case *dag.SecureVirtualHost:
-		data := vh.Data()
-		if data == nil {
-			// no secret for this vhost, skip it
-			return
-		}
 		filters := []listener.Filter{
 			envoy.HTTPConnectionManager(ENVOY_HTTPS_LISTENER, v.httpsAccessLog()),
 		}
@@ -246,11 +242,15 @@ func (v *listenerVisitor) visit(vertex dag.Vertex) {
 			alpnProtos = nil // do not offer ALPN
 		}
 
+		var tlsContext *auth.DownstreamTlsContext
+		if data := vh.Data(); data != nil {
+			tlsContext = envoy.DownstreamTLSContext(data[v1.TLSCertKey], data[v1.TLSPrivateKeyKey], vh.MinProtoVersion, alpnProtos...)
+		}
 		fc := listener.FilterChain{
 			FilterChainMatch: &listener.FilterChainMatch{
 				ServerNames: []string{vh.Host},
 			},
-			TlsContext:    envoy.DownstreamTLSContext(data[v1.TLSCertKey], data[v1.TLSPrivateKeyKey], vh.MinProtoVersion, alpnProtos...),
+			TlsContext:    tlsContext,
 			Filters:       filters,
 			UseProxyProto: bv(v.UseProxyProto),
 		}
-- 
2.20.1

Will tell you something with our conclusions or problems we encounter on the road.

In case someone is interested this is already implemented & published in: docker.io/glerchundi/contour:v0.8.1-cors_tlspassthrough.

@davecheney
Copy link
Contributor

@glerchundi thank you for working on this. I need you to not set patches but signed PRs. Sorry this is a lawyer thing.

@glerchundi
Copy link
Contributor

glerchundi commented Dec 19, 2018 via email

glerchundi added a commit to glerchundi/contour that referenced this issue Jan 13, 2019
Allow an IngressRoute for TCP forwarding but without a `tls` section
which implicitly configures it as a TLS Passthrough. TLS Passthrough
only reads the very first TLS packets and routes the traffic without
doing any kind of TLS termination. The chosen `virtualhost` is
discovered by reading the TLS SNI (Server Name Indication) extension.

Close projectcontour#15
glerchundi added a commit to glerchundi/contour that referenced this issue Jan 13, 2019
Allow an IngressRoute for TCP forwarding but without a `tls` section
which implicitly configures it as a TLS Passthrough. TLS Passthrough
only reads the very first TLS packets and routes the traffic without
doing any kind of TLS termination. The chosen `virtualhost` is
discovered by reading the TLS SNI (Server Name Indication) extension.

Closes projectcontour#15
glerchundi added a commit to glerchundi/contour that referenced this issue Jan 13, 2019
Allow an IngressRoute for TCP forwarding but without a `tls` section
which implicitly configures it as a TLS Passthrough. TLS Passthrough
only reads the very first TLS packets and routes the traffic without
doing any kind of TLS termination. The chosen `virtualhost` is
discovered by reading the TLS SNI (Server Name Indication) extension.

Closes projectcontour#15

Signed-off-by: Gorka Lerchundi Osa <glertxundi@gmail.com>
lrouquette pushed a commit to lrouquette/contour that referenced this issue Jan 29, 2020
…nit-tests-adobe

unit tests for adobe customization
sunjayBhatia pushed a commit that referenced this issue Jan 30, 2023
Using `(*testing.T)Run()` to run named subtest removed a bit of test
boilerpplate, and makes it possible to run individual test cases.

Signed-off-by: James Peach <jpeach@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

No branches or pull requests

7 participants