From b721d870a3ff72265168b39df821e328ebeebe41 Mon Sep 17 00:00:00 2001 From: joncrall Date: Sun, 3 Jul 2022 15:54:01 -0400 Subject: [PATCH] Rework logic for simpler base-salt UX --- CHANGELOG.md | 2 +- contrib/zsh/_transcrypt | 2 +- tests/test_transcrypt.py | 168 +++++++++++++++++++++++++++++++-------- transcrypt | 137 ++++++++++++++++++++++--------- 4 files changed, 233 insertions(+), 76 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a6fbce6..bcfa0bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ The format is based on [Keep a Changelog][1], and this project adheres to - Add support for pbkdf2 - Add support for user specified digest -- Add support for new configured salt method +- Add support for new configured base salt - Add .transcrypt versioned directory - Support for OpenSSL 3.x - Add support for development editable install diff --git a/contrib/zsh/_transcrypt b/contrib/zsh/_transcrypt index 97f8939..9b0a3fa 100644 --- a/contrib/zsh/_transcrypt +++ b/contrib/zsh/_transcrypt @@ -12,7 +12,7 @@ _transcrypt() { '(- 1 *)'{-h,--help}'[view help message]' \ '(-c --cipher -d --display -f --flush-credentials -u --uninstall)'{-c,--cipher=}'[specify encryption cipher]:cipher:->cipher' \ '(-md --digest -d --display -f --flush-credentials -u --uninstall)'{-md,--digest=}'[specify encryption digest]:digest' \ - '(-sm --salt-method -d --display -f --flush-credentials -u --uninstall)'{-md,--digest=}'[specify salt-method]:salt-method' \ + '(-bs --base-salt -d --display -f --flush-credentials -u --uninstall)'{-md,--digest=}'[specify base-salt]:base-salt' \ '(-cs --config-salt -d --display -f --flush-credentials -u --uninstall)'{-md,--digest=}'[specify config-salt]:config-salt' \ '(-pbkdf2 --kdf -d --display -f --flush-credentials -u --uninstall)'{-md,--digest=}'[specify use-pbkdf2]:use-pbkdf2' \ '(-p --password -d --display -f --flush-credentials -u --uninstall)'{-p,--password=}'[specify encryption password]:password:' \ diff --git a/tests/test_transcrypt.py b/tests/test_transcrypt.py index 0dab255..470fd2d 100644 --- a/tests/test_transcrypt.py +++ b/tests/test_transcrypt.py @@ -84,14 +84,19 @@ def _cmd(self, command, shell=False, check=True, verbose=None): shell=shell, check=check) def _config_args(self): - arg_templates = [ - "-c", self.config['cipher'], - "-p", self.config['password'], - "-md", self.config['digest'], - "--kdf", self.config['kdf'], - "-bs", self.config['base_salt'], + flags_and_keys = [ + ('-c', 'cipher'), + ('-p', 'password'), + ('-md', 'digest'), + ('--kdf', 'kdf'), + ('-bs', 'base_salt'), ] - args = [template.format(**self.config) for template in arg_templates] + args = [] + for flag, key in flags_and_keys: + value = self.config[key] + if value is not None: + args.append(flag) + args.append(value) return args def is_configured(self): @@ -144,7 +149,8 @@ def version(self): return self._cmd(f'{self.transcript_exe} --version')['out'].rstrip() def _crypt_dir(self): - info = self._cmd('git config --local transcrypt.crypt-dir', check=0) + info = self._cmd('git config --local transcrypt.crypt-dir', check=0, + verbose=0) if info['err'] == 0: crypt_dpath = ub.Path(info['out'].strip()) else: @@ -198,13 +204,15 @@ def upgrade(self): return self._cmd(f'{self.transcript_exe} --upgrade -y') def _load_unversioned_config(self): + if self.verbose > 0: + print('Loading unversioned config') local_config = { - 'cipher': self._cmd('git config --get --local transcrypt.cipher')['out'].strip(), - 'digest': self._cmd('git config --get --local transcrypt.digest')['out'].strip(), - 'kdf': self._cmd('git config --get --local transcrypt.kdf')['out'].strip(), - 'base_salt': self._cmd('git config --get --local transcrypt.base-salt')['out'].strip(), - 'password': self._cmd('git config --get --local transcrypt.password')['out'].strip(), - 'openssl_path': self._cmd('git config --get --local transcrypt.openssl-path')['out'].strip(), + 'cipher': self._cmd('git config --get --local transcrypt.cipher', verbose=0)['out'].strip(), + 'digest': self._cmd('git config --get --local transcrypt.digest', verbose=0)['out'].strip(), + 'kdf': self._cmd('git config --get --local transcrypt.kdf', verbose=0)['out'].strip(), + 'base_salt': self._cmd('git config --get --local transcrypt.base-salt', verbose=0)['out'].strip(), + 'password': self._cmd('git config --get --local transcrypt.password', verbose=0)['out'].strip(), + 'openssl_path': self._cmd('git config --get --local transcrypt.openssl-path', verbose=0)['out'].strip(), } return local_config @@ -237,7 +245,7 @@ def setup(self): self._setup_gpghome() self._setup_gitrepo() self._setup_contents() - if self.verbose > 2: + if self.verbose > 1: self._show_manual_env_setup() return self @@ -314,6 +322,16 @@ def _show_manual_env_setup(self): class TestCases: """ Unit tests to be applied to different transcrypt configurations + + xdoctest -m tests/test_transcrypt.py TestCases + + Example: + >>> from test_transcrypt import * # NOQA + >>> self = TestCases(verbose=2) + >>> self.setup() + >>> self.sandbox._show_manual_env_setup() + >>> self.test_round_trip() + >>> self.test_export_gpg() """ def __init__(self, config=None, dpath=None, verbose=0): @@ -412,7 +430,7 @@ def test_legacy_defaults(): 'password': 'correct horse battery staple', 'digest': 'md5', 'kdf': 'none', - 'base_salt': 'password', + 'base_salt': '', } verbose = 1 self = TestCases(config=config, verbose=verbose) @@ -437,6 +455,10 @@ def test_secure_defaults(): def test_configured_salt_changes_on_rekey(): + """ + CommandLine: + xdoctest -m tests/test_transcrypt.py test_configured_salt_changes_on_rekey + """ config = { 'cipher': 'aes-256-cbc', 'password': 'correct horse battery staple', @@ -444,32 +466,88 @@ def test_configured_salt_changes_on_rekey(): 'kdf': 'pbkdf2', 'base_salt': 'random', } - verbose = 1 + verbose = 2 self = TestCases(config=config, verbose=verbose) self.setup() before_config = self.tc._load_unversioned_config() self.tc.rekey({'password': '12345', 'base_salt': ''}) self.sandbox.git.commit('-am commit rekey') after_config = self.tc._load_unversioned_config() - assert before_config['password'] != after_config['password'] + assert before_config['password'] != after_config['password'], 'password should have changed!' + assert before_config['base_salt'] != after_config['base_salt'], 'salt should have changed!' assert before_config['cipher'] == after_config['cipher'] assert before_config['kdf'] == after_config['kdf'] - assert before_config['base_salt'] == after_config['base_salt'] assert before_config['openssl_path'] == after_config['openssl_path'] +def test_unspecified_salt_without_kdf(): + """ + In this case the salt should default to the password method + """ + config = { + 'cipher': 'aes-256-cbc', + 'password': 'correct horse battery staple', + 'digest': 'sha512', + 'kdf': '', + 'base_salt': None, + } + verbose = 2 + self = TestCases(config=config, verbose=verbose) + self.setup() + config1 = self.tc._load_unversioned_config() + assert config1['base_salt'] == 'password' + + +def test_unspecified_salt_with_kdf(): + config = { + 'cipher': 'aes-256-cbc', + 'password': 'correct horse battery staple', + 'digest': 'sha512', + 'kdf': 'pbkdf2', + 'base_salt': None, + } + verbose = 1 + self = TestCases(config=config, verbose=verbose) + self.setup() + config1 = self.tc._load_unversioned_config() + assert len(config1['base_salt']) == 64 + + +def test_salt_changes_when_kdf_changes(): + config = { + 'cipher': 'aes-256-cbc', + 'password': 'correct horse battery staple', + 'digest': 'sha512', + 'kdf': '', + 'base_salt': None, + } + verbose = 2 + self = TestCases(config=config, verbose=verbose) + self.setup() + config1 = self.tc._load_unversioned_config() + assert config1['base_salt'] == 'password' + # Test rekey, base-salt should still be password + self.tc.rekey({'password': '12345'}) + config2 = self.tc._load_unversioned_config() + assert config2['base_salt'] == 'password' + self.sandbox.git.commit('-am commit rekey') + + # Test rekey with kdf=pbkdf2 base-salt should now randomize + self.tc.rekey({'password': '12345', 'kdf': 'pbkdf2', 'base_salt': None}) + config3 = self.tc._load_unversioned_config() + assert len(config3['base_salt']) == 64, 'should have had new random salt' + self.sandbox.git.commit('-am commit rekey') + + # Test rekey going back to no kdf + self.tc.rekey({'password': '12345', 'kdf': 'none', 'base_salt': None}) + config4 = self.tc._load_unversioned_config() + assert config4['base_salt'] == 'password' + + def test_configuration_grid(): """ CommandLine: xdoctest -m tests/test_transcrypt.py test_configuration_grid - - Example: - >>> from test_transcrypt import * # NOQA - >>> self = TestCases() - >>> self.setup() - >>> self.sandbox._show_manual_env_setup() - >>> self.test_round_trip() - >>> self.test_export_gpg() """ # Test that transcrypt works under a variety of config conditions basis = { @@ -477,28 +555,48 @@ def test_configuration_grid(): 'password': ['correct horse battery staple'], 'digest': ['md5', 'sha256'], 'kdf': ['none', 'pbkdf2'], - 'base_salt': ['password', 'random', 'mylittlecustomsalt'], + 'base_salt': ['password', 'random', 'mylittlecustomsalt', None], } + test_grid = list(ub.named_product(basis)) - verbose = 3 + + def validate_test_grid(params): + if params['kdf'] == 'none' and params['base_salt'] != 'password': + return False + if params['kdf'] != 'none' and params['base_salt'] == 'password': + return False + return True + + # Remove invalid configs + valid_test_grid = list(filter(validate_test_grid, test_grid)) + print('valid_test_grid = {}'.format(ub.repr2(valid_test_grid, sort=0, nl=1))) + + verbose = 2 dpath = 'special:temp' dpath = 'special:cache' - for params in ub.ProgIter(test_grid, desc='test configs', freq=1): + for params in ub.ProgIter(valid_test_grid, desc='test configs', freq=1, + verbose=verbose + 1): + if verbose: + print('\n\n') + print('=================') + print('params = {}'.format(ub.repr2(params, nl=1))) + print('=================') + config = params.copy() self = TestCases(config=config, dpath=dpath, verbose=verbose) self.setup() - if 1: - # Manual debug - self.sandbox._show_manual_env_setup() - self.test_round_trip() self.test_export_gpg() self.test_rekey() - + if verbose: + print('=================') if __name__ == '__main__': """ CommandLine: python tests/test_transcrypt.py + + # Runs everything + pytest tests/test_transcrypt.py -v """ test_configuration_grid() diff --git a/transcrypt b/transcrypt index 49a4d89..1eaa0b4 100755 --- a/transcrypt +++ b/transcrypt @@ -29,14 +29,13 @@ readonly VERSION='3.0.0-pre' readonly DEFAULT_CIPHER='aes-256-cbc' readonly DEFAULT_DIGEST='md5' readonly DEFAULT_KDF='none' -readonly DEFAULT_SALT_METHOD='password' # These are config variables we do not allow to be used in the versioned # configuration readonly VERSIONED_CONFIG_BLOCKLIST="transcrypt.password transcrypt.openssl-path" # Set to 1 to enable a development editable installation -readonly EDITABLE_INSTALL=${TRANSCRYPT_EDITABLE_INSTALL:=0} +readonly EDITABLE_INSTALL=${TRANSCRYPT_EDITABLE_INSTALL:=1} # Proposed option: can remove or disable later readonly USE_VERSIONED_CONFIG="0" @@ -241,7 +240,6 @@ _load_transcrypt_config_vars() { base_salt=$(_load_config_var "transcrypt.base-salt") || (echo "failed to load transcrypt.base-salt" && false) openssl_path=$(_load_config_var "transcrypt.openssl-path") || (echo "failed to load transcrypt.openssl-path" && false) password=$(_load_unversioned_config_var transcrypt.password) || (echo "failed to load transcrypt.password" && false) - ensure_base_salt validate_kdf || die "invalid value of kdf in config" validate_digest || die "invalid value of digest in config" } @@ -570,7 +568,15 @@ validate_kdf() { } validate_base_salt() { - true + if [[ "$kdf" != "none" ]]; then + if [[ "$base_salt" == "" ]]; then + die 1 'The base salt is required when using a kdf, but it is unspecified.' + elif [[ "$base_salt" == "random" ]]; then + die 1 'The base=salt is set to random, but it has not been randomized'. + elif [[ "$base_salt" == "password" ]]; then + die 1 'Using base-salt="password" method with a KDF is insecure. Use "random" instead'. + fi + fi } # ensure we have a digest to hash the salted password @@ -579,7 +585,6 @@ get_digest() { prompt=$(printf 'Encrypt using which digest? [%s] ' "$DEFAULT_DIGEST") if [[ "$digest" == "" ]]; then digest=$(_load_versioned_config_var "transcrypt.digest" || echo "") - # echo "Loaded digest = $digest from local config" fi _get_user_input digest "$DEFAULT_DIGEST" "validate_digest" "$prompt" } @@ -590,7 +595,6 @@ get_cipher() { prompt=$(printf 'Encrypt using which cipher? [%s] ' "$DEFAULT_CIPHER") if [[ "$cipher" == "" ]]; then cipher=$(_load_versioned_config_var "transcrypt.cipher" || echo "") - # echo "Loaded cipher = $cipher from local config" fi _get_user_input cipher "$DEFAULT_CIPHER" "validate_cipher" "$prompt" } @@ -600,20 +604,91 @@ get_kdf() { prompt=$(printf 'Which key derivation function? [%s] ' "$DEFAULT_KDF") if [[ "$kdf" == "" ]]; then kdf=$(_load_versioned_config_var "transcrypt.kdf" || echo "") - # echo "Loaded kdf = $kdf from local config" fi _get_user_input kdf "$DEFAULT_KDF" "validate_kdf" "$prompt" } get_base_salt() { - local prompt - prompt=$(printf 'Which base salt? [%s] ' "$DEFAULT_SALT_METHOD") - if [[ "$base_salt" == "" ]]; then - base_salt=$(_load_versioned_config_var "transcrypt.base-salt" || echo "") - # echo "Loaded base_salt = $base_salt from local config" - fi - _get_user_input base_salt "$DEFAULT_SALT_METHOD" "validate_base_salt" "$prompt" - ensure_base_salt + __doc__=" + +-----------+-------------+------------------+--------------------+------------------------+ + | Action | Using_KDF | BaseSaltInArgv | BaseSaltInConfig | Outcome | + |-----------+-------------+------------------+--------------------+------------------------| + | configure | True | True | True | Use Argv and Overwrite | + | configure | True | True | False | Use Argv and Overwrite | + | configure | True | False | True | Use Config Value | + | configure | True | False | False | Randomize | + | configure | False | True | True | Ignored | + | configure | False | True | False | Ignored | + | configure | False | False | True | Ignored | + | configure | False | False | False | Ignored | + +-----------+-------------+------------------+--------------------+------------------------+ + +----------+-------------+------------------+--------------------+------------------------+ + | Action | Using_KDF | BaseSaltInArgv | BaseSaltInConfig | Outcome | + |----------+-------------+------------------+--------------------+------------------------| + | rekey | True | True | True | Use Argv and Overwrite | + | rekey | True | True | False | Use Argv and Overwrite | + | rekey | True | False | True | Randomize | + | rekey | True | False | False | Randomize | + | rekey | False | True | True | Ignored | + | rekey | False | True | False | Ignored | + | rekey | False | False | True | Ignored | + | rekey | False | False | False | Ignored | + +----------+-------------+------------------+--------------------+------------------------+ + + +---------------+-------------+--------------------+------------------+ + | Action | Using_KDF | BaseSaltInConfig | Outcome | + |---------------+-------------+--------------------+------------------| + | helper-script | True | True | Use Config Value | + | helper-script | True | False | Error | + | helper-script | False | True | Ignored | + | helper-script | False | False | Ignored | + +---------------+-------------+--------------------+------------------+ + + # Developer notes to help enumerate cases + import pandas as pd + import ubelt as ub + df = pd.DataFrame(list(ub.named_product({ + 'Action': ['configure', 'rekey', 'helper-script'], + 'Using_KDF': [True, False], + 'BaseSaltInArgv': [True, False], + 'BaseSaltInConfig': [True, False], + }))) + df.loc[~df.Using_KDF, 'Outcome'] = 'Ignored' + df.loc[(df.Action == 'helper-script') & df.BaseSaltInArgv, 'Outcome'] = 'Impossible' + df.loc[df.Using_KDF & (df.Action == 'configure') & df.BaseSaltInArgv, 'Outcome'] = 'Use Argv and Overwrite' + df.loc[df.Using_KDF & (df.Action == 'configure') & ~df.BaseSaltInArgv * ~df.BaseSaltInConfig, 'Outcome'] = 'Randomize' + df.loc[df.Using_KDF & (df.Action == 'configure') & ~df.BaseSaltInArgv * df.BaseSaltInConfig, 'Outcome'] = 'Use Config Value' + df.loc[df.Using_KDF & (df.Action == 'rekey') & df.BaseSaltInArgv, 'Outcome'] = 'Use Argv and Overwrite' + df.loc[df.Using_KDF & (df.Action == 'rekey') & ~df.BaseSaltInArgv, 'Outcome'] = 'Randomize' + df.loc[df.Using_KDF & (df.Action == 'helper-script') & ~df.BaseSaltInArgv & df.BaseSaltInConfig, 'Outcome'] = 'Use Config Value' + df.loc[df.Using_KDF & (df.Action == 'helper-script') & ~df.BaseSaltInArgv & ~df.BaseSaltInConfig, 'Outcome'] = 'Error' + for key, group in df.groupby('Action'): + print(group.to_markdown(index=0, tablefmt='psql')) + " + # When calling `get_base_salt`, the action is always configure or rekey. + if [[ "$kdf" == "none" ]]; then + if [[ "$base_salt" == "" ]]; then + base_salt="password" + fi + else + if [[ "$base_salt" == "" ]]; then + # The user did not specify a base salt. + if [[ $rekey ]]; then + base_salt="random" + else + base_salt=$(_load_versioned_config_var "transcrypt.base-salt" || echo "random") + fi + else + # The user specified something for base-salt + if [[ "$base_salt" == "password" ]]; then + die 1 'Using the "password" method with a KDF is insecure. Use "random" instead'. + fi + fi + # Randomize the base salt if requested. + if [[ "$base_salt" == "random" ]]; then + base_salt=$(openssl rand -hex 32) + fi + fi } # ensure we have a password to encrypt with @@ -641,20 +716,6 @@ get_password() { done } -ensure_base_salt() { - # Check if randomized salt needs to be written - if [[ "$base_salt" == "random" ]]; then - # Replace random with something random. - # If we have not configured the base_salt (or we need to rekey), - # then generate new random salt - base_salt=$(openssl rand -hex 32) - fi - if [[ $rekey ]] && [[ $base_salt != "password" ]]; then - # Assume we want a new random salt unless we are explicitly using password - base_salt=$(openssl rand -hex 32) - fi -} - # confirm the transcrypt configuration confirm_configuration() { local answer= @@ -815,10 +876,7 @@ _display_git_configuration() { _display_runtime_configuration() { printf ' DIGEST: %s\n' "$digest" printf ' KDF: %s\n' "$kdf" - printf ' SALT_METHOD: %s\n' "$base_salt" - if [[ "$base_salt" == "configured" ]]; then - printf ' CONFIG_SALT: %s\n' "$base_salt" - fi + printf ' BASE_SALT: %s\n' "$base_salt" printf ' CIPHER: %s\n' "$cipher" printf ' PASSWORD: %s\n\n' "$password" } @@ -1192,19 +1250,20 @@ help() { -md, --digest=DIGEST the digest used to hash the salted password; - defaults to md5 + defaults to md5. It is strongly recommended to use + a stronger hash (e.g. sha256) if possible. --kdf=PBKDF2 the key-derivation-function to use. Currently can be either - 'pbkdf2' or 'none'. Defaults to none. + 'pbkdf2' or 'none'. Defaults to none. It is strongly + recommended to use a kdf (e.g. pbkdf2) if possible. -pbkdf2 equivalent to passing --kdf2='pbkdf2' - -bs, --base-salt=SALT_METHOD - Method used to compute deterministic salt; can be 'password', 'random', - or a custom string to be used as the salt. Unless set to password, - the salt is randomized on a rekey. + -bs, --base-salt=BASE_SALT + if specified, and a KDF is used, this overrides the text + used as the basis for salt-generation. --set-openssl-path=PATH_TO_OPENSSL use OpenSSL at this path; defaults to 'openssl' in \$PATH