Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal: filter misdirected TLS requests #2483

Merged
merged 1 commit into from
May 22, 2020

Conversation

jpeach
Copy link
Contributor

@jpeach jpeach commented Apr 29, 2020

TLS routes are specialized to a unique virtual hostname. However, if
wildcard certificates are being used, browsers will aggressively coalesce
and reuse server connections even when the full origin hostname doesn't
match. This results on 404 responses because each TLS virtual host only
has routes for one host.

We can avoid this behaviour bleeding out to users by generating a 421
Misdirected Request response if the request authority doesn't match
the FQDN of the virtual host. In this case, the browser is supposed
to understand that the request wasn't processed and re-send it on a
new connection.

Signed-off-by: James Peach jpeach@vmware.com

@jpeach jpeach requested review from stevesloka and youngnick April 29, 2020 07:01
@jpeach jpeach force-pushed the lua-misdirected-request branch from 37e4f48 to db6bea0 Compare April 29, 2020 07:08
@jpeach
Copy link
Contributor Author

jpeach commented Apr 29, 2020

When using the wrong host, envoy responds with a 421:

$ /usr/bin/curl -v -k -H "Host: foo.jpeach.org" --resolve echo-no-auth.projectcontour.io:443:127.0.0.1 https://echo-no-auth.projectcontour.io/get
* Added echo-no-auth.projectcontour.io:443:127.0.0.1 to DNS cache
* Hostname echo-no-auth.projectcontour.io was found in DNS cache
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to echo-no-auth.projectcontour.io (127.0.0.1) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/cert.pem
  CApath: none
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-CHACHA20-POLY1305
* ALPN, server accepted to use h2
* Server certificate:
*  subject: O=cert-manager
*  start date: Apr 29 06:37:40 2020 GMT
*  expire date: Jul 28 06:37:40 2020 GMT
*  issuer: O=cert-manager; OU=io + OU=projectcontour + OU=testsuite; CN=issuer
*  SSL certificate verify result: unable to get local issuer certificate (20), continuing anyway.
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x7fd16f00d600)
> GET /get HTTP/2
> Host: foo.jpeach.org
> User-Agent: curl/7.64.1
> Accept: */*
>
* Connection state changed (MAX_CONCURRENT_STREAMS == 2147483647)!
< HTTP/2 421
< content-length: 0
< date: Wed, 29 Apr 2020 07:08:14 GMT
< server: envoy
<
* Connection #0 to host echo-no-auth.projectcontour.io left intact
* Closing connection 0

When using the correct host, routing works right:

* Added echo-no-auth.projectcontour.io:443:127.0.0.1 to DNS cache
* Hostname echo-no-auth.projectcontour.io was found in DNS cache
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to echo-no-auth.projectcontour.io (127.0.0.1) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/cert.pem
  CApath: none
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-CHACHA20-POLY1305
* ALPN, server accepted to use h2
* Server certificate:
*  subject: O=cert-manager
*  start date: Apr 29 06:37:40 2020 GMT
*  expire date: Jul 28 06:37:40 2020 GMT
*  issuer: O=cert-manager; OU=io + OU=projectcontour + OU=testsuite; CN=issuer
*  SSL certificate verify result: unable to get local issuer certificate (20), continuing anyway.
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x7febc980b400)
> GET /get HTTP/2
> Host: echo-no-auth.projectcontour.io
> User-Agent: curl/7.64.1
> Accept: */*
>
* Connection state changed (MAX_CONCURRENT_STREAMS == 2147483647)!
< HTTP/2 200
< content-type: application/json
< date: Wed, 29 Apr 2020 07:08:20 GMT
< content-length: 435
< x-envoy-upstream-service-time: 1
< server: envoy
<
* Connection #0 to host echo-no-auth.projectcontour.io left intact
{"TestId":"ingress-conformance-echo","Path":"/get","Host":"echo-no-auth.projectcontour.io","Method":"GET","Proto":"HTTP/1.1","Headers":{"Accept":["*/*"],"Content-Length":["0"],"User-Agent":["curl/7.64.1"],"X-Envoy-Expected-Rq-Timeout-Ms":["15000"],"X-Envoy-Internal":["true"],"X-Forwarded-For":["172.17.0.1"],"X-Forwarded-Proto":["https"],"X-Request-Id":["0a813dec-dc78-4e4e-b1f2-44c15d41608f"],"X-Request-Start":["t=1588144100.581"]}}* Closing connection 0

@jpeach
Copy link
Contributor Author

jpeach commented Apr 29, 2020

@stevesloka @youngnick LMK what you think about this approach. As far I know, this is the only way to fix #1493 .

@stevesloka
Copy link
Member

The right place is to fix upstream in Envoy, but this is a good workaround for now. This has existing for some time, nice work @jpeach!

@youngnick
Copy link
Member

If it's the only way for now, I'm okay with it. I think we need to be clear that this doesn't mean we'll be adding support for arbitrary Lua filters (yet). I would like to see this removed in favour of a better upstream solution if it's implemented.

@jpeach
Copy link
Contributor Author

jpeach commented May 1, 2020

If it's the only way for now, I'm okay with it. I think we need to be clear that this doesn't mean we'll be adding support for arbitrary Lua filters (yet). I would like to see this removed in favour of a better upstream solution if it's implemented.

I don't see much upstream activity on this. If we want something upstream we probably need to make a proposal and drive it.

@youngnick
Copy link
Member

I can't do C++, so I can't do the coding, but I agree.

@stevesloka
Copy link
Member

I just chatted with another Contour user who ran into this. They are going to make a build and test it out to verify (in their environment) that this solves, but I think we should merge this if the tests go well and then release a v1.14.1 to include this and the ingress.class status info fixes.

@jpeach jpeach force-pushed the lua-misdirected-request branch 2 times, most recently from 64ca014 to b9ef916 Compare May 11, 2020 02:53
@jpeach jpeach marked this pull request as ready for review May 11, 2020 02:53
@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #2483 into master will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2483      +/-   ##
==========================================
+ Coverage   77.10%   77.20%   +0.10%     
==========================================
  Files          70       70              
  Lines        5736     5770      +34     
==========================================
+ Hits         4423     4455      +32     
- Misses       1218     1219       +1     
- Partials       95       96       +1     
Impacted Files Coverage Δ
internal/contour/listener.go 93.47% <100.00%> (+0.10%) ⬆️
internal/envoy/listener.go 97.15% <100.00%> (+0.50%) ⬆️
internal/featuretests/envoy.go 100.00% <100.00%> (ø)
internal/dag/cache.go 95.25% <0.00%> (-0.73%) ⬇️

@jpeach jpeach force-pushed the lua-misdirected-request branch 3 times, most recently from 09a3f79 to 191af02 Compare May 11, 2020 05:54
@primeroz
Copy link

In my use case this seems to fix the issue , even though i am confused by the regex in https://github.com/projectcontour/contour/pull/2483/files#diff-98b92a08d0022a6c73eceeb2e1d99a43R3128 which i would expect to be a negation ... but i obviously am not understanding this properly since it works :)

my setup

with 1.4.0

  • Open Incognito Chromium
  • Hit https://podinfo-tls.example.com works
  • Hit , in the same incognito session, https://vault.example.com fail with 404

Using a build of this branch

  • Open Incognito Chromium
  • Hit https://podinfo-tls.example.com works
  • Hit , in the same incognito session, https://vault.example.com works

@jpeach jpeach force-pushed the lua-misdirected-request branch 2 times, most recently from 2efdcc0 to b58edfb Compare May 20, 2020 03:56
@jpeach
Copy link
Contributor Author

jpeach commented May 20, 2020

@youngnick @stevesloka Could you please review? The diff is pretty big unfortunately, due to testing impact. The main functional change is around the new FilterMisdirectedRequests API.

@jpeach jpeach force-pushed the lua-misdirected-request branch from b58edfb to 70e2937 Compare May 21, 2020 00:53
TLS routes are specialized to a unique virtual hostname. However, if
wildcard certificates are being used, browsers will aggressively coalesce
and reuse server connections even when the full origin hostname doesn't
match. This results on 404 responses because each TLS virtual host only
has routes for one host.

We can avoid this behaviour bleeding out to users by generating a 421
Misdirected Request response if the request authority doesn't match
the FQDN of the virtual host. In this case, the browser is supposed
to understand that the request wasn't processed and re-send it on a
new connection.

This fixes projectcontour#1493.

Signed-off-by: James Peach <jpeach@vmware.com>
@jpeach jpeach force-pushed the lua-misdirected-request branch from 70e2937 to a78acf1 Compare May 22, 2020 03:07
Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice. One small question about how this and the fallback cert interact.

internal/contour/listener.go Show resolved Hide resolved
@jpeach jpeach merged commit e596934 into projectcontour:master May 22, 2020
@jpeach jpeach deleted the lua-misdirected-request branch May 22, 2020 06:54
@jpeach jpeach added this to the 1.5.0 milestone May 28, 2020
@ashirvadgupta
Copy link

ashirvadgupta commented Jun 3, 2020

@jpeach In my setup, I am changing the envoy https port from 443 to 8443. Its trying to match the host portal.example.com but its getting host as portal.example.com:8443.
How should I make it work?

Here is curl output

curl https://portal.devtest.example.com:8443/devtest -v

  • Trying xx.xx.xx.xx...
  • TCP_NODELAY set
  • Connected to portal.devtest.example.com (xx.xx.xx.xx) port 8443 (#0)
  • ALPN, offering h2
  • ALPN, offering http/1.1
  • Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@strength
  • successfully set certificate verify locations:
  • CAfile: /etc/ssl/cert.pem
    CApath: none
  • TLSv1.2 (OUT), TLS handshake, Client hello (1):
  • TLSv1.2 (IN), TLS handshake, Server hello (2):
  • TLSv1.2 (IN), TLS handshake, Certificate (11):
  • TLSv1.2 (IN), TLS handshake, Server key exchange (12):
  • TLSv1.2 (IN), TLS handshake, Server finished (14):
  • TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
  • TLSv1.2 (OUT), TLS change cipher, Client hello (1):
  • TLSv1.2 (OUT), TLS handshake, Finished (20):
  • TLSv1.2 (IN), TLS change cipher, Client hello (1):
  • TLSv1.2 (IN), TLS handshake, Finished (20):
  • SSL connection using TLSv1.2 / ECDHE-RSA-CHACHA20-POLY1305
  • ALPN, server accepted to use h2
  • Server certificate:
  • subject: C=US; ST=California; L=San Jose; O=example Inc; OU=ESD; CN=*.devtest.example.com
  • start date: May 19 00:00:00 2020 GMT
  • expire date: May 20 12:00:00 2022 GMT
  • subjectAltName: host "portal.devtest.example.com" matched cert's "*.devtest.example.com"
  • issuer: C=US; O=DigiCert Inc; CN=DigiCert SHA2 Secure Server CA
  • SSL certificate verify ok.
  • Using HTTP2, server supports multi-use
  • Connection state changed (HTTP/2 confirmed)
  • Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
  • Using Stream ID: 1 (easy handle 0x7ffb20806600)

GET /devtest HTTP/2
Host: portal.devtest.example.com:8443
User-Agent: curl/7.54.0
Accept: /

  • Connection state changed (MAX_CONCURRENT_STREAMS updated)!
    < HTTP/2 421
    < content-length: 0
    < date: Wed, 03 Jun 2020 20:19:13 GMT
    < server: envoy
    <
  • Connection #0 to host portal.devtest.example.com left intact

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants