Implement upstream tls backend verification with ca cert and required san #1045

stevesloka · 2019-04-25T04:10:40Z

Fixes #813 by implementing tls backend verification via CA cert as well as an optional subject alt name verification on the cert.

Signed-off-by: Steve Sloka slokas@vmware.com

stevesloka · 2019-04-25T04:18:34Z

I still need to do a full test of this on a live cluster, but wanted to get the code out in front of eyes. One thing I want to do is const the ca.crt in the key unless we go with a dynamic route.

Also, I'm aware the design still isn't merged but thought this was enough to get us started and could easily adapt.

stevesloka · 2019-04-26T00:10:41Z

I've been doing some testing of this and I think it needs some more design thought:

If I specify the tls annotation on the service, I can immediately proxy tls to the upstream without verification. This may not be a desire that administrators want to allow, nothing forces a user to specify the ca cert. I wonder if we should rename the validation struct we added to the route's service and add what functionality the tls annotation is doing. (@davecheney)
If the cert is missing then the same for Make bootstrap mode optional #1 above applies. I think we should also be setting the ingressroute status and reject the change since it's invalid.
I need to further test, but I'm not sure if rotating a cert happened automatically which it should

robbiemcmichael · 2019-04-26T12:59:54Z

internal/dag/builder.go

+func (b *builder) addUpstreamValidation(uv *ingressroutev1.UpstreamValidation, svc *v1.Service) *UpstreamValidation {
+	if uv != nil {
+		val := &UpstreamValidation{
+			Certificate: b.lookupSecret(meta{name: uv.CASecret, namespace: svc.Namespace}),


The current strawman design specifies that the secret should be in the same namespace as the ingress route instead of the service: https://github.com/heptio/contour/pull/1040/files#diff-aa37a99afb27f0131b45f334343232beR70

yes that is correct. You can't reference a service from a different namespace from an IngressRoute and I need a way to see what namespace the IngressRoute is currently located in.

robbiemcmichael · 2019-04-26T13:03:21Z

internal/dag/dag.go

+type UpstreamValidation struct {
+	// Certificate holds a reference to the Secret containing the CA to be used to
+	// verify the upstream connection.
+	Certificate *Secret


I think it would be good for the for the field name to be clear that it's a CA certificate.

Yeah I agree that more clear.

stevesloka · 2019-04-29T15:34:44Z

I think this is ready to go! =)

davecheney

Thank you for working on this. There are a number of minor nits

The major concern I have is CA validation is a property of the path from route to service, it is neither a property of the service, or the route; many routes can point to the same service, and many services can be pointed to by one route.

The dag.Cluster object is intended to hold these properties -- there are others that are currently on service which should not be, we'll have to take those as they come.

davecheney · 2019-05-06T06:12:41Z

apis/contour/v1beta1/ingressroute.go

+	// Name of the Kubernetes secret be used to validate the certificate presented by the backend
+	CACertificate string `json:"caSecret"`
+	// Ket which is expected to be present in the 'subjectAltName' of the presented certificate
+	SubjectName string `json:"subjectName,omitempty"`


Please remove omit empty, I decided at the last minute that validation of the CA is not sufficient. We'll make both of these parameters mandatory in 0.12

davecheney · 2019-05-06T06:12:58Z

docs/ingressroute.md

+This annoation tells Contour which port should be used for the TLS connection.
+In this example, the upstream service is named `https` and uses port `443`.
+Additionally, it is possible for Envoy to verify the backend service's certificate.
+The service of an `IngressRoute` can optionally specify a `validation` struct which has a manditory `caSecret` key as well as an optional `subjectName`.


s/optional/manadatory

davecheney · 2019-05-06T06:14:18Z

internal/dag/builder.go

+		port:               int32(port.IntValue()),
+		strategy:           strategy,
+		healthcheck:        healthcheckToString(hc),
+		upstreamvalidation: upstreamValidationToString(uv),


Sorry, this is not correct. The upstream validation is not a property of the service, it is a propery of the cluster -- the path from the route to the service.

davecheney · 2019-05-06T06:14:32Z

internal/dag/builder.go

+			CACertificate: b.lookupSecret(meta{name: uv.CACertificate, namespace: svc.Namespace}),
+			SubjectName:   uv.SubjectName,
+		}
+		fmt.Println("-------- val found: ", val)


debug code, please delete

davecheney · 2019-05-06T06:15:55Z

internal/dag/builder.go

 		},
 		Protocol: protocol,
 	}
 	b.services[s.toMeta()] = s
 	return s
 }

+func (b *builder) addUpstreamValidation(uv *ingressroutev1.UpstreamValidation, svc *v1.Service) *UpstreamValidation {


please don't call this add, it doesn't add anything to the state of the builder. lookupUpstreamValidation might be a better name, but perhaps a different design is needed.

davecheney · 2019-05-06T06:17:11Z

internal/e2e/cds_test.go

@@ -18,14 +18,14 @@ import (
 	"testing"
 	"time"

-	"github.com/envoyproxy/go-control-plane/envoy/api/v2"
+	v2 "github.com/envoyproxy/go-control-plane/envoy/api/v2"


goimports turds here and elsewhere

Is there a way to disable this? I can turn off the imports, but it's nice to manage for me.

I'm not sure, it just started to happen to me when I upgraded to vim-go 1.20. Maybe we need to file an issue upstream about goimports being silly.

This is apparently by design: golang/go#30051 (comment)

I'll turn off goimports for now and fix up manually.

davecheney · 2019-05-06T06:17:55Z

internal/envoy/auth.go

@@ -47,12 +47,34 @@ var (
 // UpstreamTLSContext creates an auth.UpstreamTlsContext. By default
 // UpstreamTLSContext returns a HTTP/1.1 TLS enabled context. A list of
 // additional ALPN protocols can be provided.
-func UpstreamTLSContext(alpnProtocols ...string) *auth.UpstreamTlsContext {
-	return &auth.UpstreamTlsContext{
+func UpstreamTLSContext(cert []byte, subjaltname []string, alpnProtocols ...string) *auth.UpstreamTlsContext {


davecheney · 2019-05-06T06:20:17Z

internal/envoy/auth.go

+	return context
+}
+
+func validationContext(cert []byte, subjaltname []string) *auth.CommonTlsContext_ValidationContext {


two things

please put the guard clause first; ie

if cert == nil { // no validation required return nil

b. please don't push this logic into validationContext. If its not supposed to be called with cert == nil, then don't call it with cert == nil. The reason I ask this is no validationContext sometimes returns nil depending on its inputs, that means we have nils floating around the data structures, this is how you get java :). We should at all times be following these two rules

Don't use nil to indicate a failure

Reduce the propagation of nils through the data structures.

davecheney · 2019-05-06T06:22:18Z

internal/envoy/cluster.go

@@ -51,6 +60,22 @@ func Cluster(c *dag.Cluster) *v2.Cluster {
 	}
 }

+func upstreamValidationCACert(upstream *dag.HTTPService) []byte {
+	if upstream.UpstreamValidation != nil {


Please change these conditions so that the success case proceeds down the page. The guard clause should be

if upstreamValidation == nil { // right, it's nil, what next return nil } return upstream.UpstreamValidation.CACertificate.Object.Data[CACertificateKey]

davecheney · 2019-05-06T06:23:12Z

internal/envoy/cluster.go

@@ -147,6 +172,12 @@ func Clustername(cluster *dag.Cluster) string {
 		}
 		buf += hc.Path
 	}
+	if uv := service.UpstreamValidation; uv != nil {
+		buf += uv.CACertificate.Object.ObjectMeta.Name
+		if len(uv.SubjectName) > 0 {


you can elide this check

buf += "" is just buf

davecheney · 2019-05-07T03:08:28Z

apis/contour/v1beta1/tlscertificatedelegation.go

@@ -40,7 +40,7 @@ type CertificateDelegation struct {
 // +genclient
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

-// TLSCertificateDelefgation is an TLS Certificate Delegation CRD specificiation.
+// TLSCertificateDelefgation is an TLS CACertificate Delegation CRD specificiation.


Please revert this, I think it was an unintended change.

The comment also has a typo: TLSCertificateDelefgation

davecheney · 2019-05-08T05:41:55Z

@stevesloka ping, this is over to you to respond to review comments. Please ping me when it's ready for the next round of review.

stevesloka · 2019-05-08T20:33:31Z

@davecheney I think this is ready for another look. The travis build keeps failing, not sure why, I'll kick it again in a bit.

subject alt name Signed-off-by: Steve Sloka <slokas@vmware.com>

davecheney

Thank you for working on this. I've addressed the comments inline while committing this change. Thank you.

davecheney · 2019-05-09T04:15:25Z

internal/dag/builder.go

+		return nil
+	}
+	return &UpstreamValidation{
+		CACertificate: b.lookupSecret(meta{name: uv.CACertificate, namespace: namespace}),


If the secret is not found then we should return nil here,

also if the subjectname is blank we should return nil here.

davecheney · 2019-05-09T04:16:21Z

internal/dag/builder.go

@@ -668,7 +668,20 @@ func (b *builder) processRoutes(ir *ingressroutev1.IngressRoute, prefixMatch str
 				}
 				m := meta{name: service.Name, namespace: ir.Namespace}
 				if s := b.lookupHTTPService(m, intstr.FromInt(service.Port), service.Strategy, service.HealthCheck); s != nil {
-					r.addHTTPService(s, service.Weight)
+					uv := b.lookupUpstreamValidation(service.UpstreamValidation, ir.Namespace)
+					if service.UpstreamValidation != nil {


If service.UpstreamValidation is nil then uv will be nil, so the check should probably be

if service.UpstreamValidation != nil && uv == nil {
// upstream validation requested, some components were missing

davecheney · 2019-05-09T04:16:51Z

internal/dag/builder.go

-					r.addHTTPService(s, service.Weight)
+					uv := b.lookupUpstreamValidation(service.UpstreamValidation, ir.Namespace)
+					if service.UpstreamValidation != nil {
+						if uv.CACertificate == nil {


but you do need to do this check because uv can be nil if lookupUpstreamValidation returned nil because there was a problem

theproblem is probably that lookupUPstreamValidation cannot be factored out of this call, we probably need to construct the uv inline while checking each value and setting status.

davecheney · 2019-05-09T04:34:06Z

internal/dag/builder.go

@@ -714,6 +727,16 @@ func (b *builder) processRoutes(ir *ingressroutev1.IngressRoute, prefixMatch str
 	b.setStatus(Status{Object: ir, Status: StatusValid, Description: "valid IngressRoute", Vhost: host})
 }

+func (b *builder) lookupUpstreamValidation(uv *ingressroutev1.UpstreamValidation, namespace string) *UpstreamValidation {


This could do with some unit tests. Its annoying to test as written because of all the parameters but I have an idea to really DRY up the testing of lookup helpers without the horror show of buidler_test.go

I'll leave this as a TODO

davecheney · 2019-05-09T04:51:44Z

internal/envoy/auth.go

@@ -46,12 +46,35 @@ var (
 // UpstreamTLSContext creates an auth.UpstreamTlsContext. By default
 // UpstreamTLSContext returns a HTTP/1.1 TLS enabled context. A list of
 // additional ALPN protocols can be provided.
-func UpstreamTLSContext(alpnProtocols ...string) *auth.UpstreamTlsContext {
-	return &auth.UpstreamTlsContext{
+func UpstreamTLSContext(ca []byte, subjaltname []string, alpnProtocols ...string) *auth.UpstreamTlsContext {


subjectaltname is a slice, the name should probably be a plural; subjectNames would be fine i think

In fact we can probably make it easier on ourselves by only passing a single subject name -- that's all we allow from ingressroute so we can hide the fact that envoy takes a slice of strings, that's a triviality.

davecheney · 2019-05-09T04:52:23Z

internal/envoy/auth.go

+		ValidationContext: &auth.CertificateValidationContext{
+			TrustedCa: &core.DataSource{
+				Specifier: &core.DataSource_InlineBytes{
+					InlineBytes: ca,


we need a todo to update this for SDS

davecheney · 2019-05-09T04:54:28Z

internal/envoy/auth.go

+}
+
+func validationContext(ca []byte, subjaltname []string) *auth.CommonTlsContext_ValidationContext {
+	if ca == nil {


This isn't quite correct; if validationContext returns nil if ca is nil, it should also return nil if subjectname contains < 1 element and that element is blank.

davecheney · 2019-05-09T05:11:50Z

internal/envoy/cluster_test.go

@@ -508,3 +573,11 @@ func service(s *v1.Service) dag.TCPService {
 		ServicePort: &s.Spec.Ports[0],
 	}
 }
+
+func tlsservice(s *v1.Service, cert, subjectaltname string) dag.TCPService {


cert and subjectaltname are not used

davecheney · 2019-05-09T05:23:39Z

internal/envoy/auth.go

 		CommonTlsContext: &auth.CommonTlsContext{
 			AlpnProtocols: alpnProtocols,
 		},
 	}
+
+	validation := validationContext(ca, subjaltname)


urgh, it turns out we have to do it this way because passing back a nil here creates a typed nil because context.CommonTlsContext.ValidationContextType is an interface. This comment is too short to explain why, but I'll leave a comment in the code to explain why

stevesloka force-pushed the tlsVerification branch from e8332ca to f193ae1 Compare April 25, 2019 16:22

stevesloka marked this pull request as ready for review April 25, 2019 19:55

stevesloka force-pushed the tlsVerification branch from f193ae1 to 3ffa92b Compare April 25, 2019 20:05

robbiemcmichael reviewed Apr 26, 2019

View reviewed changes

stevesloka force-pushed the tlsVerification branch from 4568cda to 3fceb9f Compare April 28, 2019 02:23

stevesloka changed the title ~~Implement upstream tls backend verification with ca cert and optional san~~ wip: Implement upstream tls backend verification with ca cert and optional san Apr 28, 2019

stevesloka force-pushed the tlsVerification branch from 3fceb9f to a8ed6fd Compare April 29, 2019 15:33

stevesloka changed the title ~~wip: Implement upstream tls backend verification with ca cert and optional san~~ Implement upstream tls backend verification with ca cert and optional san Apr 29, 2019

stevesloka requested a review from davecheney April 29, 2019 15:34

stevesloka added this to the 0.12.0 milestone Apr 29, 2019

davecheney suggested changes May 6, 2019

View reviewed changes

davecheney reviewed May 7, 2019

View reviewed changes

davecheney mentioned this pull request May 8, 2019

TLS verification of backend services #852

Closed

stevesloka force-pushed the tlsVerification branch 4 times, most recently from 9a34ba2 to 2dccfbf Compare May 8, 2019 20:15

stevesloka changed the title ~~Implement upstream tls backend verification with ca cert and optional san~~ Implement upstream tls backend verification with ca cert and required san May 8, 2019

stevesloka force-pushed the tlsVerification branch from 2dccfbf to bb286ed Compare May 8, 2019 20:24

stevesloka requested a review from davecheney May 8, 2019 20:33

stevesloka force-pushed the tlsVerification branch from bb286ed to 5895bc5 Compare May 8, 2019 20:36

Implement upstream tls backend verification with ca cert and optional

0a2d06e

subject alt name Signed-off-by: Steve Sloka <slokas@vmware.com>

stevesloka force-pushed the tlsVerification branch from 5895bc5 to 0a2d06e Compare May 9, 2019 01:50

davecheney merged commit 0a2d06e into projectcontour:master May 9, 2019

davecheney reviewed May 9, 2019

View reviewed changes

mattmoyer mentioned this pull request May 15, 2019

Design doc for externalNames #1095

Merged

davecheney mentioned this pull request Jun 3, 2019

internal/dag: dag.Route contains an unused UpstreamValidation field #1147

Closed

Implement upstream tls backend verification with ca cert and required san #1045

Implement upstream tls backend verification with ca cert and required san #1045

Conversation

stevesloka commented Apr 25, 2019

stevesloka commented Apr 25, 2019

stevesloka commented Apr 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevesloka commented Apr 29, 2019

davecheney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecheney commented May 8, 2019

stevesloka commented May 8, 2019

davecheney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment