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

Only https for requests #124

Closed
somehowchris opened this issue Apr 29, 2022 · 12 comments · Fixed by #343
Closed

Only https for requests #124

somehowchris opened this issue Apr 29, 2022 · 12 comments · Fixed by #343
Labels
Report: enhancement Request for improvements to existing features or code

Comments

@somehowchris
Copy link
Contributor

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

@passcod
Copy link
Member

passcod commented Apr 29, 2022

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
Copy link
Member

passcod commented Apr 29, 2022

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.

@somehowchris
Copy link
Contributor Author

somehowchris commented Apr 29, 2022

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

@passcod
Copy link
Member

passcod commented Apr 29, 2022

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.

@somehowchris
Copy link
Contributor Author

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

@passcod
Copy link
Member

passcod commented May 31, 2022

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

@passcod passcod added the Report: enhancement Request for improvements to existing features or code label Jun 10, 2022
@NobodyXu
Copy link
Member

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

@passcod
Copy link
Member

passcod commented Aug 22, 2022

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
Copy link
Member

passcod commented Aug 23, 2022

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.

@NobodyXu
Copy link
Member

NobodyXu commented Aug 23, 2022

@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.

@passcod
Copy link
Member

passcod commented Aug 23, 2022

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
Copy link
Member

passcod commented Aug 23, 2022

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 added a commit to passcod/cargo-binstall that referenced this issue Sep 5, 2022
passcod added a commit to passcod/cargo-binstall that referenced this issue Sep 5, 2022
passcod added a commit that referenced this issue Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Report: enhancement Request for improvements to existing features or code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants