Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement WebCrypto interface #19826

Closed
YuryStrozhevsky opened this issue Apr 5, 2018 · 34 comments
Closed

Implement WebCrypto interface #19826

YuryStrozhevsky opened this issue Apr 5, 2018 · 34 comments
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@YuryStrozhevsky
Copy link

Yes, I did a search for WebCrypto and found a lot of closed issues.

Yes, I saw comments like

I do not see a reason why to implement it - user could get the same using existing crypto interface

I have more than 20 years of experience in software development, many working implemented products, expert in all crypto-related stuff but even for me making such same using existing crypto interface is VERY untrivial.

Yes, I also saw comments like this

User could generate RSA (EC) keys via spawn openssl <blah-blah>

Why you need the crypto module itself? Why not just saying spawn openssl instead?

But why not just unify API between browsers and Node? WebCrypto even in a standard stage now.
It is hard to implement and would get much time? Here is EXISTING code already implemented WebCrypto API in Node environment as a native plugin. It is tested, verified and confirmed to be aligned with WebCrypto API implementations in browser. All you need is to adopt EXISTING code as native Node module.

As you could see it is not a common issue - I have a real feeling that it is a time to discuss WebCrypto in Node again. I did not participate in prev discussions and would like to start a new one.

@mscdex
Copy link
Contributor

mscdex commented Apr 5, 2018

If anything, I would rather consider adding bits of missing functionality than adopt a new API in core. At least 90% of the WebCrypto functionality should already be covered by crypto, so it's trivial for a userland module to present a different API on top of that.

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js. labels Apr 5, 2018
@mscdex
Copy link
Contributor

mscdex commented Apr 5, 2018

I should also add that the fact that there is a userland addon that already accomplishes what you want is evidence that it doesn't need to live in core.

@YuryStrozhevsky
Copy link
Author

YuryStrozhevsky commented Apr 5, 2018

At least 90% of the WebCrypto functionality should already be covered by crypto

I am working on a kind of native WebCrypto shim replacing WebCrypto API by direct calls to Node crypto. And should say that even simple generate key + sign + verify chain implementation is not trivial:

  • For EC keys I need to call createECDH and then generateKeys (WTF, why not just make direct interface to EC keys generation?);
  • Then I need to fill crypto-specific structure ECPrivateKey and encode it into Buffer type. Without a side library making it for a simple user would be impossible - Node does not provide such fuctionality;
  • Then I need to encode BASE-64 from data from prev step and then append all nasty BEGIN/END EC PRIVATE KEY;
  • Only after that I can call sign (finally!);
  • Then I need to fill crypto-specific structure PublicKeyInfo and encode it in Buffer type (thanks that I have PKIjs as a library covering it);
  • Then I need to encode BASE-64 from data from prev step and then append all nasty BEGIN/END PUBLIC KEY;
  • And only after that I can pass the all the data to verify function;

As I said I have a ton of experience writing crypto soft plus I have a support from PKIjs library. For a simple user finish all the steps I described above would be nearly impossible. And I was describing just one possible way how user could use crypto lib.

I should also add that the fact that there is a userland addon that already accomplishes what you want (by your own words) is evidence that it doesn't need to live in core.

The node-webcrypto-ossl package is god, but not the best. For some reasons (especially for installation reason) I do not want to process with the package anymore. And do see the best choice to move all this functionality into native Node module.

@devsnek
Copy link
Member

devsnek commented Apr 5, 2018

as always I'm +1 on adding things to make browsers and node more similar, all we need is the implementation

@bnoordhuis
Copy link
Member

Not if it means duplicating a lot of existing functionality. Cryptographic code is subtle.

@YuryStrozhevsky If you want to see this happen, you should write up a concrete plan of attack.

@YuryStrozhevsky
Copy link
Author

@bnoordhuis A plan:

  1. Get node-webcrypto-ossl source code and port it to Node native module;
  2. Have a fun! (mandatory item);

@devsnek
Copy link
Member

devsnek commented Apr 5, 2018

porting that module to napi would be a pretty nice test of napi, make it very pluggable, and enforce node core's usage of napi helping make sure that napi is as good as possible

@YuryStrozhevsky
Copy link
Author

@bnoordhuis

Not if it means duplicating a lot of existing functionality

Probably there is the time when you need to start moving crypto to deprecated status.

@jasnell
Copy link
Member

jasnell commented Apr 5, 2018

One of the more significant factors in implementing the WebCrypto interface is the fact that most of the operations return Promise. Our crypto methods are almost exclusively synchronous at this point and testing has shown little to no performance benefit switching to an async model. In fact, in most cases, the additional async context makes the operations quite a bit slower. Ultimately that means that implementing a Promise version will mean wrapping the existing functionality which is something that can be done in userland just as easily as it can be done in core.

That's not to say that we shouldn't do this at some point, there's just not a strong motivation or justification to do so yet.

@YuryStrozhevsky
Copy link
Author

YuryStrozhevsky commented Apr 5, 2018

@jasnell

In fact, in most cases, the additional async context makes the operations quite a bit slower

What a problem - make it as set of sync functions (not strictly follow WebCrypto standard). The most biggest problem with existing crypto is that existing interface is complicated and in most my own cases was very hard to use in real applications. Frankly speaking I do not like (in soft words) existing crypto.

@bnoordhuis
Copy link
Member

Get node-webcrypto-ossl source code and port it to Node native module
Probably there is the time when you need to start moving crypto to deprecated status
not strictly follow WebCrypto standard

Okay, I'm convinced by now you're not taking this seriously - or if you are, that you didn't put nearly enough thought in it. Start studying the existing crypto code and come back when you have a plan forward.

@YuryStrozhevsky
Copy link
Author

YuryStrozhevsky commented Apr 5, 2018

@bnoordhuis Ben, why you always try to close "WebCrypto-related" issues so fast? What you are scary of? What "plan" did you mean? You have already written code, the only problem it is a Node plugin, not a native module. Or you are suggesting me to write a full-featured PR with the new code? I am really confused with the style you drive Node issues.

@bnoordhuis
Copy link
Member

You have already written code

Remember I said 'duplicating a lot of existing functionality'? Your suggestion of vendoring node-webcrypto-ossl does exactly that. Instead of addressing that you handwave it away.

What "plan" did you mean?

If you want to see webcrypto in core happen, you'll need to detail how you plan to implement it. Talk about deviating from the spec or deprecating the existing crypto module because you "do not like it" doesn't help your credibility.

@YuryStrozhevsky
Copy link
Author

I had no illusions when writing this issue - all of them were closed. But I had a hope on a constructive conversation. At almost I had no such a thing.

duplicating a lot of existing functionality

All crypto interfaces do almost the same. The difference between them that one is perfect and other sucks.

you'll need to detail how you plan to implement it

I do not have a time for it, sorry.

Wish in next reincarnation of iojs we would have WebCrypto API. Finally.

@bnoordhuis
Copy link
Member

I'm genuinely curious how you were hoping to have a "constructive conversation". You reopen an old discussion but at no point do you formulate a realistic proposal and - by your own admission - you weren't going to either.

Were you hoping someone would say "good idea!" and run with it? OSS very rarely works like that.

@jasnell
Copy link
Member

jasnell commented Apr 5, 2018

At this point the only constructive way forward would be for someone that wants WebCrypto in core to put together either a concrete plan that can be reasonably discussed or open a PR with code that can be reviewed. Comments about how someone should maybe consider doing this at some point are not overly helpful and further discussion about how it's being discussed are even less helpful. I would encourage anyone who wants to see this move forward in a constructive way to bring a constructive plan to the table.

@bnoordhuis
Copy link
Member

I'm still interested in @YuryStrozhevsky's thoughts. I have hope he returns in the future and then it will help to have the right expectations.

@YuryStrozhevsky
Copy link
Author

YuryStrozhevsky commented Apr 5, 2018

As I said I made a check for almost all WebCrypto-related issues here. And in everyone I saw the same - few words like we would not duplicate existing functionality, few words like absent functions you could get from spawning OpenSSL, few words like using Promises is not so interesting.

So, when I open the issue I did hope for at least two things:

  • You missed already existing code of node-webcrypto-ossl before
  • You would provide something new against WebCrypto API

Nothing I got - all the same.

As for open a PR with code that can be reviewed - I am a professional developer and my time is my money. I have provided you a link to existing code, described you a concrete situation when working with existing crypto interface is nearly impossible. I described just one situation I had and can provide more. But no, seems no one cares. Anyway, seems I missed the way how Node team works if you want it - go and get it. Okay, now I got it.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 5, 2018

@YuryStrozhevsky

I am a professional developer and my time is my money.

If the "Node team" that you referred to means the Node.js collaborators, then the truth is most of them are also professional developers whose time is also money (and some of them are even students whose time is priceless). And most of them don't get paid anything by doing any work in any projects under the Node.js organization. A very small portion of them may do, but definitely not enough if you look at the number of open issues and PRs in the organization. Therefore, if you truly care about something, then the way Node.js works, like almost any other open source project, is that you'll have to contribute more work than a list instead of expecting free labour, or expect enough people cares and someone who has time can jump in to do the work. Looking at the download stats of node-webcrypto-ossl, the chance of the latter is probably quite thin.

@tniessen
Copy link
Member

tniessen commented Apr 5, 2018

But why not just unify API between browsers and Node? WebCrypto even in a standard stage now.

@YuryStrozhevsky I am honestly curious without wanting to judge your proposal: Why do you think the WebCrypto API is the best choice for node.js, to the extent that you would suggest to replace the whole crypto module with it? The WebCrypto API was designed specifically for browsers and their specific security requirements.

Node.js and browsers happen to both use JavaScript, and sharing similar APIs on web clients and servers can be beneficial in some situations, but that is not always the case. There have always been browser-side APIs which node does not support and vice-versa, often because there are better alternatives on either side, and in this case, there are non-trivial downsides to adopting the browser WebCrypto API.

@YuryStrozhevsky
Copy link
Author

YuryStrozhevsky commented Apr 6, 2018

most of them are also professional developers

Probably yes. The only thing I said I do not have a time for it, sorry.

if you truly care about something

I do not care. I am not a Node developer - I am a "consumer". And from my real point of view existing crypto interface is ugly. And in this issue I was saying about the problem and also proposed a solution. But seems no one cares about the problem and no one want to solve a problem. All goes to discussion about how hard would be to integrate WebCrypto and to conclusion that nothing need to be done.

Why do you think the WebCrypto API is the best choice for node.js

I do not think so. It was just a proposed solution. There are number of benefits of existing crypto:

  • Streaming data (update functionality). Inside WebCrypto we should update all data at once;
  • Support for legacy algorithms. WebCrypro strict algorithm's set to a very limited number, and sometimes it is not so good;

Anyway, I did expect WebCrypto realization inside Node at around 2015. Just because your "roots" are inside Chromium and this project has WebCrypto implemented since beginning of 2014. But no way, even at 2018 - I can see growing number of crypto-related packages for Node which fixes different aspects of existing crypto and no progress in internal Node development.

OK, this is my final comment at this thread. I did hope for a conversation but all I had is a lecture how to process with requested Node's features. Anyway, thank you for the lecture guys!

@benjamingr
Copy link
Member

I don't see the benefit over using a polyfill at this stage to be honest

@MichaelJCole
Copy link

MichaelJCole commented Jun 28, 2018

@bnoordhuis @YuryStrozhevsky

LOL, just found this closed issue after Ben closed #21580

Please note the OP's repo hasn't been updated in months and says: "At this time this solution should be considered suitable for research and experimentation, further code and security review is needed before utilization in a production application."

This is a lack of responsibility to the developer community. Tickets are $750

@devsnek
Copy link
Member

devsnek commented Jun 28, 2018

blink has a very generic implementation of WebCrypto as a sort of internal c++ library which could be wrapped quite easily by node. that would also have the advantage of being backed by a very well written and tested implementation which is also actively maintained. the interface comes from https://cs.chromium.org/chromium/src/components/webcrypto/ and some usage of it is here: https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/crypto/subtle_crypto.cc. notice that all they're doing is translating js to c++ and then wrapping it with promises.

@bnoordhuis does that fulfill your needs for security and maintainability? (and your wanting of a plan?) i'd be happy to help out with wrapping this for node but i'm not a security expert by any stretch.

@bnoordhuis
Copy link
Member

@devsnek #2833 (comment). The links you posted need, at a conservative estimate, at least 20,000 lines of supporting code.

@Trott
Copy link
Member

Trott commented Jun 28, 2018

Tickets are $750

At the risk of feeding the (seeming to me, anyway) trolling by responding: That event is not a Foundation or project event. It is an independent entity. Not really sure what your point would be even if it was, TBH.

@YuryStrozhevsky
Copy link
Author

YuryStrozhevsky commented Jun 29, 2018

@MichaelJCole The node-webcrypto-ossl successfully using in real production applications, internal to PeculiarVentures. Probably in a set of external projects - have no real information about it. It is tested by PKIjs tests and passed all the possible test cases. I think at the moment it is the best existing implementation of WebCrypto API for Node. Also there is other project - webcrypto-liner but it is for browser only. So, the proposed template for WebCrypto API development for Node (node-webcrypto-ossl) is a really mature and tested product. The remark you mentioned is from years before now :) I will talk with node-webcrypto-ossl developers and probably they would change it soon.

@MichaelJCole
Copy link

@YuryStrozhevsky That's good to know, thanks!

The remark you mentioned is from years before now :)

Yury, that's open source and I appreciate your work on it, thank you!
For something as 🎉 and mature as Nodejs I'd think there'd be more project responsibility for standards and security than issue-swatting.

@devsnek Wow, yes that code and plan seems worth looking into.

@bnoordhuis that's an estimate, but you seem uncomfortable and dug-in for years on the topic, so maybe there's a simpler way you're not seeing. Maybe JS security standards are important enough for a mainstream JS project to code for. It seems like an open issue on the project to me.

@hackygolucky
Copy link
Contributor

@MichaelJCole If you would like to contribute to the various groups within the Node.js project who are working on standards and/or security, we can point them to you. It is not helpful nor encouraging to folks trying to contribute to these repositories to make assumptions about design decisions that the project may or may not have made and that you may not have had the time to surface for context. You can assume, however, that folks are acting in good faith to work on making Node.js better, and they are happy to provide context if you have questions that reflect that level of interest.

@MichaelJCole
Copy link

@hackygolucky Reading your comments alongside @bnoordhuis from #21580:

If you don't have anything relevant to say, stay quiet.

and

Now stop trying to read the motivations of someone you don't know. You suck at it.

Taken together, I experience your commends as cobwebbing to duck the issue and not take responsibility. I'll pass as well.

@jasnell
Copy link
Member

jasnell commented Jun 29, 2018

At this point, the best approach forward for those who feel that Node.js should provide an implementation of WebCrypto would be to present a workable implementation plan, preferably along with a working proof of concept, that can be evaluated more concretely. That plan would need to take into consideration backwards compatibility (that is, the existing API won't be going anywhere any time soon), performance (it shouldn't be significantly worse than what we have now), feasibility as a userland module (to land in core, there needs to be a solid enough line of reasoning as to why a userland module wouldn't be better), and maintainability.

@oliverjanik

This comment has been minimized.

@tniessen

This comment has been minimized.

@nodejs nodejs locked as too heated and limited conversation to collaborators Jul 1, 2018
@benjamingr
Copy link
Member

Unfortunately this conversation has become too heated. We might unlock it after some time has
passed.

Note that Node.js is committed to a safe collaborating environment. Attacks will not be tolerated.

I encourage anyone who has criticism regarding how we handled this issue to reach out to me personally at benjamingr@gmail.com or to anyone else from the Node.js moderation team (members listed here). We also have a user feedback group https://github.com/nodejs/user-feedback to discuss specific broader community feedback.

The foundation conducts user surveys and user usage analytics. If you believe those misrepresent what our users want or need or believe we could be doing a better job or collecting it to contact us about it. Constructive feedback is welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests