shttp, skip: use net/http instead of quic-go/http3, support tunneling by matzf · Pull Request #198 · netsec-ethz/scion-apps

matzf · 2021-08-13T11:19:47Z

The "default" HTTP package for HTTP over SCION, shttp, now uses HTTP/1
or HTTP/2 based on the net/http standard library.
This allows our applications to talk to "normal" (i.e. non-HTTP/3)
web servers / clients when suitably proxied/tunneled.
Also, this now supports http://, which is useful for quickly running the
demo applications without nasty tricks or having to hassle with TLS
setup.

The existing library for HTTP/3 over SCION is moved to a separate
package shttp3, overhauled and reduced to the bare necessities.
This will need some additional love to make it useful (and used).
Normally HTTP/3 is using the same port as HTTPS but on UDP instead of
TCP -- we run both over UDP so we'll have to come up with something to
disambiguate.

Implement CONNECT in skip to support HTTPS. Change the way that SCION
hosts are identified, as the .scion-TLD approach is incompatible with
HTTPS (wrong name for certificates). Instead, it just looks at the list
of hosts for which a SCION address is configured in /etc/hosts or
/etc/scion/hosts.

Add a web gateway that acts as a proxy/gateway to allow mirroring some
content on the TCP/IP web into the SCION based web.

This change is

matzf · 2021-08-13T11:32:46Z

Note: this is a breaking change, both in terms of API and protocol. Applications using shttp now are not be compatible with applications built before this change as they talk a different protocol (HTTP/1 or 2 now vs HTTP/3 before).

Note 2: I'm planning to add more API breaking changes soon by changing the shttp libraries to "pan", #187.

FR4NK-W

Neat stuff! 🎉

Reviewed 22 of 22 files at r1, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

_examples/shttp/README.md, line 30 at r1 (raw file):

 run `example-shttp-server`:

Nit:
-> "run the example-shttp-server:"

_examples/shttp/README.md, line 74 at r1 (raw file):

obtain **certificate**

-> "obtain the certificate"

_examples/shttp/server/main.go, line 85 at r1 (raw file):

log.Fatal(shttp.ListenAndServe(":80", handler))

I think it would be nice if we could have the example be "secure", now that we actually use TLS on the outer connection.
I was hopping this was still updated and we could reuse it:
https://github.com/lucas-clemente/quic-go/tree/v0.10.0/example providing valid keys for quic.clemente.io.

Or maybe simply duplicate the two lines for the cert and key flags from shttp/fileserver/main.go and have a separate section about using the examples with TLS since you are already providing the setup instruction for the certificate later on for the fileserver?

bat/bat.go, line 193 at r1 (raw file):

shttp.MangleSCIONAddrURL(proxy)

Does this work, does bat support connecting to the proxy via SCION?
What is the use case for also connecting to the proxy over SCION?

pkg/shttp/transport.go, line 36 at r1 (raw file):

}).DialContext,

Maybe add some more detailed comment here and for the DialContext function that we are basically providing a RoundTripper with overridden DialContext.
Setting DialContext also disables HTTP/2, right?

pkg/shttp3/transport.go, line 1 at r1 (raw file):

2021 🙂

skip/main.go, line 33 at r1 (raw file):

text/template

Should we use package html/template to benefit from JSEscaper/autoescaping when expanding the .pac template to avoid issues with invalid hostnames?

skip/main.go, line 130 at r1 (raw file):

other

Nit:

I'd call the other handler the default handler.

skip/main.go, line 159 at r1 (raw file):

w.WriteHeader(http.StatusOK)

Is there a reason we reply with the HTTP code 200 before we hijack the connection?
Would it not make things easier for debugging and so on, to do that after the hijack? Or is that because we want to keep the CONNECT semantic where the proxy switches to tunnel mode immediately after the successful response?

skip/main.go, line 164 at r1 (raw file):

clientConn, _, err := hijacker.Hijack()

Support for HTTP/2 is mentioned in the PR comment, but the docu for the Hijackerinterface mentions:

The default ResponseWriter for HTTP/1.x connections supports Hijacker, but HTTP/2 connections intentionally do not.

Is my docu outdated, or is there an other reason why this still would works for HTTP/2 ?

skip/main.go, line 230 at r1 (raw file):

// parseHostsFile, copied/simplified from pkg/appnet/hostsfile.go

Could we not simply get the keys from the hostsTable?
Or move these helper functions to pkg/appnet/hostfile.go to reduce duplication?

skip/skip.pac, line 2 at r1 (raw file):

{{range .SCIONHosts}}  "{{.}}",

I think this breaks if SCIONHosts is empty, has length 0, so => {{range .SCIONHosts}} "{{.}}", {{else}} {{end}}

web-gateway/main.go, line 115 at r1 (raw file):
Nit:

// Status code

TLS has "alert codes"/Messages, https://datatracker.ietf.org/doc/html/rfc8446#appendix-B.2 , but I guess that is not applicable here.

(arbitrary, this is not HTTP).

So maybe just say that we are (re)using the http status codes 501-504 with similar meaning? Or directly only log the error string, and not a status code, if we don't want to define the status codes more precisely.

web-gateway/README.md, line 18 at r1 (raw file):

scionlab.org www.scionlab.org www.scion-architecture.net

I guess that should go into a config file in a future version.
Do subdomains always need to be configured separately, or only if they don't share the same TLS certificate/server?

matzf

Reviewable status: 10 of 22 files reviewed, 12 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

_examples/shttp/README.md, line 30 at r1 (raw file):

Previously, FR4NK-W wrote…

 run `example-shttp-server`:
Nit:
-> "run the example-shttp-server:"

Done.

_examples/shttp/README.md, line 74 at r1 (raw file):

Previously, FR4NK-W wrote…

obtain **certificate**
-> "obtain the certificate"

Done.

_examples/shttp/server/main.go, line 85 at r1 (raw file):

Previously, FR4NK-W wrote…

log.Fatal(shttp.ListenAndServe(":80", handler))
I think it would be nice if we could have the example be "secure", now that we actually use TLS on the outer connection.
I was hopping this was still updated and we could reuse it:
https://github.com/lucas-clemente/quic-go/tree/v0.10.0/example providing valid keys for quic.clemente.io.

Or maybe simply duplicate the two lines for the cert and key flags from shttp/fileserver/main.go and have a separate section about using the examples with TLS since you are already providing the setup instruction for the certificate later on for the fileserver?

Haha, that's pretty funny. I've added the same optional TLS support here, I guess that's the most straight straight forward approach.
Perhaps we could also obtain a similar valid but useless certificate to include in the repository for testing. That'd make the README a bit shorter too :)

bat/bat.go, line 193 at r1 (raw file):

Previously, FR4NK-W wrote…

shttp.MangleSCIONAddrURL(proxy)
Does this work, does bat support connecting to the proxy via SCION?
What is the use case for also connecting to the proxy over SCION?

Yes it works. It has also previously worked for named SCION hosts but not for "naked" SCION addresses. I don't have a specific use case, this is just fixing an edge case in existing functionality.

pkg/shttp/transport.go, line 36 at r1 (raw file):

Previously, FR4NK-W wrote…

}).DialContext,
Maybe add some more detailed comment here and for the DialContext function that we are basically providing a RoundTripper with overridden DialContext.
Setting DialContext also disables HTTP/2, right?

I've read through the http docs a bit and noticed that the net/http.DefaultTransport sets DialContext too, but also sets ForceAttemptHTTP2. I've adapted our code to do the same thing. In the process, I've changed the API a bit, replacing the NewRoundTripper function with a DefaultTransport variable -- this seems a bit more in-line with the organisation in net/http.

I've also extended the doc strings a bit, is this roughly what you meant?

pkg/shttp3/transport.go, line 1 at r1 (raw file):

Previously, FR4NK-W wrote…

2018
2021 🙂

Done.

skip/main.go, line 33 at r1 (raw file):

Previously, FR4NK-W wrote…

text/template
Should we use package html/template to benefit from JSEscaper/autoescaping when expanding the .pac template to avoid issues with invalid hostnames?

Great catch! I saw that there is a js filter that I've added to the appropriate places in the .pac template. The autoescaping seems a bit daunting to me.

skip/main.go, line 130 at r1 (raw file):

Previously, FR4NK-W wrote…

other
Nit:

I'd call the other handler the default handler.

default is a keyword. Renamed it to next, this is a "convention" used e.g. in gorilla/mux.

skip/main.go, line 159 at r1 (raw file):

Previously, FR4NK-W wrote…

w.WriteHeader(http.StatusOK)
Is there a reason we reply with the HTTP code 200 before we hijack the connection?
Would it not make things easier for debugging and so on, to do that after the hijack? Or is that because we want to keep the CONNECT semantic where the proxy switches to tunnel mode immediately after the successful response?

At this point we've established a connection to the remote host, and we're know that things will work. The only thing that still happens before starting the "tunnel" is to do this Hijacking, which is just a library operation to get access to the underlying connection. Once we've Hijacked, the ResponseWriter can no longer write to this connection so writing the Header afterwards would fail. I guess we could hijack the connection and then manually write the header back, but I can't see any advantage here.

skip/main.go, line 164 at r1 (raw file):

Previously, FR4NK-W wrote…

clientConn, _, err := hijacker.Hijack()
Support for HTTP/2 is mentioned in the PR comment, but the docu for the Hijackerinterface mentions:

The default ResponseWriter for HTTP/1.x connections supports Hijacker, but HTTP/2 connections intentionally do not.

Is my docu outdated, or is there an other reason why this still would works for HTTP/2 ?

That's a fair point and honestly I've not looked at or tested HTTP/2 at all, I mentioned it in the docs because net/http mentions it.

Anyway, this here will always work because this proxy is not serving with TLS, and so HTTP/2 is disabled. Added a comment to clarify this.

skip/main.go, line 230 at r1 (raw file):

Previously, FR4NK-W wrote…

// parseHostsFile, copied/simplified from pkg/appnet/hostsfile.go
Could we not simply get the keys from the hostsTable?
Or move these helper functions to pkg/appnet/hostfile.go to reduce duplication?

Because that's not exported and I think that it shouldn't be.
This here is a hack; there is currently no good way to select for which hosts this proxy should be used and for which it shouldn't be, and this here won't make any sense once RAINS/DNS is really used -- a different mechanism will be needed, either something configurable in the browser UI or some racing/happy eyeballs mechanism here in skip.
I prefer to not add functionality to the API that is only used for hacks. Duplicating a few dozen lines of code seems more appropriate ;)

skip/skip.pac, line 2 at r1 (raw file):

Previously, FR4NK-W wrote…

{{range .SCIONHosts}}  "{{.}}",
I think this breaks if SCIONHosts is empty, has length 0, so => {{range .SCIONHosts}} "{{.}}", {{else}} {{end}}

The docs for this range statement say: "If the value of the pipeline has length zero, nothing is output;". This seems what we want, and I've just also just tested it it, looks ok. It expands to:

const scionHosts = new Set([
])

web-gateway/main.go, line 115 at r1 (raw file):

So maybe just say that we are (re)using the http status codes 501-504 with similar meaning.

That's exactly what i intended to say. I've rephrased it a bit.
The idea was really to spit out something like the "Common Log Format" (as written out by LoggingHandler), to keep it somewhat consistent and, potentially, machine readable. And that only has room for a code, not a message.

web-gateway/README.md, line 18 at r1 (raw file):

Previously, FR4NK-W wrote…

scionlab.org www.scionlab.org www.scion-architecture.net
I guess that should go into a config file in a future version.
Do subdomains always need to be configured separately, or only if they don't share the same TLS certificate/server?

Sure, we could have config file.

Yes, every subdomain needs to be configured; we use the normal ServeMux to route the requests to individual proxy handlers, and this needs to be set explicitly for every subdomain, AFAIU. We could get rid of this restriction, by doing things a bit more manually.

FR4NK-W

Reviewed 5 of 7 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

_examples/shttp/client/main.go, line 39 at r3 (raw file):

// Create a standard client with our custom RoundTripper

-> "// Create a client with our Transport/Dialer:" to match the README

pkg/shttp/transport.go, line 36 at r1 (raw file):