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

Improve KDF handling #527

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Improve KDF handling #527

wants to merge 6 commits into from

Conversation

Narrat
Copy link
Collaborator

@Narrat Narrat commented Jul 24, 2024

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().

@Narrat
Copy link
Collaborator Author

Narrat commented Jul 25, 2024

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).

@jaromil
Copy link
Member

jaromil commented Jul 25, 2024

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

@Narrat
Copy link
Collaborator Author

Narrat commented Jul 25, 2024

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?

@Narrat
Copy link
Collaborator Author

Narrat commented Jul 28, 2024

Tests are adjusted, so this PR should be generally ready for review

@jaromil
Copy link
Member

jaromil commented Oct 30, 2024

Many thanks! I've just solved conflicts with other merged PRs, upcoming release will include this very useful improvement!

@jaromil
Copy link
Member

jaromil commented Oct 30, 2024

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.

@Narrat
Copy link
Collaborator Author

Narrat commented Oct 30, 2024

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 <schaefer@zsh.org>
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.

@jaromil
Copy link
Member

jaromil commented Oct 30, 2024

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
image
when --kdf is the last option

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 --kdf is the last option before an argument. Or - to separate. Third option would be use -k to specify the keyname.
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 (nice to have if someone wants to adjust memory or iteration costs without increasing the time that much)
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.

Closes dyne#526
The check if the argument of --kdfiter is an integer is unnecessary.
Neither argon2 nor tomb-kdb-pbkdf2-getiter care about float input.
rename original kdf test accordingly to be pbkdf2 related.
@Narrat
Copy link
Collaborator Author

Narrat commented Oct 30, 2024

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.

@Narrat
Copy link
Collaborator Author

Narrat commented Oct 30, 2024

Okay, theoretically works.
Checks went through.

Edit: https://github.com/dyne/tomb/actions/runs/11601340887/job/32303835038?pr=527

While possible to make the argument optional, it introduced some wonkyness.
It kinda required a fixed ordering of arguments then, which wasn't needed before.
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.

2 participants