-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Option to disable validation #21
Comments
Hi @ipietika! Thanks for taking the time to create a GitHub account and reach out with your feedback. I've been on vacation this past week, so apologies for taking a few days to respond. The validation certainly takes a non-zero amount of time, I don't expect it would be a particularly large amount of time though (not enough for "serveral seconds" at least). I'm certainly interested and will do some benchmarking, the option could be a reasonable trade-off in some situations if it shows a difference. May take a few days as I get caught up on messages and e-mails. Curious about your test scenario though. Part of the initial call will likely include computation of the curve from its parameters. Perhaps you're seeing these delays on cold starts? Let me know! |
Are you running the code with every Lambda execution, or just once? Remember you can share resources across multiple lambda executions. |
I wrote the following test function to be used with AWS Lambda
The output log for the function is the following:
As can be seen the jwk-to-pem conversion took over 7.5 seconds (and there were some even longer executions). In my use case the key validation is not needed (the keys are validate at other part of application). |
I did some more testing and I modified the jwk-to-pem and elliptic libraries. I think that when the library calls EC from index.js
At that point the elliptic library does some pre-compute routine that takes time. I made an experiment and removed the call for the precompute (in elliptic library) and removed the validation from jwk-to-pem. With the modifications the conversion time from jwk to pem is couple of milliseconds (using the test function from my previous comment). |
Hi @ipietika - do you think you'd be able to submit those changes as a pull request, which assumedly completely skips using elliptic? Would give a starting off point if nothing else. I'm also still curious what your non cold-start looks like? Here's my local converting a P-521 key, showing how, in the current state, the first call for a curve is expected to be slower due to loading the curve definitions, but it would settle out after that (keep in mind P-521 is much slower than P-256):
|
Hi @omsmith - This is my first project with AWS Lambda, but I think that the scripts are always executed as a 'cold start' (when script is completed it exists). I made some changes to src/ec.js and I think this works better for my use case (and the code no longer depends on elliptic library but the built-in crypto). However I was not able to pass the tests as my openssl does not support curve P-256 (the P-384 and P-521 tests were passed) and so I did not prepare a pull request (edit: the reason was that the openssl does not use name secp256r1 for P-256, but prime256v1. I edit the code and now it passes the P-256 tests also). However below is the src/ec.js and if you wish you can use my changes in the library. 'use strict';
var asn1 = require('asn1.js'),
crypto = require('crypto'),
base64url = require('base64url'),
Buffer = require('safe-buffer').Buffer;
var curves = {
'P-256': 'p256',
'P-384': 'p384',
'P-521': 'p521'
},
oids = {
'P-256': [1, 2, 840, 10045, 3, 1, 7],
'P-384': [1, 3, 132, 0, 34],
'P-521': [1, 3, 132, 0, 35]
};
function ecJwkToBuffer(jwk, opts) {
if ('string' !== typeof jwk.crv) {
throw new TypeError('Expected "jwk.crv" to be a String');
}
var hasD = 'string' === typeof jwk.d;
var xyTypes = hasD
? ['undefined', 'string']
: ['string'];
if (-1 === xyTypes.indexOf(typeof jwk.x)) {
throw new TypeError('Expected "jwk.x" to be a String');
}
if (-1 === xyTypes.indexOf(typeof jwk.y)) {
throw new TypeError('Expected "jwk.y" to be a String');
}
if (opts.private && !hasD) {
throw new TypeError('Expected "jwk.d" to be a String');
}
var curveName = curves[jwk.crv];
if (!curveName) {
throw new Error('Unsupported curve "' + jwk.crv + '"');
}
var result = keyToPem(jwk, opts);
return result;
}
function publicKeyBuffer(jwk) {
var x = base64url.toBuffer(jwk.x);
var y = base64url.toBuffer(jwk.y);
return Buffer.concat([ Buffer.from([ 0x04 ]), x, y ], 1 + x.length + y.length);
}
function publicKeyBufferFromPrivateKey(jwk) {
var ecdh;
switch (jwk.crv) {
case 'P-521':
ecdh = crypto.createECDH('secp521r1');
break;
case 'P-384':
ecdh = crypto.createECDH('secp384r1');
break;
case 'P-256':
ecdh = crypto.createECDH('prime256v1');
break;
default:
throw new Error('Unsupported curve "' + jwk.crv + '"');
}
var d = base64url.toBuffer(jwk.d);
ecdh.setPrivateKey(d);
return ecdh.getPublicKey();
}
function privateKeyBuffer(jwk) {
return base64url.toBuffer(jwk.d);
}
function keyToPem(jwk, opts) {
var subjectPublicKey;
if (opts.private) {
subjectPublicKey = publicKeyBufferFromPrivateKey(jwk);
} else {
if (jwk.x && jwk.y) {
subjectPublicKey = publicKeyBuffer(jwk);
}
else {
subjectPublicKey = publicKeyBufferFromPrivateKey(jwk);
}
}
subjectPublicKey = Buffer.from(subjectPublicKey);
subjectPublicKey = {
unused: 0,
data: subjectPublicKey
};
var parameters = ECParameters.encode({
type: 'namedCurve',
value: oids[jwk.crv]
}, 'der');
var result;
if (opts.private) {
result = ECPrivateKey.encode({
version: ecPrivkeyVer1,
privateKey: privateKeyBuffer(jwk),
parameters: parameters,
publicKey: subjectPublicKey
}, 'pem', {
label: 'EC PRIVATE KEY'
});
} else {
result = SubjectPublicKeyInfo.encode({
algorithm: {
algorithm: [1, 2, 840, 10045, 2, 1],
parameters: parameters
},
subjectPublicKey: subjectPublicKey
}, 'pem', {
label: 'PUBLIC KEY'
});
}
// This is in an if incase asn1.js adds a trailing \n
// istanbul ignore else
if ('\n' !== result.slice(-1)) {
result += '\n';
}
return result;
}
var ECParameters = asn1.define('ECParameters', /* @this */ function() {
this.choice({
namedCurve: this.objid()
});
});
var ecPrivkeyVer1 = 1;
var ECPrivateKey = asn1.define('ECPrivateKey', /* @this */ function() {
this.seq().obj(
this.key('version').int(),
this.key('privateKey').octstr(),
this.key('parameters').explicit(0).optional().any(),
this.key('publicKey').explicit(1).optional().bitstr()
);
});
var AlgorithmIdentifier = asn1.define('AlgorithmIdentifier', /* @this */ function() {
this.seq().obj(
this.key('algorithm').objid(),
this.key('parameters').optional().any()
);
});
var SubjectPublicKeyInfo = asn1.define('SubjectPublicKeyInfo', /* @this */ function() {
this.seq().obj(
this.key('algorithm').use(AlgorithmIdentifier),
this.key('subjectPublicKey').bitstr()
);
});
module.exports = ecJwkToBuffer; I also run the test with code you provided:
|
Hi, is there any plan to incorporate the changes? I have also ran into exactly similar issue. With aws lambda@edge you cannot afford the luxury of slow code. If disabling validation is not desirable, then I am interested about the alternative. |
Sorry, I have misunderstood. The solution above points out to the bottleneck from |
These changes caused failure of test cases when private option is specified |
Hello,
Is it possible to add option that disables key validation for EC keys?
I am running the code in AWS Lambda and for some reason converting JWK to PEM with this library can take a lot of time (several seconds). I assume that the validation is the biggest cause for that?
The text was updated successfully, but these errors were encountered: