crypto: add a crypto.uuid() method

[tlhunter]

This PR introduces a new method called crypto.uuid(). This method generates a UUID v4 string. It can be used like so:

const crypto = require('crypto');
const uuid = crypto.uuid();
console.log(uuid); // f933d282-560c-4464-aff2-56bf65c0a431

Why introduce this into Node.js Core

The Node.js community has a great need for this method. For example, the npm package uuid receives 11.4mm downloads per week, and the deprecated node-uuid still receives 1.6mm downloads per week.

This same functionality is often provided by other languages/platforms such as the C#/.NET implementation System.Guid.NewGuid() or Pythons import uuid.

Anecdotally, most projects I build require a UUID generator.

What's a UUID

A UUID string is a random series of 32 hexadecimal characters separated by hyphens at well-known locations for a total length of 36 characters. The 13th character is always a '4', so that leaves 31 hexadecimal characters of entropy. According to Wikipedia that means the overall string has 2^122, or 5.3x1036 (5.3 undecillion) possible variations.

Why UUID v4?

This is the most popular method as far as I've seen used in the wild. It's also the easiest to generate since it's just random characters. The other versions take current time into consideration and may generate ever-increasing values. Some attempt to maintain uniqueness across different systems which also adds complexity.

This approach would be forward compatible if we did want to support different UUID versions in the future. For example .uuid could have further methods attached, such as .uuid.v1() or even .uuid.v4() which would be an alias for .uuid().

Checklist

I haven't added documentation yet since this method may move around before being accepted (if it's accepted at all). I'll definitely write docs after the first round of feedback!

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[addaleax]

I'm not sold on this location for the method. A better location might be crypto.uuid() due to its dependency on crypto internals. It doesn't feel cryptographic in nature FWIW.

It’s going to break if Node.js is compiled without openssl support… That’s one of the reasons all of these functions tend to end up in crypto – it’s an all-or-nothing module.

For example, the npm package uuid receives 11.4mm downloads per week, and the deprecated node-uuid still receives 1.6mm downloads per week.

I’m not against this PR in any way, but this might be a good sign that this works really well as an npm package :) A lot of us feel that things that can be implemented easily in userland should be. On the other hand, this does not add much complexity, so I’d personally be fine with it.

[addaleax]

CI: https://ci.nodejs.org/job/node-test-pull-request/17750/

[tlhunter]

@addaleax is it possible to detect if openssl support is present? If so, I could still use internal/crypto, if not, I could generate something with Math.random().

I can just move it to crypto entirely, though a UUID generator would probably still be useful to users without openssl.

[addaleax]

@tlhunter Checking using if (process.versions.openssl) should work :)

I don’t know whether this generator needs any specific cryptographic properties, but if there’s a Math.random() fallback, that should probably be documented clearly.

[tlhunter]

@addaleax in this commit ec68ac2b346e71a8566f1fff78d96975213d64fd I've added a Math.random() based fallback. Unfortunately I don't know how to test it. Is there a sneaky test suite way to override the values within process.versions?

[apapirovski]

It doesn't feel like the 'why' answer above really answers the question... I mean, yes, uuid is a common thing and yes, those packages are popular but that in itself isn't a good reason. This works just as well as a user land package. I don't see a particularly strong case for why things can't stay as they already are.

[addaleax]

@tlhunter delete process.versions.openssl; should work, but I guess if that happens in a test that’s already too late… there’s always the option of running an extra OpenSSL-less CI whenever we touch this code?

[tlhunter]

@addaleax oh yeah good point, lib/util.js would have already run the conditional by then.

I do think that running an OpenSSL-less CI would make sense for Node.js in general since there are some conditionals in the codebase around openssl support.

[tlhunter]

@apapirovski I think this UUID PR is in a similar vein as the recently merged mkdirp PR: #21875. In that PR @bcoe mentions the following when referring to the mkdirp module (which gets a little over 8mm weekly downloads):

... some modules are so prolific, and the behavior at this point in Node's history so standardized, that the functionality should probably simply be part of the core API.

[thefourtheye]

This would require corresponding doc changes as well.

[mscdex]

-1 I think this is best left to userland as there are many different expectations about UUIDs in general and in behavior (e.g. should it fall back to Math.random()? should it allow returning a different type/format of value? should it include shelling out to an openssl child process as a fallback?) and providing and promoting this one particular implementation/interpretation in core is not a good idea.

[sam-github]

Falling back to Math.random() is not likely a good idea, UUIDs are often used for security purposes, they need cryptographic PRNG.

I'm -1, the uuid modules on npm do this well, and there are a number of UUID formats (time-based, ethernet card based, etc), good UUID support would be more than this one function.

[tlhunter]

@sam-github I agree that falling back to Math.random() isn't ideal. I've moved the code to crypto which makes the fallback moot. This way, in the rare situation where someone compiles without openssl support, they would expect the method to be missing and could fall back to userland.

[tniessen]

I'm not a fan of having this in core since

• it can easily be implemented in userland and is almost trivial,
• as it currently is, it can even block the event loop since randomBytes is not guaranteed to return immediately (even though this is highly unlikely), and
• UUID v4 is commonly used, yes, but there are dozens of variants and not a definitive standard.

[bcoe]

I'm not 100% 👍 or 👎 (comes to mind that this would sometimes be useful in Node.js itself, for instance when generating random file names for coverage output).

a few thoughts:

1. I don't think the algorithm quite matches rfc4122; one of the bits needs to be "8", "9", "A" or "B" I gather?
2. It might be worth looping in @broofa and getting there opinion about including uuid functionality in core.
3. We just had a first meeting of @nodejs/tooling at Node+JS Interactive \o/ One of the action items was to work with the community to figure out potential additional API additions for folks writing tooling, and to develop a roadmap we could share. Would love your involvement: docs: minutes for node+js interactive meeting tooling#6

[mcollina]

I'm kind of -1 to add this. What are the benefits of adding this directly into core?
There has been quite a few reasons why we added recursive mkdir, but I'm failing to see the actual benefits here.

What's @nodejs/tooling point of view?

[jasnell]

I'm also -1 on this one for all the same reasons stated above.

[broofa]

Hey gang, thanks for looping me in. I'll refrain from expressing a 👍 or 👎 on including this in core as I'm not familiar with the criteria that apply to that decision. I do have a few comments though ...

First, do not use Math.random(). Just don't even go near it. Its implementation is unspecified in the ES spec and, thus, it is not a reliable source of random #'s. That the uuid module falls back on it has been problematic, leading to issues like this. Support for it as a fallback is going away in the next version.

Edit: 'Realized that the above reaction is a result of dealing with all the various browser Math.random() implementations out there. In the context of this PR, though, we need only worry about Node's implementation. But that comes from v8, right? Thus, this is likely still concern. From https://v8.dev/blog/math-random:

even though xorshift128+ is a huge improvement over MWC1616, it still is not cryptographically secure.

Regarding:

The 13th character is always a '4',

... except when it's 1 or 3 or 5. Also, @bcoe is correct about needing a variant field (the 8,9,A, or B char) in addition to the version field for this to be RFC4122 compliant.

For example .uuid could have further methods attached, such as .uuid.v1() or even .uuid.v4() which would be an alias for .uuid().

As soon as you support v4 uuids, you'll have people clammoring for the other UUID versions. Count on it. Specifically, they'll want v1, v5, and v3 support, in that order. Also, since that's basically a given, I would advise not having uuid be a function. Keep the API simple and explicit. uuid.v4() to start with, adding other versions as necessary.

this does not add much complexity, so I’d personally be fine with it.

This may add more complexity than you think. Specifically, the inevitable demand for other UUID versions may introduce dependencies on things like the host's MAC address (for v1 uuids), and MD5 and SHA1 hashes (v3 and v5 uuids), respectively.

One final comment: it'd be nice if whatever uuid API ends up in core were as cross-compatible as possible with the existing uuid module. E.g. the module methods take arguments of various sorts.
I'm not sure the core API can/should be a 100% drop-in replacement, but hopefully this implementation will at least take cues from what is currently in place if/when there is a need for such features (e.g. passing an options object that contains explicit values for the various fields).

[broofa]

I think this UUID PR is in a similar vein as the recently merged mkdirp PR

I see UUID support as more of a slippery slope. mkdirp is just one option on a pre-existing method, and that's really all there is to it. UUID v4 support is just the lowest-hanging fruit in the UUID problem space. Supporting other versions - which I see as more or less inevitable - will lead to other methods being added, and along with those, you have the additional dependencies I mentioned previously.

[jasnell]

Given the -1's and the general sentiment expressed in the thread, I'm inclined to close this. @tlhunter ... thank you for the work here on this, even if it didn't land, the contribution and discussion are still helpful.

[boneskull]

agree w @jasnell @broofa etc. @tlhunter happy to chat in the tooling group or wherever about the differences between UUID and mkdirp if you wish

[tlhunter]

Thanks for the detailed feedback, everyone.