Auth: createHash fix < v0.11.5

[jas-]

@dougwilson Here is a grid of tests I just ran... you are correct < v0.10.5 & v0.11.0 - v0.11.4 are a problem. With that in mind this pr will expand the check to accommodate the PATCH and only apply the newer hash functions for anything > v0.11.5.

Ref: #735

version	results
v0.10.4	failed
v0.10.5	pass
v0.10.6	pass
v0.10.7	pass
v0.10.8	pass
...	...
v0.10.25	pass
v0.10.26	pass
...	...
v0.11.0	failed
v0.11.1	failed
v0.11.2	failed
v0.11.3	pass
v0.11.4	failed (compile errors)
v0.11.5	pass

[felixge]

Thanks for working on this. Is there any way to do this via feature testing rather than version sniffing?

[dougwilson]

@felixge we could not revert to the old sha1 function completely, as I believe that works everywhere still, is that right, @jas- ?

Edit: Meant to say "could just revert" above

[dougwilson]

@jas- can you come up with a way we can choose which sha1 implementation to use without looking at the node.js version? If not, does the old implementation of the sha1 function (with .digest() and .update()) not actually work with some version of node.js and if so, which ones?

[jas-]

I have experienced problems using createHash with the older .digest() & .update() methods even though they should still be supported.

An example is to use v0.11.5 with those older methods.

In regards to abandoning the version sniffing, I will have to look into it maybe some implementation of if (typeof hash.digest() == 'function') { ... }. @felixge I am assuming that is what your referring to.

[felixge]

@jas- so I'm a bit out of touch with the details on this one, but if I understand things correctly, then we need to test if a new method is present to use it in favor of deprecated API, but the new method behaves differently in different versions, so we also need to test the behavior of this function (i.e. feed it a string that allows us to tell if it was interpreted as 'binary' or 'utf-8'). Does that make sense? If not, I'll look up the details and write a more intelligent response :)

[dougwilson]

@felixge sounds good to me. So @jas- it sounds like you need to remove the version detection and do something like the typeof test you suggested, but in addition, you need to make sure the test detection the binary string corruption in the new API under node.js 0.10.5. Your initial matrix seems to have missed that fact.

The detection needs to basically do these things, it seems:

1. Detect that the new crypto stream API is present
2. That the crypto stream API does not have the bug fixed in node.js 0.10.5 (nodejs/node-v0.x-archive@4bf1d10)
3. That the crypto stream API does not have the bug fixed in node.js 0.10.6 (nodejs/node-v0.x-archive@d515857)

I'm not familiar with the node.js 0.11.x versions, but I assume that they are also incorporating those patches in there somewhere. For node.js 0.10, the test should not start passing until node.js 0.10.6.

[dougwilson]

In the meantime, master is using version detection for the node.js 0.10 cases so we can release a 2.1.1 release that unbreaks node.js 0.10.0-0.10.5 users while @jas- gets us something more robust :)

[jas-]

@dougwilson Yeah the matrix of tests I ran before I had to run out the door did omit the binary vs. utf8 bugs found in the commits you mention. I will look at the core test cases to ensure this is handled properly, it might take me bit to test after a patch however. I want to ensure the fix doesn't cause problems (my fault, I should have tested it with older versions of node first).

[dougwilson]

It's no problem! I don't think we could have expected you to possibly have tested "ancient" versions on node.js 0.10 :) Even Travis CI doesn't give us a way to test them (blargh).

[jas-]

Here is the test matrix again using the checks for the newer hash stream methods.

If Travis CI did have functionality to test minor.patch versions it would have saved me from the exhaustive tests outlined below, +1 blargh.

Sorry about that, I should have done some more extensive tests.

*v0.10.x tests

version	results
v0.10.0	pass
v0.10.1	pass
v0.10.2	pass
v0.10.3	pass
v0.10.4	pass
v0.10.5	pass
v0.10.6	pass
v0.10.7	pass
v0.10.8	pass
v0.10.9	pass
v0.10.10	pass
v0.10.11	pass
v0.10.12	pass
v0.10.13	pass
v0.10.14	pass
v0.10.15	pass
v0.10.16	pass
v0.10.17	pass
v0.10.18	pass
v0.10.19	pass
v0.10.20	pass
v0.10.21	pass
v0.10.22	pass
v0.10.23	pass
v0.10.24	pass
v0.10.25	pass
v0.10.26	pass

*v0.11.x tests

version	results
v0.11.0	pass
v0.11.1	pass
v0.11.2	pass
v0.11.3	pass
v0.11.4	compile errors
v0.11.5	pass
v0.11.6	pass
v0.11.7	pass
v0.11.8	pass

[dougwilson]

Awesome! The pass means the passing of the test suite, is that correct?

[sidorares]

👍 great team effort! :)

[jas-]

@dougwilson Yeah, took forever on this vm with a gig of ram. +1 @sidorares

[dougwilson]

So one question I have is that the test is now typeof Crypto.createHash.write == 'function', but I'm having trouble even finding a version of node.js where that returns true. I feel like you may have meant Crypto.createHash.prototype.write, which if that's the case, you may have to re-run all our tests, because then the test would be pretty different.

If you need to change and re-test, please rebase your change off commit 19a8a38 so a test for auth is included (it doesn't seen like there was any real auth test prior).

[jas-]

@dougwilson Well that means that every test passed (against the 19a8a38 commit) and I didn't experience the original problem I described in issue #735. Which was for node versions v0.10.25, v0.10.26 & v0.11.5.

I just re-ran the tests for the major versions that were affected and it seems it very well could have been my development environment was borked. I think you can simply revert my change to use the newer hash stream interface for the .digest() & .update() functions.

Well I feel dumb, sorry.

[jas-]

@dougwilson I just confirmed that it must have been a borked development environment because I was seeing problems with the .digest() & .update() functions with this library as well as the tls module when using requestCert.

I just setup two new dev environments to confirm this.

[dougwilson]

Thanks for all the research. For now I have just reverted it back to the "old-style" to remove the version sniffing it needs to switch between them.

[sidorares]

@dougwilson - could you publish v2.1.1 so that less new people with node 0.10.5 see the problem?

[dougwilson]

@sidorares I was actually going to ask you guys if you thought so, since it seems important :) I'll publish 2.1.1 to npm within the hour.

[sidorares]

cool! since you and @jas- did all the job - let it be you who publish bugfix release :) Don't forget to move changelog head

[dougwilson]

No problem. I have 544f8ef as an example ;)

[felixge]

💖 thx guys