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

Add SCION HTTP Proxy to shttp lib #142

Merged
merged 12 commits into from
Jun 16, 2020
Merged

Conversation

martenwallewein
Copy link
Contributor

@martenwallewein martenwallewein commented May 3, 2020

Hi @ALL,

I've worked on setting up DVB-T video streaming over SCION and implemented a proxy component that can proxy requests from HTTP/1.1 to HTTP/3 over SCION and vice versa. We needed the proxy

  1. To make the stream over SCION accessible by the vlc media player
  2. To provide the DVB-T stream grabbed from the receiver via HTTP/3 oiver SCION

To better understant the setup, please consider the following example:

scion-video-setup

Considering the figure, the SCION HTTP Ingress Proxy was realised using the "toScion" direction and the Egress proxy using the "fromScion" direction as can be seen from the code.

It would be cool to have this implementation upstream, because we plan to integrate a demo into SCION apps that can play video streams using HTTP/3 over SCION considering a similar example as shown above, but play the stream in the browser instead of using vlc media player.

Please feel free to give me feedback on this PR :)

Best,
Marten


This change is Reviewable

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Hi Marten, thanks a lot for your contribution!

I would like to propose an alternative approach to implement this. Have a look at httplib.ReverseProxy or more specifically httplib.NewSingleHostReverseProxy. This implements a proxy as a handler for a normal http server. This is really nice, because it allows an application to compose this, e.g. adding the setconfig handlers that you now added in the library.
So my proposal would be: add a function e.g. NewSingleSCIONHostReverseProxy to shttp that creates a ReverseProxy with the Transport configured with a the SCION/QUIC round tripper.
With this, you can then easily create a proxy application that covers all four cases of local and remote SCION or IP combinations:

mux := http.NewServeMux()
if isScionAddress(remote) {
   mux.Handle("/", shttp.NewSingleSCIONHostReverseProxy(remote))
} else {
   mux.Handle("/", http.NewSingleHostReverseProxy(remote))
}

if isScionAddress(local) { 
   shttp.ListenAndServe(mux,...)
} else {
   http.ListenAndServe(mux,...)
}

Reviewed 4 of 5 files at r1.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @martenwallewein)


pkg/shttp/proxy.go, line 100 at r1 (raw file):

	log.Println("Perform config update")
	s.config = &newConfig

This seems unsafe; requests can probably be concurrent so updates to this config would have to be guarded against concurrent read (and writes).

@martenwallewein
Copy link
Contributor Author

Hi Matthias,

thanks for the feedback, I think is a more cleaner way to implement this and furthermore, the setConfig part can be moved outside of the lib itself. I'll give it a try!

@martenwallewein
Copy link
Contributor Author

martenwallewein commented May 5, 2020

I have three more questions, maybe you can help me with them:

  • is there an existing isScionAddress function? If not, could something like parseUdpAddress from https://github.com/scionproto/scion/blob/master/go/lib/snet/udpaddr.go help?
  • I would add *tls.Config parameters to shttp.ListenAndServe and shttp.NewSingleSCIONHostReverseProxy so custom tls configuration/certs can be used, is it okay to add those parameters (especially for the listenAndServe method, I don't want to break exisiting implementations)
  • Should I squash my commits in the end?

@matzf
Copy link
Contributor

matzf commented May 5, 2020

I don't think there is a public function like that, but you can use snet.ParseUDPAddr and check for errors, as you suggested.

  • I would add *tls.Config parameters to shttp.ListenAndServe and shttp.NewSingleSCIONHostReverseProxy so custom tls configuration/certs can be used, is it okay to add those parameters

Sure, that sounds good to me.

  • Should I squash my commits in the end?

You can if you like, but don't worry about it. We squash merge all pull requests anyway, so whatever history you have will be fine.

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @martenwallewein)


_examples/shttp/proxy/main.go, line 27 at r2 (raw file):

	// which we can use to proxy to SCION
	if _, err := snet.ParseUDPAddr(*remote); err == nil {
		mux.Handle("/", shttp.NewSingleSCIONHostReverseProxy(*remote, nil))

I think you will want to pass &tls.Config{InsecureSkipVerify: true} here as this is no longer added by default (this has just changed on master).


pkg/shttp/proxy.go, line 19 at r2 (raw file):

	}

	handler := func(wr http.ResponseWriter, r *http.Request) {

This implements the proxy-ing manually, and I'm guess there are many subtle ways to do this wrong. Is there a reason to not use the ReverseProxy handler from httputil, e.g. like so?

func NewSingleSCIONHostReverseProxy(remote string, cliTLSCfg *tls.Config) *httputil.ReverseProxy {
   targetURL := url.Parse(MangleSCIONURL(remote))
   proxy := httputil.NewSingleHostReverseProxy(targetURL);
   proxy.Transport = NewRoundTripper(cliTLSCfg, nil)
   return proxy
}

pkg/shttp/README.md, line 71 at r2 (raw file):

The proxy can handle two directions: From HTTP/1.1 to SCION and from SCION to HTTP/1.1. It's idea is to make resources provided over HTTP accessible over the SCION network. 

To use the proxy, consider the proxy example in _examples. This implementation detects from the format of the `remote` and `local` argument if it should listen on SCION/HTTP/1.1 and proxy to SCION/HTTP/1.1.

Unimportant, but quic-go actually implements http/3.


pkg/shttp/README.md, line 73 at r2 (raw file):

To use the proxy, consider the proxy example in _examples. This implementation detects from the format of the `remote` and `local` argument if it should listen on SCION/HTTP/1.1 and proxy to SCION/HTTP/1.1.

```Go

Don't replicate the entire example here. You can add a link to the go file, like so: [_examples/shttp/proxy](../../_examples/shttp/proxy/main.go) (not completely sure about the relative path though)

@martenwallewein
Copy link
Contributor Author

Hi Matthias, sorry for the late response. I added your feedback, compared to the initial implementation this one is now a lot smarter, thanks!

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

No worries, thank you for your contribution. Just one small comment left.

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @martenwallewein)


_examples/shttp/proxy/main.go, line 27 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

I think you will want to pass &tls.Config{InsecureSkipVerify: true} here as this is no longer added by default (this has just changed on master).

Done.


pkg/shttp/proxy.go, line 19 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

This implements the proxy-ing manually, and I'm guess there are many subtle ways to do this wrong. Is there a reason to not use the ReverseProxy handler from httputil, e.g. like so?

func NewSingleSCIONHostReverseProxy(remote string, cliTLSCfg *tls.Config) *httputil.ReverseProxy {
   targetURL := url.Parse(MangleSCIONURL(remote))
   proxy := httputil.NewSingleHostReverseProxy(targetURL);
   proxy.Transport = NewRoundTripper(cliTLSCfg, nil)
   return proxy
}

Done.


pkg/shttp/proxy.go, line 18 at r3 (raw file):

	targetURL, err := url.Parse(sUrl)
	if err != nil {
		log.Fatalf("Failed to parse SCION URL %s", err)

Just return the error so the application code can decide how to continue.


pkg/shttp/README.md, line 73 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Don't replicate the entire example here. You can add a link to the go file, like so: [_examples/shttp/proxy](../../_examples/shttp/proxy/main.go) (not completely sure about the relative path though)

Done.

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @martenwallewein)


pkg/shttp/proxy.go, line 18 at r3 (raw file):

Previously, matzf (Matthias Frei) wrote…

Just return the error so the application code can decide how to continue.

Done.

@matzf
Copy link
Contributor

matzf commented Jun 9, 2020

Thanks @martenwallewein. If you could quickly update your branch, we'll get this merged :shipit:

@martenwallewein
Copy link
Contributor Author

Thanks, I updated the branch.

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, 2 of 2 files at r6.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@matzf matzf merged commit 47ed345 into netsec-ethz:master Jun 16, 2020
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.

2 participants