dyne
Improve KDF handling
Previously it wasn't possible to use argon2 as KDF function without the tomb tools from extras/kdf-keys being available. To change that behaviour introduce checks on the ARGON2 variable. Additionally add a fallback function to create a salt that is compatible to tomb-kdb-pbkdf2-gensalt.
Options specific for the different supported KDF algorithm are reorganized. Some options align between the various KDF and some are unique to them. The output of -h is enhanced with the various --kdf options and depends on the available optional tools. argon2 specific cli arguments won't be displayed if argon2 is not available.
Add case for results beside argon2 and pbkdf2. Key creation won't be stopped, just a warning is issued that the resulting key won't be protected via KDF.
Regarding the cli options. The argument for the suboption --kdf is made optional. In that regard one needs to make sure, that if --kdf is the last option before an argument one needs to use - to separate or use -k. Example: tomb forge --kdf - testkey.tomb Example: tomb forge --kdf -k testkey.tomb Example: tomb forge -k testkey.tomb --kdf
Additonally the kdf options are reorganized, which is a possible breaking change for scripts or GUI helpers.
- --kdftype is changed to --kdf
- --kdfiter is introduced as replacement the for previous --kdf definition
- --kdfpar is introduced to support the parallelism option of argon2
Only --kdf is mandatory to get a key which is protected with KDF. For every other option safe defaults are set and can be optionally adjusted. KDF related subcommand options are removed where they don't come into play. gen_key() is only called in forge and passwd.
Description and such will be added if the path forward is clear. This is a WIP draft of the things mentioned in #526
Currently done: Make ARGON2 useful for the usage().
The general change in behaviour is implemented. Adjustment of docs and tests are next on the agenda. Although I saw the CI also failed for the weblate update? Fails the CI in general? I don't really see a descriptive error from those runs.
And more thoroughly testing.
Currently testing was done with the holy trinity of dig, forge and lock in all variations (argon2 only, argon2 and pbkdf2 tools available, pbkdf2, pbkdf2 without tomb-kdb-pbkdf2-gensalt).
the tests should not fail in general. I see the problem here https://github.com/dyne/tomb/actions/runs/10100450578/job/27931822219?pr=527#step:6:706
Okay. Hints at least at the expected location if it fails at the kdf tests. But why does it at also happen at the same stage for the weblatePR?
Many thanks! I've just solved conflicts with other merged PRs, upcoming release will include this very useful improvement!
The problem with this is that --kdf needs an argument. I think the built-in option parsing in zsh (zparseopts) does not support the "default or optional values" mechanism. I am not sure why you put -kdf:: in the option declaration, maybe those double colons activate it in more recent zsh versions? else we need to change and make the value required.
It doesn't seem to be widely known (it took me a while to find a hint when I initially wrote the changes), but zsh does support optional arguments: https://zsh.sourceforge.io/Doc/Release/Zsh-Modules.html#index-zparseopts
If one or two colons are given, the option takes an argument; with one colon, the argument is mandatory and with two colons it is optional.
And it seems to exists since a long time. I couldn't find out since when, but it was mentioned in a doc change from January 2016.
commit 8ddcdad0c1559612b72b733e907d10806415d4fb
Author: Barton E. Schaefer <[email protected]>
Date: Tue Jan 26 22:44:14 2016 -0800
37802: Clarify zparseopts documention. zparseopts "X::" specifier
should also look for the optional argument in the word after "-X".
Which made it into zsh 5.3 (December 2016). But it was introduced earlier.
So it shouldn't be an issue with more recent zsh versions.
What seems to be finicky, is the recognition if there is something optional. Therefore the explicit explanatory part from the commit message (which has some grammatical issues as I now noticed...):
Regarding the cli options. The argument for the suboption --kdf is made optional. In that regard one needs to make sure, that if --kdf is the last option before an argument one needs to use - to separate or use -k.
Example: tomb forge --kdf - testkey.tomb
Example: tomb forge --kdf -k testkey.tomb
Example: tomb forge -k testkey.tomb --kdf
From a UX point of view, it would be nicer, if it were to be an optional parameter. Without reading into it you don't need to know what parameter are possible, but you can directly use the defaults the script advocates.
Didn't look at the failures yet, but I assume it is because of argument ordering and got something to parse as optional argument. It could pass the tests before. Will take a look.
I am not 100% sure is better UX, at least it got me tripping. I'd prefer --kdf to always need a parameter, or not, because explaining the special rules to sort options in the synopsis make me feel uncomfortable..
the test fails here
when --kdf is the last option
With the current changes available in this PR, the part were the argument for --kdf is made optional is missing. That could explain the failing test.
Will force push the original intent, just to see if the CI succeeds again. Locally went through without an issue.
But as you're not entirely convinced on the optional part, I will add a commit which will reintroduce it as required.
Okay, theoretically works. Checks went through.
Edit: https://github.com/dyne/tomb/actions/runs/11601340887/job/32303835038?pr=527
Great! It is time to merge this, as it improves the KDF handling. Also, the mandatory argument now makes people more aware that argon2 is an option (the best one IMO)