Expose mTLS related APIs, introduce ForeignTypeExt + ForeignTypeRefExt, add corresponds macro #262

rushilmehra · 2024-08-09T11:16:20Z

ForeignTypeExt and ForeignTypeRefExt are inspired by sfackler/rust-openssl#1345, which make dealing with FFI safer and more ergonomic. The new APIs also allow for gracefully handling instances where the initial API call results in NULL

hcstern · 2024-08-13T18:33:08Z

boring/src/ssl/mod.rs

It would probably be easier overall to review if the "corresponds" macro change was split in to a separate PR. It looks like github now allows you to review each commit individually (at least that's new to me) which is helpful though.

Yeah given how difficult it is to get reviews, I highly prefer leaving everything in one PR and relying on the per-commit diff viewer. I can update the title to be more sensible

hcstern · 2024-08-13T18:35:45Z

boring/src/ssl/mod.rs

+        unsafe {
+            let ptr = cert_store.as_ptr();
+            cvt(ffi::SSL_set0_verify_cert_store(self.as_ptr(), ptr) as c_int)?;
+            mem::forget(cert_store);


I took a brief look at the manpage and it's not clear to me why we need to pass cert_store by value and then forget it, rather than just passing a reference. Is it just that this only happens once on startup and it's okay to leak? It might be nice to have a // SAFETY: block

Also for some reason when I inspect the first commit individually it says the function as a whole is new, but when I look at master it's already there and looking at the second commit it only shows the actual cert_store.as_ptr() change. weird.

On master you may be seeing the SSL_CTX_set0_verify_cert_store variant

There are generally two variants for each API - one that operates on SSL_CTX (applies to the server generally), and another that operates on SSL (connection specific)

Re: calling mem::forget manually, it's a good question, but I'm mainly following prior art: https://github.com/sfackler/rust-openssl/blob/master/openssl/src/ssl/mod.rs#L3408-L3414

To summarize: boringssl takes ownership of the X509Store (as Rushil mentioned above) and gets freed by boringssl, which is why we use the 0 variant and forget the Rust struct. Thanks for taking the time to help me understand!

For posterity, this works by virtue of refcounted objects in c++. When initialized, a X509Store will have a refcount of 1. Any time the destructor is called, the refcount will be decremented and checked against == 0 before the object is fully freed. By using the set0 variant of the API, we "hand off" the object at refcount == 1 to c++ and leave the memory's lifetime at mercy of the next destructor call

Given that this is a common pattern I still think it would be nice to document somewhere

The intent should be conveyed by using <X509Store as ForeignType>::into_ptr

Opened #267

rushilmehra · 2024-08-14T17:06:23Z

boring/src/x509/mod.rs

+    ///
+    /// This function will return `None` if the underlying string contains invalid utf-8.
+    #[corresponds(X509_NAME_oneline)]
+    pub fn oneline(&self) -> Option<String> {


This API is technically deprecated by openssl, and its use is strongly discouraged. I'm probably going to remove it from this PR and call some unsafe code directly where I need to, until we're able to stop using this API entirely

evanrittenhouse · 2024-08-14T17:09:06Z

boring/src/x509/mod.rs

+            let out = CStr::from_ptr(ptr).to_str().map(|s| s.to_string());
+            // SAFETY: `X509_NAME_oneline` dynamically allocates memory for the string that's
+            // ultimately returned. As such, it's the caller's responsibility to free that memory
+            // after allocating an owned String in rust land.


Should this be in the doc comment if user action is required?

E: oh I see, you mean the bindings, nvm

`ForeignTypeExt` and `ForeignTypeRefExt` are inspired by sfackler/rust-openssl#1345, which make dealing with FFI safer and more ergonomic. The new APIs (e.g. from_const_ptr_opt`) also allow for gracefully handling instances where the initial API call results in `NULL`. Instead of crashing the program, `None` will be returned.

Our rustdocs are miserably broken. We manually link to openssl docs in most binding definitions, and openssl keeps changing their documentation URL, so in order to fix everything I'd have to touch every single binding definition in every single file. Instead, we should use the `corresponds` macro from the openssl-macros crate which nicely adds a link to the openssl documentation on our behalf. If the openssl documentation url ever changes again in the future, a simple dependency bump should solve the issue.

… X509NameRef::print_ex

bwesterb · 2024-08-15T21:56:49Z

boring/src/ssl/mod.rs

+        );
+
+        unsafe { ffi::SSL_set_client_CA_list(self.as_ptr(), list.as_ptr()) }
+        mem::forget(list);


Why is this legitimate?

SSL_set_client_CA_list takes ownership of the list: https://github.com/google/boringssl/blob/master/include/openssl/ssl.h#L2966

Constructing the Stack<X509Name> requires the application to own the Stack, and the owned type will free the underlying memory if drop is called. The general pattern here is to pass ownership to boringssl, and call mem::forget to release that ownership without calling drop and destroying the underlying stack

rushilmehra force-pushed the add-mtls-apis branch 2 times, most recently from c2993c6 to c88f335 Compare August 9, 2024 11:29

rushilmehra requested review from inikulin and nox August 9, 2024 12:33

evanrittenhouse approved these changes Aug 13, 2024

View reviewed changes

rushilmehra force-pushed the add-mtls-apis branch 3 times, most recently from 5419a22 to df2386d Compare August 13, 2024 05:25

hcstern reviewed Aug 13, 2024

View reviewed changes

rushilmehra changed the title ~~Expose mTLS related APIs~~ Expose mTLS related APIs, introduce ForeignTypeExt + ForeignTypeRefExt, add corresponds macro Aug 13, 2024

hcstern reviewed Aug 13, 2024

View reviewed changes

rushilmehra requested a review from bwesterb August 13, 2024 20:01

Expose mTLS related APIs

51ac3b5

rushilmehra force-pushed the add-mtls-apis branch from df2386d to 9017225 Compare August 14, 2024 00:37

rushilmehra commented Aug 14, 2024

View reviewed changes

evanrittenhouse reviewed Aug 14, 2024

View reviewed changes

rushilmehra force-pushed the add-mtls-apis branch from 9017225 to fd66d4c Compare August 14, 2024 21:20

rushilmehra added 3 commits August 15, 2024 12:45

Expose X509NameRef::print_ex

786ee18

rushilmehra force-pushed the add-mtls-apis branch from fd66d4c to 786ee18 Compare August 15, 2024 19:47

rushilmehra added 2 commits August 15, 2024 13:46

Add tests for X509Ref::subject_key_id, X509Ref::authority_key_id, and…

2b51e29

… X509NameRef::print_ex

Fix macos FIPS crossbuild

b2c1e4c

rushilmehra force-pushed the add-mtls-apis branch from 5129145 to b2c1e4c Compare August 15, 2024 20:46

bwesterb reviewed Aug 15, 2024

View reviewed changes

bwesterb approved these changes Aug 15, 2024

View reviewed changes

rushilmehra merged commit a7bfe0d into master Aug 15, 2024
23 checks passed

rushilmehra deleted the add-mtls-apis branch August 15, 2024 22:09

rushilmehra mentioned this pull request Sep 4, 2024

Upgrade ForeignTypes to grab ForeignType::into_ptr sfackler/rust-openssl#2297

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose mTLS related APIs, introduce ForeignTypeExt + ForeignTypeRefExt, add corresponds macro #262

Expose mTLS related APIs, introduce ForeignTypeExt + ForeignTypeRefExt, add corresponds macro #262

rushilmehra commented Aug 9, 2024 •

edited

Loading

hcstern Aug 13, 2024

rushilmehra Aug 13, 2024

hcstern Aug 13, 2024

hcstern Aug 13, 2024

rushilmehra Aug 13, 2024

rushilmehra Aug 13, 2024

rushilmehra Aug 13, 2024

hcstern Aug 13, 2024 •

edited

Loading

rushilmehra Aug 14, 2024

evanrittenhouse Aug 14, 2024

nox Sep 3, 2024

rushilmehra Sep 3, 2024

rushilmehra Aug 14, 2024

evanrittenhouse Aug 14, 2024 •

edited

Loading

bwesterb Aug 15, 2024

rushilmehra Aug 15, 2024

Expose mTLS related APIs, introduce ForeignTypeExt + ForeignTypeRefExt, add corresponds macro #262

Expose mTLS related APIs, introduce ForeignTypeExt + ForeignTypeRefExt, add corresponds macro #262

Conversation

rushilmehra commented Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hcstern Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanrittenhouse Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushilmehra commented Aug 9, 2024 •

edited

Loading

hcstern Aug 13, 2024 •

edited

Loading

evanrittenhouse Aug 14, 2024 •

edited

Loading