Httpverifysni

[lambdai]

Commit Message:
Background
The SNI of a connection is persistent but the host header of http request within that connection can be different.
The attack could provide a guest.example.com SNI and an admin.example.com header.
There is risk that route config does not explicitly deny the admin.example.com host,
I have seen 3 different orgs discover the issue and used their own lua filter as mitigation.

Alternatives

1. Use route config for HCM instance under different SNI matches.
   It's likely that 2 route configs have thousands routes the same but only 1 or 2 for deny. This almost double the route config items in memory.
2. RBAC
   It seems possible but I don't see anyone adopting it as mitigation.
3. Refined SNI matches in filter chain.
   e.g. always allow *.exmaple.com and use the RBAC or route to execute the deny.
   This is need a restructure of configs from listener - filter chain - HCM - http route config.

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
Fixes #6767
[Optional Deprecated:]
[Optional API Considerations:]

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17223 was opened by lambdai.

see: more, trace.

[lizan]

I'm still not convinced why this has to be a separate filter. If 2. RBAC works and just no one adopt, we should just add a FAQ or a suggestion in SNI config for that?

[mattklein123]

+1 let's just better document using RBAC.

/wait

[lambdai]

I give it try in RBAC: there is something missing. Please advice if I don't fully get the ability of rbac

1. DENY with response code 421 So it's another field response code added with DENY policy.
   Other response code may confuse the browser.
2. SNI match other than StringMatcher. It's awkward to match the SNI requirement.
   https://github.com/envoyproxy/envoy/blob/main/api/envoy/config/listener/v3/listener_components.proto#L154-L163
3. Set based match with wildcard
   In host http match we create suffix or prefix tree and in filter chain match. Ideally we should use the similar tech here instead of series of "and_match" and "or_match". This requirement might be light.

[mattklein123]

RBAC seems like the right place to do this, so let's propose changes to RBAC to make this possible. I don't know why we would have a new filter for this. The alternative would be that this is so important that we make this a core security feature.

/wait

[lambdai]

RBAC seems like the right place to do this, so let's propose changes to RBAC to make this possible. I don't know why we would have a new filter for this. The alternative would be that this is so important that we make this a core security feature.

What do you think if put the require_host_equal_to_sni in HCM and the complicate allow + deny in RBAC?

Suppose we have 2 filter chains, one for for SNI=*.example.com and one for SNI=admin.example.com

The require_host_equal_to_sni well on

• allow SNI=foo.example.com Host=foo.example.com
• reject SNI=foo.example.com Host=admin.example.com

But if anyone want to additionally allow SNI=foo.example.com Host=bar.example.com the user should use RBAC.

[mattklein123]

If we believe this is a serious enough security problem that it's hitting multiple people, I think having this be a core feature is reasonable. I think your proposal of how to handle is also reasonable. I'm OK with adding a core feature for this, but I would like to here from @alyssawilk @antoniovicente @htuch @yanavlasov on this. If we do add the core feature my preference would be to make this secure by default (with runtime override for the default).

[alyssawilk]

"The attack could provide a guest.example.com SNI and an admin.example.com header."

So I think by default Envoy should enforce that host header is allowed by the cert served (admin.foo.com should not be served under bar.foo.com cert but should be allowed under *.foo.com cert). I'd be all for that being on by default, runtime guarded off if that's not the current behavior. But if you want to serve *.foo.com certs and reject admin.foo.com I'd think you could just have normal route actions and static response "disallowed". Either way I'm not sure why additional configuration should be required?

[mattklein123]

Either way I'm not sure why additional configuration should be required?

I think the only reason would be if this change breaks some deployment. I think we could start with a runtime guard though and see if anyone complains.

[alyssawilk]

Yeah, let's runtime guard, and but folks complain my fail-over would be adding deny routes over additional config. I don't think it would necessarily double route memory but we can debate that if runtime guarding exposes problems :-)

[mattklein123]

my fail-over would be adding deny routes over additional config

Yeah agreed we can cross this bridge later. My preference would actually be to build this into RBAC/WAF vs. new route matching but we can discuss later.

[lambdai]

So I think by default Envoy should enforce that host header is allowed by the cert served (admin.foo.com should not be served under bar.foo.com cert but should be allowed under *.foo.com cert).

+1 That's a sub-issue. As long as a runtime guard is provided to bridge the migration people should be good.

But if you want to serve *.foo.com certs and reject admin.foo.com I'd think you could just have normal route actions and static response "disallowed". Either way I'm not sure why additional configuration should be required?

That works for sure as mitigation. My only concern is that this is not practical. The common route configure between admin.example.com HCM in network filter chain and *.example.com HCM in network filter chain. It's helpful and greatly simplify the control plane and the overall config to make AUTH and route orthogonal

From the Envoy config level, they differs only at listener config.

Envoy config	*.example.com	admin.example.com
listener servername	*.example.com	admin.example.com
listener tls require peer cert	no	yes
listener tls socket	*.example.com	*.example.com
listener HCM	same	same
My Proposal: req.host == connection.sni	so not admin.example.com	admin.example.com
Route Config in HCM	includes *.example.com	includes *.example.com

[alyssawilk]

ah, so it's not just that you want to make sure admin.foo.com isn't served under cert bar.foo.com, you want to make sure that if the cert is *.foo.com and you sni for bar.foo.com you only can send requests for bar.foo.com?

That's certainly something we don't want to support by default, I believe all the major browsers (and Envoy itself will soon) pool admin.foo.com into the same connection with bar.foo.com if the cert allows and DNS matches. The 421 is supported in the spec (https://datatracker.ietf.org/doc/html/rfc7540#section-9.1.1) but I don't think most clients (envoy included) would take the 421 as an indication to retry with a new connection so I'd lean towards that being a custom filter or lua since I think it won't be handled on the internet at large, but if Matt says RBAC is the place to go I won't gainsay.

Edit: nope I'm wrong, it is at least supported in Chrome.

[lambdai]

I create dedicated issue #17258 for certs dns names so we can focus on solutions of SNI.

I am closing this PR to propose a RBAC based solution for SNI