-
-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Crypto buffers #4179
Crypto buffers #4179
Conversation
lib/crypto.js
Outdated
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.
May be just put prototypes of both to some variables to reduce repetition count?
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.
Yeah, that can be organized better, you're right.
Generally, it appears to be correct to me, but you've copy-pasted a lot of js code which made crypto.js even more unreadable that it is now, otherwise lgtm (though would be good if someone else will review it too /cc @piscisaureus @bnoordhuis ) |
doc/api/crypto.markdown
Outdated
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.
I'm not a huge fan of us exposing this internal 'buffer'
encoding in the docs. I think instead we should say something like "Defaults to returning a Buffer
instance", and just never mention 'buffer'
.
doc/api/crypto.markdown
Outdated
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.
Outdated now that we bundle openssl?
@isaacs I landed some Diffie-Hellman bug fixes in v0.8 that you probably should merge before you land this (or merge into master first, then merge master into your branch). |
crypto: Hash and Hmac default to buffers crypto: Move Cipher encoding logic to JS crypto: Move Cipheriv encoding logic to JS crypto: Move Decipher encoding logic to JS crypto: Move Decipheriv into JS, default to buffers crypto: Move Sign class to JS crypto: Better encoding handling in Hash.update crypto: Move Verify class to JS crypto: Move DiffieHellman to JS, default to buffers crypto: Move DiffieHellmanGroup to JS, default to buffers Also, create a test for this feature
This is a flag to make it easier for users to upgrade through the breaking crypto change, and easier for us to switch it back if it's a problem. Explicitly set default encoding to 'buffer' in other tests, in case it ever changes back.
Of course, the commits should be squashed, but I figured it might be good to see them split out, for easier reviewing in a more atomic way.
This makes the node crypto module default to using Buffers. There is a flag that can be set to make it use 'binary' encoded strings by default, for ease of upgrading. All of the buffer/string handling code is moved into JS, so it can use the same Buffer implementation as everything else.
It would be nice if the crypto functions used Buffers instead of SlowBuffers, but that's a much more involved change.