Downstream TLS: Allow Skip Verify of client cert

[sunjayBhatia]

Enables users to configure Envoy to not verify client certs.
This can be used in conjunction with External Authorization as
Envoy will still require client certs are supplied and will pass
them along to an ExtAuthz server if configured.
Contour does not provision Envoy with a CA for client CA validation
if downstream validation is skipped.

Also adds a Gatekeeper template and constraint policy for operators
who want to limit HTTPProxies that try to use this setting in case
it is desired to restrict it.

Fixes #3253

[codecov]

Codecov Report

Merging #3611 (05bd0f8) into main (d9a0d51) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3611      +/-   ##
==========================================
+ Coverage   76.71%   76.73%   +0.01%     
==========================================
  Files         100      100              
  Lines        7142     7147       +5     
==========================================
+ Hits         5479     5484       +5     
  Misses       1542     1542              
  Partials      121      121              

Impacted Files	Coverage Δ
internal/dag/cache.go	95.73% <ø> (-0.08%)	⬇️
internal/dag/dag.go	96.93% <ø> (ø)
internal/dag/httpproxy_processor.go	91.71% <100.00%> (+0.14%)	⬆️
internal/envoy/v3/auth.go	100.00% <100.00%> (ø)

[stevesloka]

Overall this looks good to me, just worried that the dependency on gatekeeper will be valid for all users since this introduces a field which could create an insecure HTTPProxy.

[sunjayBhatia]

Overall this looks good to me, just worried that the dependency on gatekeeper will be valid for all users since this introduces a field which could create an insecure HTTPProxy.

Ah interesting, do you think a better path would be to add a Contour config file field so an operator could disable skip verify (require cert verification to undo the double negative) globally instead

[stevesloka]

Ah interesting, do you think a better path would be to add a Contour config file field so an operator could disable skip verify (require cert verification to undo the double negative) globally instead

I don't know what users are ok with. Just ends up being a large ask for folks when upgrading. The only other path (to get feedback but still implement), is to add a "features" flag to enable globally, then use what you have in this PR. Then after a while can remove that feature flag when folks are sure of the changes that are included.

Not sure if I'm over thinking this though.

[sunjayBhatia]

Ah interesting, do you think a better path would be to add a Contour config file field so an operator could disable skip verify (require cert verification to undo the double negative) globally instead

I don't know what users are ok with. Just ends up being a large ask for folks when upgrading. The only other path (to get feedback but still implement), is to add a "features" flag to enable globally, then use what you have in this PR. Then after a while can remove that feature flag when folks are sure of the changes that are included.

Not sure if I'm over thinking this though.

right now I don't think there is anything in Contour that ensures all HTTPProxies have downstream TLS validation set so this new field is similar to that when in use, except Envoy will require a client cert is sent as well

if operators already have a mechanism to restrict HTTPProxies that do not have downstream TLS settings this would fall into the same category

[youngnick]

I think that, in this case, having Gatekeeper as a requirement is okay because users have to specify "make this insecure by bypassing certs", rather than it being insecure by default. All we can do is make sure we've warned people in the field docs.

[youngnick]

Overall LGTM, but I think some doc changes to make the security risks of using this a bit clearer would be good.

[skriss]

Per #3253 (comment) do we need to set include_peer_certificate: true for the ext auth filter?

[sunjayBhatia]

Per #3253 (comment) do we need to set include_peer_certificate: true for the ext auth filter?

We're currently setting on the ext auth filter always which was nice:

contour/internal/envoy/v3/listener.go

Line 632 in 4fbf655

IncludePeerCertificate: true,

[skriss]

Ah, 👍

[skriss]

I think this looks good, it's a valid point that a user can already just not configure client validation and be "insecure" so to me it doesn't seem like we're adding risk with this new field.

[skriss]

Approving but it'd be good to make sure @stevesloka is OK with this too before merge