Implement webcrypto import ECDSA key and export as SPKI #200
Conversation
# Conflicts: # src/keys.ts
|
Thanks for this PR as well, this is really amazing stuff Oscar!!!! ❤️ It's quite a big PR though, give us some time to go through the changes - we'll internally go through everything this week :) |
|
Awesome stuff @ospfranco! |
|
I see there are some conflicts now, I will resolve them later. It also is somehow not picking up the changes that were just merged. I will take a look at all of those later. |
# Conflicts: # android/build.gradle # example/src/testing/TestList.ts # src/keys.ts
|
Ready for review @Szymon20000 @mrousavy |
|
Will try to review over the weekend. Thanks! |
|
|
||
| void PKEY_SPKI_Export( | ||
| KeyObjectData* key_data, | ||
| ByteSource* out) { |
There was a problem hiding this comment.
I'm curious... is this pattern of a void function and passing in the return value as an argument the one we should use?
Or should the function return the value?
I'm asking, because I am attempting to use std::variant in another PR, and am having to deal with these ByteSource values in MGLWebCrypto.cpp
There was a problem hiding this comment.
This is c style of pointer passing
There was a problem hiding this comment.
right, but is there any performance advantage? Should we use the C++ return values style? Should we be consistent?
There was a problem hiding this comment.
It's just the code from Node. Don't deviate too much as sometimes multiple params need to be returned and pointer passing is the easiest way. Otherwise, you will end up creating a bunch of unnecessary structs. The node code also serves as a reference for a proper implementation.
|
|
||
| // const generateKeyPair = promisify(_generateKeyPair); | ||
|
|
||
| // function verifyAcceptableEcKeyUse(name, isPublic, usages) { |
There was a problem hiding this comment.
Do we need all of those comments?
There was a problem hiding this comment.
I'm slowly uncommenting some of them (most🤞)
There was a problem hiding this comment.
It's just the remaining source code for the rest of the web crypto implementation. It will come back as the rest of the API is filled.
|
Looks good to me! However, it's not clear what is implemented in webcrypto and what is not. We probably should throw some errors if something is not implemented. Currently from what I understand we just commented out unimplemented methods? |
Unimplemented methods will not be there on the typescript types, error should be thrown for the current import/export code when trying to select a non-supported algo I believe. |
|
Ping @Szymon20000 |
|
@ospfranco merged :) |
|
Nice! @mrousavy let me know when you release it :) |
|
Sure, thanks! We got some iOS build error to figure out first and we'll probably also pull in the other PR, then we'll release it :) |
This is the first step towards implementing webcrypto (actually all the other encryption algorithms). Instead of following the same route we took when we first implemented the other Node APIs, I have massively simplified the JS side of things, relying on TypeScript instead of raw validation and callbacks.
It also unlocks a lot of the necessary infrastructure to continue implementing the rest of
webcryptoor even maybe swapping the old implementations for a more orderly one (we just need to copy the individual implementations e.g. crypto_ec.cc).I added a test just for the use case a client needed: import raw ECDSA key and exporting it as SPKI, but getting the rest done should now be peanuts (at least for import/export).
I've also solved some minor issues on the UI of the test app.
This branch work is based on #199, so it would be good to merge that one first.