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

The crypto module #339

Closed
drifkin opened this issue Jul 11, 2018 · 15 comments
Closed

The crypto module #339

drifkin opened this issue Jul 11, 2018 · 15 comments

Comments

@drifkin
Copy link
Contributor

drifkin commented Jul 11, 2018

This is maybe a little premature, since nodejs/node#21766 is still being discussed, but I think as a group we should discuss:

  1. What are the goals of the crypto module? Do we imagine that it's only used by people who know what they're doing (i.e., cryptographers)? Should we strive to expose all OpenSSL functionality? Should the interface we expose match as closely as possible with OpenSSL?
  2. Supposing crypto is in fact intended to only be used by the very small percentage of people using Node that are very comfortable thinking about cryptography, should we create another crypto module that's intended for the average Node developer to use (say crypto-simple)? If so, we would need to propose how to build it (perhaps getting cryptographers involved to build it on top of the existing crypto module, or using a more modern crypto library with an interface that is likely to be used correctly by average developers).
  3. Should this group create an additional process to review changes to APIs that are security related? It would've been much better to have the discussion in Password Hashing API node#21766 before crypto: add scrypt() and scryptSync() methods node#20816 was merged. This need may be mitigated by whatever comes from discussing points 1 and 2, but it is still something to consider.

edit: Based on the results of our discussions, we may want to propose significant changes to documentation to make sure that our users' expectations about the use of crypto match ours

@tniessen
Copy link
Member

create another crypto module that's intended for the average Node developer to use (say crypto-simple)

While the built-in crypto interface should be as easy to use as possible, this is something I have thought about a lot in the past. Simply put, a user-friendly wrapper around the crypto module that simplifies design decisions, e.g. which cipher and parameters to choose. That's something I don't see as part of Node.js core, but something that could coexist.

@drifkin
Copy link
Contributor Author

drifkin commented Jul 11, 2018

Thanks for the response @tniessen. When you say

the built-in crypto interface should be as easy to use as possible

how are you thinking about evaluating whether a particular change makes it "easy to use", and who's the target audience? It seems to me that the existing API design goal is to make it look as much like OpenSSL as possible, which I guess would mean easy to use for people that are very familiar with OpenSSL. Or are you thinking something else?

I wonder if we can make expectations clearer by deprecating the name crypto and changing it to openssl in the future. In my opinion, I think very safe, high-level crypto APIs should be a part of Node .js core because I believe on average it will make Node applications much more secure, but I do realize that there is a lot of debate about what belongs in Node core vs. userland.

@deian
Copy link
Member

deian commented Jul 12, 2018

Simply put, a user-friendly wrapper around the crypto module that simplifies design decisions, e.g. which cipher and parameters to choose. That's something I don't see as part of Node.js core, but something that could coexist.

Out of curiosity, why not? (We have a user-friendly wrapper around the net module that simplifies implementing HTTP clients and servers.)

@tniessen
Copy link
Member

how are you thinking about evaluating whether a particular change makes it "easy to use", and who's the target audience?

Optimally, people with more advanced crypto knowledge should have all the options and possibilities they want, while less experienced users should have an easy-to-use interface.

It seems to me that the existing API design goal is to make it look as much like OpenSSL as possible

I am probably one of the most active people working on crypto and this has never been one of my design goals, even though our implementation is obviously restricted by OpenSSL's design and thus cannot completely deviate from it. I personally don't like being bound to a vendor, that's one of the reasons why I did not proceed with RSA key generation in Node.js, I don't like an API that is tailored to its implementation (as suggested in nodejs/node#15116 (comment)). Whether Node.js uses OpenSSL, OpenSSL with FIPS, LibreSSL or any other library should not affect users of the crypto module.

I wonder if we can make expectations clearer by deprecating the name crypto and changing it to openssl in the future.

It makes sense that the crypto module resembles a subset of the features of OpenSSL: Implementing that is the hard part, using the crypto module is much simpler than writing native code that hooks into OpenSSL, and that's the primary reason this module exists. I still don't think users should have to care about the underlying implementation, so the name crypto seems appropriate.

Simply put, a user-friendly wrapper around the crypto module that simplifies design decisions, e.g. which cipher and parameters to choose. That's something I don't see as part of Node.js core, but something that could coexist.

Out of curiosity, why not? (We have a user-friendly wrapper around the net module that simplifies implementing HTTP clients and servers.)

I don't say I wouldn't want it in core, I am just saying that I don't see it getting into core at this point. Some reasons off the top of my head:

  • People tend to like a small core. I won't repeat all the points here, this topic has been discussed numerous times whenever someone suggested adding a feature to core.
  • Such a high-level API would most likely deviate a lot from established standards. The Node.js (or OpenSSL, if you think they resemble each other) crypto API is not too different from many other popular crypto APIs, and I believe that's a good thing.
  • There are a lot of things I'd like to implement and/or change about the crypto module, but @bnoordhuis and I are probably the only people who frequently contribute to that module, and those contributions tend to receive very little attention.

@drifkin
Copy link
Contributor Author

drifkin commented Jul 12, 2018

Got it, I appreciate the clarifications.

I think we can start by being a little idealistic and first explore what an ideal high-level API might look like in core, and then investigate the feasibility and try to analyze the cost/benefit with respect to the whole small core problem. Based on the response from the other issue, I think we may be able to get more volunteers to work on a higher-level API (we also have a few cryptographer friends that might be interested in helping out too; API design seems like an increasingly relevant topic in cryptography these days and this could have a huge impact).

@joepie91
Copy link

@tniessen: Simply put, a user-friendly wrapper around the crypto module that simplifies design decisions, e.g. which cipher and parameters to choose. That's something I don't see as part of Node.js core, but something that could coexist.

I have to strongly disagree there. As I already explained in the other thread, many developers assign particular value and importance to the core libraries, to the point of actively choosing worse or less-safe implementations in the core libraries over better external options. If there's one place where we can't afford this, it's in cryptography.

Yes, I understand that the intention of Node.js core is to remain small, and I fully agree with that intention; however, it should remain an intention and not something to preserve at all costs. When looking at how developers actually view core, providing a safe-by-default cryptography API would significantly improve ecosystem security, and so it seems reasonable to me to still add this to core.

(The main reason I'm treating 'sub-optimal developer decisions around security' specially compared to 'regular sub-optimal developer decisions', is that security failures are generally silent; the developer will have no feedback that their implementation is wrong until they get compromised. For most non-security sub-optimal developer decisions, the developer will usually discover the awkwardness/bugs by themselves.)


As a general policy note (since I'm not sure where else to file this), over the past few years I've gotten the impression that two significant things are missing from the core development process:

  1. Security review of the APIs that are being designed, by somebody who has experience in secure API design. For example, the crypto.scrypt PR seems to have just been rubberstamped through in that aspect, with only the implementation being reviewed. Similarly, the new Buffer API would likely not have passed such API review either.

    Shouldn't there be a policy to do an 'API security' review of all potentially security-sensitive API changes? Has this been attempted in the past?

  2. Please don't take this the wrong way, but I get the idea that core developers generally have a poor idea of how Node.js is used in practice; how developers learn about it, how they write code, what walls they run into, what mistakes they make, where common issues come from, and so on.

    This leads to problems when, for example, measures like "just add a warning to the documentation" are suggested, because that only works on paper; in practice, developers do not read those warnings because they don't learn about it from the official documentation, and third-party tutorials don't copy this documentation.

    This is not something that is unique to this issue; I've seen the general approach to be "run analytics on the ecosystem to see what is being used where", but that only ever tells you half the story.

    Is there any ongoing effort to bridge this apparent gap between core developers and 'end user developers'?

I feel like it may be worth looking at how this is handled in the Rust community, as core developers there tend to have a good handle on how core is being used, and on how to design APIs ergonomically such that they are safe to use.

@tniessen
Copy link
Member

Simply put, a user-friendly wrapper around the crypto module that simplifies design decisions, e.g. which cipher and parameters to choose. That's something I don't see as part of Node.js core, but something that could coexist.

I have to strongly disagree there.

@joepie91 Let me cite my previous comment:

I don't say I wouldn't want it in core, I am just saying that I don't see it getting into core at this point. Some reasons off the top of my head:

So yeah, personally, I am not generally opposed to having a "safe-by-default" API, and I completely understand your reasoning.

Security review of the APIs that are being designed, by somebody who has experience in secure API design.

This might be something for the security WG to discuss, but Node.js is still OSS and as such depends on the work of volunteers. If someone gives feedback on a change based on their perception of its security, we will certainly pay attention to it.

Idea: Maybe we could make upcoming API changes more public, such that people such as yourself can provide feedback before it gets released? This is not the first time a newly introduced API received criticism, I have been in your situation as well, only that the API (luckily) had not been released at that point.

For example, the crypto.scrypt PR seems to have just been rubberstamped through in that aspect, with only the implementation being reviewed.

At this point I think it is fair to clarify that scrypt itself is secure and so is our implementation, and the API design matches the low-level nature of the crypto module. Only improper use diminishes the security, and the same applies to most if not all other crypto functions, to other Node.js APIs, to the Linux kernel, to web browsers, to cars and to knives. Our goal should be to hide the knife from children, not to remove the knife from the kitchen. (→ Separate high-level API)

Is there any ongoing effort to bridge this apparent gap between core developers and 'end user developers'?

Yes, there is the community committee, maybe @bnb would like to add to this.

@joepie91
Copy link

So yeah, personally, I am not generally opposed to having a "safe-by-default" API, and I completely understand your reasoning.

Ah, apologies, I misinterpreted your comment then :)

This might be something for the security WG to discuss, but Node.js is still OSS and as such depends on the work of volunteers. If someone gives feedback on a change based on their perception of its security, we will certainly pay attention to it.

Idea: Maybe we could make upcoming API changes more public, such that people such as yourself can provide feedback before it gets released? This is not the first time a newly introduced API received criticism, I have been in your situation as well, only that the API (luckily) had not been released at that point.

Yeah, I was thinking something along the same lines. I can't commit to a full-blown core team role for time constraint reasons, but I'd certainly be happy to review (especially security-critical) APIs, if I could be notified of their proposals somehow :)

I'm certainly not expecting core developers to do all the work, to be clear; just suggesting to have a more accessible and consistent process in place, to allow others (me included) to pitch in.

At this point I think it is fair to clarify that scrypt itself is secure and so is our implementation, and the API design matches the low-level nature of the crypto module. Only improper use diminishes the security, and the same applies to most if not all other crypto functions, to other Node.js APIs, to the Linux kernel, to web browsers, to cars and to knives. Our goal should be to hide the knife from children, not to remove the knife from the kitchen.

While I understand that view, I think it's a mistake to draw such a hard distinction between 'child-safe' and 'adult' tooling (or any similar analogy), because ergonomics matter at every level. Several of the things you've mentioned make or made errors in this regard, so I don't think we should uphold Linux/cars/browsers/etc. as some sort of gold standard.

I do agree that there may be valid usecases for a more custom usage of scrypt specifically, but this should not translate into a "let's keep it as low-level as we can" approach. Even in an additional low-level API, only the actually needed switches should be present, and they should be made to look exactly as dangerous as they are.

Incidentally, I'm not aware of any justifiable cases, even outside of password hashing, to provide a custom salt. But perhaps somebody could correct me on that. (@paragonie-scott?)

Yes, there is the community committee, maybe @bnb would like to add to this.

Just looking at the member list there, I don't recall any of them being particularly active (or active at all) in developer help venues such as the IRC channel, subreddits, and so on. I feel like without such participation, it's not really possible to bridge this gap.

@mcollina
Copy link
Member

I do not think we should include easy-to-use crypto APIs in core. Maintaining a module in userland is extremely simpler, and it allows to have a stable API across multiple major versions of Node.js. I think core should just expose the primitives.

I think that there is a need for an official module hosted within the Node.js github organization that we recommend for easy to use crypto. We can support that across all our supported lines (currently 10, 8 and 6) and provide a solid base for applications and module author to rely on.

@tniessen
Copy link
Member

Right now, that would be my preference as well, @mcollina. I am currently discussing design goals of such an API with @joepie91.

@drifkin
Copy link
Contributor Author

drifkin commented Jul 12, 2018

If we don't have an easy-to-use crypto API in core, I think we need to rename scrypt to something to discourage non-crypto experts from using it. To quote myself (apologies!) from nodejs/node#21766:

Developers are commonly told to "just use scrypt", so by providing this API, I suspect this will lead to a lot of people unknowingly creating security vulnerabilities in their applications.

Given that that's overwhelmingly the common recommendation (that or bcrypt), I think an average developer would assume that require('crypto').scrypt is what crypto people are recommending they use (but from this thread many are saying that the scrypt most of the Node community should use should be provided by a third-party module rather than the function that appears "blessed" by being shipped with core – this seems way too nuanced to become widespread knowledge).

@ChALkeR
Copy link
Member

ChALkeR commented Jul 18, 2018

@drifkin @nodejs/security-wg, PTAL at nodejs/node#21766 (comment). What do you think? I based that proposal on input from here among other things.

@canterberry
Copy link

We may be jumping the gun here on calling the distinction here between "easy to use" and "fully featured". We're talking about what is currently a theoretical, yet-to-be-designed evolution of the crypto API. Let's not rule out the possibility that such a thing can be done without necessarily having to make that trade-off, and both can be chosen as design goals.

To illustrate some use cases I have run into over the years that have involved direct usage of cryptography APIs, and hopefully paint somewhat of a picture of the types of problems the community at large may be looking to use a native crypto module to help solve:

  • Implementing an end-to-end secure storage solution for local data on a mobile device, using PBKDF2 to derive an AES256 master key and using that to encrypt another AES256 storage key generated from a secure PRNG.
  • Interacting with the Ethereum network, using Keccak256 for hashes and EC key-pairs on curve secp256k1 for signing and verification, hex encoding/decoding.
  • Generating keys and certificates for TLS infrastructure, using EC key-pairs on curve secp384r1 and/or 4096-bit RSA keys. Generating key-pairs, creating certificate signing requests, signing certificates, adding a trusted CA certificate to TLS client (for server connections), adding a trusted CA certificate to a TLS server (for client connections), configuring X.509 key/certificate usage parameters, configuring X.509 subjectAlternativeNames.
  • Signing and verifying JWT tokens using either symmetric (HMAC-SHA256) or asymmetric (EC256) ciphers, for service-to-service authentication in a microservices architecture, and for end-user authentication in public-facing web services.
  • Deriving deterministic identifiers for objects based on the hash of attributes considered immutable; typically SHA256 (or, for globally unique deterministic identifiers, HMAC-SHA256 with an AES256 secret key). Used for persistence and partitioning in distributed key-value stores.
  • Procedurally generating a video game world map from a SHA256-based PRNG initialized with a specific seed.

Some general concepts I think are important for a crypto module:

  • Configurability: When integrating with another library, project, or standard (e.g: JWT, Ethereum, etc), there may be constraints on the accepted algorithms, EC curves, key sizes, etc. In these cases, it is important to be able to specify these cryptographic parameters.
  • Secure by default: This has been a common problem with integrating with cryptography APIs. As an example, several years ago, Android's SecureRandom had a default constructor which used a pre-defined static seed. Many developers just used the default constructor assuming it was safe to use, and exposed themselves to vulnerabilities resulting from having a predictable PRNG [reference]. Any parameters that can be generated automatically or set to reasonable+secure defaults, should do so. Examples include an HMAC's IV (use a buffer filled using a PRNG instead of one with all zeroes); a PRNG's seed (use a built-in entropy source instead of a static value); the iteration count for a PBKDF2 algorithm (derive or use a sufficiently high default value instead of just 1 iteration); etc.
  • Simple: Provide an intuitive API that focuses on solving for core cryptographic primitives and operations involving those primitives. By making these core pieces well-refined and well-defined, it becomes easier for a newcomer to cryptography to explore and learn.
  • Internally compatible: This is where the current crypto API really falls short; it creates output that it cannot, itself, consume; and it accepts input that it cannot, itself, produce. The ideal API would be able to consume its own output, and not rely on third-party libraries, tools, or services, to bridge these gaps.

There's no shortage of cryptography libraries/APIs across a variety of programming languages that we can learn from. Perhaps one step forward might be to find some others that are particularly well-designed in ways that align with this group's design goals, and look to these for inspiration.

@sam-github
Copy link
Contributor

I don't agree that the distinction is between "safe" and "unsafe". I think the distinction being drawn here would be better described as "purpose built" and "general purpose".

Openssl's APIs, and Node's crypto module, are general purpose crypto APIs. The advantage of this is that they can be used to implement protocols and formats that are defined by third parties.

The disadvantage is that when a developer controls both the producer and consumer, such as the password hashing example that kicked off this conversation, the developer is forced to figure out how to use the API for their specific purpose, and can easily do this wrong. It would be better for them to use a function that was specific to that purpose.

The problem with purpose built crypto APIs is that they have an internally specified format for their intermediate formats that makes them hard or impossible to use to interoperate with the crypto APIs from other libraries, languages, or standards.

Node's crypto library is clearly general purpose in design. It does not have the kind of APIs that NACL does, for example. The design of the current scrypt() function fits perfectly with this design, I don't agree that a password hash function would have landed in place of it, or that its design is flawed. I do agree it is not designed to be a high-level password hashing API, such as PHP has.

I appreciate the frustration of the scrypt-for-humans author that some people are avoiding his better (for some purposes) API because a "scrypt" function exists in node.

Node is having enough trouble getting sufficient developers to fix the existing problems (the "internally compatible" point made above is particularly painful to me). I'm not sure how we can extend to designing, implementing, and supporting an entirely new higher level/functional API.

My personal preference would be for such an API to be provided out of core as an npm module, and for the Node.js crypto API docs to point to it. If instead someone PRed a functional API into Node.js and got support for it, I wouldn't object.

I'm not familiar with https://www.w3.org/TR/WebCryptoAPI/. Node has a general trend to supporting Web standard APIs when it can. Would WebCrypto be an acceptable high-level API? If so, implementing it would be following a well-paved path into node core.

@sam-github
Copy link
Contributor

This discussion seems to have run its course. I can reopen if anyone feels it should remain, but while a closed issue can continue to be used to discuss things, if there aren't concrete actions we plan to take, I don't think it needs to remain open. If anyone wants it reopened, please ping me here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants