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

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jmurty
Copy link
Collaborator

@jmurty jmurty commented Aug 31, 2021

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
  • maybe use something other than MD5 for hashing the passphrase with -md MD5, upgrade to -md sha512?

…c:pbkdf2:1024'

- set default cipher to 'aes-256-cbc:pbkdf2:1024'
- relevant code paths can split compound cipher value into components:
  cipher, key derivation function, iterations
- prevent `transcrypt.openssl-path` setting getting clobbered on --rekey
Re-key example sensitive_file with command:

    ./transcrypt --rekey \
        -c 'aes-256-cbc:pbkdf2:1024' \
        -p 'correct horse battery staple' \
        --yes
SC2155: Declare and assign separately to avoid masking return values.
@jmurty jmurty requested a review from elasticdog August 31, 2021 14:18
Re-key example sensitive_file with command:

    ./transcrypt --rekey \
        -c 'aes-256-cbc:pbkdf2:1024' \
        -p 'correct horse battery staple' \
        --yes
@brianmay
Copy link

brianmay commented Mar 6, 2022

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?

@jmurty
Copy link
Collaborator Author

jmurty commented Mar 7, 2022

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

@Erotemic
Copy link

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.

@jmurty
Copy link
Collaborator Author

jmurty commented Apr 27, 2022

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.

@Erotemic
Copy link

@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
Copy link

Erotemic commented Apr 27, 2022

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 

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

Successfully merging this pull request may close these issues.

None yet

3 participants