Merge #1499

1499: Replace `curl` with `reqwest` r=sgrif a=sgrif

**The diff stats may look scary, but the vast majority of the added lines is because `reqwest` depends on `tokio`, which caused 448 lines to be added to `Cargo.lock`. The actual code changes are not particularly large**

Why should we do this?
---

`curl::Easy` is anything but. Its API is verbose, and consistently misunderstood in our codebase. These commits replace the `curl` crate with `reqwest`, which is designed to be a higher level HTTP client. It doesn't have every random nicety we might want (like setting `Date` on `POST`/`PUT` requests), but does have a ton of things we want built in like helpers for JSON request/response bodies that assumes the use of serde, and "turn 4xx/5xx response codes into an error" helper functions.

At best the curl crate required some very verbose code, or confusing APIs to make it possible to work with stack data, but at worst it led to some code that was confusing or even dangerous. A few things that were weird or gave me pause while writing this were:

- Setting the `Host` header manually, which is at best pointless and at worst a major latent bug waiting to strike. I suspect that the first instance of this was in the S3 code, which was done because `Host` is mentioned in the S3 docs for common headers for some reason, and the other places this was done were due to that code being copied from the S3 code
- Specifically calling a CURL function that opted into `Expect: 100-Continue`, and then manually overriding the `Expect` header with a comment that it's not clear what it's purpose is
- Setting the `Date` header on requests for no reason (which on requests with no body, is actually a violation of the HTTP/1.1 spec)
- Choosing "hello!" as a User-Agent string for endpoints which require it

What changes occur in our HTTP requests?
----

The `http-data` files are basically impossible to review, so here are the changes that were made:

All requests have the following headers added:

- `User-Agent: reqwest/0.9.1`
- `Accept-Encoding: gzip`

All requests had the following header removed:

- `Proxy-Connection: Keep-Alive`
  - Uncyclopedia describes this header as "Implemented as a misunderstanding of the HTTP specifications. Common because of mistakes in implementations of early HTTP versions." It has been removed from Firefox and more recent versions of curl.

Some requests had the following header removed:
 
- `User-Agent: hello!`
  - The HTTP specifications states that all user agents SHOULD send this header. All requests that we make are a direct result of user interaction, making us a user agent (and it's pretty standard to expect this header even from non-user-agents like crawlers). Arguably we should be setting this to `crates.io`, but this is at least marginally useful unlike "hello!" which is not appropriate here.

What notable changes happened in the code?
---

For the most part, the code is just replacing manual stuff with built-in stuff. I've removed anything that set a `Date` or `User-Agent` header manually (unless `Date` was required, which is the case for S3, since the date is used as part of the auth signature). I've removed all cases that were manually setting a `Host` header, since if this ever differed from the URL we used to lookup the domain, it would be very bad.

While making this change, a few refactorings occurred.

The S3 code was taking a `file_length` parameter as a `u64`. This was expected to be the length of the bytes passed in, but... we have a slice so we can just ask it the length. Due to a combination of the API reqwest wants to expose, and the fact that its backed by tokio, it can only work with static data. This was only relevant for S3 uploads, but changing the relevant functions from taking `&[u8]` to `Vec<u8>` was a trivial change.

The functions for interacting with the GitHub API were split into two pieces, one which made the request, and one which processed it. The only reason for this split was so that one consumer could intercept 404 responses and return `Ok(false)`. I've opted to merge all of this into a single function, and return `NotFound` for 404, which we can check for in that one place. I was expecting to use `Any::is` there, but due to the way its written, it required duplicating its implementation (see the commit message for that commit for more details)

The only other point of note is that the changes to `GhUser::init` are not tested, since they only run if some files in this repo are not present, and for that code to run we'd need the passwords for two GitHub users that we don't have access to. I suspect we should probably just delete that code and panic if we don't have the file, but I haven't done so here.

Co-authored-by: Sean Griffin <[email protected]>

Author: bors-voyager[bot]

Cargo.lock

@@ -36,7 +36,6 @@ url = "1.2.1"
 tar = "0.4.13"
 
 openssl = "0.9.14"
-curl = "0.4"
 oauth2 = "0.3"
 log = "0.3"
 env_logger = "0.5"
@@ -58,6 +57,7 @@ docopt = "0.8.1"
 itertools = "0.6.0"
 scheduled-thread-pool = "0.2.0"
 derive_deref = "1.0.0"
+reqwest = "0.9.1"
 
 lettre = "0.8"
 lettre_email = "0.8"

Cargo.toml

@@ -5,12 +5,13 @@ use std::path::PathBuf;
 use std::sync::{Arc, Mutex};
 use std::time::Duration;
 
-use curl::easy::Easy;
 use diesel::r2d2;
 use git2;
 use oauth2;
+use reqwest;
 use scheduled_thread_pool::ScheduledThreadPool;
 
+use util::CargoResult;
 use {db, Config, Env};
 
 /// The `App` struct holds the main components of the application like
@@ -99,16 +100,16 @@ impl App {
         }
     }
 
-    /// Returns a handle for making HTTP requests to upload crate files.
+    /// Returns a client for making HTTP requests to upload crate files.
     ///
     /// The handle will go through a proxy if the uploader being used has specified one, which
     /// is only done in test mode in order to be able to record and inspect the HTTP requests
     /// that tests make.
-    pub fn handle(&self) -> Easy {
-        let mut handle = Easy::new();
+    pub fn http_client(&self) -> CargoResult<reqwest::Client> {
+        let mut builder = reqwest::Client::builder();
         if let Some(proxy) = self.config.uploader.proxy() {
-            handle.proxy(proxy).unwrap();
+            builder = builder.proxy(reqwest::Proxy::all(proxy)?);
         }
-        handle
+        Ok(builder.build()?)
     }
 }

src/app.rs

@@ -10,27 +10,24 @@ extern crate serde_derive;
 
 extern crate cargo_registry;
 extern crate chrono;
-extern crate curl;
 extern crate diesel;
 extern crate docopt;
 extern crate flate2;
 extern crate itertools;
+extern crate reqwest;
 extern crate tar;
 extern crate toml;
-extern crate url;
 
 use chrono::{TimeZone, Utc};
-use curl::easy::{Easy, List};
 use diesel::dsl::any;
 use diesel::prelude::*;
 use docopt::Docopt;
 use flate2::read::GzDecoder;
 use itertools::Itertools;
-use std::io::{Cursor, Read};
+use std::io::Read;
 use std::path::Path;
 use std::thread;
 use tar::Archive;
-use url::Url;
 
 use cargo_registry::render::readme_to_html;
 use cargo_registry::Config;
@@ -146,15 +143,13 @@ fn main() {
                 }
                 let readme = readme.unwrap();
                 let readme_path = format!("readmes/{0}/{0}-{1}.html", krate_name, version.num);
-                let readme_len = readme.len();
                 config
                     .uploader
                     .upload(
-                        Easy::new(),
+                        &reqwest::Client::new(),
                         &readme_path,
-                        readme.as_bytes(),
+                        readme.into_bytes(),
                         "text/html",
-                        readme_len as u64,
                     ).unwrap_or_else(|_| {
                         panic!(
                             "[{}-{}] Couldn't upload file to S3",
@@ -174,53 +169,32 @@ fn main() {
 
 /// Renders the readme of an uploaded crate version.
 fn get_readme(config: &Config, version: &Version, krate_name: &str) -> Option<String> {
-    let mut handle = Easy::new();
-    let location = match config
+    let location = config
         .uploader
-        .crate_location(krate_name, &version.num.to_string())
-    {
-        Some(l) => l,
-        None => return None,
-    };
-    let date = Utc::now().to_rfc2822();
-    let url = Url::parse(&location)
-        .unwrap_or_else(|_| panic!("[{}-{}] Couldn't parse crate URL", krate_name, version.num));
-
-    let mut headers = List::new();
-    headers
-        .append(&format!("Host: {}", url.host().unwrap()))
-        .unwrap();
-    headers.append(&format!("Date: {}", date)).unwrap();
+        .crate_location(krate_name, &version.num.to_string())?;
 
-    handle.url(url.as_str()).unwrap();
-    handle.get(true).unwrap();
-    handle.http_headers(headers).unwrap();
-
-    let mut response = Vec::new();
-    {
-        let mut req = handle.transfer();
-        req.write_function(|data| {
-            response.extend(data);
-            Ok(data.len())
-        }).unwrap();
-        if let Err(err) = req.perform() {
+    let mut response = match reqwest::get(&location) {
+        Ok(r) => r,
+        Err(err) => {
             println!(
                 "[{}-{}] Unable to fetch crate: {}",
                 krate_name, version.num, err
             );
             return None;
         }
-    }
-    if handle.response_code().unwrap() != 200 {
-        let response = String::from_utf8_lossy(&response);
+    };
+
+    if !response.status().is_success() {
         println!(
             "[{}-{}] Failed to get a 200 response: {}",
-            krate_name, version.num, response
+            krate_name,
+            version.num,
+            response.text().unwrap()
         );
         return None;
     }
-    let reader = Cursor::new(response);
-    let reader = GzDecoder::new(reader);
+
+    let reader = GzDecoder::new(response);
     let mut archive = Archive::new(reader);
     let mut entries = archive.entries().unwrap_or_else(|_| {
         panic!(

src/bin/render-readmes.rs

@@ -166,10 +166,10 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
         // Upload the crate, return way to delete the crate from the server
         // If the git commands fail below, we shouldn't keep the crate on the
         // server.
-        let (cksum, mut crate_bomb, mut readme_bomb) =
-            app.config
-                .uploader
-                .upload_crate(req, &krate, readme, file_length, maximums, vers)?;
+        let (cksum, mut crate_bomb, mut readme_bomb) = app
+            .config
+            .uploader
+            .upload_crate(req, &krate, readme, maximums, vers)?;
         version.record_readme_rendering(&conn)?;
 
         let mut hex_cksum = String::new();

src/controllers/krate/publish.rs

@@ -100,8 +100,7 @@ pub fn github_access_token(req: &mut dyn Request) -> CargoResult<Response> {
         .exchange(code.clone())
         .map_err(|s| human(&s))?;
 
-    let (handle, resp) = github::github(req.app(), "/user", &token)?;
-    let ghuser: GithubUser = github::parse_github_response(handle, &resp)?;
+    let ghuser = github::github::<GithubUser>(req.app(), "/user", &token)?;
 
     let user = NewUser::new(
         ghuser.id,

src/controllers/user/session.rs

@@ -1,88 +1,58 @@
 //! This module implements functionality for interacting with GitHub.
 
-use curl;
-use curl::easy::{Easy, List};
-
 use oauth2::*;
+use reqwest::{self, header};
 
-use serde::Deserialize;
-use serde_json;
+use serde::de::DeserializeOwned;
 
 use std::str;
 
 use app::App;
-use util::{human, internal, CargoResult, ChainError};
+use util::{errors::NotFound, human, internal, CargoError, CargoResult};
 
 /// Does all the nonsense for sending a GET to Github. Doesn't handle parsing
 /// because custom error-code handling may be desirable. Use
 /// `parse_github_response` to handle the "common" processing of responses.
-pub fn github(app: &App, url: &str, auth: &Token) -> Result<(Easy, Vec<u8>), curl::Error> {
+pub fn github<T>(app: &App, url: &str, auth: &Token) -> CargoResult<T>
+where
+    T: DeserializeOwned,
+{
     let url = format!("{}://api.github.com{}", app.config.api_protocol, url);
     info!("GITHUB HTTP: {}", url);
 
-    let mut headers = List::new();
-    headers
-        .append("Accept: application/vnd.github.v3+json")
-        .unwrap();
-    headers.append("User-Agent: hello!").unwrap();
-    headers
-        .append(&format!("Authorization: token {}", auth.access_token))
-        .unwrap();
-
-    let mut handle = app.handle();
-    handle.url(&url).unwrap();
-    handle.get(true).unwrap();
-    handle.http_headers(headers).unwrap();
-
-    let mut data = Vec::new();
-    {
-        let mut transfer = handle.transfer();
-        transfer
-            .write_function(|buf| {
-                data.extend_from_slice(buf);
-                Ok(buf.len())
-            }).unwrap();
-        transfer.perform()?;
-    }
-    Ok((handle, data))
+    let client = app.http_client()?;
+    client
+        .get(&url)
+        .header(header::ACCEPT, "application/vnd.github.v3+json")
+        .header(
+            header::AUTHORIZATION,
+            format!("token {}", auth.access_token),
+        ).send()?
+        .error_for_status()
+        .map_err(|e| handle_error_response(&e))?
+        .json()
+        .map_err(Into::into)
 }
 
-/// Checks for normal responses
-pub fn parse_github_response<'de, 'a: 'de, T: Deserialize<'de>>(
-    mut resp: Easy,
-    data: &'a [u8],
-) -> CargoResult<T> {
-    match resp.response_code().unwrap() {
-        // Ok!
-        200 => {}
-        // Unauthorized or Forbidden
-        401 | 403 => {
-            return Err(human(
-                "It looks like you don't have permission \
-                 to query a necessary property from Github \
-                 to complete this request. \
-                 You may need to re-authenticate on \
-                 crates.io to grant permission to read \
-                 github org memberships. Just go to \
-                 https://crates.io/login",
-            ));
-        }
-        // Something else
-        n => {
-            let resp = String::from_utf8_lossy(data);
-            return Err(internal(&format_args!(
-                "didn't get a 200 result from \
-                 github, got {} with: {}",
-                n, resp
-            )));
-        }
-    }
-
-    let json = str::from_utf8(data)
-        .ok()
-        .chain_error(|| internal("github didn't send a utf8-response"))?;
+fn handle_error_response(error: &reqwest::Error) -> Box<dyn CargoError> {
+    use reqwest::StatusCode as Status;
 
-    serde_json::from_str(json).chain_error(|| internal("github didn't send a valid json response"))
+    match error.status() {
+        Some(Status::UNAUTHORIZED) | Some(Status::FORBIDDEN) => human(
+            "It looks like you don't have permission \
+             to query a necessary property from Github \
+             to complete this request. \
+             You may need to re-authenticate on \
+             crates.io to grant permission to read \
+             github org memberships. Just go to \
+             https://crates.io/login",
+        ),
+        Some(Status::NOT_FOUND) => Box::new(NotFound),
+        _ => internal(&format_args!(
+            "didn't get a 200 result from github: {}",
+            error
+        )),
+    }
 }
 
 /// Gets a token with the given string as the access token, but all

src/github.rs

@@ -12,7 +12,6 @@
 extern crate ammonia;
 extern crate chrono;
 extern crate comrak;
-extern crate curl;
 #[macro_use]
 extern crate derive_deref;
 #[macro_use]
@@ -32,6 +31,7 @@ extern crate log;
 extern crate oauth2;
 extern crate openssl;
 extern crate rand;
+extern crate reqwest;
 extern crate s3;
 extern crate scheduled_thread_pool;
 extern crate semver;

src/lib.rs

@@ -2,7 +2,7 @@ use diesel::prelude::*;
 
 use app::App;
 use github;
-use util::{human, CargoResult};
+use util::{errors::NotFound, human, CargoResult};
 
 use models::{Crate, CrateOwner, Owner, OwnerKind, User};
 use schema::{crate_owners, teams};
@@ -142,8 +142,7 @@ impl Team {
         // links. A hundred teams should be enough for any org, right?
         let url = format!("/orgs/{}/teams?per_page=100", org_name);
         let token = github::token(req_user.gh_access_token.clone());
-        let (handle, data) = github::github(app, &url, &token)?;
-        let teams: Vec<GithubTeam> = github::parse_github_response(handle, &data)?;
+        let teams = github::github::<Vec<GithubTeam>>(app, &url, &token)?;
 
         let team = teams
             .into_iter()
@@ -165,8 +164,7 @@ impl Team {
         }
 
         let url = format!("/orgs/{}", org_name);
-        let (handle, resp) = github::github(app, &url, &token)?;
-        let org: Org = github::parse_github_response(handle, &resp)?;
+        let org = github::github::<Org>(app, &url, &token)?;
 
         NewTeam::new(&login.to_lowercase(), team.id, team.name, org.avatar_url)
             .create_or_update(conn)
@@ -225,14 +223,11 @@ fn team_with_gh_id_contains_user(app: &App, github_id: i32, user: &User) -> Carg
 
     let url = format!("/teams/{}/memberships/{}", &github_id, &user.gh_login);
     let token = github::token(user.gh_access_token.clone());
-    let (mut handle, resp) = github::github(app, &url, &token)?;
-
-    // Officially how `false` is returned
-    if handle.response_code().unwrap() == 404 {
-        return Ok(false);
-    }
-
-    let membership: Membership = github::parse_github_response(handle, &resp)?;
+    let membership = match github::github::<Membership>(app, &url, &token) {
+        // Officially how `false` is returned
+        Err(ref e) if e.is::<NotFound>() => return Ok(false),
+        x => x?,
+    };
 
     // There is also `state: pending` for which we could possibly give
     // some feedback, but it's not obvious how that should work.

src/models/team.rs

@@ -213,7 +213,6 @@ mod tests {
 
         // All other error types are propogated up the middleware, eventually becoming status 500
         assert!(C(|_| Err(internal(""))).call(&mut req).is_err());
-        assert!(C(|_| err(::curl::Error::new(0))).call(&mut req).is_err());
         assert!(
             C(|_| err(::serde_json::Error::syntax(
                 ::serde_json::error::ErrorCode::ExpectedColon,