feat: subtle.generateKey support for ecdsa, ecdh
#304
Conversation
boorad
force-pushed
the
feat/subtle-generatekey-ecdsa
branch
from
81a6e23
to
1237592
Compare
boorad
changed the title
subtle.generateKey support for ECDSAsubtle.generateKey support for ecdsa, ecdh
boorad
force-pushed
the
feat/subtle-generatekey-ecdsa
branch
from
9016f81
to
a2032f9
Compare
|
|
| std::shared_ptr<RsaKeyPairGenConfig> rsaConfig; | ||
| std::shared_ptr<EcKeyPairGenConfig> ecConfig; | ||
|
|
||
| // switch on variant to get proper config from arguments | ||
| // outside of lambda 🤮 | ||
| if (variant == kvRSA_SSA_PKCS1_v1_5 || | ||
| variant == kvRSA_PSS || | ||
| variant == kvRSA_OAEP | ||
| ) { | ||
| rsaConfig = std::make_shared<RsaKeyPairGenConfig>( | ||
| prepareRsaKeyGenConfig(runtime, arguments)); | ||
| } else | ||
| if (variant == kvEC) { | ||
| ecConfig = std::make_shared<EcKeyPairGenConfig>( | ||
| prepareEcKeyGenConfig(runtime, arguments)); | ||
| } else { | ||
| throw std::runtime_error("KeyVariant not implemented" | ||
| + std::to_string((int)variant)); | ||
| } |
There was a problem hiding this comment.
Hm, can't we make a single abstract base class and only construct the type we need? Do those things share logic?
Something like
class KeyPairGenConfig { ... }
class RsaKeyPairGenConfig: public KeyPairGenConfig { ... }
class EcKeyPairGenConfig: public KeyPairGenConfig { ... }Inheritance/abstraction is better here.
| auto keys = generateRSAKeyPair(runtime, config); | ||
| auto publicKey = toJSI(runtime, keys.first); | ||
| auto privateKey = toJSI(runtime, keys.second); | ||
| jsCallInvoker->invokeAsync([&runtime, resolve, |
There was a problem hiding this comment.
Wait what? What's the point of this Thread? It will immediately jump back to the JS Thread because of callInvoker->invokeAsync, why do we do that??
Nothing is running on a separate Thread here.
We probably want to call generateKeyPair(...) on the Thread, and instead of constructing jsi::Values we construct an intermediate type first, then call back to JS using jsCallInvoker->invokeAsync, and then create the jsi::Values and resolve the promise.
| if (variant == kvRSA_SSA_PKCS1_v1_5 || | ||
| variant == kvRSA_PSS || | ||
| variant == kvRSA_OAEP | ||
| ) { | ||
| keys = generateRsaKeyPair(runtime, rsaConfig); | ||
| } else | ||
| if (variant == kvEC) { | ||
| keys = generateEcKeyPair(runtime, ecConfig); | ||
| } else { |
There was a problem hiding this comment.
Again - duplicated logic, and an unnecessary pre-allocation. Just do a single abstract class which has generateKeyPair - both RSA and EC have different implementations. Much cleaner and less code.
| // switch on variant to get proper config/genKeyPair | ||
| if (variant == kvRSA_SSA_PKCS1_v1_5 || | ||
| variant == kvRSA_PSS || | ||
| variant == kvRSA_OAEP | ||
| ) { | ||
| auto config = std::make_shared<RsaKeyPairGenConfig>( | ||
| prepareRsaKeyGenConfig(runtime, arguments)); | ||
| keys = generateRsaKeyPair(runtime, config); | ||
| } else | ||
| if (variant == kvEC) { | ||
| auto config = std::make_shared<EcKeyPairGenConfig>( | ||
| prepareEcKeyGenConfig(runtime, arguments)); | ||
| keys = generateEcKeyPair(runtime, config); | ||
| } else { | ||
| throw std::runtime_error("KeyVariant not implemented: " + | ||
| std::to_string((int)variant)); | ||
| } | ||
| // keys.first = publicKey keys.second = privateKey |
There was a problem hiding this comment.
Again, abstraction. Code on interface level.
| return key_ctx; | ||
| } | ||
|
|
||
| std::pair<jsi::Value, jsi::Value> generateEcKeyPair(jsi::Runtime& runtime, |
There was a problem hiding this comment.
I like that we go away from JSVariant, but I wonder if it makes sense to use C++ types instead of jsi::Value here.
If we do that, we could avoid Runtime specific calls and allow this method to run on any Thread.
Since we use jsi::Runtime here, we can only call this method on the JS-thread. So this cannot be called async.
Add support for
subtle.generateKey()forECformats (ECDSA, ECDH)closes #59
closes #80
closes #288