search

etix / mirrorbits

Mirrorbits is a geographical download redirector written in Go for distributing files efficiently across a set of mirrors.
MIT License
503 stars 91 forks source link

http/context: require HTTPS based on X-Forwarded-Proto #97

Closed florolf closed 3 years ago

florolf commented 5 years ago

If mirrorbits is running behind a reverse proxy, it should honor the X-Forwarded-Proto header to avoid downgrading the connection from HTTPS to HTTP.

etix commented 4 years ago

Hi @florolf,

I see your point and it makes sense. But what if you have only a few mirrors and most of them are not running over a secure connection? Your patch would exclude them automatically right?

Take VLC for example, our binaries are cryptographically signed and our updater doesn't really care about being on an HTTP or HTTPS connection. Since our mirrorbits server is served only in HTTPS we would de facto exclude all http-only mirrors (and we still have a bunch running without HTTPS).

I originally introduced the ?https=1 parameter to force the initial download of VLC over a secure connection and then we drop the option for updates.

Maybe you can tell me more about your precise use-case ?

darix commented 4 years ago

Yes it would mean you have to track https state of your mirrors and encourage them to setup https. which luckily with letsencrypt got a lot easier.

The problem is ... many clients block protocol downgrades. so they cant access the mirror after the redirect.

yol commented 3 years ago

Can we have this as an option so it would not break VLC HTTP downloads, but can be enabled by interesting parties? At Kodi we have the same problem; we would have to add ?https=1 to every public download link because some browsers nowadays block HTTPS->HTTP downgrade.

etix commented 3 years ago

@yol does the provide patch works for you?

yol commented 3 years ago

we have deployed it now, will report back with results

wsnipex commented 3 years ago

we ended up using https://github.com/xbmc/mirrorbits/commit/f6ff77155348c16fdba2d6998e1cb8b62bbc1938

While playing around with this, I realized that it would be nice to also also set WITHOUTTLS when the proto is http, but that automatically excludes all mirrors that have an https url, which is more then half for us. Unless we add each mirror twice, with http and https urls, which also means we would check them twice. I guess an option to test mirrors for https automatically or a config option for both protocols would be helpful here.

etix commented 3 years ago

The version of @wsnipex is a bit cleaner than the original PR so I cherry-picked this one into the master but they are functionally equivalent.

About the WITHOUTTLS issue, this would indeed require testing each mirror for HTTP and HTTPS availability during the background checks. But I don't think it's worth the time implementing it since unsecured HTTP mirrors are more and more getting phased out but I'm open for a PR if someone thinks otherwise.

Anyway thanks for your contribution @florolf, I'm now closing this issue since it's now merged into the master.