crypto: add crypto.createMac()

[bnoordhuis]

Per #32448 (comment).

cc @mscdex @tniessen - this doesn't implement the one-shot API yet but, on the flip side, it supports CMACs.

[tniessen]

Maybe I am missing something, why does this return an empty buffer / string?

[bnoordhuis]

That's what the other classes in this file do. @jasnell added that in c75f87c but it's not clear to me why (and the commit log doesn't mention it.)

[jasnell]

No, I did not add that, it was the behavior that was implemented before... Hmac.prototype.digest returns either an empty Buffer or string when called multiple times, whereas Hash.prototype.digest throws on multiple calls. I did not add that behavior and found it surprising when I did the refactor but it was not the time to change it. I will note, however, that the implemented behavior (that's been there since at least the Node.js 0.12 days) does not match what is documented -- which states that an error would be thrown if digest is called more than once.

C:\Users\jasne>nvs use 0.12
PATH += %LOCALAPPDATA%\nvs\node\0.12.10\x64

C:\Users\jasne>node
> var h = crypto.createHmac('sha256', 'secret')
undefined
> h.update('foo')
{ _handle: {}, _options: undefined }
> h.digest('buffer')
<Buffer 77 3b a4 46 93 c7 55 3d 6e e2 0f 61 ea 5d 27 57 a9 a4 f4 a4 4d 28 41 ae 4e 95 b5 2e 4c d6 2d b4>
> h.digest('buffer')
<Buffer >
> h.digest('utf8')
''
> .exit

C:\Users\jasne>nvs use 13
PATH -= %LOCALAPPDATA%\nvs\node\0.12.10\x64
PATH += %LOCALAPPDATA%\nvs\node\13.11.0\x64

C:\Users\jasne>node
Welcome to Node.js v13.11.0.
Type ".help" for more information.
> var h = crypto.createHmac('sha256', 'secret')
undefined
> h.update('foo')
Hmac {
  _options: undefined,
  [Symbol(kHandle)]: {},
  [Symbol(kState)]: { [Symbol(kFinalized)]: false }
}
> h.digest('buffer')
<Buffer 77 3b a4 46 93 c7 55 3d 6e e2 0f 61 ea 5d 27 57 a9 a4 f4 a4 4d 28 41 ae 4e 95 b5 2e 4c d6 2d b4>
> h.digest('utf8')
''
>

vs..

C:\Users\jasne>nvs use 0.12
PATH -= %LOCALAPPDATA%\nvs\node\13.11.0\x64
PATH += %LOCALAPPDATA%\nvs\node\0.12.10\x64

C:\Users\jasne>node
> var h = crypto.createHash('md5')
undefined
> h.update('foo')
{ _handle: {}, _options: undefined }
> h.digest()
<Buffer ac bd 18 db 4c c2 f8 5c ed ef 65 4f cc c4 a4 d8>
> h.digest()
Error: Not initialized
    at Error (native)
    at Hash.digest (crypto.js:126:23)
    at repl:1:3
    at REPLServer.defaultEval (repl.js:132:27)
    at bound (domain.js:284:14)
    at REPLServer.runBound [as eval] (domain.js:297:12)
    at REPLServer.<anonymous> (repl.js:279:12)
    at REPLServer.emit (events.js:107:17)
    at REPLServer.Interface._onLine (readline.js:214:10)
    at REPLServer.Interface._line (readline.js:553:8)
> .exit

C:\Users\jasne>nvs use 13
PATH -= %LOCALAPPDATA%\nvs\node\0.12.10\x64
PATH += %LOCALAPPDATA%\nvs\node\13.11.0\x64

C:\Users\jasne>node
Welcome to Node.js v13.11.0.
Type ".help" for more information.
> var h = crypto.createHash('md5')
undefined
> h.update('foo')
Hash {
  _options: undefined,
  [Symbol(kHandle)]: {},
  [Symbol(kState)]: { [Symbol(kFinalized)]: false }
}
> h.digest()
<Buffer ac bd 18 db 4c c2 f8 5c ed ef 65 4f cc c4 a4 d8>
> h.digest()
Uncaught Error [ERR_CRYPTO_HASH_FINALIZED]: Digest already called
    at Hash.digest (internal/crypto/hash.js:97:11)
    at repl:1:3
    at Script.runInThisContext (vm.js:131:20)
    at REPLServer.defaultEval (repl.js:432:29)
    at bound (domain.js:429:14)
    at REPLServer.runBound [as eval] (domain.js:442:12)
    at REPLServer.onLine (repl.js:759:10)
    at REPLServer.emit (events.js:327:22)
    at REPLServer.EventEmitter.emit (domain.js:485:12)
    at REPLServer.Interface._onLine (readline.js:337:10) {
  code: 'ERR_CRYPTO_HASH_FINALIZED'
}
>

[mscdex]

Personally I'd prefer an exception be thrown on multiple calls.

[jasnell]

I would agree. For the new API it should throw and I think we should go back and deprecate the current behavior on createHmac() and eventually switch it to the throw behavior.

[bnoordhuis]

Well, I disagree. It's an artificial restriction for no good reason.

[mscdex]

I think we could get around having to use prefixes by using OpenSSL's EVP_MAC naming as a guide, for example:

crypto.createMac('cmac', key, { cipher: 'aes-128-cbc' });

crypto.createMac('hmac', key, { digest: 'sha1' });

crypto.createMac('poly1305', key);

crypto.createMac('siphash', key);

[mscdex]

Also I think using .digest() instead of something like .final() can be misleading since we're now dealing with more than just HMACs, so we need something more generic.

[bnoordhuis]

@mscdex @tniessen To answer most of your questions: I wrote it the way I did for compatibility with Hash and Hmac.

I want to reimplement createHmac() in terms of createMac() so it probably can't deviate too much from Hmac but I'll give it a shot.

[mscdex]

Suggested change

	this.push(this[kHandle].digest());
	this.push(this[kHandle].final());

[mscdex]

I still think moving all parameters (e.g. cipher name, digest name) to the object in the third parameter will keeps things neater signature-wise and will be simpler once EVP_MAC is available (we could just pass the object to C++ where we'd just have a series of conditionals). We will probably want to rename options to something else at that point.

[jasnell]

@mscdex:

I still think moving all parameters (e.g. cipher name, digest name) to the object in the third parameter will keeps things neater signature-wise

Yes please.

@mscdex:

We will probably want to rename options to something else at that point.

These appear to be the options for the underlying LazyTransform yes? I wouldn't separate those out, I would just use a single options object that includes those properties also, then extract those out for the call to super... e.g.

crypto.createMac('cmac', key, { cipher: 'aes-128-cbc', highWaterMark: 10 });

then inside the Mac constructor...

const { highWaterMark } = options;
super({ highWaterMark });

[mscdex]

These appear to be the options for the underlying LazyTransform yes?

It would probably be both, for the stream settings and for the computation parameters. I suggested changing the parameter name in the signature to something else because it may/may not be optional, depending on the type (e.g. at least cipher required for the 'cmac' type). I don't have any suggestions for a new name at the moment though.

I wouldn't separate those out, I would just use a single options object that includes those properties also, then extract those out for the call to super

That's exactly what I was suggesting. Although you could probably just pass the object as-is to the LazyTransform constructor and it would just pick out what it needs/recognizes as there shouldn't be any conflict in property names between stream options and MAC computation parameter names.

[bnoordhuis]

I still think moving all parameters (e.g. cipher name, digest name) to the object in the third parameter will keeps things neater signature-wise

I considered that when I reworked the first draft but I have a mild-to-serious dislike for that approach.

1. it conflates two unrelated things (mac config and stream config), and

2. it deviates from the other crypto stream classes. The docs for Cipher, Hash, Hmac, etc. all state it's just for "stream.transform options".

and will be simpler once EVP_MAC is available (we could just pass the object to C++ where we'd just have a series of conditionals)

Not sure I follow, can you elaborate? Working with JS objects at the C++ level is a lot more tedious and error-prone than dealing with primitives.

edit: not that it really matters. (Lack of) ease of use at the C++ layer should not inform the public API's design.

[mscdex]

2. it deviates from the other crypto stream classes. The docs for Cipher, Hash, Hmac, etc. all state it's just for "stream.transform options".

I think it's ok to deviate from the other classes in this way because this function is encompassing different types of (MAC) computation with each type requiring different parameters.

Also we don't have a precedent for passing context-specific values as separate parameters for any of these other classes. If anything, by keeping all of the context-specific values in the object, it's more like the existing classes by keeping the function signature consistent.

Another minor reason to do it this way is it might appease V8 because parameter types wouldn't possibly change across multiple calls.

[bnoordhuis]

On your head be it. Updated, PTAL.

[nodejs-github-bot]

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

[mscdex]

Except for missing documentation, LGTM.

[jasnell]

Looking good so far but needs

• docs
• expanded tests (invalid arg type coverage, checking for proper error.code instead of error message)

Will review again once docs and tests are updated.

[tniessen]

Not being able to create a key from a string is just too annoying.

Fair point. No strong opinion on my side, but I am not a fan of Node.js allowing to use strings in cryptographic APIs such as createCipheriv. Strings require assumptions about encoding etc., and fixed-length strings will usually not come from a source with the required entropy. I am not so sure about the implications for MAC.

[jasnell]

@bnoordhuis ... is this stalled or still work in progress?

[justablob]

Can we merge this in its current state? I'm trying to use CMAC in an application and building Node isn't an option. We could add a warning that this is unstable and untested?

[panva]

Tried to use this for an I-D implementation.

const key = crypto.randomBytes(16)
const mac = crypto.createMac('cmac', key, { cipher: 'aes-128-cbc' })
mac.update(crypto.randomBytes(30))
console.log(mac.final())
// => <Buffer b9>

This doesn't seem right to me.

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[github-actions]

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

[paragonie-security]

This should be reopened and support for keyed hashes (i.e. SHA3, BLAKE2) considered in scope.

[panva]

@paragonie-security ad blake2 advanced options see #40921. Bottom line, it's not even in openssl as per openssl/openssl#980

https://www.openssl.org/docs/man1.1.1/man3/EVP_blake2b512.html
NOTES
While the BLAKE2b and BLAKE2s algorithms supports a variable length digest, this implementation outputs a digest of a fixed length (the maximum length supported), which is 512-bits for BLAKE2b and 256-bits for BLAKE2s.