Revert "http_client: Add integrity checks for GitHub binaries using digest checks (#43737)" (#44086)

Conrad Irwin created

This reverts commit 05764e8af797b5abb8076bc78ce32d4130505e93.

Internally we've seen a much higher incidence of macOS code-signing
failing on
the download rust analyzer than we did before this change.

It's unclear why this would be a problem, but we want to try reverting
to see if that fixes it.

Release Notes:

- Reverted a change that seemed to cause problems with code-signing on
rust-analyzer

Change summary

crates/http_client/src/github_download.rs | 61 ------------------------
crates/languages/src/c.rs                 | 55 +++++++++++++++------
crates/languages/src/rust.rs              | 62 ++++++++++++++++++------
3 files changed, 86 insertions(+), 92 deletions(-)

Detailed changes

crates/http_client/src/github_download.rs 🔗

@@ -1,4 +1,4 @@
-use std::{future::Future, path::Path, pin::Pin, task::Poll};
+use std::{path::Path, pin::Pin, task::Poll};
 
 use anyhow::{Context, Result};
 use async_compression::futures::bufread::GzipDecoder;
@@ -85,65 +85,6 @@ pub async fn download_server_binary(
     Ok(())
 }
 
-pub async fn fetch_github_binary_with_digest_check<ValidityCheck, ValidityCheckFuture>(
-    binary_path: &Path,
-    metadata_path: &Path,
-    expected_digest: Option<String>,
-    url: &str,
-    asset_kind: AssetKind,
-    download_destination: &Path,
-    http_client: &dyn HttpClient,
-    validity_check: ValidityCheck,
-) -> Result<()>
-where
-    ValidityCheck: FnOnce() -> ValidityCheckFuture,
-    ValidityCheckFuture: Future<Output = Result<()>>,
-{
-    let metadata = GithubBinaryMetadata::read_from_file(metadata_path)
-        .await
-        .ok();
-
-    if let Some(metadata) = metadata {
-        let validity_check_result = validity_check().await;
-
-        if let (Some(actual_digest), Some(expected_digest_ref)) =
-            (&metadata.digest, &expected_digest)
-        {
-            if actual_digest == expected_digest_ref {
-                if validity_check_result.is_ok() {
-                    return Ok(());
-                }
-            } else {
-                log::info!(
-                    "SHA-256 mismatch for {binary_path:?} asset, downloading new asset. Expected: {expected_digest_ref}, Got: {actual_digest}"
-                );
-            }
-        } else if validity_check_result.is_ok() {
-            return Ok(());
-        }
-    }
-
-    download_server_binary(
-        http_client,
-        url,
-        expected_digest.as_deref(),
-        download_destination,
-        asset_kind,
-    )
-    .await?;
-
-    GithubBinaryMetadata::write_to_file(
-        &GithubBinaryMetadata {
-            metadata_version: 1,
-            digest: expected_digest,
-        },
-        metadata_path,
-    )
-    .await?;
-
-    Ok(())
-}
-
 async fn stream_response_archive(
     response: impl AsyncRead + Unpin,
     url: &str,

crates/languages/src/c.rs 🔗

@@ -3,7 +3,7 @@ use async_trait::async_trait;
 use futures::StreamExt;
 use gpui::{App, AsyncApp};
 use http_client::github::{AssetKind, GitHubLspBinaryVersion, latest_github_release};
-use http_client::github_download::fetch_github_binary_with_digest_check;
+use http_client::github_download::{GithubBinaryMetadata, download_server_binary};
 pub use language::*;
 use lsp::{InitializeParams, LanguageServerBinary, LanguageServerName};
 use project::lsp_store::clangd_ext;
@@ -85,32 +85,55 @@ impl LspInstaller for CLspAdapter {
         };
 
         let metadata_path = version_dir.join("metadata");
-
-        let binary_path_for_check = binary_path.clone();
-        fetch_github_binary_with_digest_check(
-            &binary_path,
-            &metadata_path,
-            expected_digest,
-            &url,
-            AssetKind::Zip,
-            &container_dir,
-            &*delegate.http_client(),
-            || async move {
+        let metadata = GithubBinaryMetadata::read_from_file(&metadata_path)
+            .await
+            .ok();
+        if let Some(metadata) = metadata {
+            let validity_check = async || {
                 delegate
                     .try_exec(LanguageServerBinary {
-                        path: binary_path_for_check,
+                        path: binary_path.clone(),
                         arguments: vec!["--version".into()],
                         env: None,
                     })
                     .await
                     .inspect_err(|err| {
-                        log::warn!("Unable to run clangd asset, redownloading: {err:#}")
+                        log::warn!("Unable to run {binary_path:?} asset, redownloading: {err:#}",)
                     })
-            },
+            };
+            if let (Some(actual_digest), Some(expected_digest)) =
+                (&metadata.digest, &expected_digest)
+            {
+                if actual_digest == expected_digest {
+                    if validity_check().await.is_ok() {
+                        return Ok(binary);
+                    }
+                } else {
+                    log::info!(
+                        "SHA-256 mismatch for {binary_path:?} asset, downloading new asset. Expected: {expected_digest}, Got: {actual_digest}"
+                    );
+                }
+            } else if validity_check().await.is_ok() {
+                return Ok(binary);
+            }
+        }
+        download_server_binary(
+            &*delegate.http_client(),
+            &url,
+            expected_digest.as_deref(),
+            &container_dir,
+            AssetKind::Zip,
         )
         .await?;
-
         remove_matching(&container_dir, |entry| entry != version_dir).await;
+        GithubBinaryMetadata::write_to_file(
+            &GithubBinaryMetadata {
+                metadata_version: 1,
+                digest: expected_digest,
+            },
+            &metadata_path,
+        )
+        .await?;
 
         Ok(binary)
     }

crates/languages/src/rust.rs 🔗

@@ -5,7 +5,7 @@ use futures::StreamExt;
 use gpui::{App, AppContext, AsyncApp, SharedString, Task};
 use http_client::github::AssetKind;
 use http_client::github::{GitHubLspBinaryVersion, latest_github_release};
-use http_client::github_download::fetch_github_binary_with_digest_check;
+use http_client::github_download::{GithubBinaryMetadata, download_server_binary};
 pub use language::*;
 use lsp::{InitializeParams, LanguageServerBinary};
 use project::lsp_store::rust_analyzer_ext::CARGO_DIAGNOSTICS_SOURCE_NAME;
@@ -574,34 +574,64 @@ impl LspInstaller for RustLspAdapter {
             AssetKind::Zip => destination_path.clone().join("rust-analyzer.exe"), // zip contains a .exe
         };
 
-        let metadata_path = destination_path.with_extension("metadata");
+        let binary = LanguageServerBinary {
+            path: server_path.clone(),
+            env: None,
+            arguments: Default::default(),
+        };
 
-        let server_path_for_check = server_path.clone();
-        fetch_github_binary_with_digest_check(
-            &server_path,
-            &metadata_path,
-            expected_digest,
-            &url,
-            Self::GITHUB_ASSET_KIND,
-            &destination_path,
-            &*delegate.http_client(),
-            || async move {
+        let metadata_path = destination_path.with_extension("metadata");
+        let metadata = GithubBinaryMetadata::read_from_file(&metadata_path)
+            .await
+            .ok();
+        if let Some(metadata) = metadata {
+            let validity_check = async || {
                 delegate
                     .try_exec(LanguageServerBinary {
-                        path: server_path_for_check,
+                        path: server_path.clone(),
                         arguments: vec!["--version".into()],
                         env: None,
                     })
                     .await
                     .inspect_err(|err| {
-                        log::warn!("Unable to run rust-analyzer asset, redownloading: {err:#}")
+                        log::warn!("Unable to run {server_path:?} asset, redownloading: {err:#}",)
                     })
-            },
+            };
+            if let (Some(actual_digest), Some(expected_digest)) =
+                (&metadata.digest, &expected_digest)
+            {
+                if actual_digest == expected_digest {
+                    if validity_check().await.is_ok() {
+                        return Ok(binary);
+                    }
+                } else {
+                    log::info!(
+                        "SHA-256 mismatch for {destination_path:?} asset, downloading new asset. Expected: {expected_digest}, Got: {actual_digest}"
+                    );
+                }
+            } else if validity_check().await.is_ok() {
+                return Ok(binary);
+            }
+        }
+
+        download_server_binary(
+            &*delegate.http_client(),
+            &url,
+            expected_digest.as_deref(),
+            &destination_path,
+            Self::GITHUB_ASSET_KIND,
         )
         .await?;
-
         make_file_executable(&server_path).await?;
         remove_matching(&container_dir, |path| path != destination_path).await;
+        GithubBinaryMetadata::write_to_file(
+            &GithubBinaryMetadata {
+                metadata_version: 1,
+                digest: expected_digest,
+            },
+            &metadata_path,
+        )
+        .await?;
 
         Ok(LanguageServerBinary {
             path: server_path,