crypto: implement randomuuid

[jasnell]

/cc @bcoe

Signed-off-by: James M Snell jasnell@gmail.com

Checklist

• 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

[targos]

Is it being added to the WebCrypto spec? I can't find any reference to this method.

[jasnell]

@targos:

Is it being added to the WebCrypto spec?

This is based on a proposal by @bcoe being discussed here: https://discourse.wicg.io/t/proposal-rfc4122-v4-uuids/4986. The actual proposal as it currently stands is introduction of a new global function but there's also discussion about it being exposed via Web Crypto.

[ChALkeR]

data[6]/data[8] is incorrect, otherwise big +1 on this!

[jasnell]

Updated to resolve the comments. Also batched the random data generation to cover generating up to 64 uuid's before having to generate the random data again -- yields a significant performance bump (from ~200k ops/sec up to ~1.8 million / ops/sec) at a minor memory cost.

Fixed

[jasnell]

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

[panva]

Adding to the SubtleCrypto interface is imho premature. Otherwise it's a nice addition.

[ChALkeR]

@jasnell how about this:

const hexDigits = [
  48, 49, 50, 51, 52, 53, 54, 55,
  56, 57, 97, 98, 99, 100, 101, 102
];

// Implements an RFC 4122 version 4 random UUID.
// To improve performance, random data is generated in batches
// large enough to cover 64 UUID's at a time.
// The uuidData array and uuid buffer are reused for each.
// Each uuid consumes 16 bytes of random data from the array.
const uuidBatches = 64; // a buffer for this many uuids is prefilled
let uuidBatch = 0;
let uuidData;
let uuid;
function randomUUID() {
  if (uuidData === undefined) {
    uuidData = new Uint8Array(16 * uuidBatches);
    uuid = Buffer.alloc(36, '-');
  }

  if (uuidBatch === 0) getRandomValues(uuidData);
  const base = 16 * uuidBatch;
  uuidBatch = (uuidBatch + 1) % uuidBatches;

  // Version byte: 0100xxxx (version 4)
  uuidData[base + 6] = (uuidData[base + 6] & 0xf) | 0x40;

  // Variant byte: 10xxxxxx (variant 1)
  uuidData[base + 8] = (uuidData[base + 8] & 0x3f) | 0x80;

  let p = 0;
  for (let n = base; n < base + 16; n++) {
    uuid[p++] = hexDigits[(uuidData[n] & 0xf0) >> 4];
    uuid[p++] = hexDigits[(uuidData[n] & 0xf)];
    if (p === 8 || p === 13 || p === 18 || p === 23) p++;
  }

  return uuid.toString();
}

Changes:

• Split random data buffer from stringification buffer to improve clarity
• Fixed 63 -> 64 and simplified the logic (starts with 0, use %).
  I doubt there is any effect on performance, but that could be reverted if there is.
• No need to set '-' each time, can pre-fill those and just skip those bytes.
• No allocUnsafe()

[jasnell]

That works @ChALkeR and the performance is looking really good now also...

[jasnell]

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

(but it looks like CI is still broken ... @nodejs/build)

[jasnell]

@panva:

Adding to the SubtleCrypto interface is imho premature

Well, it's exposed via require('crypto').webcrypto (at the same level as getRandomBytes() but is not on SubtleCrypto. We can mark it experimental for now giving room for us to move it around as necessary later.

[panva]

We can mark it experimental for now giving room for us to move it around as necessary later.

But that benefits noone - if I can have it stable in 'crypto' why would i ever require it from webcrypto, knowing it can get renamed/removed in the future. Just have it stable in crypto and if/when picked up by w3c it can be added to webcrypto.

[ChALkeR]

+1 to what @panva said, can we expose that on require('crypto') for now, and not on require('crypto').webcrypto, until it gets into upstream spec?

Implementation LGTM.

[jasnell]

But that benefits noone - if I can have it stable in 'crypto' why would i ever require it from webcrypto, knowing it can get renamed/removed in the future. Just have it stable in crypto and if/when picked up by w3c it can be added to webcrypto.

Partially because the API proposal is still being discussed and could end up being quite a bit different in the end. I'd rather not mark it stable prematurely.

[panva]

@jasnell, fine, but still - we can have that in node's crypto only. Imho it doesn't make sense in webcrypto until it gets picked up by the standards body maintaining that namespace.