curl client checkpoint #1

ctz · 2024-02-23T17:21:48Z

The commits here are kinda artificial for the purposes of review -- I started from a single commit that contained everything. So there's are some commits that introduce fully-fledged modules that only pay off later. That is especially true of ffi.rs, which is mostly cloned from rustls-ffi.

The initial commit currently on main also contains things wanting review -- please leave comments there and I will collect some fixes on this PR.

At the end of this branch, on ubuntu linux¹ you should be able to:

$ make
(...)
$ LD_LIBRARY_PATH=target/debug curl https://google.com
[2024-02-23T16:42:51Z ERROR ssl::error] raising Error { lib: Ssl, reason: Unsupported, string: Some("_SSL_CTX_set_next_proto_select_cb") }
[2024-02-23T16:42:51Z ERROR ssl::error] raising Error { lib: Ssl, reason: Unsupported, string: Some("_SSL_CTX_set_post_handshake_auth") }
[2024-02-23T16:42:51Z ERROR ssl::error] raising Error { lib: Ssl, reason: Unsupported, string: Some("_SSL_CTX_sess_set_new_cb") }
[2024-02-23T16:42:51Z ERROR ssl::error] raising Error { lib: User, reason: OperationFailed, string: Some("operation would block") }
<HTML><HEAD><meta http-equiv="content-type" content="text/html;charset=utf-8">
<TITLE>301 Moved</TITLE></HEAD><BODY>
<H1>301 Moved</H1>
The document has moved
<A HREF="https://www.google.com/">here</A>.
</BODY></HTML>
[2024-02-23T16:42:51Z ERROR ssl::error] raising Error { lib: User, reason: OperationFailed, string: Some("operation would block") }

In other words, the libssl.so.3 is enough to be a drop-in for really very basic uses of curl.

others untested, but probably work ↩

cpu · 2024-02-23T20:10:10Z

Nice! I will give this some 👀 early-to-mid next week.

djc · 2024-03-20T10:22:24Z

Do we have a good rationale for keeping this work private? It's public in our roadmap, so might make sense to just make this repo public, too?

@cpu is this waiting for something in particular? ISTR that this is relatively high priority?

cpu · 2024-03-20T13:30:12Z

@cpu is this waiting for something in particular?

Sorry, this just fell off my task list when I went out of office. I will do a review pass today.

I don't have any preference w.r.t keeping the repo private or public in the meantime.

ctz · 2024-03-20T15:56:25Z

I don't mind making it public now, really. My thought was: in case the initial commit wants edits, that would mean force-pushing main. But likely not a big deal as I doubt there would be anyone looking at this repo until it contains something useful and substantial.

djc · 2024-03-20T16:04:49Z

I don't mind making it public now, really. My thought was: in case the initial commit wants edits, that would mean force-pushing main. But likely not a big deal as I doubt there would be anyone looking at this repo until it contains something useful and substantial.

It sounds like we have at least a couple of people who would be looking at this repo today if they could, and I remember it came up one time before when I inadvertently linked to it. But yeah, force pushing main is probably okay for now.

cpu

Phew, that's a lot to digest 😆 In general: this is really, really cool.

I left a lot of comments, some of them are actionable feedback, some of them are questions, and some are just me rubber ducking in the hopes it might save someone else reviewing this branch from chasing the same threads as I did. I hope it's useful!

rustls-libssl/src/constants.rs

rustls-libssl/src/entry.rs

rustls-libssl/src/lib.rs

cpu · 2024-03-20T18:46:49Z

rustls-libssl/src/entry.rs

+entry! {
+    pub fn _SSL_get_peer_cert_chain(ssl: *const SSL) -> *mut stack_st_X509 {
+        let ssl = try_clone_arc!(ssl);
+        ssl.lock()
+            .ok()
+            .and_then(|mut ssl| ssl.get_peer_cert_chain().map(|x509| x509.pointer()))
+            .unwrap_or_else(ptr::null_mut)
+    }
+}
+
+entry! {
+    pub fn _SSL_get0_verified_chain(ssl: *const SSL) -> *mut stack_st_X509 {
+        _SSL_get_peer_cert_chain(ssl)
+    }
+}


I wonder if there's somewhere we should be documenting small divergences like occur here. The upstream API has an important distinction between the peer's presented chain (returned in SSL_get_peer_cert_chain) and the verified chain (returned in SSL_get0_verified_chain). Rustls (sensibly) doesn't have this distinction, and so the API surface is flattened here. I don't know that it's super important to call out the change that now SSL_get_peer_cert_chain is returning a verified chain but it gave me reason to think.

rustls-libssl/src/lib.rs

cpu · 2024-03-20T18:53:31Z

rustls-libssl/test-ca/rsa/end.cert

@@ -0,0 +1,24 @@
+-----BEGIN CERTIFICATE-----


This expires in 2029, so we have a decent window before needing to regenerate it, but there might still be value in lifting some of the rcgen test PKI tooling in here to make these easier to recreate.

rustls-libssl/src/entry.rs

cpu · 2024-03-20T19:10:02Z

At the end of this branch, on ubuntu linux you should be able to:

Can I ask what curl version you're getting on your Ubuntu system? I haven't looked closely yet, but on my system w/ curl 8.6.0 the repro fails with a complaint about the SSL_CTX_set_cert_store symbol being missing:

[daniel@blanc:~/Code/Rust/rustls-openssl-compat/rustls-libssl]$ curl --version
curl 8.6.0 (x86_64-pc-linux-gnu) libcurl/8.6.0 OpenSSL/3.0.13 zlib/1.3.1 brotli/1.1.0 zstd/1.5.5 libidn2/2.3.7 libpsl/0.21.5 libssh2/1.11.0 nghttp2/1.59.0
Release-Date: 2024-01-31
Protocols: dict file ftp ftps gopher gophers http https imap imaps ipfs ipns mqtt pop3 pop3s rtsp scp sftp smb smbs smtp smtps telnet tftp
Features: alt-svc AsynchDNS brotli GSS-API HSTS HTTP2 HTTPS-proxy IDN IPv6 Kerberos Largefile libz NTLM PSL SPNEGO SSL threadsafe TLS-SRP UnixSockets zstd

[daniel@blanc:~/Code/Rust/rustls-openssl-compat/rustls-libssl]$ LD_LIBRARY_PATH=target/debug curl https://google.com
curl: symbol lookup error: /nix/store/2hapkajcapp9vzwrlj58jwsrjpr4vj70-curl-8.6.0/lib/libcurl.so.4: undefined symbol: SSL_CTX_set_cert_store, version OPENSSL_3.0.0

[daniel@blanc:~/Code/Rust/rustls-openssl-compat/rustls-libssl]$ nm target/debug/libssl.so | grep "SSL_CTX_set_cert"

[daniel@blanc:~/Code/Rust/rustls-openssl-compat/rustls-libssl]$ nm /nix/store/h24v1xs4i9l3k18ygxvs62361rl77804-openssl-3.0.13/lib/libssl.so.3 | grep "SSL_CTX_set_cert"
0000000000039180 T SSL_CTX_set_cert_cb
000000000003a170 T SSL_CTX_set_cert_store
0000000000039150 T SSL_CTX_set_cert_verify_callback

ctz · 2024-03-21T10:05:49Z

Thanks for the review! I'll work my way through these today.

Can I ask what curl version you're getting on your Ubuntu system?

Ubuntu 22.04 has curl 7.81.0.

rustls-libssl/src/constants.rs

Introduce integration test programs -- written in C and linked against the openssl known to pkg-config -- that are required to produce the same output when run against rustls-libssl or openssl-libssl. Run `make integration`. Note that openssl project has no test coverage of these functions, so they produce incorrect output (eg, `SSL_alert_desc_string_long(TLS13_AD_MISSING_EXTENSION)` gives "unknown"). This commit gives bug-for-bug compatibility because that is easiest to test for.

These are notable because they have different return value convention, `error::MysteriouslyOppositeReturnValue` represents that fact.

This includes `SSL_ctrl` because it backs `SSL_set_tlsext_host_name` for setting the SNI hostname. `SSL_CTX_ctrl` comes along for the ride.

Some of these certainly will be implemented later, others almost certainly will not.

cpu · 2024-04-02T20:14:13Z

Ubuntu 22.04 has curl 7.81.0.

I came back around to see if I could repro the check-point success by matching this curl version and the answer was: almost 🎉

If I stub SSL_CTX_set_default_verify_paths and then also specify -capath or -cadir manually the demo works as expected. I think it's probably a difference in configure options between what Ubuntu/Debian ships and what the NixPkgs derivation I hacked up to use 7.81.0 is doing. Would it be helpful if I tried to implement this fn properly? It seems doable but maybe it's mostly worthwhile as a task to get familiar with this repo vs a critical piece of functionality for our goals.

Otherwise it seems like all of my review feedback was addressed. Should we merge? 🚀

ctz · 2024-04-03T12:47:06Z

Would it be helpful if I tried to implement this fn properly?

Yes that sounds like a good call!

cpu · 2024-04-03T14:04:57Z

cpu deleted the jbp-curl-checkpoint branch now

I turned on the automatic branch tidying in the settings to match the other repos. Hope that's OK.

ctz · 2024-04-03T14:13:14Z

Yep! I have just protected the main branch too, and made this repo public.

cpu reviewed Mar 20, 2024

View reviewed changes

cpu reviewed Mar 21, 2024

View reviewed changes

rustls-libssl/src/constants.rs Outdated Show resolved Hide resolved

ctz force-pushed the jbp-curl-checkpoint branch 2 times, most recently from 7145fb9 to b33f41e Compare March 26, 2024 15:23

ctz added 15 commits March 26, 2024 15:29

Use latest stable rust for valgrind/fmt jobs

e58d98e

Implement SSL_CIPHER functions

db5e32e

Implement SSL_CTX_set_alpn_protos/SSL_set_alpn_protos

1f70de6

These are notable because they have different return value convention, `error::MysteriouslyOppositeReturnValue` represents that fact.

Implement SSL_set_fd and others involving BIOs

938e98a

Implement SSL and SSL_CTX options functions

5a7d345

Implement hostname configuration functions

e272964

This includes `SSL_ctrl` because it backs `SSL_set_tlsext_host_name` for setting the SNI hostname. `SSL_CTX_ctrl` comes along for the ride.

Implement client/server modal, inc. SSL_is_server

c0646e0

Implement SSL_connect, inc. server verifier

6f1936a

Implement SSL_shutdown and associated

ea92424

Implement SSL_read, SSL_write and associated

ccdbd0c

Implement certificate, ALPN & error accessor functions

9277393

Run an integration test covering TLS client APIs

a5fef15

Implement BIO_f_ssl stub

8f04bb5

Stub out a wedge of functions

dae2189

Some of these certainly will be implemented later, others almost certainly will not.

ctz force-pushed the jbp-curl-checkpoint branch from 736cc01 to dae2189 Compare March 26, 2024 15:29

ctz merged commit 902c17a into main Apr 3, 2024
20 checks passed

cpu deleted the jbp-curl-checkpoint branch April 3, 2024 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

curl client checkpoint #1

curl client checkpoint #1

ctz commented Feb 23, 2024

cpu commented Feb 23, 2024

djc commented Mar 20, 2024

cpu commented Mar 20, 2024

ctz commented Mar 20, 2024

djc commented Mar 20, 2024 •

edited

Loading

cpu left a comment

cpu Mar 20, 2024

cpu Mar 20, 2024

cpu commented Mar 20, 2024

ctz commented Mar 21, 2024

cpu commented Apr 2, 2024

ctz commented Apr 3, 2024

cpu commented Apr 3, 2024

ctz commented Apr 3, 2024

curl client checkpoint #1

curl client checkpoint #1

Conversation

ctz commented Feb 23, 2024

Footnotes

cpu commented Feb 23, 2024

djc commented Mar 20, 2024

cpu commented Mar 20, 2024

ctz commented Mar 20, 2024

djc commented Mar 20, 2024 • edited Loading

cpu left a comment

Choose a reason for hiding this comment

cpu Mar 20, 2024

Choose a reason for hiding this comment

cpu Mar 20, 2024

Choose a reason for hiding this comment

cpu commented Mar 20, 2024

ctz commented Mar 21, 2024

cpu commented Apr 2, 2024

ctz commented Apr 3, 2024

cpu commented Apr 3, 2024

ctz commented Apr 3, 2024

djc commented Mar 20, 2024 •

edited

Loading