-
Notifications
You must be signed in to change notification settings - Fork 104
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
Alternative enhancement #136
base: main
Are you sure you want to change the base?
Conversation
Thanks for the simplified PR. I'm aiming to get a new 2.2.0 release with the fairly urgent OpenSSL v3 fix out to clear the way for this change which I think may be large enough to warrant a The main block on getting this merged is for myself or @elasticdog to have sufficient time available. I can't promise when this will happen, but after we've cut the 2.2.0 release I will be willing to merge to the Thanks for your patience; we'll get there. |
Hi @Erotemic I have updated this branch with changes, mostly focussed on getting the existing BATS tests to pass to give us a solid base to work from. I haven't quite managed it yet, but it's getting close |
I do have a bit more work that needs to be done on this branch. Specifically, I need to rename the arguments used in the program to the arguments I've described above. I agree that getting 2.2.0 out with the SALTED changes is by far the most important thing. Keep me updated with status on that. |
1e4173f
to
eac308b
Compare
I squashed everything down to a single commit in order to make rebases nicer. I also change the "--use-pbkdf2" arg to simply "kdf", which has a much better UX feel. I decided it's probably best to keep "--salt_method" as-is, so the user doesn't think that the salt they specify is actually the "final_salt" to be used in encryption. Also there already was an existing "-s" option, so I wanted to keep that distinguished. If this is the version we go with, I'll need to clean up the writeup to target this specific implementation (which should basically be removing pieces because this is simpler). |
Hi @Erotemic I have pushed some updates to get the existing BATS tests to pass for all platforms. With these tests now passing I'm more confident we can progress the feature changes in this PR without breaking things. I am finding the salt method configuration and storage confusing, the Lines 257 to 264 in 61a352d
I think we can simplify this as follows to reduce the number of configuration keys and variables in the code:
Does this make sense, or am I missing something? |
Yes, the salt-method currently is a bit weird. I did that to reduce the parameter space, but I think it's worth revisiting. I like the idea of disallowing the password mode when a KDF is enabled. I also like the idea of storing the real value we are going to use in the config as "base-salt". One important use-case that I'm not sure this covers is the one of "plausible deniability" if the user does not want to check in a "versioned" .transcrypt config so they would have to explicitly specify all the parameters to the repo at configuration time: e.g.
This way there is no indication that transcrypt was used to encrypt the files (I suppose other than the fact that all the encrypted files will start with "salted", so maybe this isn't that important). I think it's also a TODO on my part to enable the user to disable the versioned config. I also think that the logic to conditionally skip prompting for a salt method is somewhat inelegant. How about this modification?
The only thing I'm unsure about in the above scheme is what happens on a rekey. My thought was: check if the salt-method is "random", and if so always regenerate a new base-salt. But if it is explicitly specified, then just use that. But I also want to make it easy for the user to configure a repo with a single command that specifies the entire configuration. My thought is maybe if "salt-method" is prefixed with "random:" then it signals so store "random" in the salt-method and the base-salt should be whatever is after the prefix on this configuration, so when you rekey you do get a randomized salt. But on the other hand, we might just force that the user always randomizes the salt on a rekey for security (which is what it currently does). So the above case single-line configuration would be:
Thoughts? |
I think a rekey should regenerate a new Overall I still don't see the point of prompting for or storing a salt method. Is there a scenario not covered in the table below where we need to know the salting method?
|
Yes: the case where the user wants to keep their base salt on a rekey. We need to know if the base-salt was generated via a random process or if the user explicitly set it to that. But we could always not support that case. It does weaken the security. |
We should definitely discourage keeping the same base salt on a rekey. In this case I don't think we need to know whether an existing base salt was randomly or explicitly set, just whether the user wants to keep it. For an interactive rekey we could prompt to keep the existing base salt – with No as the default – then let them enter a new base salt or leave blank to generate new a random one. For a non-interactive rekey, accept a new argument like |
Though I'm very tempted to always regenerate the base salt on a rekey, and add preservation of the base salt only if/when someone actually asks for it. As you say it weakens security, and it complicates the UI and our code for an unlikely and ill-advised use-case. |
Catch validation errors on CLI Remove bad echo Update references to obsolete argument --use-pbdkf2 to --kdf
`--kdf` default is now 'none' not '0' `--md` default is now 'md5' not 'MD5'
- Use `:+` bash parameter expansion to include pbkdf2 argument to openssl only if variable is set - Simplify variable `pbkdf2_arg` from list to string, since the `[@]` referencing doesn't work in all cases for MacOS (at least not for the unit tests): an empty list errors with `unbound variable`
61a352d
to
b721d87
Compare
Hi @Erotemic I have just released version 2.2.0 of Transcrypt, so the main branch is now clearer to prepare the improvements in this branch for merging. I'd like to focus on the KDF/PBKDF2 key derivation function (and base salt) first, since these are the most important changes. I'd like to focus on the usability of the new arguments, add some explanations for what the settings mean at important points (e.g. the interactive config step), and make sure it's as easy as possible to use transcrypt in the new and best way: with PBKDF2 and SHA256. Some initial thoughts/questions:
By the way, I don't expect you to make all these changes, I'll work on them too. This list ☝️ is mainly a brain-dump of things I plan to start on. Although I think it makes sense to keep the versioned config feature in this branch alongside the KDF changes, I expect I will merge just the KDF changes first, then follow up on the versioned config feature in a second pass. |
We don't have algorithm-specific shorthands for other things like cipher, and if transcrypt will support multiple potential KDFs it will be unwieldy to add arguments for every one.
On Versioned ConfigsSo your thought is that we will store the base salt and everything else locally without the versioned config, which adds in all of those features, but just forces the user to copy-paste rather long commands to initialize new clones (as there is no other mechanism for passing around the salt value)? I think that's fine in terms of splitting up the PR, but I just want to make sure you're not thinking about releasing transcrypt without the option for a versioned config. Ok KDFsI like removing -pbkdf2 and using -k. I'm all for removing 1 and 0, they were really placeholders anyway to make getting the proof-of-concept out easier.
I strongly recommend against removing kdf=none. I'm ok changing it to Also, using the absence of On SaltI don't have a problem with using 16 instead of 32 characters. It was an arbitrary choice, and I don't think it is a significant reduction in any security. We might want to bump it up a little from 16, maybe to 20 or 24, because unlike openssl we aren't choosing a random salt each time, we are reusing it. It might make sense to push up the entropy here just in case. I don't have any theoretical justification for this, it's just a feel-fact. On DefaultsI'm all for using sha256 and pbkdf2 as defaults... but I'm not sure about the UX for people already using legacy settings. If they upgrade transcrypt and try to initialize their legacy repo does that work? The salt change isn't a big deal, although every file in the repo will show up as having a diff, but the kdf change will not correctly decrypt the data unless the user who is upgrading realizes that their has been a change they need to account for (which 9 / 10 times they will not). I think we need to write a few test cases that test how the program works through the upgrade process from 2.x to 3.x. I don't have those in my python test suite right now, but I can put some effort into adding them. Not sure how easy it would be to add those cases to the bats suite. My thought is that the first release of 3.0 should contain all of these options, but not modify any defaults. It could emit warnings to user to give them a heads up that they are using legacy settings as well as instruction on how to upgrade them and that defaults will change in the future. I think a 3.1 release could then switch the defaults with minimal disruption. Note, if we do release a version with kdf, digest, and salt without the versioned config, then we should certainly not change the defaults in that case. But once the versioned config is available, I think there is a path for sensible default upgrades. |
Quick clarifications:
|
So, it doesn't seem to be straightforward to switch between the underlying kdfs. It would be nice to get an opinion about the salt length from someone who really knows what they are doing wrt to cryptography. I know it's best to use a unique salt for each file --- and we effectively do by mixing in the hash of the file --- but I don't have a sense for if lengthening the salt provides any extra security that could make up for the fact that there is this public content that contributes to the security of all data in the repo. I also think we shouldn't worry too much about it. Using a salt of length 16 is probably fine, and there is nothing stopping the user from specifying a custom base-salt that is longer. It might be best to just change it to 16 and leave this as a known open question. Again on the key/valueI'm going to argue again in favor of reserving some value for kdf that indicates it is not used (I don't care what it is). The "--option" and "--no-option" makes sense in the case where option takes on a binary value. For things that could take on more than one value (we would like to be able to say (Note: previously I said I could live with |
FYI: I noticed I accidentally reverted the fix in #119. That is back in now. |
… be the same as using it if it exists)
I want to note that I've been using this in my day-to-day, and it does seem that there is an issue with it that should be resolved before merging. When I checkout different branches, even though they were encrypted with this branch, it complains that there would be conflicts in the encrypted files (even though in the plaintext version of them there isn't). That means that there is some non-deterministic or different thing happening either between machines (where perhaps I have different version of openssl installed) or on the same machine. I haven't localized it yet. Just noting it here for reference. |
Hi @Erotemic thanks for noting those cross-machine issues. Can you note down anything that might help to debug these kinds of problems, like the platform and OpenSSL versions on the different machines? |
They were both Ubuntu machines. I think one is 22.04 and the other is 20.04, so they come with their respective versions of openssl. Should be fairly simple to fire up a few docker containers to test this. |
I made a small proof-of-concept file: tests/test_multi_docker.py This ports the docker-management file I think this is exposing some issues in this branch, and I'll have to nail those down... eventually. |
In excellent news, it looks like the latest version of MacOS 13 Ventura includes a newer version of LibreSSL (3.3.6) with an Using the built-in MacOS version of $ /usr/bin/openssl version
LibreSSL 3.3.6
# Encrypt with PBKDF2, 2500 iterations
$ echo "TEST" | ENC_PASS=abc123 /usr/bin/openssl enc -e -a -aes-256-cbc -md sha256 \
-S deadbeef00000000 -pass env:ENC_PASS -pbkdf2 -iter 2500
U2FsdGVkX1/erb7vAAAAAN4Q77gUNYGeQo3tucwenVM=
# Decrypt with PBKDF2, 2500 iterations
$ echo U2FsdGVkX1/erb7vAAAAAN4Q77gUNYGeQo3tucwenVM= \
| ENC_PASS=abc123 /usr/bin/openssl enc -d -a -aes-256-cbc -md sha256 \
-S deadbeef00000000 -pass env:ENC_PASS -pbkdf2 -iter 2500
TEST The MacOS / LibreSSL version is also compatible with OpenSSL version 1.1: $ echo "TEST" | ENC_PASS=abc123 /opt/homebrew/opt/openssl@1.1/bin/openssl enc -e -a -aes-256-cbc -md sha256 \
-S deadbeef00000000 -pass env:ENC_PASS -pbkdf2 -iter 2500
U2FsdGVkX1/erb7vAAAAAN4Q77gUNYGeQo3tucwenVM= And with OpenSSL version 3, once our (ugly) work-around is applied to include the salted prefix that version 3 stopped including (see #133): # OpenSSL v3+ without salt prefix DOES NOT MATCH
$ echo "TEST" | ENC_PASS=abc123 /opt/homebrew/opt/openssl@3/bin/openssl enc -e -a -aes-256-cbc -md sha256 \
-S deadbeef00000000 -pass env:ENC_PASS -pbkdf2 -iter 2500
3hDvuBQ1gZ5Cje25zB6dUw==
# OpenSSL v3+ WITH salt prefix work-around does match
$ (
printf "Salted__" && printf "%s" "deadbeef00000000" | xxd -r -p && \
echo "TEST" | ENC_PASS=abc123 /opt/homebrew/opt/openssl@3/bin/openssl enc -e -aes-256-cbc -md sha256 \
-S deadbeef00000000 -pass env:ENC_PASS -pbkdf2 -iter 2500
) | /opt/homebrew/opt/openssl@3/bin/openssl base64
U2FsdGVkX1/erb7vAAAAAN4Q77gUNYGeQo3tucwenVM= Annoyingly our work-around for OpenSSL v3 no longer including the salt prefix is probably broken on MacOS Ventura, because LibreSSL's [Edit: LibreSSL vs OpenSSL version checking is fixed via #147] |
As an example of the kinds of compatibility problems we're facing that I worried about in #134 (comment), I just wasted some time trying to get the BATS tests to pass for this PR's code using the LibreSSL 3+ The stumbling block was in trying to get a list of available digests from LibreSSL, which:
I'm not sure how best to work around the faulty error code result to detect failed commands?
|
Hi @Erotemic I just created (yet another) PR for the PBKDF2 work: #162 I have made some time recently to push the PBKDF2 improvements ahead, and thanks to your work here I have made a lot of progress. I started a new PR instead of building on this one so I could build things up slowly, keep everything in my head as I went, and make the smallest number of changes possible to get things working. I have been lifting your code pretty liberally across to the new PR, but only the pieces I need as I need them. A key difference in the new PR is how the base salt is handled. I think I've come up with an easier and strong-enough way of doing this so we don't need to store or prompt for a long salt value: the base (called "project" in the new PR) salt is derived from the password using the same PBKDF2 settings as all other files, to make the partial hash value used in per-file salts just as difficult to brute-force as any other file in the repo. I describe this in more detail in the PR comment section Deterministic salting for PBKDF2 (Feedback needed). I'd really appreciate your thoughts on this approach. |
This is an alternative to #132 that aims to be simpler. The new CLI args are:
By default these will always write a new
.transcrypt/config
file that is thengit add
-ed to the repo. In my own experience it is a pain to have to re-enter these non-secret data that could instead be stored in plaintext in the repo. We should also make an option to optionally disable it, but I think 99% of users benefit.The priority of loading a variable is:
The exception is
transcrypt.password
andtranscrypt.openss-path
which are never saved or loaded from the.transcrypt/config
file (only the unversioned.git/config
file). The other behavior of note is that when--salt=random
, the program will replace it with a random value before the main logic starts.@elasticdog @jmurty The other posts and writeup I've made haven't generated much discussion. I think enhancements along this line are a clear win, and I'd like to start recommending transcrypt to people, so it would be helpful if I knew what it would take to get this merged here. Otherwise I can continue using my fork.
This also adds:
TRANSCRYPT_TRACE
- if set to 1, willset -x
to help with debuggingEDITABLE_INSTALL
- if set to 1, will symlink transcrypt into the git repo instead of copying it. Also helpful for debugging.