crypto: add Hash.prototype.copy() method #29910

bnoordhuis · 2019-10-09T22:38:16Z

Make it possible to clone the internal state of a Hash object
into a new Hash object, i.e., to fork the state of the object.

Fixes: #29903

nodejs-github-bot · 2019-10-09T22:44:22Z

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

doc/api/crypto.md

overlookmotel · 2019-10-10T07:51:27Z

Author of #29903 here. I wasn't able to help with implementation of this feature as I don't know C/C++. But if anything I can do here on docs or testing, or anything involving JS, please shout. Really appreciate this being added to Node.

overlookmotel

Could be good to add to end of hash.digest() docs:

Use hash.copy() first if you wish to continue to add data to the hash after hash.digest().

sam-github · 2019-10-10T15:25:23Z

doc/api/crypto.md

+of the current `Hash` object.
+
+The optional `options` argument controls stream behavior. For XOF hash
+functions such as `'shake256'`, the `outputLength` option can be used to


https://nodejs.org/api/stream.html#stream_new_stream_transform_options doesn't have an outputLength option!

It looks like the options are "whatever crypto.createHash() accepts", and that the crypto.createHash docs are wrong, https://nodejs.org/api/crypto.html#crypto_crypto_createhash_algorithm_options, and got copied here.

You could fix the createHash docs, but that's unrelated to this feature, perhaps just change the docs here to link to them and say "same options as over there", so when the createHash docs get fixed this will be fixed, too.

Personally, I don't think it's too confusing but I can split it off into a separate issue if you like.

doc/api/crypto.md

tniessen · 2019-10-13T03:06:05Z

Could this suffer from problems similar to #28245 depending on OpenSSL internals, since we don't always set state[kFinalized]?

bnoordhuis · 2019-10-14T13:49:31Z

Could this suffer from problems similar to #28245 depending on OpenSSL internals, since we don't always set state[kFinalized]?

Interesting question. I don't believe it actually hurts to copy a finalized hash object, it's just pointless (and that's why I added the guard.)

nodejs-github-bot · 2019-10-14T21:03:56Z

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


        crypto: add Hash.prototype.copy() method

Make it possible to clone the internal state of a Hash object into a new Hash object, i.e., to fork the state of the object. Fixes: #29903


        fixup! add example

bnoordhuis · 2019-10-15T10:41:13Z

Example added to the documentation.

Use hash.copy() first if you wish to continue to add data to the hash after hash.digest().

I didn't use that because it can be construed as meaning that copy() after digest() should work when it doesn't.

Food for thought: we could also make digest() omnipotent by cloning the hash object internally. I.e., this would work:

const hash = crypto.createHash('sha256');
const one = hash.update('one').digest();
const two = hash.update('two').digest();

The downside is that it imposes a (possibly small) performance penalty on single-use hash objects.

overlookmotel · 2019-10-15T11:23:44Z

@bnoordhuis Only problem with that is that some people feel that the hash object should be reusable in that its internal state is reset after a call to .digest() (#25857). Personally, that doesn't make sense to me, but it could cause confusion it seems.

It's not so hard to call .copy() explicitly I think, and some people will blanche at even a small performance cost.

tniessen · 2019-10-15T20:49:38Z

I don't think it is a good idea to implicitely reset the internal state of the object in .digest(), it will only lead to bugs and problems.

.copy() behaves differently than what you are describing, it does not reset the state, instead, it allows to obtain a second Hash object with the same internal state.

nodejs-github-bot · 2019-10-16T05:18:53Z

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

overlookmotel · 2019-10-16T10:55:14Z

@tniessen Ah sorry I wasn't clear. I understand this is not what .copy() does. What I was trying to say is that there are seemingly two different behaviors that people might expect if you are allowed to call .digest() more than once on a hash.

With this code:

const hash = crypto.createHash('md5');
hash.update('abc');
hash.digest('hex');

hash.update('def');
const res = hash.digest('hex');

res could be expected to be either:

Hash of "abcdef" (rolling hash)
Hash of "def" (1st call to .digest() reset hash)

My expectation would be (1), but it seems from #25857 that some people would expect (2). Personally I think there's little merit to the arguments made by the OP in that issue. But since there are seemingly different opinions, it could lead to confusion and bugs as you say.

So I was trying to say: probably it's safer to keep behavior of .digest() as it is, to avoid any ambiguity.

Hope that make more sense.

nodejs-github-bot · 2019-10-16T16:45:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/26060/ ✔️

Trott · 2019-10-16T17:01:04Z

Landed in 9f203f9


        crypto: add Hash.prototype.copy() method

Make it possible to clone the internal state of a Hash object into a new Hash object, i.e., to fork the state of the object. Fixes: #29903 PR-URL: #29910 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

overlookmotel · 2019-10-16T18:30:43Z

Fantastic. Thanks very much @bnoordhuis for implementing this and everyone else for reviewing.

Can I ask: Is this likely to get into a Node 12.x release before it goes LTS?

Trott · 2019-10-16T19:44:33Z

Can I ask: Is this likely to get into a Node 12.x release before it goes LTS?

Probably not before it goes LTS but probably the next release after that one. /ping @nodejs/releasers in case I'm wrong about that.

overlookmotel · 2019-10-16T20:53:30Z

Ah OK great. My mistake - I was confusing the Active and Maintenance phases in terms of when new features can be added, so was wrongly thinking this might not land in an LTS release line until 14.x.

tniessen · 2019-10-16T23:18:06Z

@overlookmotel Thanks for the clarification, I absolutely agree. Resetting the state in .digest() (or silently copying the state) will only lead to confusion.

vsemozhetbyt · 2019-10-17T15:46:36Z

It seems the YAML processing is broken for this doc section. IIRC, we usually use this structure with PR URL field only in history entries.


        doc: make YAML matter consistent in crypto.md

This fixes YAML that gets incorrectly processed by our tooling. Refs: nodejs#29910 (comment)


        doc: make YAML matter consistent in crypto.md

This fixes YAML that gets incorrectly processed by our tooling. Refs: nodejs#29910 (comment) PR-URL: nodejs#30016 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>


        crypto: add Hash.prototype.copy() method

Make it possible to clone the internal state of a Hash object into a new Hash object, i.e., to fork the state of the object. Fixes: #29903 PR-URL: #29910 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>


        doc: make YAML matter consistent in crypto.md

This fixes YAML that gets incorrectly processed by our tooling. Refs: #29910 (comment) PR-URL: #30016 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>


        crypto: add Hash.prototype.copy() method

Make it possible to clone the internal state of a Hash object into a new Hash object, i.e., to fork the state of the object. Fixes: #29903 PR-URL: #29910 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>


        doc: make YAML matter consistent in crypto.md

This fixes YAML that gets incorrectly processed by our tooling. Refs: #29910 (comment) PR-URL: #30016 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>


        2019-11-05, Version 13.1.0 (Current)

Notable changes: * cli: * Added a new flag (`--trace-uncaught`) that makes Node.js print the stack trace at the time of throwing uncaught exceptions, rather than at the creation of the `Error` object, if there is any. This is disabled by default because it affects GC behavior. #30025 * crypto * Added `Hash.prototype.copy()` method. It returns a new `Hash` object with its internal state cloned from the original one. #29910 * dgram * Added source-specific multicast support. This adds methods to Datagram sockets to support RFC 4607 (https://tools.ietf.org/html/rfc4607) for IPv4 and IPv6. #15735 * fs * Added a `bufferSize` option to `fs.opendir()`. It allows to control the number of entries that are buffered internally when reading from the directory. #30114 * meta * Added Chengzhong Wu (https://github.com/legendecas) to collaborators. #30115 PR-URL: #30262


        2019-11-05, Version 13.1.0 (Current)

Notable changes: * cli: * Added a new flag (`--trace-uncaught`) that makes Node.js print the stack trace at the time of throwing uncaught exceptions, rather than at the creation of the `Error` object, if there is any. This is disabled by default because it affects GC behavior. #30025 * crypto * Added `Hash.prototype.copy()` method. It returns a new `Hash` object with its internal state cloned from the original one. #29910 * dgram * Added source-specific multicast support. This adds methods to Datagram sockets to support RFC 4607 (https://tools.ietf.org/html/rfc4607) for IPv4 and IPv6. #15735 * fs * Added a `bufferSize` option to `fs.opendir()`. It allows to control the number of entries that are buffered internally when reading from the directory. #30114 * meta * Added Chengzhong Wu (https://github.com/legendecas) to collaborators. #30115 PR-URL: #30262

nodejs-github-bot added C++ lib / src labels Oct 9, 2019

bnoordhuis added the crypto label Oct 9, 2019

bnoordhuis referenced this pull request Oct 9, 2019

Calculating rolling hashes #29903

Closed

jasnell reviewed Oct 9, 2019

View changes

doc/api/crypto.md Show resolved Hide resolved

overlookmotel suggested changes Oct 10, 2019

View changes

cjihrig approved these changes Oct 10, 2019

View changes

sam-github approved these changes Oct 10, 2019

View changes

Trott reviewed Oct 12, 2019

View changes

doc/api/crypto.md Outdated Show resolved Hide resolved

Trott force-pushed the bnoordhuis:fix29903 branch from 69f0e10 to 939ad26 Oct 13, 2019

tniessen approved these changes Oct 13, 2019

View changes

devnexen approved these changes Oct 14, 2019

View changes

bnoordhuis added 2 commits Oct 9, 2019

crypto: add Hash.prototype.copy() method

bb039ef

Make it possible to clone the internal state of a Hash object into a new Hash object, i.e., to fork the state of the object. Fixes: #29903

fixup! add example

Loading status checks…

0291c30

bnoordhuis force-pushed the bnoordhuis:fix29903 branch from 939ad26 to 0291c30 Oct 15, 2019

jasnell approved these changes Oct 15, 2019

View changes

Trott added the semver-minor label Oct 16, 2019

Trott closed this Oct 16, 2019

Trott added a commit to Trott/io.js that referenced this pull request Oct 18, 2019

doc: make YAML matter consistent in crypto.md

fdfc434

This fixes YAML that gets incorrectly processed by our tooling. Refs: nodejs#29910 (comment)

Trott referenced this pull request Oct 18, 2019

doc: make YAML matter consistent in crypto.md #30016

Merged

3 of 3 tasks complete

targos referenced this pull request Nov 5, 2019

v13.1.0 release proposal #30262

Merged

nodejs/node

Join GitHub today

crypto: add Hash.prototype.copy() method #29910

Conversation

bnoordhuis commented Oct 9, 2019

This comment has been minimized.

nodejs-github-bot commented Oct 9, 2019

This comment has been minimized.

overlookmotel commented Oct 10, 2019

overlookmotel left a comment • edited

This comment has been minimized.

sam-github Oct 10, 2019

This comment has been minimized.

bnoordhuis Oct 15, 2019

This comment has been minimized.

tniessen commented Oct 13, 2019

This comment has been minimized.

bnoordhuis commented Oct 14, 2019

This comment was marked as outdated.

nodejs-github-bot commented Oct 14, 2019

This comment has been minimized.

bnoordhuis commented Oct 15, 2019

This comment has been minimized.

overlookmotel commented Oct 15, 2019

This comment has been minimized.

tniessen commented Oct 15, 2019

This comment has been minimized.

nodejs-github-bot commented Oct 16, 2019

This comment has been minimized.

overlookmotel commented Oct 16, 2019

This comment has been minimized.

nodejs-github-bot commented Oct 16, 2019 • edited by Trott

This comment has been minimized.

Trott commented Oct 16, 2019

This comment has been minimized.

overlookmotel commented Oct 16, 2019

This comment has been minimized.

Trott commented Oct 16, 2019

This comment has been minimized.

overlookmotel commented Oct 16, 2019

This comment has been minimized.

tniessen commented Oct 16, 2019

This comment has been minimized.

vsemozhetbyt commented Oct 17, 2019 • edited

overlookmotel left a comment •

edited

nodejs-github-bot commented Oct 16, 2019 •

edited by Trott

vsemozhetbyt commented Oct 17, 2019 •

edited