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

curl: 8.6.0 -> 8.7.1 #299580

Merged
merged 2 commits into from Apr 10, 2024
Merged

curl: 8.6.0 -> 8.7.1 #299580

merged 2 commits into from Apr 10, 2024

Conversation

LeSuisse
Copy link
Contributor

@LeSuisse LeSuisse commented Mar 27, 2024

Description of changes

Fixes CVE-2024-2466, CVE-2024-2398, CVE-2024-2379 and CVE-2024-2004.

Changes:
https://curl.se/changes.html#8_7_1

Things done

  • Built on platform(s)
    • x86_64-linux up to curlFull and curl.passthru.tests
    • aarch64-linux up to curlFull and curl.passthru.tests
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@mweinelt
Copy link
Member

Needs more build testing, as we regularly run into issues with darwin.

@LeSuisse
Copy link
Contributor Author

I will try to cover Linux aarch64 during the week-end but I will need a hand for darwin

@mweinelt
Copy link
Member

Will look into darwin.

@mweinelt
Copy link
Member

x86_64-darwin passthru tests fail to eval. This is rebased onto master.

❯ x86_64-darwin-build -A curl.tests
error:
       … while calling the 'derivationStrict' builtin
         at /builtin/derivation.nix:1:205:
       … while evaluating derivation 'curl-static-x86_64-apple-darwin-8.7.1'
         whose name attribute is located at /home/hexa/git/nixos/staging/pkgs/stdenv/generic/make-derivation.nix:331:7

       … while evaluating attribute 'CXX' of derivation 'curl-static-x86_64-apple-darwin-8.7.1'
         at /home/hexa/git/nixos/staging/pkgs/tools/networking/curl/default.nix:151:3:
          150|
          151|   CXX = "${stdenv.cc.targetPrefix}c++";
             |   ^
          152|   CXXCPP = "${stdenv.cc.targetPrefix}c++ -E";

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: don't yet have a `targetPackages.darwin.LibsystemCross for x86_64-apple-darwin`
nix-repl> curl.tests
{
  coeurl = «derivation /nix/store/bqppr9zyls0ngmcjwcr2k32ygrpwvmab-coeurl-0.3.0.drv»;
  curlpp = «derivation /nix/store/fkqy9pbwwz73byyn72cbpvlv950k289f-curlpp-0.8.1.drv»;
  haskell-curl = «derivation /nix/store/c3sjbf6nvnnl7n569q425zdi2az27rc4-curl-1.3.8.drv»;
  nginx-http3 = ^[[A{ ... };
  ocaml-curly = «derivation /nix/store/v8zvag71n6b3jpxxwacdl6zzfi6b228k-ocaml5.1.1-curly-0.3.0.drv»;
  php-curl = «derivation /nix/store/vrdzs5rwvjw4480l3sfgwnc6rlhf40d3-php-curl-8.2.17.drv»;
  pkg-config = «derivation /nix/store/2vdn264km8dwad8x8pc9gg7aqlkmnkqj-check-meta-pkg-config-modules-for-curl-8.7.1.drv»;
  pycurl = «derivation /nix/store/f3v66mlrj0qjz2aq5p2s89dllpzb3d53-python3.11-pycurl-7.45.3.drv»;
  static = «error: don't yet have a `targetPackages.darwin.LibsystemCross for x86_64-apple-darwin`»;
  withCheck = «derivation /nix/store/ky3l3a7iilqkdb3y8ghvqbda2wyg6j1q-curl-8.7.1.drv»;
}

Added in 817d201 by @alyssais. PTAL.

@risicle
Copy link
Contributor

risicle commented Mar 30, 2024

Successfully built all relevant curl.tests (apart from haskell one, ghc too big for me) and curlFull.tests.withCheck on macos 12 x86_64.

Also built equivalent for, nixos x86_64, along with pkgsStatic, pkgsMusl, pkgsCross.aarch64-multiplatform, pkgsi686Linux variants of curl.tests.withCheck and pkgsMusl, pkgsCross.aarch64-multiplatform, pkgsi686Linux variants of curlFull.tests.withCheck.

@mweinelt
Copy link
Member

curlMinimal, curl and curlFull including passthru.tests on aarch64-darwin and x86_64-darwin are looking good. Both ultimately fail their withCheck tests, but that does not look like a blocker.

Couldn't create tcp server socket: IO::Socket::INET: Operation not permitted
make[1]: *** [Makefile:839: quiet-test] Error 1
make[1]: Leaving directory '/private/tmp/nix-build-curl-8.7.1.drv-3/curl-8.7.1/tests'
make: *** [Makefile:1721: test] Error 2

@mweinelt
Copy link
Member

diff --git a/pkgs/tools/networking/curl/default.nix b/pkgs/tools/networking/curl/default.nix
index fbc430b7afc9..82100c95a4e6 100644
--- a/pkgs/tools/networking/curl/default.nix
+++ b/pkgs/tools/networking/curl/default.nix
@@ -196,6 +196,7 @@ stdenv.mkDerivation (finalAttrs: {
       # nginx-http3 = useThisCurl nixosTests.nginx-http3;
       nginx-http3 = nixosTests.nginx-http3;
       pkg-config = testers.testMetaPkgConfig finalAttrs.finalPackage;
+    } // lib.optionalAttrs (!stdenv.isDarwin && !stdenv.isx86_64) {
       static = pkgsStatic.curl;
     } // lib.optionalAttrs (!stdenv.isDarwin) {
       fetchpatch = tests.fetchpatch.simple.override { fetchpatch = (fetchpatch.override { fetchurl = useThisCurl fetchurl; }) // { version = 1; }; };

@LeSuisse
Copy link
Contributor Author

Done, thanks!

@alyssais
Copy link
Member

alyssais commented Apr 1, 2024

Ughhh, their fix for static linking libpsl doesn't actually work, so we have to keep our hack for it.

pkgs/tools/networking/curl/default.nix Outdated Show resolved Hide resolved
@alyssais
Copy link
Member

alyssais commented Apr 2, 2024

Should we add a case to meta.broken for x86_64-darwin static?

@cpu
Copy link
Contributor

cpu commented Apr 3, 2024

Since curl 8.7.0 switched to rustls-ffi 0.12 I think this will break the optional withRustls feature unless that derivation is bumped from 0.10. That's a little tricky because Apache HTTPD also depends on the rustls-ffi derivation, but that project hasn't updated for 0.12.

@LeSuisse Do you have any thoughts? Is the best approach to split the rustls-ffi package into two versions? As an upstream maintainer of rustls-ffi I'm interested in trying to pitch in with the downstream packaging but would appreciate input before I start making PRs. Also happy to chat elsewhere if this is too off-topic for the curl update.

@LeSuisse
Copy link
Contributor Author

LeSuisse commented Apr 3, 2024

Yeah I need to take a look. It's likely I'm going to propose to have the 2 versions in nixpkgs.

Expect something by the end of the week.

@cpu
Copy link
Contributor

cpu commented Apr 3, 2024

It's likely I'm going to propose to have the 2 versions in nixpkgs.

Thanks! 👍 Feel free to CC me. We recently landed pkg-config/.so support using cargo-c to build and I suspect that might be interesting to think about too.

@risicle
Copy link
Contributor

risicle commented Apr 3, 2024

If you want to ensure we don't break withRustls I suggest you add a target to passthru.tests that builds covers this.

Come to think, it doesn't look like we have any passthru targets for alternative tls implementations.

@cpu
Copy link
Contributor

cpu commented Apr 3, 2024

If you want to ensure we don't break withRustls I suggest you add a target to passthru.tests that builds covers this.

There is a passthru test in the rustls-ffi derivation for curl & apache HTTPD w/ the feature activated. Is the suggestion to add one in reverse in the curl derivation? (I'm not especially familiar with the passthru test mechanism).

It currently does not build but the situation is being handled upstream.
@LeSuisse
Copy link
Contributor Author

LeSuisse commented Apr 7, 2024

Ok given there is also curl/curl#13248 I will deal with the rustls-ffi upgrade once this PR reaches master. It will be less annoying to work on it and given the curl + Rustls backend is not accessible via a direct pkgs attribute it should not bother too many people.

@vcunat vcunat merged commit d68f826 into NixOS:staging Apr 10, 2024
24 checks passed
@LeSuisse LeSuisse deleted the curl-8.7.1 branch April 10, 2024 07:10
@LeSuisse LeSuisse mentioned this pull request Apr 19, 2024
13 tasks
@jpotier
Copy link
Contributor

jpotier commented Apr 24, 2024

Not major, but a bug in 8.7 breaks newsboat

@cpu
Copy link
Contributor

cpu commented Apr 24, 2024

Not major, but a curl/curl#13219 newsboat/newsboat#2728

Looks easy enough to cherry-pick a fix patch: #306633 @jpotier Can you test if that fixes newsboat for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants