crypto functions should accept buffers as arguments

[bnoordhuis]

#1027 fixes this for HMAC.

Maybe deprecate string arguments altogether. It's the most widely misunderstood part of node if bug reports, the ML and IRC are anything to go by.

CC @pquerna

[baudehlo]

They also need to return buffers. e.g. hash.digest() should return a Buffer and if you want to decode it to hex then that's optional.

[sh1mmer]

@bnoordhuis @pquerna is this still relevent?

Is there a list of crypto functions that don't support Buffers?

[sh1mmer]

I tried createHash and it took Buffers just fine on a sha1 Hash object.

[betamos]

I asked a similar question on Stack Overflow. Subscribing.

[net147]

Similar problem with the signer.sign and verifier.verify. I tried to use signature in a buffer with verifier.verify and it didn't work. It worked when I converted the buffer to a hex string and passed it into verifier.verify as hex though.

[thiagoarrais]

I started to fix this at https://github.com/thiagoarrais/node/tree/crypto-buffer

My initial plan is to fix all the binary string arguments then get into the return values. I'll use the current convention of a request for null encoding returning a buffer.

This is my first patch for node and I would really appreciate if someone could take a look at this to help me make it easier to be accepted and merged back.

[bnoordhuis]

@thiagoarrais: It's easiest if you submit it as a pull request. One thing I noticed is long lines, we have a 80 characters limit.

[thiagoarrais]

I'll take a look at the 80 char limit. I didn't want to make it a pull request while it is too raw, but can do that if you guys prefer it that way.

[thiagoarrais]

Please take a look at the pull request. I think with these last commits I've finished converting the arguments (inputs). I have also updated the docs and will start working on the return values (outputs) next.

But I think we should consider removing encoded strings altogether. There certainly are backwards compatibility issues, but that's what 0.x is for, isn't it? Buffers have been around and recommended for quite some time now after all.

[thiagoarrais]

Any updates here?

[isaacs]

@bnoordhuis This (at least partly) landed in 0.8, correct?

[bnoordhuis]

This (at least partly) landed in 0.8, correct?

Yes, commit 900196e.

[thiagoarrais]

As far as I can tell, this updates only inputs to cypher/decypher. Outputs should be able to be buffers too and there are other functions (sign and verify, for example) that still need to be adapted. I am in a hurry and looking this commit only. Maybe I'm wrong and everything is already in place in 0.8.

I don't think my pull request applies cleanly anymore, but I can port it to the current codebase if there is interest.

[isaacs]

@thiagoarrais Yes, please feel free to rebase to master and resubmit. We would most likely take a pull req for this on v0.9 if it didn't break anything else, but it's too late to get into v0.8.

[thiagoarrais]

@isaacs done! The old patch applied almost without any modification. I just squashed the old commits together and adjusted the tests.

Please take this as a conversation starter and let me know if there is anything that needs to be changed.

[thiagoarrais]

any updates here?

[bnoordhuis]

Closing. This has been fixed in v0.10.