mark openssl classes as shareable when frozen

[HoneyryderChuck]

And freeze OpenSSL::SSL::DEFAULT_CERT_STORE by default.

Closes #521

[rhenium]

Unfortunately, it's not that simple. Some more changes are required before we can safely set the flag.

Currently, quite a number of methods in ruby/openssl don't take into account the frozen state of the Ruby object. This is a bug which should be fixed anyway. But simply inserting a rb_check_frozen(self) call in each method is not enough.

What makes it tricky is that some OpenSSL objects are reference counted. We can create multiple Ruby objects wrapping the same OpenSSL object, so whenever making one of them shareable, we must somehow ensure all Ruby objects are frozen.

p1 = OpenSSL::PKey::EC.generate("prime256v1")
cert = OpenSSL::X509::Certificate.new.tap { |x| x.public_key = p1 }
p2 = cert.public_key
# p1 and p2 wrap the same EVP_PKEY object

Since most of referenced counted OpenSSL objects have fields to store arbitrary data, it should be doable, but it's not a trivial task.

Some objects can't be made shareable. One example I can think of is OpenSSL::X509::StoreContext (when instantiated in ossl_verify_cb_call()). The underlying X509_STORE_CTX object is owned by OpenSSL and modified without proper locking, so we can't make it shareable safely. We'll have to check carefully each of the classes.

[HoneyryderChuck]

Nothing is ever that simple with openssl 😂

I'm listing here the changed classes, accessing for each, and can, depending on feedback, break it down into smaller PRs / add missing details:

OpenSSL::BN

There are a few functions (set_bit!, clear_bit!...) which change its state, even after it's frozen:

a = OpenSSL::BN.new(2).freeze
a.set_bit!(3) #=> #<OpenSSL::BN 10>, should have raised!

In order to comply, a few rb_check_frozen calls need to be added. That should make BNs both frozen and shareable.

OpenSSL::Cipher

There are many functions here which change internal state, which require a rb_check_frozencheck: besides the obvious=-terminated ones, there's also #encryptand#decrypt`. That should be enough to make sure it's frozen status be respected.

About it being shareable, that's harder for me to access. It all falls down to EVP_CipherUpdate and EVP_CipherFinal calls with the EVP_CIPHER_CTX object, and this SO post says that's not thread-safe. So in a ractor context, we either have a CRuby API ensuring exclusive access to the object, store the object in ractor storage, or we'd consider ciphers unshareable across ractors (can't think of another outcome).

OpenSSL::Config

This seems like a no-brainer: frozen when shareable (whenever initialized, its state does not change).

OpenSSL::Digest/OpenSSL::HMAC

There does not seem any value on making them shareable. It should probably not be possible to freeze it either. The latter does not seem to be enforceable, ,so it should at least be impossible to do dig << "bang" when frozen, for consistency.

OpenSSL::NETSCAPE::SPKI

When frozen, assigning public key should raise an error. Freezing it should (probably) also free the key. Can't find any docs on whether NETSCAPE_SPKI_sign and NETSCAPE_SPKI_verify are thread-safe, so perhaps not shareable.

OpenSSL::OCSP::Request/Response/*

When frozen, none of the =-terminated calls should be possible. Considering that its state should not change beyond, it should be considered shareable.

OpenSSL::PKCS12

No setter, should probably be frozen and shareable by default.

OpenSSL::PKCS7

Setters and methods such as #add.certificate should be forbidden. Once initialized/configured, PKCS7_SIGNER_INFO and PKCS7_RECIP_INFO should be considered usable across threads, so it should be shareable.

OpenSSL::PKey

When frozen (and as there are no setters), should be considered safe to use across threads: Both #encrypt and #decrypt use on-the-fly EVP_PKEY_CTX objects.

OpenSSL::PKey::EC

tricky 😂 can the same as above apply? (minus setters)

OpenSSL::Provider

frozen when shareable (no setters).

OpenSSL::SSL::SSLSocket

Probably not shareable, like TCP/UDP Sockets aren't?

OpenSSL::SSL::SSLContext

Already freeze-able. Perhaps due to above, not shared?

OpenSSL::SSL::Session

Shareable when frozen.

OpenSSL::Timestamp::Request/Response

Shareable when frozen

OpenSSL::X509::Attribute/Certificate/CRL/ExtensionFactory/Name/Revoked

Setters raise on frozen. Shareable when frozen.

OpenSSL::X509::Store/StoreContext

Setters and add_* raise on frozen. Shareable when frozen.

WDYT? while reading over docs about openssl, the general idea I got was that, once configured, most objects can be used safely across threads.

[rhenium]

OpenSSL::BN

Looks good to me: #808

OpenSSL::Config

#809

OpenSSL::Digest/OpenSSL::HMAC

There does not seem any value on making them shareable. It should probably not be possible to freeze it either. The latter does not seem to be enforceable, ,so it should at least be impossible to do dig << "bang" when frozen, for consistency.

Agreed, same with OpenSSL::Cipher.

OpenSSL::NETSCAPE::SPKI

When frozen, assigning public key should raise an error. Freezing it should (probably) also free the key. Can't find any docs on whether NETSCAPE_SPKI_sign and NETSCAPE_SPKI_verify are thread-safe, so perhaps not shareable.

#sign modifies itself, so it's most likely unsafe. #verify sounds safe, but it's worth a double check (looking at other objects like PKCS7, as commented below).

OpenSSL::OCSP::Request/Response/*

When frozen, none of the =-terminated calls should be possible. Considering that its state should not change beyond, it should be considered shareable.

#sign methods would be unsafe, too.

OpenSSL::PKCS12

No setter, should probably be frozen and shareable by default.

OpenSSL::PKCS7

Setters and methods such as #add.certificate should be forbidden. Once initialized/configured, PKCS7_SIGNER_INFO and PKCS7_RECIP_INFO should be considered usable across threads, so it should be shareable.

PKCS7 seems tricky...

OpenSSL::PKCS7#decrypt (PKCS7_decrypt()) is supposed to be a read-only operation.

• PKCS7_decrypt() takes a non-const pointer and calls an internal function named PKCS7_dataDecode(): https://github.com/openssl/openssl/blob/a3660729e68dc11c01edb4a349ff2610b6b59ee0/crypto/pkcs7/pk7_smime.c#L478-L549
• PKCS7_dataDecode() sets an internal field PKCS7::state to PKCS7_S_HEADER: https://github.com/openssl/openssl/blob/a3660729e68dc11c01edb4a349ff2610b6b59ee0/crypto/pkcs7/pk7_doit.c#L436-L716
• Apparently, the field PKCS7::state is only ever written and never read.
• From a quick look, there is nothing that may be thread-unsafe, but it definitely needs a more careful code reading.

OpenSSL::PKey

When frozen (and as there are no setters), should be considered safe to use across threads: Both #encrypt and #decrypt use on-the-fly EVP_PKEY_CTX objects.

OpenSSL::PKey::EC

tricky 😂 can the same as above apply? (minus setters)

It does have setters if ruby/openssl is compiled with an older version of OpenSSL. Since EVP_PKEY is a reference counted object, checking the frozen state is insufficient.

However, it seems unlikely someone using Ractors uses such an old version of OpenSSL.

OpenSSL::Provider

frozen when shareable (no setters).

OpenSSL::SSL::SSLSocket

Probably not shareable, like TCP/UDP Sockets aren't?

I think so. Frozen socket doesn't seem useful.

OpenSSL::SSL::SSLContext

Already freeze-able. Perhaps due to above, not shared?

SSL_CTX is supposed to be thread-safe. It should be able to be made shareable once the dependencies (PKey, X509::Certificate, and X509::Store) are fixed.

SSL_CTX is reference counted, but there is only a single Ruby object.

OpenSSL::SSL::Session

Shareable when frozen.

SSL_SESSION is reference counted and we can have multiple Ruby objects wrapping the same object.

I don't have an immediate idea how this can be done.

OpenSSL::Timestamp::Request/Response

Shareable when frozen

I think so.

OpenSSL::X509::Attribute/Certificate/CRL/ExtensionFactory/Name/Revoked

Setters raise on frozen. Shareable when frozen.

OpenSSL::X509::Store/StoreContext

Setters and add_* raise on frozen. Shareable when frozen.

I added a comment in #807, but this is tricky...