Per Route TLS Configuration #35

danehans · 2020-01-14T00:59:19Z

Currently, Gateway provides the ability to specify one or more certificates to use with a TLS listener. This requires application developers to coordinate with cluster operators when creating a TLS HTTPRoute. This PR introduces the following changes:

Allows the Gateway specified certificate to be used for a TLS HTTPRoute when the route does not specify a certificate.
Introduces TLS configuration for an HTTPRoute that includes the ability to specify how TLS connections should be terminated and the TLS assets (i.e. certificate) used for TLS connections.

/cc @bowei

k8s-ci-robot · 2020-01-14T00:59:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danehans
To complete the pull request process, please assign thockin
You can assign the PR to them by writing /assign @thockin in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

danehans · 2020-01-28T23:19:33Z

Fixes Issue #52

hbagdi · 2020-01-30T21:58:06Z

api/v1alpha1/gateway_types.go

 	//
 	// Support: Core (Kubernetes Secrets)
 	// Support: Implementation-specific (Other resource types)
-	Certificates []core.TypedLocalObjectReference `json:"certificates,omitempty"`
+	Certificate core.TypedLocalObjectReference `json:"certificate,omitempty"`


We should specify if this is required or optional.
I think this should be required.

This is essentially the fallback certificate for any request and has the lowest preference.

There is another issue though. It should be possible to specify SNI <-> secret mapping at this level as well to specify different certificates for different domains.

I think this should be required.

@hbagdi TLS is optional for a listener. For example, a listener is configured to listen on port 80 for HTTP. Maybe GatewaySpec should have a Certificates or DefaultCertificates field for a gateway to use as "fall back" certs?

My bad for not explaining properly.
What I mean is that if it is a TLS listener, then we should encourage user to provide a default certificate, which takes the lowest priority. As you said, it's the "fall back" cert.

Having more than one default certificate means that you need to have some sort of SNI mapping to those certs. Both cases make sense to me.

hbagdi · 2020-01-30T22:00:39Z

api/v1alpha1/httproute_types.go

+	// Support: Implementation-specific (For other resource types)
+	//
+	// +optional
+	Certificate core.TypedLocalObjectReference `json:"certificate,omitempty"`


Same comment as: #35 (comment)

an HTTPRoute could have multiple SNIs.

There's also other cases for allowing multiple server certificates (e.g. serving different type types).

serving different type types

Not sure I understand. Can you elaborate?

A single application, served over HTTPs that has a collection of server certificates with different key types. The client will negotiate the matching certificate.

For example, envoy TLS context lets you add ECDSA and RSA certificates, at least in theory, the docs are contradictory :)

hbagdi · 2020-01-30T22:07:39Z

api/v1alpha1/httproute_types.go

+type TLSConfig struct {
+	// Termination defines how to terminate TLS connections.
+	//
+	// If unspecified, TLS termination type "Edge" will be used.


A significant number of users don't understand the details of TLS and higher-level protocols like HTTP and hence they associate TLS with a specific route, which is incorrect, since TLS session is established first and then the route is selected based on metadata of the higher-level protocol.

And then, there is the case were two different HTTPRoute (or other types of Route) have the same SNI but different cert. Sometimes we see that there are two secrets (no cross-namespace referencing of secret) but the content it same, other times, they will have different content as well. We usually honor the oldest Ingress resource in that case usually.

Here, we should be explicit about the behavior. I'm not sure what the solution should be but it should be hard to break production setups easily.

A significant number of users don't understand the details of TLS and higher-level protocols like HTTP and hence they associate TLS with a specific route, which is incorrect, since TLS session is established first and then the route is selected based on metadata of the higher-level protocol.

TLS cannot decide whether it is transport or application layer and has some unfortunate metadata redundancy with respect to HTTP. Does the API need to reflect that?

And then, there is the case were two different HTTPRoute (or other types of Route) have the same SNI but different cert. Sometimes we see that there are two secrets (no cross-namespace referencing of secret) but the content it same, other times, they will have different content as well. We usually honor the oldest Ingress resource in that case usually.

You could have two certificates with overlapping CNs or SANs in a Gateway object's spec.listeners.tls.certificates specification. I am not confident the API can be designed in such a way that it cannot be easily and accidentally used to express certificates that conflict with each other; it will be necessary to detect and report conflicts at run time.

Here, we should be explicit about the behavior. I'm not sure what the solution should be but it should be hard to break production setups easily.

Report conflicts in a status condition?

A significant number of users don't understand the details of TLS and higher-level protocols like HTTP and hence they associate TLS with a specific route, which is incorrect, since TLS session is established first and then the route is selected based on metadata of the higher-level protocol.

IMHO, the default common case should be to require the SNI and HTTP names to match (see #72).

TLS cannot decide whether it is transport or application layer and has some unfortunate metadata redundancy with respect to HTTP. Does the API need to reflect that?

No. I'm advocating to have TLS not be associated with our HTTP metadata based resources like HTTPRoute. That removes the possibility of expressing a certificate associated with a specific route (and that is not possible to enforce because of how TLS and HTTP work).

@hbagdi A couple of times I started writing a response here, but I'm not sure I understand your position well enough :)

WDYT about filing a separate issue to capture a user story for the case you are advocating?

hbagdi · 2020-01-30T22:18:55Z

api/v1alpha1/httproute_types.go

+	// TLSTerminationReencrypt terminates the TLS connection at the gateway.
+	// The gateway creates an encrypted connection to the destination service
+	// using the provided certificate from DestinationCACertificate.
+	TLSTerminationReencrypt TLSTerminationType = "Reencrypt"


I was confused when I saw this option.

If we are doing passthrough, it should not be an HTTPRoute, I'd rather see that in TCPRoute or a TLSRoute, since we can't even know the protocol or muck around in it as it's an encrypted stream.

Upstream connection details should not be part of the route level details but HTTPRouteAction maybe. Each service or action can potentially tweak this part a little differently.

Right. I think that it is useful to think about this hop-by-hop. What does the hop from client to gateway look like? What does the hop from gateway to the next endpoint look like? I this model the name "reencrypt" doesn't really apply because we are focused only on hops.

ironcladlou · 2020-01-31T01:03:03Z

/cc @smarterclayton

jpeach · 2020-02-11T01:20:43Z

api/v1alpha1/httproute_types.go

+	// TLS is the TLS configuration of an HTTPRouteHost.
+	//
+	// If unspecified, the TLS configuration of the binding gateway
+	// will be used.


The gateway gets a server TLS config, but here we need a client TLS config because this is the TLS policy describing how to connect to a backend service, right? If so, how can we re-use the gateway policy?

Kubernetes is adding support for describing app-protocol on the core.Service.
I would like to see if that suffices to describe the basic use case.

https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20191227-app-protocol.md

we need a client TLS config because this is the TLS policy describing how to connect to a backend service, right?

That should be part of a HTTPRouteAction since you can have different TLS configs for different services.

@bowei The AppProtocol solves the problem of which protocol to use, but an operator can still want to specify protocol level configuration settings, such as CAs to use, client cert to use (if need be).

Having said that, my interpretation is that this field represents downstream TLS configs(TLS settings between client and proxy) and not upstream TLS config (between proxy and service).

OK, I got mixed up here. I was thinking this was the TLS context for the upstream hop. but it's the server context for termination the client TLS session.

One important thing to keep in mind is that while all the bells and whistles are really cool to have, for the initial version, we don't/can't/don't have to do it all.
Remember, there is a core that needs to be as universal as Ingress, extended that we can reasonably live with.
Things beyond that we should really try to aim towards understanding how they would be expressed as custom.

As things progress, it's easier to add than to remove.

jpeach · 2020-02-11T01:41:44Z

api/v1alpha1/httproute_types.go

+	//    ca-bundle.crt: |
+	//      -----BEGIN CERTIFICATE-----
+	//      Destination Service CA Certificate Bundle.
+	//      -----END CERTIFICATE-----


I filed #74 to discuss certificates more generally.

jpeach · 2020-02-11T01:44:08Z

api/v1alpha1/httproute_types.go

+
+	// DestinationCACertificate is a reference to a CA certificate used
+	// for establishing a TLS connection with the final destination when
+	// using TLS termination type "Reencrypt".


This is a CA bundle? Then we can call this CACertificates and I'd suggest the comment read something more like:

CACertificates is a reference to the bundle of trusted CA certificates used to validate the server certificate when establishing a TLS connection ...

jpeach · 2020-02-11T01:45:47Z

api/v1alpha1/httproute_types.go

+	// Support: Implementation-specific (For other resource types)
+	//
+	// +optional
+	Certificate core.TypedLocalObjectReference `json:"certificate,omitempty"`


There's also other cases for allowing multiple server certificates (e.g. serving different type types).

jpeach · 2020-02-11T01:48:10Z

api/v1alpha1/httproute_types.go

+	Termination TLSTerminationType `json:"termination,omitempty"`
+
+	// Certificate is a reference to a Kubernetes object containing
+	// the identity certificate, key and CA certificate used to terminate


I think this needs to be specced in detail for implementations to be interoperable. I raised most of the issues that occurred to me in #74 . We should also consider whether to allow separation of the key from all the certificate material.

jpeach · 2020-02-11T01:50:36Z

api/v1alpha1/httproute_types.go

+type TLSConfig struct {
+	// Termination defines how to terminate TLS connections.
+	//
+	// If unspecified, TLS termination type "Edge" will be used.


A significant number of users don't understand the details of TLS and higher-level protocols like HTTP and hence they associate TLS with a specific route, which is incorrect, since TLS session is established first and then the route is selected based on metadata of the higher-level protocol.

IMHO, the default common case should be to require the SNI and HTTP names to match (see #72).

danehans · 2020-02-12T20:29:16Z

/hold

in favor of #71.

danehans · 2020-02-12T20:39:11Z

@jpeach ptal at #35 (comment) and the associated PR.

k8s-ci-robot · 2020-02-13T22:11:17Z

@danehans: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

danehans · 2020-03-05T19:33:09Z

Closing in favor of #71.

Adds support for per route user-specified tls config

9659cc4

k8s-ci-robot requested a review from bowei January 14, 2020 00:59

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 14, 2020

bowei added this to Inbox in Main Jan 15, 2020

bowei moved this from Inbox to Discussion items in Main Jan 15, 2020

This was referenced Jan 24, 2020

Better modeling of a "virtual host" #49

Closed

Insecure Connection Policy #51

Closed

TLS Termination Policy #52

Closed

danehans changed the title ~~Adds support for per route user-specified tls config~~ Per Route TLS Configuration Jan 29, 2020

hbagdi reviewed Jan 30, 2020

View reviewed changes

k8s-ci-robot requested a review from smarterclayton January 31, 2020 01:03

danehans mentioned this pull request Feb 10, 2020

Adds Per Route TLS Config to Gateway API Type #71

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 10, 2020

jpeach suggested changes Feb 11, 2020

View reviewed changes

Main automation moved this from Discussion items to In Progress Feb 11, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2020

danehans closed this Mar 5, 2020

bowei moved this from In Progress to Done in Main May 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Per Route TLS Configuration #35

Per Route TLS Configuration #35

danehans commented Jan 14, 2020 •

edited

k8s-ci-robot commented Jan 14, 2020

danehans commented Jan 28, 2020

hbagdi Jan 30, 2020

danehans Feb 7, 2020

hbagdi Feb 14, 2020

hbagdi Jan 30, 2020

jpeach Feb 11, 2020

hbagdi Feb 14, 2020

jpeach Feb 14, 2020

hbagdi Jan 30, 2020

Miciah Feb 10, 2020

jpeach Feb 11, 2020

hbagdi Feb 14, 2020

jpeach Feb 14, 2020

hbagdi Jan 30, 2020

jpeach Feb 14, 2020

ironcladlou commented Jan 31, 2020

jpeach Feb 11, 2020

bowei Feb 13, 2020

bowei Feb 13, 2020

hbagdi Feb 14, 2020

jpeach Feb 14, 2020

bowei Feb 14, 2020

jpeach Feb 11, 2020

jpeach Feb 11, 2020

jpeach Feb 11, 2020

jpeach Feb 11, 2020

jpeach Feb 11, 2020

danehans commented Feb 12, 2020

danehans commented Feb 12, 2020

k8s-ci-robot commented Feb 13, 2020

danehans commented Mar 5, 2020

Per Route TLS Configuration #35

Per Route TLS Configuration #35

Conversation

danehans commented Jan 14, 2020 • edited

k8s-ci-robot commented Jan 14, 2020

danehans commented Jan 28, 2020

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

ironcladlou commented Jan 31, 2020

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

danehans commented Feb 12, 2020

danehans commented Feb 12, 2020

k8s-ci-robot commented Feb 13, 2020

danehans commented Mar 5, 2020

danehans commented Jan 14, 2020 •

edited