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

Key Generation Best Practices & Possibly Enforcing Secure Keys #311

Open
ankane opened this Issue May 28, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@ankane
Copy link
Contributor

ankane commented May 28, 2018

Hey @saghaulor, what I like most about this gem is makes encryption accessible to anyone with a few lines of code, in a way that's difficult to make mistakes. I think one of the areas that could be improved is key generation. I suspect many developers are generating a random human-readable 32 length string instead of 32 random bytes. It'd be great to give guidance on how to generate a secure key.

For instance, for those who use ENV variables or Rails credentials/secrets for keys, one approach is to create a hex key with SecureRandom.hex(32), then unpack this before passing it to the gem with [hex_key].pack("H*"). Once a best practice is established, it'd be nice to have a built-in support for it. For instance, if the user passes a hex key, the gem does the conversion internally.

The rbnacl gem (bindings for libsodium) has a neat way to enforce secure keys. It checks if the string is binary before allowing it as a key. This means that SecureRandom.random_bytes(32) works as a key and "This is a key that is 256 bits!!" does not.

Curious to hear your thoughts. @david-a-wheeler and I have been discussing this here: ankane/blind_index#5

@david-a-wheeler

This comment has been minimized.

Copy link

david-a-wheeler commented May 29, 2018

@ankane - thanks for starting this. Protecting the developers against Bad Keys is a good idea, because so many people really do not understand keys, and mistakes are often not noticed. Requiring that the keys be binary is a neat way to prevent some problems. Another sanity check you could use, in addition, is to warn or prohibit keys where every byte has a leading zero. That will detect keys that are in as ii but are supposed to be in binary, and also to text him hideously badly selected keys. The probability of a correctly randomly selected key having this property is 1/(2^32), or 2e-8%. People could still choose stupid keys, but at least they would be protected from some common mistakes that are hard to detect later.

@softwaregravy

This comment has been minimized.

Copy link

softwaregravy commented Aug 5, 2018

Just found this and am very grateful to have found a gem that does exactly what I was looking for (thank you). I found this issue while looking for best practices for generating my key. While I'm indifferent to the feature of preventing bad keys, I am very much in favor of providing a best practices section in the main doc. Just adding a section with a few options on best practices to generate they key(s) would be helpful and probably protect against at lease some bad key selections.

@adiakritos

This comment has been minimized.

Copy link

adiakritos commented Sep 14, 2018

So does the key that I use to encrypt data need to be stored or can it be a unique value every time a variable is stored? Is this what the IV column is for? To decrypt the other value?

@david-a-wheeler

This comment has been minimized.

Copy link

david-a-wheeler commented Sep 14, 2018

@adiakritos : You must store an encryption key somewhere if you want to use it to decrypt data later. In many environments the encryption key is stored as an environment variable (be sure to not display that environment variable to untrusted parties - e.g., don't print it as part of your CI test suite).

The IV column is something different. Many encryption algorithms need not just the key, but also a random "initialization vector" (IV) that doesn't need to be kept secret but must be stored for later use. More information here: https://en.wikipedia.org/wiki/Initialization_vector . For most people, this is just a low-level detail... just create the column & you'll be happy.

@ankane

This comment has been minimized.

Copy link
Contributor Author

ankane commented Oct 22, 2018

Quick update: I wrote a post on the topic.

@saghaulor

This comment has been minimized.

Copy link
Contributor

saghaulor commented Nov 13, 2018

@ankane @david-a-wheeler thanks for the great feedback. I've definitely been tossing around the idea of how to make key handling dead simple. Certainly some best practices in the README will go a long way.

If I could go back to the beginning, knowing what I know today, I'd totally rewrite this gem to serialize everything into a single column, everything would include metadata about the key used to encrypt, so as to make rotation significantly easier.

I think forcing the key to be binary is a good idea. Given that it would likely break crypto operations for many users, I think we'd have to introduce is as being configurable. We could even have it on by default with upgrade notes indicating that it may cause problems if keys were not generated correctly.

@ankane

This comment has been minimized.

Copy link
Contributor Author

ankane commented Nov 13, 2018

I honestly think instructions in the readme will make most of the difference. I'm still somewhat torn around the binary key enforcement. The code to convert hex -> binary is not very pretty. One way to get around this would be to accept an encoded string as well if detected.

I've actually been thinking a lot about the single column approach as well, and it'd be great to work towards that. I'm happy to help discuss and/or code. I think a simple interface could look something like:

class User
  attr_encrypted :email, keys: {v1: "key-1", v2: "key-2"}
end

However, the ideal interface would let you specify a different algorithm and other settings with the version.

class User
  attr_encrypted :email, versions: {v1: {key: "key-1", algorithm: "algo-1"}, v2: {key: "key-2", algorithm: "algo-2"}}
end

The value stored in the encrypted_email column could be something like v1:iv:encrypted-data. As you said, this would make it much easier to rotate keys.

@ankane

This comment has been minimized.

Copy link
Contributor Author

ankane commented Nov 13, 2018

Actually, the algorithm can be stored in the column as well, so maybe it's just the key that needs versioned to keep the interface simple.

@saghaulor

This comment has been minimized.

Copy link
Contributor

saghaulor commented Nov 13, 2018

@ankane at my employer we've already built a solution. I'd have to talk with our legal team to see if I'm allowed to disclose the details, but the gist of it is that the metadata is stored with the encrypted payload, so each object knows how to decrypt itself, no matter what the circumstances of it's encryption were.

The challenge about moving to a solution like this is how do we provide a migration path forward. We've solved that problem as well, but it wasn't pretty.

Additionally, I hesitate to suggest a solution where the key is sitting in your app. Our solution externalized crypto from the Ruby process. I worry that it makes the key less safe.

Regarding the key verification, I wonder if simply validating against binary is sufficient. It seems like packing hex to binary may be going to far for the concerns of this gem. A user can already pass a method or proc as a key, this method or proc can contain the necessary logic to pack the key to binary.

That being said, I wonder if validating the key material should even be the concern of attr_encrypted. It seems like it should be a concern of the underlying cryto library.

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