transcrypt icon indicating copy to clipboard operation
transcrypt copied to clipboard

Add support for pbkdf2 key derivation with iterations, and make this the default

Open jmurty opened this issue 2 years ago • 6 comments

This is some initial, and very rough, progress towards adding support for pbkdf2 key derivation as available in (and strongly recommended by) newer versions of OpenSSL and LibreSSL, aiming to solve lingering issues #55 and #59

This branch/PR is taking an ugly but easy approach where the existing transcrypt.cipher configuration setting stores compound values where a key derivation function and iteration count can be set as cipher:key-derivation:iterations — i.e. aes-256-cbc:pbkdf2:1024 to replace the prior aes-256-cbc

As of now:

  • init a new repository with the new default "cipher" aes-256-cbc:pbkdf2:1024
  • re-key existing secrets in a repository to set the stronger "cipher" with a command like: transcrypt --rekey -c 'aes-256-cbc:pbkdf2:1024' -p 'correct horse battery staple' --yes
  • still compatible with old repos and old versions of OpenSSL when cipher is set to the old default aes-256-cbc

TODO:

  • [ ] check whether overloading the transcrypt.cipher setting is a reasonable approach?
  • [ ] detect old or incompatible OpenSSL versions and give useful tips for fixing it, e.g. on Mac:
    • brew install openssl
    • For existing init in repo: transcrypt --set-openssl-path=$(brew --prefix openssl)/bin/openssl
    • To init a new repo: transcrypt -c aes-256-cbc:pbkdf2:1024 -p 'correct horse battery staple' --set-openssl-path=$(brew --prefix openssl)/bin/openssl --yes
  • [ ] add tests for re-keying with the new cipher, and --rekey in general
  • [ ] improve behaviour of --rekey to complain less about non-problems, like existing pre-commit Git hook
  • [x] maybe use something other than MD5 for hashing the passphrase with -md MD5, upgrade to -md sha512?

jmurty avatar Aug 31 '21 14:08 jmurty

What implications does this have, if any, for existing repos?

I am assuming that it will be possible to upgrade the cipher used for a repo, and at that point you will need a version with this change. Is that correct?

brianmay avatar Mar 06 '22 21:03 brianmay

The main implication for the improved crypto approach is that, for updated repos, transcrypt will no longer work out-of-the-box on systems with outdated versions of OpenSSL: in particular, on Macs.

So the necessary steps would be:

  • upgrade to newer transcrypt once available
  • set or update the crypto hash setting
  • rekey, if in a repo that is already transcrypted
  • ensure all downstream users also upgrade transcrypt and apply the updated settings
  • ensure all downstream users have a recent version of OpenSSL, or install a newer version and configure transcrypt to use it

jmurty avatar Mar 07 '22 08:03 jmurty

Is there still momentum here? I have a fork where I enable pbkdf2, I can help with tasks to get this landed sooner if that is helpful.

Erotemic avatar Apr 26 '22 19:04 Erotemic

Hi @Erotemic although there isn't much momentum at present – I've had a busy few months – I'd appreciate your help to push things forward.

The main outstanding tasks are listed in the TODOs above, but I think the most difficult thing to get right will be the user experience: helping people to upgrade existing repos, recognising when the user's system isn't compatible with the new approach (especially macOS) and helping them to fix that.

The discussion over in #55 about whether simply switching to pbkdf2 is sufficiently secure given how the salt is handled is also worth resolving. If we're going to break backwards compatibility with a new version we should make sure the approach is as strong as we expect. I'm no cryptographer however, so am out of my depth on that discussion.

jmurty avatar Apr 27 '22 13:04 jmurty

@jmurty my thought is that a good first pass would be to simply make all of these new options configurable, while keeping existing defaults. If we restrict the scope of the PR to just allowing the user to configure a more secure transcrypt repo rather than defaulting to those secure settings, then it will be a lot easier to write a stable patch. Then once that is merged and users have the option of creating new repos with secure settings, we can think more about the process of helping users seamlessly upgrade.

I have a pass at this in my fork, but I've hard-coded all of the config settings that I like. To port those changes here I'd like to know: what is the best way for storing "transcrypt state"? Just add new git configuration variables with a transcrypt prefix?

Erotemic avatar Apr 27 '22 17:04 Erotemic

Another though that I had. Instead of creating a custom cipher scheme that we need to use awk to parse (which imo is somewhat awkward), would it make more sense to simply let the user pass custom options to openssl? Or should we lock specific options that are supported to prevent foot-shooting? In any case, I think the cipher variable should be left alone and we should use new state variables to store if we are using a KDF and any options to that KDF (I'm also thinking if we don't support custom options, maybe don't allow the user to change iters - at least in this PR for simplicity? If a user really wants that they will make a patch, and we can discuss it there).

Something like this:


# Configurable Sandalone OpenSSL Demo
password="12345"
kdf_options="-pbkdf2 -iter 1000"
cipher=aes-256-cbc
md_digest="sha256"
salt=deadbeaf00000bad

read -ra kdf_option_array <<<"${kdf_options}"
echo "secret_text" | SECRET_PASSWORD=${password} openssl enc -"${cipher}" -md "${md_digest}" "${kdf_option_array[@]}" -pass env:SECRET_PASSWORD -S "${salt}" -e -a 
echo "U2FsdGVkX1/erb6vAAALrcaDZJ0Ixb0qwGA4VBjTQS4=" | SECRET_PASSWORD=${password} openssl enc -"${cipher}" -md "${md_digest}" "${kdf_option_array[@]}" -pass env:SECRET_PASSWORD -d -a 

Erotemic avatar Apr 27 '22 17:04 Erotemic