BIGNUM utility cleanups #2325
BIGNUM utility cleanups #2325
Conversation
| @@ -293,6 +294,14 @@ const SslDisposer<T, sslFree> SslDisposer<T, sslFree>::INSTANCE; | |||
| // Using BN_clear_free here ensures that any potentially sensitive information in the | |||
| // BIGNUM is also cleansed when it is freed. | |||
|
|
|||
| using UniqueBignum = std::unique_ptr<BIGNUM, void(*)(BIGNUM*)>; | |||
| kj::Maybe<kj::Own<BIGNUM>> toBignum(kj::ArrayPtr<const kj::byte> data); | |||
There was a problem hiding this comment.
stylistical suggestion: ArrayPtr/Array have .as() method that is designed to help with things like ptr.as<Bignum>(). I found it very helpful for Rust conversions and makes code much easier to read.
There was a problem hiding this comment.
Perhaps if we introduce a Bignum helper that wraps BIGNUM at some point. Given that this is mostly just shifting around the way the existing code already worked, I'd prefer to keep it as is for now but I'm going to be continuing to iterate on this area.
| @@ -1065,8 +1055,8 @@ kj::Own<EVP_PKEY> rsaJwkReader(SubtleCrypto::JsonWebKey&& keyDataJwk) { | |||
| // RSA_set0_*() transfers BIGNUM ownership to the RSA key, so we don't need to worry about | |||
| // calling BN_free(). | |||
| OSSLCALL(RSA_set0_key(rsaKey.get(), | |||
| BN_bin2bn(modulus.begin(), modulus.size(), nullptr), | |||
| BN_bin2bn(publicExponent.begin(), publicExponent.size(), nullptr), | |||
| toBignumUnowned(modulus), | |||
There was a problem hiding this comment.
I wonder if we should add T* kj::Own<T>::disown() rather than create separate functions for such use-cases?
There was a problem hiding this comment.
Yeah, I was considering the same thing. The ability to release ownership is the only reason why I didn't eliminate the std::unique_ptr use here entirely.
jasnell
force-pushed
the
jsnell/crypto-rework-step2
branch
from
69f78a3 to
8acd220
Compare
Eliminates some duplication and condenses some of the handling of BIGNUM types... just one more of a number of PRs that'll be reworking and cleaning up the crypto impl a bit before coalescing with the node.js crypto impl details.