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

SIP006 - Getting rid of key derivation once and for all #35

Closed
Mygod opened this issue Feb 3, 2017 · 107 comments
Closed

SIP006 - Getting rid of key derivation once and for all #35

Mygod opened this issue Feb 3, 2017 · 107 comments

Comments

@Mygod
Copy link
Contributor

Mygod commented Feb 3, 2017

Instead of letting possibly dumb server masters choose password, we are going to choose secure password for them.

A truly high-entropy, secure key can be generated using GnuPG:

$ gpg --armor --gen-random 2 <count>

where count is the length of the key we wish to use. Server masters shouldn't specify key themselves but can request a key renewal.

This key will be directly used for AEAD proposed in SIP004 (#30). When putting in SS urls or JSON file it should be base64 url-friendly encoded.

@riobard
Copy link
Contributor

riobard commented Feb 3, 2017

+1

Finally on the correct path to security :)

@Mygod
Copy link
Contributor Author

Mygod commented Feb 3, 2017

True but as I've said having less security hurts the main goal of Shadowsocks.

@Mygod
Copy link
Contributor Author

Mygod commented Feb 3, 2017

Pros:

  • Better security;
  • Better performance.

Cons:

  • Takes a little extra time to set up.

@Mygod
Copy link
Contributor Author

Mygod commented Feb 3, 2017

@wongsyrone Unfortunately having session key introduces handshake which hurts the main goal of Shadowsocks. Also it's super un-friendly to low end boxes.

@riobard
Copy link
Contributor

riobard commented Feb 3, 2017

I agree the KDF is not a very urgent issue for us. That being said, getting rid of it will

  1. simplify the code base
  2. avoid any arguments on which one to use (and future issue for replacement)
  3. allow more compatibility between implementations in different languages
  4. be more friendly to low end boxes

Looks like all wins to me.

@Mygod
Copy link
Contributor Author

Mygod commented Feb 3, 2017

I agree it's not very urgent but it's good to put it out there anyway.

@riobard
Copy link
Contributor

riobard commented Feb 3, 2017

We seem to have four solutions:

  1. Stick to original KDF in shadowsocks
  2. Switch to Argon2i
  3. Switch to Blake2b
  4. Switch to random keys

Please indicate your current preference and reason below.

@riobard
Copy link
Contributor

riobard commented Feb 3, 2017

Questions for Blake2b: since it's generic hashing, what's the plan to get arbitrary key sizes for different AEAD?

@madeye
Copy link
Contributor

madeye commented Feb 3, 2017

Luckily we have arbitrary size implementation in libsodium: https://download.libsodium.org/doc/hashing/generic_hashing.html

@riobard
Copy link
Contributor

riobard commented Feb 3, 2017

@madeye Sorry I wasn't being clear at my question.

The goal is to get arbitrary output sizes from generic hashing because we want to support AEADs with different key size requirement. Generic hashing produces fixed size output by default, and we must either shorten or lengthen it to get desired output length.

Password hashing has a built-in mechanism to produce arbitrary output sizes, thus more suitable for KDF (what a surprise).

@riobard
Copy link
Contributor

riobard commented Feb 3, 2017

So the question is, what's the proposed solution to shorten/lengthen output from generic hashing without hurting entropy? It's a difficult task from cryptoanalysis point of view. That's why I think dedicated password hashing algo is better than generic hashing here—experts have done the hard job for us.

@madeye
Copy link
Contributor

madeye commented Feb 3, 2017

Blake2b is able to generate at most 64 bytes hash. I think that's why @wongsyrone thinks it's suitable for current AEADs.

If we add something like AES-1024-GCM in the future, then the key size here would be a problem.

@riobard
Copy link
Contributor

riobard commented Feb 3, 2017

Shortening is also an issue. Are we just taking the first 16 bytes of the 64 bytes output for 128-bit keys? What's the effect on entropy loss?

@madeye
Copy link
Contributor

madeye commented Feb 3, 2017

@riobard Yes, it should be the biggest problem. I didn't realize that at first.

So, my choices are

  1. Original KDF for original stream cipher.
  2. Argon2i for AEAD.
  3. Random key for both stream cipher and AEAD as an option for our users.

@madeye
Copy link
Contributor

madeye commented Feb 3, 2017

For random keys, I think @Mygod can just define a new URL schema. I just need to add a new option to CLI and JSON config.

@Mygod
Copy link
Contributor Author

Mygod commented Feb 3, 2017

Wait. Why is shortening an issue? I think shortening should be absolutely fine.

@riobard
Copy link
Contributor

riobard commented Feb 3, 2017

The original KDF is similar in spirit to PBKDF2, and PBKDF2 is also an industry standard used by many (for example MacOS Keychain and 1password). So I'd add PBKDF2 as a possible candidate if we stick to password hashing.

base16/32/64 encode for random keys looks pretty good to me.

My preference would be (in the listed order)

  1. Random keys
  2. Argon2i
  3. PBKDF2
  4. Original KDF

@Mygod
Copy link
Contributor Author

Mygod commented Feb 3, 2017

Actually if we are going to use random keys, we should deprecate key derivation as one of its points is to prevent server masters using weak passwords. Otherwise I prefer to having key derivation instead of having both of them to simplify codebase.

@riobard
Copy link
Contributor

riobard commented Feb 3, 2017

@Mygod It's not that simple… For more explanation you could read https://tools.ietf.org/html/rfc5869#section-5

But anyway the point is that generic hashing has no standard way to produce arbitrary output sizes, and if we use generic hashing, we have to come up with our own (likely flawed) construction. Therefore it's better to use dedicated password hashing instead (if we stick to passwords).

I'd vote for random keys.

@Mygod
Copy link
Contributor Author

Mygod commented Feb 3, 2017

@riobard I see. In that case, why not SHAKE128/SHAKE256?

@riobard
Copy link
Contributor

riobard commented Feb 3, 2017

@Mygod The practical concern is to use something more well-proven and wide-spread because implementations in other languages might not have easy access to mature lib to newer or lesser-known algos.

@riobard
Copy link
Contributor

riobard commented Feb 3, 2017

Also, shake128/256 are generic hashing…

@madeye
Copy link
Contributor

madeye commented Feb 3, 2017

@Mygod It would be very hard for me to drop key derivation here... I need a easy way to share server info with others.

@Mygod
Copy link
Contributor Author

Mygod commented Feb 3, 2017

@madeye Random key can be embedded into JSON/ss uri after encoding to base64. I don't think it will be inconvenient.

@riobard
Copy link
Contributor

riobard commented Feb 3, 2017

@madeye Probably easier to stick to original KDF for stream ciphers, and random keys for AEAD? Is that possible?

@madeye
Copy link
Contributor

madeye commented Feb 3, 2017

@riobard The effort of user education is hard to imagine.

@Mygod What do you think of @riobard's proposal?

@Mygod
Copy link
Contributor Author

Mygod commented Feb 3, 2017

That's exactly what I currently have in mind right now.

@madeye
Copy link
Contributor

madeye commented Feb 3, 2017

Hmm, I'm OK with random keys in that way.

@Mygod
Copy link
Contributor Author

Mygod commented Feb 3, 2017

And yes, as @wongsyrone has pointed out, user education would definitely be a pain in the ass.

@madeye
Copy link
Contributor

madeye commented Feb 4, 2017

Add SIP006 via https://github.com/shadowsocks/shadowsocks-libev/tree/sip006. Any suggestion?

@Mygod
Copy link
Contributor Author

Mygod commented Feb 4, 2017

  1. Use url-safe base64, i.e. [0-9a-zA-Z-_].
  2. Persist the generated key?

@madeye
Copy link
Contributor

madeye commented Feb 4, 2017

Persist the generated key?

It'd be tricky where to store the key.

@Mygod
Copy link
Contributor Author

Mygod commented Feb 4, 2017

@madeye Good point. Ask user to change their key and exit?

@madeye
Copy link
Contributor

madeye commented Feb 4, 2017

@Mygod Yes, I think so.

@madeye
Copy link
Contributor

madeye commented Feb 4, 2017

@Mygod Any spec or implementation for URL safe BASE64.

@Mygod
Copy link
Contributor Author

Mygod commented Feb 4, 2017

@Mygod
Copy link
Contributor Author

Mygod commented Feb 4, 2017

It should be as simple as changing the alphabet.

@riobard
Copy link
Contributor

riobard commented Feb 5, 2017

@madeye @Mygod so what is the conclusion now? Stick with old password hash for stream ciphers and require key for AEAD?

@madeye
Copy link
Contributor

madeye commented Feb 5, 2017

Right, I've updated specs and shadowsocks-libev to follow this SIP.

@librehat
Copy link
Contributor

librehat commented Feb 5, 2017

I'd vote against this proposal as it is enforcing some user behaviour. Users should be allowed to screw up if they may. Even p12 can be encrypted with empty passphrase 🔓

@Mygod
Copy link
Contributor Author

Mygod commented Feb 5, 2017

@librehat The difference is that in p12, user usually needs to enter passphrase manually.

@riobard
Copy link
Contributor

riobard commented Feb 5, 2017

Ok, I took a middle ground in https://github.com/riobard/go-shadowsocks2

It takes both -key and -password arguments. If -key is empty, derive a key from -password using the original MD5-based KDF.

Users are encouraged to generate random keys. But if they have to use passwords, it's same as before.

Note that -key applies to old stream ciphers as well.

@testcaoy7
Copy link

If the server and the client has the same time, can a time-based session key be derived?

@madeye
Copy link
Contributor

madeye commented Feb 6, 2017

Actually, I personally prefer the approach from @riobard.

What do you think? @Mygod

@Mygod
Copy link
Contributor Author

Mygod commented Feb 6, 2017 via email

@madeye
Copy link
Contributor

madeye commented Feb 6, 2017

OK, let's follow @riobard's approach. Directly passing a key to shadowsocks should be useful, especially for some expert users.

To keep things simple, we won't accept keys from URL or QR code, the --key will only work in command line or config file.

@testcaoy7
Copy link

What utility shall be used to generate the key ? GPG ?

@riobard
Copy link
Contributor

riobard commented Feb 6, 2017

My go port has a -keygen option to generate one.

@librehat
Copy link
Contributor

librehat commented Feb 6, 2017

Directly specifying key for expert users is OK. As long as the old-style key derivation from password is kept.

I agree with @madeye to keep URL untouched from this proposal.

@Mygod
Copy link
Contributor Author

Mygod commented Feb 10, 2017

HKDF seems like a better solution here. Close this?

@madeye
Copy link
Contributor

madeye commented Feb 10, 2017

@Mygod LGTM.

@Mygod Mygod removed the deprecated label Feb 10, 2017
@riobard
Copy link
Contributor

riobard commented Feb 18, 2017

Now that we have --key option and SIP007 introduced per-session subkey, I believe the goal of SIP006 is achieved. Closing for now.

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

No branches or pull requests

5 participants