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

Draft: fix: consolidate http-client trees in a compatible way with use across fluvio repo #3585

Closed
wants to merge 1 commit into from

Conversation

matheus-consoli
Copy link
Contributor

bot alerted about a possible CPU denial of service in versions previous to 0.22.1 of the webpki crate.

This crate was in the dependency tree of http-client and surf when using the h1-client-tls feature.

I removed the http-client dependency, as we're not using it, and changed the surf client to hyper-client instead of h1-client-tls (the other option was to use curl-client).

Closes https://github.com/infinyon/roadmap/issues/182

@matheus-consoli matheus-consoli changed the title fix: change surf client to address security alert Draft: fix: change surf client to address security alert Oct 5, 2023
@matheus-consoli matheus-consoli marked this pull request as draft October 5, 2023 21:26
@digikata
Copy link
Contributor

digikata commented Oct 5, 2023

I don't mind the hyper-client per se, in the past there might have been some async compatibility issues that cropped up at runtime so we need to recheck that by doing a fluvio install to a k8 cluster. The CI does that, but it's probably worth trying that locally.

@sehz
Copy link
Contributor

sehz commented Oct 6, 2023

BTW, Hyper can work with std. see: https://github.com/infinyon/k8-api/blob/master/src/k8-client/src/client/mod.rs

@matheus-consoli matheus-consoli force-pushed the webpki-sec branch 3 times, most recently from 6150655 to 5a18654 Compare October 6, 2023 01:33
@matheus-consoli
Copy link
Contributor Author

Summarizing, the security alert is related to two crates we're using, http-client and surf (when using h1-client-rustls), both deps introduced in #3581, which is related to fvm.
surf has 5 options to choose from for HTTP clients

  • h1-client-rustls: depends on the vulnerable crate (webpki)
  • h1-client-no-tls: doesn't depend on webpki, but doesn't support TLS
  • h1-client: doesn't depend on webpki, but relies on native SSL, which brings some linking problems between musl and openssl
  • hyper-client: no webpki, but depends on openssl
  • curl-client: no webpki, also relies on openssl

Even more alarming, surf notably is unmaintained.

The current PR is using http-client-no-tls, but it's not acceptable, I say we should try to move out from surf and use a different HTTP client - async-std doesn't have many options here, so for the short-term using a blocking client + task::spawn_blocking may be viable (hypothesis: fvm is not heavily async)

@sehz
Copy link
Contributor

sehz commented Oct 6, 2023

Please see my comment about hyper, which works with std using a custom executor. It should also work with rustls. We had pending ticket about moving to rustls as default instead of openssl

@digikata digikata changed the title Draft: fix: change surf client to address security alert Draft: fix: consolidate http-client trees in a compatible way with use across fluvio repo Oct 11, 2023
@digikata
Copy link
Contributor

digikata commented Oct 11, 2023

Work will continue for this from a crate hosted at https://github.com/infinyon/fluvio-mini-http

The crate should use hyper + rustls + async-std for improved compatibility & maintainabilty with our codebase and cross platform builds

@digikata
Copy link
Contributor

digikata commented Dec 8, 2023

Good work
This preliminary work was incorporated with additional integration here #3761

@digikata digikata closed this Dec 8, 2023
@matheus-consoli matheus-consoli deleted the webpki-sec branch December 8, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants