Skip to content
This repository has been archived by the owner before Nov 9, 2022. It is now read-only.

go fix hkdf in JXcore or remove it #488

Closed
yaronyg opened this issue Jan 25, 2016 · 7 comments
Closed

go fix hkdf in JXcore or remove it #488

yaronyg opened this issue Jan 25, 2016 · 7 comments
Assignees

Comments

@yaronyg
Copy link
Member

yaronyg commented Jan 25, 2016

No description provided.

@yaronyg yaronyg self-assigned this Jan 25, 2016
@yaronyg yaronyg added this to the New Infra milestone Jan 25, 2016
@cicorias
Copy link
Member

cicorias commented Feb 10, 2016

@cicorias
Copy link
Member

cicorias commented Feb 10, 2016

@yaronyg - will do a quick doc on what needs remediation.. any more tests, etc.

@yaronyg
Copy link
Member Author

yaronyg commented Feb 10, 2016

See the email I sent you with a bunch of issues.

@cicorias
Copy link
Member

cicorias commented Feb 15, 2016

@yaronyg - shouldn't this be an external / addon instead of being core to JxCore (or NodeJS)? We could still get the native speed if we follow an AddOn approach. I've gone through nodejs add-ons and they're fairly straight-forward, especially with the 'nan' helper. Also, Async support if this is blocking the JS thread really should be there.

Also, what is the use case(s) and where in a codepath / call sequence is this happening? what are the performance targets? I ask as what makes the vNext hkdf method that @mpodwysocki had done not-performant enough to meet "some requirement"?

Is this called 1,000/second or is this something done per session intermittently? If it's 400/ops or 800/ops in the pure JS version, why isn't that enough?

Also, of the variables (see the benchmark code below) what is static per session, server, etc. And what truly needs to be optimized. I ask as the boringssl sync'd here, and the function HKDF
you'll see that the "keys", the "ikm", "secret" are all parameters and potentially CPU consuming as well. So, just doing "derive" may not be enough. Have we a benchmark that says "derive" is 90% of something of the overall time?

suite.add('HKDF test', function() {
  var pubKey = crypto.createECDH('secp256k1').generateKeys();
  var expirationBuffer = crypto.randomBytes(8);
  var sxy = crypto.createECDH('secp256k1');
  sxy.generateKeys();
  sxy = sxy.computeSecret(pubKey);
  HKDF('sha256', sxy, expirationBuffer).derive('', 32);
})

I've updated the gist breaking down each step and the derive is clearly NOT the significant portion of the overall call - all steps up through sxy.computeSecret

benchmark.js

cicoriasmbp13@Shawns-MBP:~/Development/thali/hdkf-bench$ node ./bench.js 
HKDF test full x 817 ops/sec ±0.57% (87 runs sampled)
HKDF prePubKey test 2 x 1,208 ops/sec ±1.16% (87 runs sampled)
HKDF preRandomBytes x 910 ops/sec ±7.14% (69 runs sampled)
HKDF preSxy x 1,045 ops/sec ±4.15% (73 runs sampled)
HKDF preGenKeys x 2,556 ops/sec ±1.08% (90 runs sampled)
HKDF preSecret x 65,177 ops/sec ±4.91% (66 runs sampled)
Fastest is HKDF preSecret
cicoriasmbp13@Shawns-MBP:~/Development/thali/hdkf-bench$ node ./bench.js 
HKDF test full x 817 ops/sec ±0.61% (87 runs sampled)
HKDF prePubKey test 2 x 1,206 ops/sec ±1.26% (90 runs sampled)
HKDF preRandomBytes x 1,209 ops/sec ±1.50% (87 runs sampled)
HKDF preSxy x 1,288 ops/sec ±0.49% (91 runs sampled)
HKDF preGenKeys x 2,520 ops/sec ±1.16% (89 runs sampled)
HKDF preSecret x 64,526 ops/sec ±5.03% (70 runs sampled)
Fastest is HKDF preSecret
cicoriasmbp13@Shawns-MBP:~/Development/thali/hdkf-bench$ 

With the above results, the big CPU intensive calls are:

 sxy.generateKeys();
 sxy = sxy.computeSecret(pubKey);

@yaronyg yaronyg removed this from the New Infra milestone Feb 16, 2016
@cicorias
Copy link
Member

cicorias commented Feb 16, 2016

today we determined that the benefit of just doing HKDF.derive based upon the updated boringssl implementation is not enough and the hkdf.js implementation is to be used until a later point. Given the primary load is done in the steps prior (computesecret & generatekeys) we'd have to extend even further the native implementation.

At a later date will revisit, and also revisit potentially native addon vs. jxcore - core implementation.

@yaronyg yaronyg added the jxcore label Apr 1, 2016
@yaronyg yaronyg assigned enricogior and unassigned cicorias Jul 14, 2016
@yaronyg yaronyg changed the title go fix hkdf in JXcore go fix hkdf in JXcore or remove it Sep 26, 2016
@orangemocha
Copy link

orangemocha commented Dec 26, 2016

Given that this code doesn't appear to be working too well, it hasn't been updated since the original commit, and it's no longer used by Thali, we have decided to delete it. We are essentially going to revert thaliproject/jxcore@9cfbe48

orangemocha added a commit to thaliproject/jxcore that referenced this issue Dec 27, 2016
enricogior pushed a commit to thaliproject/jxcore that referenced this issue Jan 9, 2017
@enricogior
Copy link
Member

enricogior commented Jan 10, 2017

The revert has been committed thaliproject/jxcore@a556067

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants