Make keys Send and Sync, remove Clone about mundane HOT 7 OPEN

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.)

from mundane.

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

from mundane.

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

from mundane.

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.

from mundane.

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.

from mundane.

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.)

from mundane.

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.

from mundane.