make cert/crl/name/attr/revoked/ext/extfactory shareable when frozen

[HoneyryderChuck]

Considering we're discussing the semantics of freezing Certs/CRLs here.

[rhenium]

These classes need more careful review of each OpenSSL function. I haven't reviewed enough, but I can spot a few issues:

• OpenSSL::X509::Certificate#tbs_bytes calls i2d_re_X509_tbs(), which has a side effect of refreshing DER-encoding cache.

• OpenSSL::X509::Certificate#sign modifies itself.

• OpenSSL::X509::Name#to_der seems to be unsafe when the object was previously modified: https://github.com/openssl/openssl/blob/ba6f115ccfbb63fbeb2bc8df3c07918a7a59a186/crypto/x509/x_name.c#L220-L227
  The whole X509_NAME appears to be not written with thread safety in mind.

[HoneyryderChuck]

Added frozen check for the mentioned functions in ossl_x509cert.c.

The whole X509_NAME appears to be not written with thread safety in mind.

Indeed. Zooming out a bit, perhaps we're better off just ensuring "shareable-when-frozen" only for certificates and crls? The whole point of the other objects is to use them to modify certs/crls via the setter methods that are now frozen-checked, so there doesn't seem to be a real usage for them outside of that use-case. LMK what you think.

[rhenium]

Added frozen check for the mentioned functions in ossl_x509cert.c.

I have not reviewed other methods. Please check the manpage of each function and the implementation if necessary, as there are so many edge cases in OpenSSL. Not having "set" or "add" in a function name doesn't necessarily mean it is completely read-only.

Zooming out a bit, perhaps we're better off just ensuring "shareable-when-frozen" only for certificates and crls? The whole point of the other objects is to use them to modify certs/crls via the setter methods that are now frozen-checked, so there doesn't seem to be a real usage for them outside of that use-case. LMK what you think.

I don't agree it's the whole point, except for OpenSSL::X509::ExtensionFactory that is indeed intended for building a certificate. That said, Name, Attribute, Revoked, and Extension are a non-reference-counted object and we can handle separately, after Certificate, etc. are done.