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

rustls-ffi: 0.10.0 -> 0.13.0 #305391

Merged
merged 1 commit into from
Aug 1, 2024
Merged

Conversation

LeSuisse
Copy link
Contributor

@LeSuisse LeSuisse commented Apr 19, 2024

Description of changes

Switched to the recently introduced cargo-c build in order to get pkg-config/.so config.

Changelog:
https://github.com/rustls/rustls-ffi/blob/v0.13.0/CHANGELOG.md

A patch have been applied to Apache in order to resolve an incompatibility.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • 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.

@LeSuisse
Copy link
Contributor Author

LeSuisse commented Apr 19, 2024

curl with the Rustls support currently does not build:

curl> checking for rustls options with pkg-config... found
curl> configure: pkg-config: SSL_LIBS: "-lrustls"
curl> configure: pkg-config: SSL_LDFLAGS: "-L/nix/store/d6gw39pnrbyap9dhpkbm6z9n82jkll4h-rustls-ffi-0.13.0/lib"
curl> configure: pkg-config: SSL_CPPFLAGS: "-I/nix/store/d6gw39pnrbyap9dhpkbm6z9n82jkll4h-rustls-ffi-0.13.0/include"
curl> configure: detected rustls
curl> configure: Added /nix/store/d6gw39pnrbyap9dhpkbm6z9n82jkll4h-rustls-ffi-0.13.0/lib to CURL_LIBRARY_PATH
curl> configure: error: TLS not detected, you will not be able to use HTTPS, FTPS, NTLM and more.
curl> Use --with-openssl, --with-gnutls, --with-wolfssl, --with-mbedtls, --with-schannel, --with-secure-transport, --with-amissl, --with-bearssl or --with-rustls to address this.

From my understanding this is supposed to have been fixed (curl/curl#13200) so it will require some troubleshooting.

@LeSuisse
Copy link
Contributor Author

cc @cpu to follow up with #299580 (comment)

@cpu
Copy link
Contributor

cpu commented Apr 19, 2024

cc @cpu to follow up with #299580 (comment)

Thanks! I will take a deeper look ASAP.

From my understanding this is supposed to have been fixed (curl/curl#13200) so it will require some troubleshooting.

Hmm that is interesting. I will see if I can reproduce.

@cpu
Copy link
Contributor

cpu commented Apr 20, 2024

I was able to checkout the curl-8_7_1 tag upstream, apply the same patches as proposed here, and build successfully. I manually pointed the PKG_CONFIG_PATH to the nix store path from building rustls-ffi from this branch when running configure --with-rustls. That gives me at least a little bit of confidence that the upstream detection code is working with the patches applied (I think the Gentoo folks confirmed it too). I'm still not sure what's breaking in the passthrough test curl build. My understanding is the pkg-config hook in curl would be doing that PKG_CONFIG_PATH work for us.

I don't think it's load-bearing, but reading "Writing packages providing pkg-config modules " in the manual I noticed there's a meta.pkgConfigModules = [ "rustls" ]; element for describing exported pkg config libs that should probably be included in the rustls-ffi derivation now.

@LeSuisse
Copy link
Contributor Author

I don't think it's load-bearing, but reading "Writing packages providing pkg-config modules " in the manual I noticed there's a meta.pkgConfigModules = [ "rustls" ]; element for describing exported pkg config libs that should probably be included in the rustls-ffi derivation now.

Thanks, I forgot about that. Adjusted in the latest change.

Everything has been fixed, curl builds with rustls and it looks OK 👍

@LeSuisse LeSuisse marked this pull request as ready for review April 20, 2024 20:59
@cpu
Copy link
Contributor

cpu commented Apr 20, 2024

Everything has been fixed, curl builds with rustls and it looks OK 👍

Nice work!

I'm reading the diff and wondering what the key fix was, conditionally adding autoreconfHook to the nativeInputs when the patches are in play?

Edit: Ah, I see you replied in the other thread to confirm.

@LeSuisse
Copy link
Contributor Author

Yes, the autoreconfHook runs autoreconf which will regenerate the configure file provided by the curl release tarball.

Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I was able to pull this branch, build, and test everything successfully on my x86_64 Linux machine.

Hope to have the Apache update sorted soon :-)

@cpu
Copy link
Contributor

cpu commented Apr 22, 2024

Hopefully support for recent version of rustls-ffi will land soon, see https://bz.apache.org/bugzilla/show_bug.cgi?id=67860

FWIW this has landed in-tree but is pending release: https://svn.apache.org/viewvc?view=revision&revision=1917270

I've used this branch to smoke test the change in Nix-land and it worked well 🎉 The main wrinkle is that it's tricky to build HTTPD from a src clone and not a released tarball. I had to hack up the apache derivation a bit to clone apr sources (using srcOnly apr was throwing some weird errors) into srclib and then run the buildconf script for generating the autotools gunk. Something like this diff (Note; sorry about the reformatting noise, made this quickly & with the wrong vim config loaded).

@LeSuisse
Copy link
Contributor Author

Thanks for the upstream fix. I applied the patch on top of sources we are using for Apache and it looks good 👍

@kpcyrd
Copy link
Contributor

kpcyrd commented May 13, 2024

I checked out this branch, built this derivation and diffed the content to the Arch Linux package for 0.13.0 (since the curl-rustls package over there sucessfully uses dynamic linking with librustls.so). The overall package structure is very similar (Arch has extra license files but lacks the static .a library and also has it's debug symbols detached into a different package):

--- is Arch Linux, +++ is NixOS

@@ -1,14 +1,9 @@
 drwxrwxrwx   0        0        0        0 1970-01-01 00:00:00.000000 ./
 drwxrwxrwx   0        0        0        0 1970-01-01 00:00:00.000000 ./include/
 -rw-rw-rw-   0        0        0    87309 1970-01-01 00:00:00.000000 ./include/rustls.h
 drwxrwxrwx   0        0        0        0 1970-01-01 00:00:00.000000 ./lib/
+-rw-rw-rw-   0        0        0 14631106 1970-01-01 00:00:00.000000 ./lib/librustls.a
 lrwxrwxrwx   0        0        0        0 1970-01-01 00:00:00.000000 ./lib/librustls.so -> librustls.so.0.13.0
--rwxrwxrwx   0        0        0  1878432 1970-01-01 00:00:00.000000 ./lib/librustls.so.0.13.0
+-rwxrwxrwx   0        0        0  2389432 1970-01-01 00:00:00.000000 ./lib/librustls.so.0.13.0
 drwxrwxrwx   0        0        0        0 1970-01-01 00:00:00.000000 ./lib/pkgconfig/
--rw-rw-rw-   0        0        0      275 1970-01-01 00:00:00.000000 ./lib/pkgconfig/rustls.pc
-drwxrwxrwx   0        0        0        0 1970-01-01 00:00:00.000000 ./share/
-drwxrwxrwx   0        0        0        0 1970-01-01 00:00:00.000000 ./share/licenses/
-drwxrwxrwx   0        0        0        0 1970-01-01 00:00:00.000000 ./share/licenses/librustls/
--rw-rw-rw-   0        0        0    10847 1970-01-01 00:00:00.000000 ./share/licenses/librustls/LICENSE-APACHE
--rw-rw-rw-   0        0        0      781 1970-01-01 00:00:00.000000 ./share/licenses/librustls/LICENSE-ISC
--rw-rw-rw-   0        0        0     1089 1970-01-01 00:00:00.000000 ./share/licenses/librustls/LICENSE-MIT
+-rw-rw-rw-   0        0        0      337 1970-01-01 00:00:00.000000 ./lib/pkgconfig/rustls.pc

The rustls.h header files are identical, the pkg-config file also seems fine:

@@ -1,10 +1,10 @@
-prefix=/usr
+prefix=/nix/store/7nlybprdl56qj9i3amalmrrlx59fhc0r-rustls-ffi-0.13.0
 exec_prefix=${prefix}
-libdir=${prefix}/lib
+libdir=${exec_prefix}/lib
 includedir=${prefix}/include
 
 Name: rustls
 Description: Rustls bindings for non-Rust languages
 Version: 0.13.0
 Libs: -L${libdir} -lrustls
 Cflags: -I${includedir}

For the binary it's normal to have large differences, in this case most of them are offset changes due to an additional symbol:

image

This is most likely because the Arch Linux package was built with a different rustc Version:

├── readelf --wide --decompress --string-dump=.comment {}
│ @@ -1,5 +1,5 @@
│    
│  String dump of section '.comment':
│ -  [     0]  GCC: (GNU) 14.1.1 20240507
│ -  [    1b]  rustc version 1.78.0 (9b00956e5 2024-04-29) (Arch Linux rust 1:1.78.0-1)
│ +  [     0]  GCC: (GNU) 13.2.0
│ +  [    12]  rustc version 1.77.1 (7cf61ebde 2024-03-27) (built from a source tarball)

I also found a difference in the Library runpath, but I assume this is related to how NixOS works:

image

Overall I think dynamic linking should work fine, I couldn't figure out how to build curl-rustls (like which nix-shell command to run) but I'd be happy to give that a try too (I noticed there's curlWithGnuTls but no curlWithRustls).

@kpcyrd
Copy link
Contributor

kpcyrd commented May 13, 2024

I've resolved the merge conflict in pkgs/tools/networking/curl/default.nix like so:

  patches = lib.optionals (lib.versionOlder finalAttrs.version "8.7.2") [
    # https://github.com/curl/curl/pull/13219
    # https://github.com/newsboat/newsboat/issues/2728
    ./8.7.1-compression-fix.patch
  ] ++ lib.optionals (rustlsSupport && lib.versionOlder finalAttrs.version "8.7.2") [
    # https://github.com/curl/curl/issues/13200 and https://github.com/curl/curl/issues/13248
    ./rustls-build-fix.patch
  ];

The rebased commit can also be found at e727f60.

@LeSuisse
Copy link
Contributor Author

Thanks for taking the time to compare!

I also found a difference in the Library runpath, but I assume this is related to how NixOS works:

Yup this one is not unexpected in the nixpkgs/NixOS context.

Overall I think dynamic linking should work fine, I couldn't figure out how to build curl-rustls (like which nix-shell command to run) but I'd be happy to give that a try too (I noticed there's curlWithGnuTls but no curlWithRustls).

At the root of your nixpkgs clone you can run nix-build -A rustls-ffi.tests.curl to build it. I did not add curlWithRustls just yet as the backend is still marked experimental upstream and it might require some work between upgrades (as seen here :) ).

@kpcyrd
Copy link
Contributor

kpcyrd commented May 14, 2024

Thanks, I built the binary but it doesn't seem to pick up ca-certificates:

$ /nix/store/4xwgm6qsbpa8bs0jnagapb6q8h1awy26-curl-8.7.1-bin/bin/curl -v https://github.com/
* Host github.com:443 was resolved.
* IPv6: (none)
* IPv4: 140.82.121.4
*   Trying 140.82.121.4:443...
* Connected to github.com (140.82.121.4) port 443
* ALPN: curl offers h2,http/1.1
* rustls_connection wants us to write_tls.
* Curl_socket_check: reading would block
* Curl_socket_check: reading would block
* rustls_connection wants us to read_tls.
* rustls_connection_process_new_packets: invalid peer certificate: BadSignature
* Closing connection
curl: (60) rustls_connection_process_new_packets: invalid peer certificate: BadSignature
More details here: https://curl.se/docs/sslcerts.html

curl failed to verify the legitimacy of the server and therefore could not
establish a secure connection to it. To learn more about this situation and
how to fix it, please visit the web page mentioned above.

This could either indicate curl/curl#13248 isn't fully fixed yet, or I'm using the test binary in a way that it's not supposed to be used.

Arch Linux currently ships curl-rustls 8.7.1 by reverting curl/curl@647e86a.

This command is working fine, so it seems rustls-ffi itself is working correctly:

$ /nix/store/4xwgm6qsbpa8bs0jnagapb6q8h1awy26-curl-8.7.1-bin/bin/curl --cacert /etc/ssl/certs/ca-certificates.crt -sSfv https://github.com/ > /dev/null 
* Host github.com:443 was resolved.
* IPv6: (none)
* IPv4: 140.82.121.3
*   Trying 140.82.121.3:443...
* Connected to github.com (140.82.121.3) port 443
* ALPN: curl offers h2,http/1.1
* rustls_connection wants us to write_tls.
* Curl_socket_check: reading would block
* rustls_connection wants us to read_tls.
* rustls_connection wants us to write_tls.
* rustls_connection wants us to read_tls.
* Done handshaking
* ALPN: server accepted h2
* using HTTP/2
* [HTTP/2] [1] OPENED stream for https://github.com/
* [HTTP/2] [1] [:method: GET]
* [HTTP/2] [1] [:scheme: https]
* [HTTP/2] [1] [:authority: github.com]
* [HTTP/2] [1] [:path: /]
* [HTTP/2] [1] [user-agent: curl/8.7.1]
* [HTTP/2] [1] [accept: */*]
> GET / HTTP/2
> Host: github.com
> User-Agent: curl/8.7.1
> Accept: */*
> 
* Request completely sent off
< HTTP/2 200 
< server: GitHub.com
< date: Tue, 14 May 2024 09:57:22 GMT
< content-type: text/html; charset=utf-8
< vary: X-PJAX, X-PJAX-Container, Turbo-Visit, Turbo-Frame, Accept-Language, Accept-Encoding, Accept, X-Requested-With
< content-language: en-US
< etag: W/"6e886ad4ab0d5247c4e88b7b241e312f"
< cache-control: max-age=0, private, must-revalidate
< strict-transport-security: max-age=31536000; includeSubdomains; preload
< x-frame-options: deny
< x-content-type-options: nosniff
< x-xss-protection: 0
< referrer-policy: origin-when-cross-origin, strict-origin-when-cross-origin
[...]

@cpu
Copy link
Contributor

cpu commented May 28, 2024

Thanks, I built the binary but it doesn't seem to pick up ca-certificates:

Very curious! I remember testing curl w/ an earlier rev of this branch and it working OK. Possible I made a mistake, or something shifted since then. In either case this is on my radar to look into when time permits.

@LeSuisse
Copy link
Contributor Author

Rebased now that we have curl 8.8.0 in master.

The cert issue is not new AFAIK, it probably requires some adjustment to handle NIX_SSL_CERT_FILE. It works as expected if a bundle of CA is explicitly provided.

@kpcyrd
Copy link
Contributor

kpcyrd commented Jul 4, 2024

I'm also in favor of merging this

Switched to the recently introduced cargo-c build in order
to get pkg-config/.so config.

Changelog:
https://github.com/rustls/rustls-ffi/blob/v0.13.0/CHANGELOG.md
@SuperSandro2000 SuperSandro2000 merged commit 83d2194 into NixOS:master Aug 1, 2024
26 checks passed
@LeSuisse LeSuisse deleted the rustls-ffi-0.13.0 branch August 1, 2024 20:15
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.

5 participants