Adds Per Route TLS Config to Gateway API Type #71

danehans · 2020-02-10T21:34:46Z

After reviewing #35 feedback from @hbagdi, managing TLS termination at the Gateway is more appropriate than within a xRoute.

Partially fixes: #49, #72
Fixes: #52

/assign @bowei
/cc @ironcladlou @Miciah

k8s-ci-robot · 2020-02-10T21:34:51Z

@danehans: GitHub didn't allow me to request PR reviews from the following users: ironcladlou, Miciah.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

After reviewing #35 feedback from @hbagdi, managing TLS termination at the Gateway is more appropriate than at the route-level. The newly proposed TLSTerminationPolicy includes a Routes field that will allow the Gateway to optionally bind the TLS termination policy to specific routes. Note that a selector-based approach may be prefered pending #12

Partially fixes #49

/assign @bowei
/cc @ironcladlou @Miciah

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.

Miciah · 2020-02-10T22:44:16Z

After reviewing #35 feedback from @hbagdi, managing TLS termination at the Gateway is more appropriate than at the route-level.

Can you elaborate on that? What is the semantics of TLS termination policy for a Gateway that has both HTTPRoute and TCPRoute resources?

jpeach · 2020-02-11T00:40:22Z

api/v1alpha1/gateway_types.go

+	// Support: Core
+	//
+	// +optional
+	Routes []core.ObjectReference `json:"routes,omitempty"`


What are these Routes for? How do these Routes interact with the Routes on the Gateway?

Currently, Routes are properties of the Gateway, which implies that they are available on all listeners. Moving Routes to a listener could make sense.

@jpeach thanks for the review. This Routes field is meant to (optionally) provide the ability to associate the TLS termination policy to specific routes. This could be done in the xRoute resource, but then Devs would have that control instead of cluster admins. These routes could be all or a subset of routes that are bound to the gateway.

I'm going to remove the route reference for now and make the termination policy a listener-scoped setting.

jpeach · 2020-02-11T00:54:10Z

api/v1alpha1/gateway_types.go

+	// Support: Core
+	//
+	// +optional
+	TerminationPolicy TLSTerminationPolicy `json:"terminationPolicy,omitempty"`


If termination type is a property of the listener, how can we satisfy different termination needs for different backends? In the common case, I expect there is only one TLS listener on port 443, and some backends will want Edge and others will want Passthrough.

Let me push a new commit for addressing this use case.

jpeach · 2020-02-11T00:58:50Z

api/v1alpha1/gateway_types.go

+
+	// TLSTerminationPassthrough terminates the TLS connection at the
+	// destination service. The destination service is responsible for
+	// decrypting data from the connection.


We should specify the requirements on the gateway for passthrough termination. The Gateway should enforce that the protocol is actually TLS. Is SNI required for route selection?

I agree that the protocol should be TLS and I would expect SNI for route selection. I tried to address some of these details in #76.

jpeach · 2020-02-11T01:01:26Z

api/v1alpha1/gateway_types.go

+	// Support: Implementation-specific (For other resource types)
+	//
+	// +optional
+	CACertificates []core.TypedLocalObjectReference `json:"caCertificates,omitempty"`


Should we require specific secret types for this? (IMHO, yes we should)

Since this is server TLS policy for the listener, I think that we should not also be specifying the client TLS policy for re-ecrypting traffic to the backend. That client policy could have different validation policy, version requirements, client certificates, and all sorts of things that are awkward to express here.

@jpeach I'm pushing a new commit where the TLS termination policy is associated to a Route within a Gateway.

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

api/v1alpha1/gateway_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'm not really sure that there are 3 cases. Another way to look at it is that we have 2 independent decisions

terminate TLS

proxy over TCP

proxy over TLS

don't terminate TLS

proxy over TCP

proxy over TLS

So, in this view, re-encryption is a property of the backend route.

I see where you're coming from. "Reencrypt" could be a route filter.

danehans · 2020-02-11T16:50:25Z

Can you elaborate on that? What is the semantics of TLS termination policy for a Gateway that has both HTTPRoute and TCPRoute resources?

@Miciah either route type can be encapsulated in TLS. The TLS handshake will occur between client and gateway (non-passthrough mode). The gateway will select the appropriate certificate based on the SNI hostname in the client's hello message. If mode is passthrough, the gateway will forward the request based on xRoute instead of creating a new connection. The gateway will create a new connection to backend services for the other modes.

k8s-ci-robot · 2020-02-11T20:55:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danehans
To complete the pull request process, please assign bowei
You can assign the PR to them by writing /assign @bowei 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-02-12T20:42:45Z

xref #74

Miciah · 2020-02-12T23:10:16Z

If mode is passthrough, the gateway will forward the request based on xRoute instead of creating a new connection.

Are you saying the gateway controller will forward the decrypted packets to a TCPRoute backend? Is there no way to express that a TLS connection should be passed through without termination?

jpeach · 2020-02-13T03:02:51Z

api/v1alpha1/gateway_types.go

+// Route defines the schema for a route.
+type Route struct {
+	// RouteRef is a reference to an object to associate with the Gateway.
+	// RouteRef defines protocol-specific routing to back-ends (e.g. Services).


Is this really what a Route is? Looking at the current spec, TcpRoute and HTTPRoute are defined. To my mind these have more in common with endpoints (since we are in a reverse-proxy configuration, they will appear to be endpoints from the perspective of the client).

I realize that this terminology is already in play, but maybe we can clarify here. The description of "routing to back-ends" seems more applicable to HTTPRouteAction.

Can you define more what "endpoint" and "client" you mention in the above.
Endpoint e.g. k8s Endpoint? Or something else?
Client as in the HTTP-client coming to the Gateway (or something else)

The purpose of the Route type is to group attributes with the route reference. Currently, we're focused on TLS, but I can foresee other attributes a Cluster Operator may want to associate to a route, e.g. a traffic shaping policy.

jpeach · 2020-02-13T03:04:23Z

api/v1alpha1/gateway_types.go

-	Routes []core.TypedLocalObjectReference `json:"routes"`
+	// Routes defines routes to associate with the Gateway.
+	//
+	// If unspecified, all routes will be associated to the Gateway.


I'm not sure how to interpret this. If there are no routes specified here, what routes are being associated?

@jpeach I think it comes down to defining the default behavior for route association, closed or open. I created #84 to address this issue. In the meantime, I'll update the language here to be default closed.

Thanks, I commented there.

jpeach · 2020-02-13T03:11:05Z

api/v1alpha1/gateway_types.go

@@ -97,7 +127,7 @@ type Listener struct {
 	//
 	// Support:
 	// +optional
-	TLS *ListenerTLS `json:"tls,omitempty"`
+	TLS *TLSConfig `json:"tls,omitempty"`


Maybe we need to step back from attaching TLS to the listener. The HTTPRouteHost has the hostname, maybe that's the right place to put a TLS configuration? That would strongly bind TLS configuration and virtual hosts in a way that's currently only implicit (even with the ListenerCertificates changes below).

@jpeach IMO we need to get #95 resolved to move forward with this or any of the other PR's that address TLS.

If the App Dev is responsible for managing TLS config, then I agree that xRoute is the right place for all TLS config management.

jpeach · 2020-02-13T03:19:27Z

api/v1alpha1/gateway_types.go

@@ -139,7 +169,7 @@ const (
 	TLS1_3 = "TLS1_3"
 )

-// ListenerTLS describes the TLS configuration for a given port.
+// TLSConfig describes configuration for establishing a TLS connection.


To me, "establishing a TLS connection" implies pretty strongly that it is being used by a network client.

Maybe TLSConfig => TLSServerConfig will be less ambiguous/

(the server also is part of connection establishment)

If this is intended to be specific to accepting a TLS session, then I agree TLSServerConfig is less ambiguous.

@bowei I considered TLSServerConfig but TLSConfig contains client-side properties such as CACertificates used by the Gateway for establishing a TLS connection to a backend Service.

jpeach · 2020-02-13T03:45:28Z

api/v1alpha1/gateway_types.go

+	//      -----BEGIN CERTIFICATE-----
+	//      Destination Service CA Certificate Bundle.
+	//      -----END CERTIFICATE-----
+	//


This is a nice improvement. Should we add a reference to #74, which hopefully will fully define the requirements around certificates?

jpeach · 2020-02-13T03:47:21Z

api/v1alpha1/gateway_types.go

@@ -167,7 +223,7 @@ type ListenerTLS struct {
 	// values.
 	//
 	// +optional
-	MinimumVersion *string `json:"minimumVersion"`
+	MinimumVersion *string `json:"minimumVersion,omitempty"`


Hmm, are there any requirements on implementations if the minimum version is omitted? Maybe track a separate issue to discuss?

jpeach · 2020-02-13T03:53:05Z

api/v1alpha1/gateway_types.go

-	Certificates []core.TypedLocalObjectReference `json:"certificates,omitempty"`
+	//
+	// +optional
+	ListenerCertificates []core.TypedLocalObjectReference `json:"listenerCertificates,omitempty"`


I think this relates back to my comment on Listener.TLS. A collection of listener certificates makes sense when you are modeling a single port that can terminate different TLS names. However, further down, TLSConfig is used in HTTPRouteAction, and in that context it's not clear to me whether this is a pinned set of certificates that the client will verify and accept, or simply not used. Attaching the TLS server configuration to the HTTPHostRoute could help to resolve this ambiguity.

My current understand is that the certs associated with the Gateway here are for downstream connection and the TLSConfig in HostRouteAction is for the upstream hop.

hbagdi · 2020-02-14T22:30:38Z

api/v1alpha1/gateway_types.go

-	Certificates []core.TypedLocalObjectReference `json:"certificates,omitempty"`
+	//
+	// +optional
+	ListenerCertificates []core.TypedLocalObjectReference `json:"listenerCertificates,omitempty"`


My current understand is that the certs associated with the Gateway here are for downstream connection and the TLSConfig in HostRouteAction is for the upstream hop.

hbagdi · 2020-02-14T22:32:31Z

api/v1alpha1/gateway_types.go

+	//      -----END CERTIFICATE-----
+	//
+	// Support: Core (Kubernetes ConfigMap)
+	// Support: Implementation-specific (For other resource types)


s/Implementation-specific/Custom to be consistent with how this is used in other places.
Just a note: #56

hbagdi · 2020-02-14T22:35:31Z

api/v1alpha1/gateway_types.go

+	// Support: Core
+	//
+	// +optional
+	TLSTermination TLSTerminationType `json:"tlsTermination,omitempty"`


This gets interesting. Sometimes one can be in a situation where the termination is decided based on the SNI. For some SNIs, the proxy terminates TLS while for other it is passthrough. It has come up a few times but I'm not sure if it should be supported by our API.

Leaving a note here but I think it is okay to keep this as is.

k8s-ci-robot · 2020-02-15T19:27:03Z

@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-18T15:43:53Z

Will reopen if virtual host model is accepted.

…es/mci-https-backend added multi-cluster ingress end to end https recipe

k8s-ci-robot assigned bowei Feb 10, 2020

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 10, 2020

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 10, 2020

danehans mentioned this pull request Feb 10, 2020

Per Route TLS Configuration #35

Closed

jpeach suggested changes Feb 11, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Feb 11, 2020

danehans force-pushed the gw_tls_term_pol branch from cc28347 to 63e2d58 Compare February 11, 2020 20:56

danehans force-pushed the gw_tls_term_pol branch 3 times, most recently from 66e5acd to 266d652 Compare February 12, 2020 19:44

danehans changed the title ~~Adds TLSTerminationPolicy to Gateway API type~~ Adds Per Route TLSTermination to Gateway API Type Feb 12, 2020

jpeach suggested changes Feb 13, 2020

View reviewed changes

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 mentioned this pull request Feb 13, 2020

Define Default Gateway/Route Association Behavior #84

Closed

Adds TLS Termination to Gateway API type

70f518b

danehans force-pushed the gw_tls_term_pol branch from 266d652 to 70f518b Compare February 14, 2020 17:22

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 14, 2020

danehans changed the title ~~Adds Per Route TLSTermination to Gateway API Type~~ Adds Per Route TLS Config to Gateway API Type Feb 14, 2020

This was referenced Feb 14, 2020

TLS Termination Policy #52

Closed

TLS: Align APIs with Personas #95

Closed

hbagdi reviewed Feb 14, 2020

View reviewed changes

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

danehans closed this Mar 18, 2020

jaison-tiu pushed a commit to jaison-tiu/gateway-api that referenced this pull request Apr 14, 2022

Merge pull request kubernetes-sigs#71 from GoogleCloudPlatform/featur…

48677cf

…es/mci-https-backend added multi-cluster ingress end to end https recipe

Adds Per Route TLS Config to Gateway API Type #71

Adds Per Route TLS Config to Gateway API Type #71

Conversation

danehans commented Feb 10, 2020 • edited Loading

k8s-ci-robot commented Feb 10, 2020

Miciah commented Feb 10, 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

danehans commented Feb 11, 2020

k8s-ci-robot commented Feb 11, 2020

danehans commented Feb 12, 2020

Miciah commented Feb 12, 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

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

k8s-ci-robot commented Feb 15, 2020

danehans commented Mar 18, 2020

danehans commented Feb 10, 2020 •

edited

Loading