Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

add encode/decode functions #1

Open
swys opened this Issue Dec 5, 2013 · 4 comments

Comments

Projects
None yet
2 participants
Contributor

swys commented Dec 5, 2013

I have a need for a pull style streaming wrapper around nodes crypto encode/decode functions. I was looking if there was anything like this when I came across this module on npm.

I wanted to see if I could get this added to this module. I am not sure if your are interested in adding these features or not but I figured I'd give it a shot.

While writing the add-on I ran into a little issue. Since I am pretty new to writing pull style streams this may be an issue that is easily explained.

I wrote gist that shows the 2 styles I wrote the encoder in and also the usage. I got the callback style (which is the same style as the hashes function is written) and the pipable style.

The closest I got with the callback style was to return a source that pipes to other streams, but this seems kind of weird to pipe inside the return of the callback rather than just adding another pipe to the end of the encoder.

https://gist.github.com/swys/7800320 (please excuse the screwy indenting, not sure why this is happening)

Basically I read your hashes function and it takes on options object and a callback. The result is in the callback, but I cannot seem to make this entire function pipable (I can only return a source that is pipable inside the calback).

Meaning I would like it best if I can choose if I want to supply a callback or just pipe the result to another destination.

So below 2 examples would work...

pull.values(['encode me then read me in the callback'])
  .pipe(encode(opts, function(err, result) {
    if (err) throw err
    console.log(result)
  }))

and...

pull.values(['encode me then pipe me to another stream'])
  .pipe(encode(opts)())
  .pipe(pull.log())

I'm just wondering if its possible to write the encoder/decoder in a way that will allow both of the above examples to work. I think it would be best to stay consistent with original hashes style but I'd also like to pipe the output somewhere else.

Thanks in advance for any advice with this issue. I really appreciate it. Even if your not interested in this add-on it will give me a better idea of how to handle pull streams.

Owner

dominictarr commented Dec 5, 2013

Hi!

Yes, if you make a pull request that adds a pull-stream version of something in node's crypto module,
I am happy to merge it. I am also happy to help you figure out how to write a pull-stream.

The indentation is deep because you are using tabs, and github doesn't have a way to set the tab-width.

Oh, by the way, the chaining syntax is legacy. I've just updated the module to have the new style that pull-streams should use.

so, I think it makes more sense to have encoding as a stream, not as a callback.
it makes sense for hashing to just be a callback, because there is only a small value.

but if you wanted to do that, I'd do it like this:

var bops = require('bops') //recommended for binary compatibility on the browser.
exports.encode = function (opts, cb) {
  if(cb)
    return pull(
      cryptoStream(opts),
      pull.collect(function (err, ary) {
        if(err) cb(err)
        else   cb(null, bops.join(ary))
      })
    )
    return cryptoStream(opts)
}

hmm, I think this is wrong:

https://gist.github.com/swys/7800320#file-encodecallbackstyle-js-L20-L25

you should create one cypher per stream, and don't write the final('hex') to it until the input stream has ended, otherwise you are just encrypting each message individually. this might work in memory,
but when you stream this across the network, the messages will be concatenated, and it will not be possible to tell which messages where which, so I don't think you'll be able to decrypt it.

Contributor

swys commented Dec 6, 2013

Thanks so much for taking the time out to review this!!!! Your suggestions were great, and helped me think of the pull-stream in a new way.
I really like using the pull-stream style, it seems to be a lot more simpler than core streams since your just working with functions. It takes a bit to get used to but definately seem much more light weight and free.

I just forked the repo tonight and commited some changes. There are a few things I want to look at before sending the PR though. It seems that either node's crypto module is acting a little funny for some of the ciphers or maybe I'm doing something wrong but there is about 106 ciphers available on my system and I cannot seem to encode/decode without error for about 12 of them. The others work fine. I had to wrap the crypto update and final methods up in a try/catch just so I'd be able to prevent the module from throwing. So at least the stream could finish and you can handle the errors without to kill it mid stream.

Also thanks for the heads up on the indentation issue, it looks like I'm probably going to have to convert tabs to spaces to get control on the indentation. I changed mine to two and saved it, commited it and still seeing to many spaces both in the repo and on my OS.

I will probably be able to submit tomorrow. I added a few test too. I agree that the encode/decode would be best suited as a stream. I ended up using the code you have above as the exports function so it can take either a callback or be used just as a stream. I think being able to support both styles is good and the callback gets the extra benifit of the bops module.

I fixed the issue with the encrypt/decrypt functions trying to call final immediately with that hardcoded 'hex' value you point out above. I now have it so it just keeps calling the read function until there is no more data and it gets the end = true then it will call final method on the crypto obj and send the data on to the next stream through the callback.

It took a little while to figure out that I can give the callback to read a name like next (which I saw you did somewhere else and was wondering what that actually meant) and then just keep calling read until I get all the data.

Again thank you so much for all of your input, it is greatly appreciated!!!! I will send that over soon and hopefully it will be working as expected. I am curious to see if you have the same issue with testing all the ciphers on your system. I actually had to blacklist a couple of algorithms because it was causing me to get max call stack errors. I hope thats not my fault :)

Owner

dominictarr commented Dec 8, 2013

cool!

hmm, that is quite odd that some just don't work.
from what you describe, it sounds like a bug in node.js
I guess it's possible that it's with some obscure crypto algorithm that few people actually use,
so the bug hasn't been discovered. Unlikely, but possible.

Hmm, looking at the crypto tests in node core, it doesn't look like they test all the possible
crypto algorithms https://github.com/joyent/node/blob/master/test/simple/test-crypto.js
just the most commonly used ones? probably this is because they are not used within node's
tsl, https modules, etc. Interesting, I'll try and reproduce when I see your code.

Contributor

swys commented Dec 9, 2013

I made a pure core test case for the issue with the algorithm failures. You can view it here : https://gist.github.com/swys/7866988

I was able to encrypt/decrypt aes-128-xts and aes-256-xts without errors in the allCiphers.js test after I read this : joyent/node#6477

Basically if you supply a message less than AES_BLOCK_SIZE which is 16 bytes then it will throw. So now with a longer test message I don't get any errors. Below is the output I'm getting from the above gist after it runs on my system.

I am now failing on 7 different algorithms on my system.

  • aes-128-gcm : TypeError: error:00000000:lib(0):func(0):reason(0)
  • aes-192-gcm : TypeError: error:00000000:lib(0):func(0):reason(0)
  • aes-256-gcm : TypeError: error:00000000:lib(0):func(0):reason(0)
  • des-ede3-cfb1 : decrypted text does not match original plain text
  • id-aes128-GCM : TypeError: error:00000000:lib(0):func(0):reason(0)
  • id-aes192-GCM : TypeError: error:00000000:lib(0):func(0):reason(0)
  • id-aes256-GCM : TypeError: error:00000000:lib(0):func(0):reason(0)

I updated the test on my fork with longer test message so it passes and could now eliminate the blacklist.
Looks like now I have these TypeError's repeated 6 times and the one error where the decrypted text doesn't match the input text.

I will see if I can find out more information about the above failures.

If you get a chance to test this out let me know if your are seeing the same errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment