cargo-binstall icon indicating copy to clipboard operation
cargo-binstall copied to clipboard

Only https for requests

Open somehowchris opened this issue 2 years ago • 7 comments

Quickly looking over some request code, it seems to me that binaries are also allowed to be downloaded over http. Am I missing something? Is there are reason to allow this? reqwest has a client builder flag https_only but it is disabled by default

somehowchris avatar Apr 29 '22 11:04 somehowchris

Am I missing something?

Probably not, though at the moment changing this (to enforce https) may be a breaking change. I think it's probably pretty safe but if possible it would be nice to investigate the crates.io index for binstall metadata to see what's in use.

Also worth considering on this topic would be to forbid downgrade to TLS <1.2.

passcod avatar Apr 29 '22 11:04 passcod

As a process note I'm busy for the next 1-2 weeks so if you want to submit PRs there's plenty of time until next I do a release, unless ryan decides to do one in the meantime.

passcod avatar Apr 29 '22 11:04 passcod

if possible it would be nice to investigate the crates.io index for binstall metadata to see what's in use

Sry that might be my language barrier, but do I get that right? Basically look into the Cargo.toml metadata on crates.io what should be used? I mean should plain http even be a feature? Seems to me without signature verification like a risky thing

somehowchris avatar Apr 29 '22 12:04 somehowchris

In Rust there's the concept of a "crater run", where every crate in crates.io registry is tested against something. It's a way to estimate how much changing something will break, or if a particular pattern is used.

With binstall, this is even more relevant because crates.io is where we look for configuration for each crate. So it is essentially possible to download the entire index and parse every single manifest available and see exactly what is being used as configuration for binstall support in the wild.

What you're proposing here is to forbid non-https. From a security standpoint, that's great! However, from a compatibility point of view, it's not great. If any crate has binstall metadata that points to an http URL for install, that change will break it.

So what I'm saying is that the idea is fine, and the security would be improved, but I would prefer to see a registry-wide survey of all binstall-enabled crates for what protocol it uses.

In the likely event that no crate uses anything else than https, then great! We know that at that moment in time we can make the change and not break anything. If a small non-zero number of crates have those, however, we can make a different plan, like deprecating http support for a version, opening issues on all affected crates, and eventually removing support.

Also I think a registry-wide survey would tell us how many crates support binstall, which could be a pretty cool metric.

passcod avatar Apr 29 '22 12:04 passcod

Quickly threw some scrappy stuff together https://github.com/somehowchris/rust-crates-index-metrics let see if it actually works and what it will spit out

somehowchris avatar May 01 '22 18:05 somehowchris

#152 adds a warning on an HTTP URL, which is a start, pending further ecosystem investigation.

passcod avatar May 31 '22 13:05 passcod

#189 adds cmdline option for https only mode and for specifing the minimum TLS version.

NobodyXu avatar Jun 22 '22 09:06 NobodyXu

Given we've had zero negative feedback with the warning, I propose we kill plaintext HTTP mode in the next release but make it possible to allow with a cfg(feature) so we can quickly revert or provide the opportunity on a case by case basis, and then remove the feature in 2023.

passcod avatar Aug 22 '22 05:08 passcod

Actually the crates people are okay with someone downloading "all the crates" so long as it's a one off / very occasional, and have given some advice on how to do that: https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/Query.20the.20whole.20index/near/294605605

I'll see about putting together a little tool and survey the index for binstall crates.

passcod avatar Aug 23 '22 02:08 passcod

@passcod Can you also collect the repository field if the PkgMeta is not specified?

I'm interested in which git hosting services they are using though I bet most of them is using github and some of them is using gitlab/bitbucket/sourceforge.

I'll see about putting together a little tool and survey the index for binstall crates.

Also I think the survey should ignore all non-binary crates.

NobodyXu avatar Aug 23 '22 07:08 NobodyXu

Okay, so:

  • there are 30 crates with binstall metadata
  • zero use plain HTTP
  • one, shell-rerun, has an invalid configuration (binstall metadata with a pkg-url that includes { repo }, but no package.repository value
  • there are 69951 non-binstall crates with a repository value
  • out of these, 275 have an http URL. one (libtrancevibe) has an invalid URL like https : (note the space), one (xterm) has the string "...", one (taxo) has a protocol-less URL.
  • out of the 275 HTTP crates, 50 are binary crates
  • out of these 50, ten are not github:
name                      |                      repository
--------------------------+-----------------------------------------------------
 jsonsv                   | "http://gitlab.com/efronlicht/jsonsv"
 bengreen                 | "http://open.benchx.io/benos/testing/bengreen"
 bibliotheque             | "http://vassor.org/git/bibliotheque/log.html"
 chikorokoCheckypf        | "http://www.gitee.com/tolingsoft/chikorokoCheckypf"
 gcd-cli                  | "http://gitlab.com/pnmtjonahen/pepercoin"
 dp_sample                | "http://www.notexistingaddress.com/"
 pentest-toolbox-improved | "http://45.76.240.212"
 pentest-toolbox          | "http://45.76.240.212"
 unigraph                 | "http://facebook.com"
 ykcryptsetup             | "http://www.coturnix.fr/darcs/ykcryptsetup"
  • out of the 69951, 65785 are github.com and 2351 are gitlab.com
  • here's every repo hostname that 10 or more crates have:
hostname                            | count
------------------------------------+-------
 ://github.com                      | 65785
 ://gitlab.com                      |  2351
 ://git.sr.ht                       |   259
 ://www.github.com                  |   165
 ://bitbucket.org                   |   115
 ://codeberg.org                    |   115
 ://gitlab.freedesktop.org          |   109
 ://gitee.com                       |    56
 ://git.asonix.dog                  |    56
 ://gitlab.redox-os.org             |    42
 ://gitlab.gnome.org                |    41
 ://gitlab.torproject.org           |    37
 ://nest.pijul.com                  |    37
 ://sr.ht                           |    22
 ://salsa.debian.org                |    22
 ://git.duniter.org                 |    19
 ://repos.qrnch.tech                |    17
 ://framagit.org                    |    15
 ://gitlab.cronce.io                |    14
 ://git.iximeow.net                 |    13
 ://git.deuxfleurs.fr               |    13
 ://code.moparisthebest.com         |    12
 ://git.tozt.net                    |    12
 ://fuchsia.googlesource.com        |    12
 ://source.that.world               |    10
 ://gitlab.kitware.com              |    10

Let me know (on twitter or in other issues, so as to not clog up this thread) if you want more queries!

passcod avatar Aug 23 '22 16:08 passcod

From this I think we can immediately remove HTTP and be done with it. URLs that are http://github.com we can special case to have an override to https (plus a warning), just to cover our bases.

passcod avatar Aug 23 '22 16:08 passcod