fix: match SNI when using TLS listeners with hostname #2942

michaelbeaumont · 2024-03-15T17:12:16Z

What type of PR is this?

A fix for TLS listener with hostname handling.

What this PR does / why we need it:

This PR sets filterChainMatch.serverNames when using TLS listeners with a hostname.

Which issue(s) this PR fixes:

Signed-off-by: Mike Beaumont <[email protected]>

arkodg · 2024-03-15T17:21:30Z

hey @michaelbeaumont, for my understanding, is this PR is trying to solve ?

Listener Protocol: TLS
TLS Mode: Terminate
Route Type: TCPRoute

https://gateway-api.sigs.k8s.io/guides/tls/#clientserver-and-tls

afaik this should be supported today
#1431
https://gateway.envoyproxy.io/v1.0.0/user/security/tls-termination/

michaelbeaumont · 2024-03-15T17:36:21Z

Hey, yeah traffic is terminated properly, but if I set hostname on that listener, there's no filtering of traffic by SNI as specified by the spec for hostname. The final commit shows the difference for this listener.

Implementations MUST apply Hostname matching appropriately for each of the following protocols:

TLS: The Listener Hostname MUST match the SNI.

arkodg · 2024-03-15T17:59:32Z

got it makes sense, thanks for clarifying !
is there a way to modify the TCP Listener IR w/o affecting the HTTP Listener IR structure and achieving the same goal ?
Currently the Hostnames field within the HTTP Listener is used for Filter Chain creation with a server_name constaint

zhaohuabing

@michaelbeaumont Thank you for addressing the issue! A few comments.

zhaohuabing · 2024-03-17T10:59:50Z

internal/ir/xds.go

 // +k8s:deepcopy-gen=true
 type TLSInspectorConfig struct {
 	// Server names that are compared against the server names of a new connection.
 	// Wildcard hosts are supported in the prefix form. Partial wildcards are not
 	// supported, and values like *w.example.com are invalid.
-	// SNIs are used only in case of TLS Passthrough.
 	SNIs []string `json:"snis,omitempty" yaml:"snis,omitempty"`


Consider moving SNIs up to TLSConfig and removing TLSInspectorConfig?

TLS Inspector is an implementation detail of Envoy, so we probably don't need to expose it to ir.

I think it's already exposed as TLS.Passthrough isn't it? I can add SNIs directly to TLSConfig if you prefer

I'd vote for making TLSInspectorConfig an embedded field within TLS

gateway/internal/ir/xds.go

Line 1088 in 4d6da72

type TLS struct {

, removing the need for a dedicated Passthrough field

passthrough logic can be computed using - isTLSPassthrough := irListener.TLS != nil && irListener.TLS.Terminate == nil

this also eliminates the need to add Inspector into the TLSConfig field, which is common to HTTPS, which uses another field (listener.Hostname) for SNI inspection

I pushed an update here, it changes a different part of the IR though, passthrough -> inspector, since that's now common between mode: Terminate and mode: Passthrough

internal/xds/translator/testdata/out/xds-ir/tcp-route-tls-terminate.listeners.yaml

zhaohuabing

Could we add multiple hostnames in the gateway API translator and XDS translator tests for TLS termination? This could cover the scenario where a single listener needs to handle multiple SNIs. Thanks!

internal/gatewayapi/helpers.go

shuji-2019 · 2024-04-11T02:58:35Z

Is there any updates for this feature ?

Signed-off-by: Mike Beaumont <[email protected]>

aoledk · 2024-05-08T09:51:11Z

@michaelbeaumont Are you still working on this? If not I can help, based on your previous work.

Signed-off-by: Mike Beaumont <[email protected]>

…hostname

michaelbeaumont · 2024-05-09T22:10:00Z

Updated and merged

arkodg · 2024-05-10T19:49:15Z

internal/gatewayapi/route.go

@@ -1082,6 +1082,14 @@ func (t *Translator) processTCPRouteParentRefs(tcpRoute *TCPRouteContext, resour
 			accepted = true
 			irKey := t.getIRKey(listener.gateway)

+			tls := ir.TLS{


shouldnt this be executed conditionally ?
e.g.

gateway/internal/gatewayapi/listener.go

Line 53 in 33443f8

if listener.TLS != nil {

I don't think anything's changed here, previously we set unconditionally:

TLS: &ir.TLS{Terminate: irTLSConfigs(listener.tlsSecrets)},

irTLSConfigs returns nil if there aren't any secrets. The line you linked to is also gated by checking the listener protocol.

Cool thanks for the checking!

arkodg · 2024-05-10T21:23:59Z

@michaelbeaumont 'make lint's is failing because of trailing whitespaces, once that's fixed, LGTM from my end !

arkodg · 2024-05-10T21:27:38Z

@michaelbeaumont can you also fix DCO and force push ?

Signed-off-by: Mike Beaumont <[email protected]>

arkodg

LGTM thanks !

zhaohuabing

LGTM thanks!

It would be helpful if we wcould cover two snis on the same port in the xds translation test. But this can be added later.

michaelbeaumont · 2024-05-10T23:58:35Z

@zhaohuabing I added another SNI to the test https://github.com/envoyproxy/gateway/pull/2942/files#diff-3481ca7cb1d20b614db1882fa576de2d303d434e519a17d12c2792436d6a0e18R28 actually

michaelbeaumont requested a review from a team as a code owner March 15, 2024 17:12

michaelbeaumont added 4 commits March 15, 2024 18:12

test: add TLS terminate with hostname

c473dcf

Signed-off-by: Mike Beaumont <[email protected]>

feat: support listener hostname with TLS termination

468eea9

Signed-off-by: Mike Beaumont <[email protected]>

test: adjust expected output

a3790cf

Signed-off-by: Mike Beaumont <[email protected]>

test: adjust new case outputs

992dd33

Signed-off-by: Mike Beaumont <[email protected]>

michaelbeaumont force-pushed the feat/tls-terminate-hostname branch from 62584b7 to 992dd33 Compare March 15, 2024 17:13

zhaohuabing reviewed Mar 16, 2024

View reviewed changes

zhaohuabing reviewed Mar 17, 2024

View reviewed changes

zhaohuabing reviewed Mar 18, 2024

View reviewed changes

internal/gatewayapi/helpers.go Outdated Show resolved Hide resolved

refactor: change function arg name from review

4d6da72

Signed-off-by: Mike Beaumont <[email protected]>

michaelbeaumont force-pushed the feat/tls-terminate-hostname branch from 522da6a to 4d6da72 Compare April 21, 2024 14:01

zirain requested review from zhaohuabing and arkodg April 23, 2024 01:23

arkodg added this to the v1.1.0-rc1 milestone May 7, 2024

refactor: remove inspector config from HTTP IR

2fdf048

Signed-off-by: Mike Beaumont <[email protected]>

michaelbeaumont force-pushed the feat/tls-terminate-hostname branch from ac557ab to 2fdf048 Compare May 9, 2024 21:49

Merge remote-tracking branch 'upstream/main' into feat/tls-terminate-…

a01aa2d

…hostname

arkodg reviewed May 10, 2024

View reviewed changes

chore: fix yaml lint errors

911aa09

Signed-off-by: Mike Beaumont <[email protected]>

michaelbeaumont force-pushed the feat/tls-terminate-hostname branch from 30c4ab6 to 911aa09 Compare May 10, 2024 21:42

arkodg approved these changes May 10, 2024

View reviewed changes

arkodg requested review from a team May 10, 2024 22:42

zhaohuabing approved these changes May 10, 2024

View reviewed changes

arkodg merged commit c30d09f into envoyproxy:main May 10, 2024
20 checks passed

michaelbeaumont deleted the feat/tls-terminate-hostname branch May 10, 2024 23:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: match SNI when using TLS listeners with hostname #2942

fix: match SNI when using TLS listeners with hostname #2942

michaelbeaumont commented Mar 15, 2024

arkodg commented Mar 15, 2024

michaelbeaumont commented Mar 15, 2024 •

edited

Loading

arkodg commented Mar 15, 2024 •

edited

Loading

zhaohuabing left a comment •

edited

Loading

zhaohuabing Mar 17, 2024 •

edited

Loading

michaelbeaumont Apr 21, 2024

arkodg Apr 23, 2024

michaelbeaumont May 9, 2024

zhaohuabing left a comment

shuji-2019 commented Apr 11, 2024

aoledk commented May 8, 2024

michaelbeaumont commented May 9, 2024

arkodg May 10, 2024 •

edited

Loading

michaelbeaumont May 10, 2024

arkodg May 10, 2024

arkodg commented May 10, 2024

arkodg commented May 10, 2024

arkodg left a comment

zhaohuabing left a comment •

edited

Loading

michaelbeaumont commented May 10, 2024

fix: match SNI when using TLS listeners with hostname #2942

fix: match SNI when using TLS listeners with hostname #2942

Conversation

michaelbeaumont commented Mar 15, 2024

arkodg commented Mar 15, 2024

michaelbeaumont commented Mar 15, 2024 • edited Loading

arkodg commented Mar 15, 2024 • edited Loading

zhaohuabing left a comment • edited Loading

Choose a reason for hiding this comment

zhaohuabing Mar 17, 2024 • edited Loading

Choose a reason for hiding this comment

michaelbeaumont Apr 21, 2024

Choose a reason for hiding this comment

arkodg Apr 23, 2024

Choose a reason for hiding this comment

michaelbeaumont May 9, 2024

Choose a reason for hiding this comment

zhaohuabing left a comment

Choose a reason for hiding this comment

shuji-2019 commented Apr 11, 2024

aoledk commented May 8, 2024

michaelbeaumont commented May 9, 2024

arkodg May 10, 2024 • edited Loading

Choose a reason for hiding this comment

michaelbeaumont May 10, 2024

Choose a reason for hiding this comment

arkodg May 10, 2024

Choose a reason for hiding this comment

arkodg commented May 10, 2024

arkodg commented May 10, 2024

arkodg left a comment

Choose a reason for hiding this comment

zhaohuabing left a comment • edited Loading

Choose a reason for hiding this comment

michaelbeaumont commented May 10, 2024

michaelbeaumont commented Mar 15, 2024 •

edited

Loading

arkodg commented Mar 15, 2024 •

edited

Loading

zhaohuabing left a comment •

edited

Loading

zhaohuabing Mar 17, 2024 •

edited

Loading

arkodg May 10, 2024 •

edited

Loading

zhaohuabing left a comment •

edited

Loading