Skip to content
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

encryption/decryption #14

Open
jonathanong opened this issue May 17, 2014 · 19 comments
Open

encryption/decryption #14

jonathanong opened this issue May 17, 2014 · 19 comments
Assignees
Milestone

Comments

@jonathanong
Copy link
Contributor

i have an initial implementation here: 427f188

Some notes/questions/etc.

  • Should we using initialization vectors? not sure how much security that adds. maybe make it optional.
  • want to change .cipher = to something else so we can do .cipher(message) and .decipher(message). .hash_algorithm= and .cipher_algorithm=?

without any feedback i'm going to just chug along

/cc @expressjs/owners

@dougwilson
Copy link
Member

Should we using initialization vectors?

node.js will derive the IV from the password, but it's not as secure. You should have both sides handshake to exchange an IV to use, but I don't think this is feasible with cookies, so it probably just has to be left out.

Basically if you leave it out, the same cleartext always encrypts to the same ciphertext. But of course if you let the user set it and it just stays the same, that doesn't do anything, because the cleartext will still be the same for the same ciphertext.

@defunctzombie
Copy link

Nonces in the cookie can help mix up the cyphertext.
On May 17, 2014 7:13 PM, "Douglas Christopher Wilson" <
[email protected]> wrote:

Should we using initialization vectors?

node.js will derive the IV from the password, but it's not as secure. You
should have both sides handshake to exchange an IV to use, but I don't
think this is feasible with cookies, so it probably just has to be left out.

Basically if you leave it out, the same cleartext always encrypts to the
same ciphertext. But of course if you let the user set it and it just stays
the same, that doesn't do anything, because the cleartext will still be the
same for the same ciphertext.


Reply to this email directly or view it on GitHubhttps://github.com//issues/14#issuecomment-43426755
.

@dougwilson
Copy link
Member

Nonces in the cookie can help mix up the cyphertext.

Make sure the nonce is at the start of the cleartext if you do this, since changes cascade from start to end :)

@jonathanong
Copy link
Contributor Author

okay updated it with optional iv support. let me know what you guys think before i release 2.0.0

@dougwilson your code coverage magic would be appreciated :)

@Fishrock123
Copy link
Member

What's the reasoning behind removing the encoding option? Just wondering, not sure if it actually mattered now.

@dougwilson
Copy link
Member

@dougwilson your code coverage magic would be appreciated :)

Sure, can do :)

@dougwilson
Copy link
Member

What's the reasoning behind removing the encoding option?

The output from this should be opaque to the user; allowing them to say if they want it in hex or base64 is weird; they should just treat it as an opaque string i.m.o.

@Fishrock123
Copy link
Member

Fair enough.

@dougwilson
Copy link
Member

Actually, @Fishrock123 re-looking at the new docs, it looks like we're just returning Buffer objects directly, which would be the real reason the encoding was removed, haha, since it's no longer stringifying the Buffers.

@jonathanong
Copy link
Contributor Author

yeah trying to make this a more generic crypto library. also, encoding is a hassle when you've got a bunch of other stuff to deal with.

@Fishrock123 Fishrock123 added this to the 2.0.0 milestone Jun 19, 2014
@dougwilson
Copy link
Member

@jonathanong I have some questions about this library's interface when you have some time; specifically regarding encodings.

@emilbayes
Copy link
Contributor

What is holding this back?

@dougwilson
Copy link
Member

What is holding this back?

There are a couple encoding issues I still need to fix (non-Latin1 chars are not stored correctly and can thus become corrupted, or unreadable by other languages), some cleanups, and documentation work.

Also @TiXz since you seem interested, if you want, you can start trying it out with npm install expressjs/keygrip and give us feedback as basically a pre-release, if you want :)

@emilbayes
Copy link
Contributor

Done! I was doing essentially what Keygrip does in a private repo, but without hiding keys in a closure and without hardening agains timing attacks. Therefore I already knew the cause of the bug ☀️

@dougwilson
Copy link
Member

Wow, @TiXz you really stepped up there, I was not expecting that, that was awesome! 🌴

@dougwilson dougwilson self-assigned this May 10, 2015
@dougwilson
Copy link
Member

Ok, PR has been merged and I'm in the process of clean up and prep for pushing a 2.0.0.

@jonathanong
Copy link
Contributor Author

@dougwilson anything i can help with?

@mircohacker
Copy link

Let's revive this ;-) After implementing this feature in expressjs/cookie-session#141, what is missing for this feature to land in master?

@hcldan
Copy link

hcldan commented Sep 19, 2022

@dougwilson what is holding this back?

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

No branches or pull requests

7 participants