-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
feat(KeyObject) #5940
feat(KeyObject) #5940
Conversation
I may just not be reading the code right but it's important that createPublicKey and createPrivateKey also handle the |
04f94f9
to
b512791
Compare
✅ |
❌ @cirospaciari 9 files with test failures on bun-darwin-aarch64:
|
3f86fbb
to
bbc6b17
Compare
❌ @cirospaciari 4 files with test failures on linux-x64-baseline:
|
❌ @cirospaciari 5 files with test failures on linux-x64:
|
❌ @cirospaciari 6 files with test failures on bun-darwin-x64-baseline:
|
key.indexOf("PRIVATE KEY-----") !== -1 && (key = _createPublicKey(key).export()); | ||
} | ||
return key; | ||
} | ||
function getArrayBufferOrView(buffer, name, encoding) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should move this one to native code in a future PR
src/js/node/crypto.js
Outdated
if (key instanceof KeyObject) { | ||
key = key.export(); | ||
} else if (key instanceof CryptoKey) { | ||
key = KeyObject.from(key).export(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should make export
a static method that does this as one function instead of two
but we can do that in a future PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed on 59ebd2d
src/js/node/crypto.js
Outdated
return hash.update(this._opad).update(h).digest(); | ||
}; | ||
module.exports = function (alg, key) { | ||
if (key instanceof KeyObject) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed on 59ebd2d
} | ||
}; | ||
|
||
function _generateKeyPairSync(algorithm, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why didn't we do these functions in native code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I am not sure either, I will change that to move most of it to native and only use KeyObject.from when needed.
|
||
switch (type) { | ||
|
||
case DataViewType: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at some point we should add a macro for this
Float16Array will be a thing in like a year probably
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great.
What does this PR do?
Add KeyObject and related methods
Fix #2036
Fix #1454
Fix #5036
Fix #4983
Fix #4692
Fix #2730
Maybe Related with #3950 #4596 #5122
How did you verify your code works?
I wrote automated tests