Make keys Send and Sync, remove Clone

[joshlf]

To err on the safe side, we initially made our key objects neither Send nor Sync. However, BoringSSL provides concurrency semantics for key objects that allow us to relax these restrictions. In particular, key objects' reference counts are incremented and decremented atomically, and all BoringSSL operations on keys have well-defined concurrency safety semantics.

I propose that we implement Sync on key objects, and use BoringSSL's definition of which functions are mutating to decide which methods to have take a &mut self vs a &self receiver.

An aside on Clone and reference counting

Currently, keys are reference counted, and cloning a Mundane key object obtains a new reference-counted reference to the same underlying object. Unfortunately, we cannot implement either Send or Sync so long as it's possible to obtain multiple references to the same underlying object.

If reference-counted key objects are Send, then different clones could be sent to different threads, and we'd have no way of preventing those two separate clones from being operated on using &mut self methods concurrently, which would be unsound.

If reference-counted key objects are Sync, then different clones owned by one thread could be accessed concurrently from different threads, and we'd have no way of preventing those two separate clones from being operated on using &mut self methods concurrently, which would be unsound.

Thus, I conclude that we must remove the ability to clone key objects. The primary benefit to reference counting in BoringSSL is to be able to use keys concurrently from multiple threads at once. Since Rust's lifetime system allows us to share references across threads safely, we get the same advantage even without reference counting. Even if reference counting were desired, we could put a Mundane key object inside of an Rc or an Arc and get the same effect.

Thus, the concrete tasks are:

• [ ] Remove the ability to Clone key objects
• [ ] Audit the codebase to ensure that all mutating BoringSSL functions are exposed via &mut self methods, and document our strategy and reasoning in code comments
• [ ] Implement Send and Sync on key objects

Old (incorrect) text:

BoringSSL key types are reference-counted, and use reference counting to implement Clone. While the reference counting itself is thread-safe (see CRYPTO_refcount_xxx, crypto/internal.h), it's not clear that all operations on keys are also thread-safe. In other words, having two key objects in different threads which are both references to the same underlying BoringSSL object may mean that calling methods on those objects concurrently is unsound. As a result, our key objects do not implement Send.

Eventually, we will want to identify which methods are thread-safe and which are not. This is not only a prerequisite for making our key objects Send, it's also a prerequisite for making them Sync. However, we can much more easily unblock making our key objects Send by just not implementing Clone so that a given key object is always the only reference to its underlying BoringSSL object.

[davidben]

The headers should already document the thread-safety rules. They contain text like:

A given object may be used concurrently on multiple threads by non-mutating functions, provided no other thread is concurrently calling a mutating function. Unless otherwise documented, functions which take a const pointer are non-mutating and functions which take a non-const pointer are mutating.

And, when some thread-safe functions aren't const for silly reasons,

These functions are considered non-mutating for thread-safety purposes and may be used concurrently.

Please let us know if we missed anything important. The intent is that you can use a key concurrently. TLS servers often do and indeed for RSA objects particularly, it's preferably to sharing them performance. (There's a blinding cache and, owing to some poor API decisions in OpenSSL, some parts of the private key need to be initialized lazily rather than when the key is created.)

(Of course, it's not safe to use a key concurrently with mutating it, hence the const rule.)

[joshlf]

Oh! Not sure how I missed that, thanks!. I'll update the description accordingly.

[joshlf]

OK, @davidben let me know if the updated issue description seems accurate to you.

[davidben]

Seems plausible. I don't know enough about Rust to evaluate the Send, Sync, and Clone proposal, so I'll leave that to you. On the subject of ref-counting or copying keys and &mut self methods, one thing to note is the non-const BoringSSL functions are all key-mutating APIs, to change what key an RSA represents. You almost certainly do not want to expose those at all.

They're a quirk of a problematic OpenSSL API pattern. Rather than making functions like RSA_new_public(n, e) which returns an immutable RSA, it's split into RSA_new() + RSA_set0_key(rsa, n, e, NULL). If you don't expose that split, your keys should be immutable, I think.

[joshlf]

Ah interesting. I think it's still probably worth not exposing Clone since Rust references are just as good here, and Rust's reference-counting facilities (Rc and Arc) allow more flexibility wrt thread safety than BoringSSL's. Plus, it will allow us the flexibility to add mutating methods in the future if we want. But I agree that we probably won't need them.

[davidben]

Seems reasonable.

For completeness, the built-in ref-counting would avoid an extra allocation and layer of indirection. For C++ code, I think we usually encourage folks to use bssl::UniquePtr directly and bssl::UpRef when they want to clone, rather than wrap in a std::shared_ptr. But sticking with more idiomatic Rust patterns also makes sense.

(bssl::UniquePtr is kind of a misnomer as a result, but it'd be too much of a nuisance to rename at this point.)

[joshlf]

For completeness, the built-in ref-counting would avoid an extra allocation and layer of indirection. For C++ code, I think we usually encourage folks to use bssl::UniquePtr directly and bssl::UpRef when they want to clone, rather than wrap in a std::shared_ptr. But sticking with more idiomatic Rust patterns also makes sense.

That's fair, although given that crypto operations are so expensive (compared to pointer indirection), I don't think it's worth making an un-idiomatic API.