csrf
csrf copied to clipboard
pillarjs
Speed up token generation
This PR reduces dependencies number (from 3 to 1) and should a little improve performance according to this benchmark with the same security (nanoid use the same hardware random generator; it’s popular and well maintained).
nanoid 413,579 ops/sec
uid.sync 354,882 ops/sec
nanoid/async 85,168 ops/sec
uid 78,426 ops/sec
nanoid/non-secure 2,718,186 ops/sec
rndm 2,544,612 ops/sec
@dougwilson what do you think? Should I rename nanoidFast to nanoidNonSecure in R to be more clear?
Needs to at least support Node.js 0.8. What makes it quicker? Is it not possible to speed up our existing dependencies? I have not had great experience with third party dependencies, especially when they seem to very quickly discard support for old Node.js versions. Now, that's their own opinion, though I try to support as far down as is reasonable with the CI infrastructure (which is 0.8 currently).
Needs to at least support Node.js 0.8
Yeap, I support Promise-less environment and it is written by the simple ES5. I especially made few changes to Nano ID to pass csrf tests (great tests, BTW).
What makes it quicker?
Thanks for answering =^_^= In secure generator it happens because of we manager with Buffers in a more optimized way. In non-secure we used a cool trick with Math.floor and old V8 optimizations for asm.js.
Is it not possible to speed up our existing dependencies?
It is possible in theory. But do you want to spend time on maintaining multiple projects and copy ideas every time, when there is a good well-maintained project, which always tries to find a tricky way of optimization? =^_^= (we are focusing not only on performance but also of client-side size)
Yeap, I support Promise-less environment and it is written by the simple ES5. I especially made few changes to Nano ID to pass csrf tests (great tests, BTW).
Ah, sorry, I assumed it didn't because the Node.js 0.8 CI failed for this PR and I looked at the module and it doesn't have any CI for Node.js 0.8 so assumed there is no support. If there is no CI, what is the guarantee that 0.8 is a supported platform in an ongoing fashion?
It is possible in theory. But do you want to spend time on maintaining multiple projects and copy ideas every time, when there is a good well-maintained project, which always tries to find a tricky way of optimization? =^_^= (we are focusing not only on performance but also of client-side size)
Right, I definitely would rather use external deps when possible if they exist and they seem to be aligned with the same target support environments.
If there is no CI, what is the guarantee that 0.8 is a supported platform in an ongoing fashion?
Jest doesn’t support Node.js 0.8 to run a test on CI :(. I think right now most of the test runners drop even Node.js 4 support.
Maybe I can write a script to test critical things manually. Or maybe you have a better idea?
Yea, I don't have a better idea besides just using a testing framework that does support 0.8 (which is what this module does). In my opinion testing frameworks should be supporting pretty much every possible version since they are going to influence what environments real modules can support. But maybe those test frameworks want to use their power to dictate the versions of Node.js people will support, idk.
Coming from the Perl system Node.js feels very fractured to me, with constant major versions of Node.js and module authors quickly discarding support for Node.js versions :(
In my opinion testing frameworks should be supporting pretty much every possible version since they are going to influence what environments real modules can support.
Totally agree. I fight with Jest community for a few months for Node.js 4 even at the moment when it was still LTS.
Fair enough note about CI and Node.js 0.8. I will try to fix it soon.
Done. I added Node.js 4, 0.12 and 0.8 to CI:
https://travis-ci.org/ai/nanoid
It was tricky, since yarn install doesn’t work for many reasons. So I used custom installer and custom task runner with manually testing core features for Node 4-0.8.
@dougwilson I thought about how to fix npm error in csrf CI and do not have a solution right now.
But maybe it is just a cache issue and if we will call it again, it will be installed?
We released Nano ID 1.3.1. It contains client-side optimization (not important for Node.js version). But maybe you will like the way. We wrote a bruteforcer to find the most gzip-friendly alphabet symbols order.
Also, your suggestion testing Nodejs 0.8 found an issue in Travis CI. Not it is fixed.
@dougwilson if you don’t want to speed up it by some reason, just say it. It is not a big deal for me, but I will be able to remove Node.js 0.8—4 support code (it was added only to fit csrf Node.js supporting policies)
@dougwilson sure. I wrote to npm support to ask about storage npm behaviour on Node.js 0.8.
Hi @ai, awesome just saw that 🎉 . But this looks like a breaking change: it would no longer install on machines that it used to install on. Our current dependencies don't have any issue with the verison of npm that is included with Node.js 0.8. Is there any particular reason why this new module does not support the same npm version?
At minimum, if a higher version of npm is required, can you update the engines field to add this requirement? If merging this is going to require a new major version, at that point it would likely just be time to remove Node.js 0.8 support and just make Node.js 0.10 the minimum version., though. I'll need to then cascade that support drop through the modules I have that uses this one, too, of course.
Circling back around, is there any way that this new module can just run under the same version of npm the rest do? Or should this just become a larger discussion around it being a major breaking change?
Circling back around, is there any way that this new module can just run under the same version of npm the rest do?
I asked npm support.
I think Node.js 0.8 is too old for npm and they serve old registry for this old npm.
Strange. Most of all my modules are Node.js 0.8+ and even ones I released last week install on Node.js 0.8 without issue, with the npm that is included out-of-the-box (npm 1.2.30).
This is a fresh build of master on Node.js 0.8 and npm 1.2.30: https://travis-ci.org/pillarjs/csrf/jobs/448623371 . It didn't seem to have any issues downloading and installing modules from the npm registry. It's just using the standard registry https://registry.npmjs.org/
👍 there is an unreleased change to master, so I'm going to release a simple patch version of this module in the meantime, don't panic if you see it. I still think this module is a better method. I may also work to add a simple benchmark to this module in the meantime as well, so (from the usage of the two libs through this module) we can add what the perf improvement is to the changelog too 🎉
So speaking of Node.js 0.8, I was playing around with this PR on Node.js 0.8 on my machine and I encountered a bug I basically forgot about: https://github.com/pillarjs/csrf/pull/10#issuecomment-220851160
Basically in Node.js 0.8 (and 0.10, perhaps some more), the underlying crypto engine in OpenSSL will sometimes throw an error that it's not seeded when the process starts and you try to grab random bytes too fast. To solve that issue, I ended up creating the module https://github.com/crypto-utils/random-bytes and that is used in turn by uid-safe. Since this PR is replacing uid-safe, but it ends up just calling crypto.randomBytes directly (https://github.com/ai/nanoid/blob/master/random.js#L14) this error resurfaces.
Is it possible to get this work-around added to the nanoid module as well?
Basically in Node.js 0.8 (and 0.10, perhaps some more), the underlying crypto engine in OpenSSL will sometimes throw an error that it's not seeded when the process starts and you try to grab random bytes too fast.
Sure, I can add it.
@dougwilson I found a way to fix npm issue in Node.js 0.8 without updating npm (npm 1.x can’t understand ^ version selector)
Oh, nice, I didn't even realize that was there in the PR. I always pin dependency versions anyway to specific versions such that I can see what the changes are before users installing these modules suddenly encounter changes. It has helped a lot in the past especially when a dependency accidentally releases a breaking change to a subset of users but it takes a long time to get it resolved, and users usually are not sure where to report the issue to.
@dougwilson done:
- I added workaround for RNG
not seedederror. - I replace
~to the direct version independencies
Awesome, @ai ! Let me just finish up this release of csrf and then I'll test out this PR again on the same machine to double-check the error is gone 👍
Hi @ai , sorry to come back again 😢. So I tested the updated PR (and yes, it installed nanoid 1.3.2), and I still had the same issue, the seed error. I hooked up a debugger and stepping through the code, it looks like there is still no handling to retry for the error in the code.
The very high-level code that triggers the code path is node -pe 'require("csrf")().secretSync()'. When I walked it through, it seems like it's just calling crypto.randomBytes directly.
What I'm finding is that secretSync is calling this function: https://github.com/ai/nanoid/blob/master/index.js#L21
That in turn calls the random function, defined at https://github.com/ai/nanoid/blob/master/index.js#L1
This just ends up being a direct reference to crypto.randomBytes here: https://github.com/ai/nanoid/blob/master/random.js#L14
I hope that helps! I also finished the simple benchmarks I promised and pushed to master. They are runnable with just npm run bench. I really quickly on 8.12.0 just ran them on the current master and then again on this PR branch (rebased onto master so the benchmarks are there). This is what I found:
verify is basically the same speed (that's fine)
secretSync is much faster with nanoid, that is awesome!
create is a bit slower with nanoid, which is weird
A typical application embedding this module has the flow of (1) call secret* once per user to initialize their session (2) call create on every page load that needs a token (but a lot of people through csurf just end up calling it on every request) and then (3) call verify on a page submit with a token.
What this means that the typical hot path in an app is the create call, as that is typically on every page load that needs a token (though there are apps out there that incorrectly call it on every request). In contrast, the secret* is only once per user session.
Ideally the hot path is the best one to get a speed boost, so I was kind of surprised (at least on my machine) to see the performance go down in the spot that is going to be the hottest path.
I'm not sure yet what is causing the decrease, but the increase in secret* is awesome. It's possible we can just sub nanoid in the secret* functions and keep the other module in the create function, to get the fastest path on all of them. Does that sound like a good solution, or no?
For reference these are the comparisons on my machine:
master
> [email protected] bench ./csrf
> node benchmark/index.js
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
modules@57
[email protected]
napi@3
[email protected]
[email protected]
[email protected]
[email protected]
tz@2018e
> node benchmark/create.js
create
1 test completed.
create x 405,126 ops/sec ±0.48% (191 runs sampled)
> node benchmark/secret.js
secret
3 tests completed.
secretSync x 334,375 ops/sec ±0.76% (183 runs sampled)
secret - callback x 53,006 ops/sec ±0.73% (177 runs sampled)
secret - promise x 49,499 ops/sec ±1.00% (176 runs sampled)
> node benchmark/verify.js
verify
2 tests completed.
verify - valid x 79,736 ops/sec ±1.07% (177 runs sampled)
verify - invalid x 78,411 ops/sec ±1.30% (180 runs sampled)
pr-14
> [email protected] bench ./csrf
> node benchmark/index.js
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
modules@57
[email protected]
napi@3
[email protected]
[email protected]
[email protected]
[email protected]
tz@2018e
> node benchmark/create.js
create
1 test completed.
create x 388,440 ops/sec ±0.43% (190 runs sampled)
> node benchmark/secret.js
secret
3 tests completed.
secretSync x 454,359 ops/sec ±0.48% (190 runs sampled)
secret - callback x 58,837 ops/sec ±0.64% (178 runs sampled)
secret - promise x 56,138 ops/sec ±0.60% (178 runs sampled)
> node benchmark/verify.js
verify
2 tests completed.
verify - valid x 78,578 ops/sec ±1.15% (182 runs sampled)
verify - invalid x 77,687 ops/sec ±1.32% (181 runs sampled)
P.S. thanks for being so responsive. You rock!
What I'm finding is that secretSync is calling this function: https://github.com/ai/nanoid/blob/master/index.js#L21
Oh, yeap. I add not seeded fix only for async version. I will add it for sync version tomorrow.
I also will think about verify performance drop
@dougwilson what Node.js version do you? Some optimization (like that asm.js optimisation for nanoid/non-secure) will work only on modern Node.js version (it is a good to question, what to do in this case)
Hi @ai I was using Node.js 8.12.0, which was the version "recommended for most users" just as of last week, though they rolled over the versions now. I was just testing on what I have most of my production projects running on just as a quick test.