internal: implement fallback certificate #2477
Conversation
stevesloka
added
the
release-note
Codecov Report
@@ Coverage Diff @@
## master #2477 +/- ##
==========================================
+ Coverage 76.79% 76.97% +0.18%
==========================================
Files 68 68
Lines 5597 5707 +110
==========================================
+ Hits 4298 4393 +95
- Misses 1203 1218 +15
Partials 96 96
|
stevesloka
force-pushed
the
fallbackCert
branch
3 times, most recently
from
c5b4497 to
2dbce5a
Compare
|
@jpeach @youngnick I think this is good for a first step. The remaining bits are to do the checks on if the delegated certificate is set correctly & docs. I think the review might be easier if we can merge this then work on that in a separate PR. |
There was a problem hiding this comment.
Reviewed 24 of 24 files at r1.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @stevesloka and @youngnick)
apis/projectcontour/v1/httpproxy.go, line 135 at r1 (raw file):
// EnableFallbackCertificate defines if the vhost should allow a default certificate to // be applied which handles all requests which don't match the SNI defined in this vhost
This is used-visible in the API documentation, so should have a period.
examples/contour/01-contour-config.yaml, line 30 at r1 (raw file):
# minimum TLS version that Contour will negotiate # minimum-protocol-version: "1.1"
Remove these lines.
internal/contour/listener.go, line 410 at r1 (raw file):
if vh.FallbackCertificate != nil && !envoy.ContainsFallbackFilterChain(v.listeners[ENVOY_HTTPS_LISTENER].FilterChains) { // Choose the higher of the configured or requested TLS version.
Update comment to xref the issue where protocol versioning will be pinned to the default.
internal/contour/listener_test.go, line 131 at r1 (raw file):
}, }, },
What is this case testing? We never have a listener called ENVOY_FALLBACK_CERTIFICATE, so to me it looks like this ends up the same as the "exact match" case.
internal/contour/listener_test.go, line 153 at r1 (raw file):
} fallbackCertFilterFor := func() *envoy_api_v2_listener.Filter {
Since this isn't a filter for anything, we can just make it a local variable named fallbackCertFilter or fallbackFilter.
internal/contour/listener_test.go, line 1455 at r1 (raw file):
} root := builder.Build()
This can be replaced with buildDAGFallback().
internal/contour/route.go, line 193 at r1 (raw file):
A fallback route contains all the routes that match a vhost who has enabled.
Suggest "A fallback route configuration contains routes for all the vhosts that have the fallback certificate enabled."
internal/contour/route_test.go, line 2142 at r1 (raw file):
}, )), envoy.RouteConfiguration("ingress_fallbackcert",
Use the ENVOY_FALLBACK_CERTIFICATE constant here and below.
internal/contour/route_test.go, line 2541 at r1 (raw file):
)), ), },
There's no test case for 2 proxies with fallback enabled ... I hate to ask for more boilerplate fixtures, but do we need one here?
internal/dag/builder.go, line 526 at r1 (raw file):
are not compatible together
"are incompatible"
internal/dag/builder.go, line 533 at r1 (raw file):
Certificate Secret
In the message below, the terminology is "fallback certificate Secret", which I think is nice and clear. Let's use that here too.
internal/dag/builder.go, line 539 at r1 (raw file):
%s/%s
You can use %q here and just print b.FallbackCertificate to get the same result.
internal/featuretests/fallbackcert_test.go, line 191 at r1 (raw file):
// Resources: nil, // TypeUrl: listenerType, //})
Remove this commented code?
|
@stevesloka I tried using I think we are in good shape here, all the above is just various nits, and we can do the last remaining functional stuff in followup PRs. If you merge, could you please update the commit message to follow the contributing conventions (i.e. the format you've used for the PR), otherwise I can just edit it in the squash UI. |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @jpeach and @youngnick)
apis/projectcontour/v1/httpproxy.go, line 135 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
This is used-visible in the API documentation, so should have a period.
Done.
examples/contour/01-contour-config.yaml, line 30 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
# minimum TLS version that Contour will negotiate # minimum-protocol-version: "1.1"Remove these lines.
Done.
internal/contour/listener.go, line 410 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
Update comment to xref the issue where protocol versioning will be pinned to the default.
I actually reworked this, it was checking for the value against itself, so I just use the default if defined.
internal/contour/listener_test.go, line 131 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
What is this case testing? We never have a listener called
ENVOY_FALLBACK_CERTIFICATE, so to me it looks like this ends up the same as the "exact match" case.
Yup copied the exact case match but with the different listener name.
internal/contour/listener_test.go, line 153 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
Since this isn't a filter for anything, we can just make it a local variable named
fallbackCertFilterorfallbackFilter.
Done.
internal/contour/listener_test.go, line 1455 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
This can be replaced with
buildDAGFallback().
Done.
internal/contour/route.go, line 193 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
A fallback route contains all the routes that match a vhost who has enabled.Suggest "A fallback route configuration contains routes for all the vhosts that have the fallback certificate enabled."
Done.
internal/contour/route_test.go, line 2142 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
Use the
ENVOY_FALLBACK_CERTIFICATEconstant here and below.
Done.
internal/contour/route_test.go, line 2541 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
There's no test case for 2 proxies with fallback enabled ... I hate to ask for more boilerplate fixtures, but do we need one here?
Done.
internal/dag/builder.go, line 526 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
are not compatible together"are incompatible"
Done.
internal/dag/builder.go, line 533 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
Certificate SecretIn the message below, the terminology is "fallback certificate Secret", which I think is nice and clear. Let's use that here too.
Done.
internal/dag/builder.go, line 539 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
%s/%sYou can use
%qhere and just printb.FallbackCertificateto get the same result.
What I dislike about it is you get "value" in the logs.
internal/featuretests/fallbackcert_test.go, line 191 at r1 (raw file):
Previously, jpeach (James Peach) wrote…
Remove this commented code?
I fixed the test. =)
|
@jpeach rebased & updated. Reveiwable is going to take some time to get used to if we continue to use that going forward. |
|
@jpeach I reverted the |
internal/contour/listener.go
Outdated
| @@ -33,6 +33,7 @@ import ( | |||
|
|
|||
| const ( | |||
| ENVOY_HTTP_LISTENER = "ingress_http" | |||
| ENVOY_FALLBACK_CERTIFICATE = "ingress_fallbackcert" | |||
There was a problem hiding this comment.
Rename this ENVOY_FALLBACK_ROUTE_CONFIG since it is the name of a RouteConfg, not name of a certificate.
There was a problem hiding this comment.
They should all be configs then, no?
e.g. ENVOY_HTTP_LISTENER_CONFIG?
There was a problem hiding this comment.
ENVOY_HTTP_LISTENER is the name of a listener object so that makes sense to my reading. Maybe ENVOY_FALLBACK_ROUTECONFIG because it is the name of a RouteConfig?
There was a problem hiding this comment.
But ENVOY_HTTP_LISTENER is also a route config same as the FALLBACK, it just so happens that that name is shared with the listener as well.
Example:
"dynamic_route_configs": [
{
"version_info": "6",
"route_config": {
"@type": "type.googleapis.com/envoy.api.v2.RouteConfiguration",
"name": "ingress_http", <-------------- route config name
"virtual_hosts": [
We should probably call it something like ENVOY_TLSFALLBACK_ROUTECONFIG to denote that it's a TLS fallback. It's also possible we'll have more in the future.
…ill still match a filterchain in envoy and will present a default/fallback certificate to the client Signed-off-by: Steve Sloka <[email protected]>
|
On May 15, 2020, at 11:34 PM, Steve Sloka ***@***.***> wrote:
ENVOY_TLSFALLBACK_ROUTECONFIG
Yeh I like this name a lot!
|
Fixes #1503 by adding a
FallbackCertificateso that clients who do not offer SNI will still match a filterchain in Envoy. Envoy will present a default/fallback certificate to the client.Note: One side-effect of this is when you have 2 fqdn's in a cluster, one's enabled for fallback and the other isn't, requests will only route without SNI for the enabled cluster, however, if you request a cert for the non-enabled fqdn, the fallback cert will still be presented. The normal result is an error from Envoy:
curl: (35) LibreSSL SSL_connect: SSL_ERROR_SYSCALL in connection to fdqn:443Docs to come in a new PR.
Signed-off-by: Steve Sloka [email protected]
This change is