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 documentation for the version (v) parameter #4

Merged
merged 2 commits into from
Apr 17, 2021

Conversation

simonepri
Copy link

@simonepri simonepri commented Apr 2, 2018

@@ -279,6 +286,8 @@ For Argon2, the following is specified:

- The identifier for Argon2ds is `argon2ds`.

- The versions are: [16, 19].
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be clearer to use 0x10, 0x13 or 16 (for Argon2 1.0), 19 (for Argon2 1.3).

Copy link
Author

@simonepri simonepri Apr 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it makes sense.
Actually I think we should completely remove the Argon2 Encoding section.
It does not make a lot of sense to have it here.

@simonepri
Copy link
Author

simonepri commented Apr 2, 2018

More changes to discuss about:

  1. Remove Argon2 Encoding section.
  2. Replace the current readme with the specs. The readme is pointless.
  3. Add references to implemented parsers like phc-format.
  4. Add explaination on why salt and secret can be optional. (Why hash and salt are both optional? #3)

What do you think about them?

phc-sf-spec.md Outdated
@@ -35,6 +36,7 @@ The string is then the concatenation, in that order, of:

- a `$` sign;
- the function symbolic name;
- optionally, a `$` sign followed by the algortihm version with a `v=version` format;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a typo: algortihm -> algorithm

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch thank you!

@@ -48,6 +50,8 @@ case of a function family); identifiers should be explicit (human
readable, not a single digit), with a length of about 5 to 10
characters. An identifier name MUST NOT exceed 32 characters in length.

The value for the version shall be a sequence of characters in: `[0-9]`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering Argon2 currently uses hex numbers, technically it's [0-9A-Fa-f] or [[:xdigit:]] class.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the values are decimal encoded as per specs.
Eg:
0x13->19
0x10->16
So the version is digits only.

@simonepri
Copy link
Author

simonepri commented Jun 7, 2018

Should we just create a new "official" repository for that instead of relying on this one?
The team definitely doesn't care about it anymore.

@dimaqq
Copy link

dimaqq commented Aug 23, 2019

Ugh, the whole version business is ugly.
IMO, if the algorithm implementation changed, just s/argon2/argon3 so that the older version can be one day deprecated.

@@ -21,11 +21,12 @@ string that differs from the format herein described.

We define the following format:

$<id>[$<param>=<value>(,<param>=<value>)*][$<salt>[$<hash>]]
$<id>[$v=<version>][$<param>=<value>(,<param>=<value>)*][$<salt>[$<hash>]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a somewhat ambiguous case:

$<id>[$v=<version>](,<param>=<value>)+][$<salt>[$<hash>]]

It'd be good to clarify what behavior parsers have for this case:

  • MUST NOT interpret the v parameter as a version
  • MUST interpret the v parameter as a version

Since a separated [$v=<version>]$... parameter is already in the wild, I would suggest MUST NOT here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it helps, but you can refer to this implementation:
https://github.com/simonepri/phc-format/blob/master/index.js#L136-L224

If I remember correctly, in the case you mentioned my parser does not interpret v as the version but as a parameter.

Since a separated [$v=]$... parameter is already in the wild

What are you referring to exactly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm referring to Argon2 having deployed $v=...$<params>, versus $v=...,<params>

tarcieri added a commit to RustCrypto/traits that referenced this pull request Jan 6, 2021
Support for PHC string variant with an additional version field:

P-H-C/phc-string-format#4

This is used by Argon2.
tarcieri added a commit to RustCrypto/traits that referenced this pull request Jan 6, 2021
Support for PHC string variant with an additional version field:

P-H-C/phc-string-format#4

This is used by Argon2.
tarcieri added a commit to RustCrypto/traits that referenced this pull request Jan 6, 2021
Support for PHC string variant with an additional version field:

P-H-C/phc-string-format#4

This is used by Argon2.
@tarcieri
Copy link
Contributor

tarcieri commented Apr 17, 2021

So I notice that as a panel member I can merge this.

I'll give it a more thorough review, but perhaps I should. Getting it merged seems very much overdue.

@khovratovich
Copy link
Member

khovratovich commented Apr 17, 2021 via email

@tarcieri tarcieri merged commit cf415da into P-H-C:master Apr 17, 2021
@tarcieri
Copy link
Contributor

Sorry that took so long.

FYI I'm maintaining an argon2 Rust crate which is using this repo for documentation. I'm happy to help review such PRs in the future and hopefully as such they'll get merged sooner than 3 years. 😉

tarcieri added a commit to RustCrypto/traits that referenced this pull request Apr 17, 2021
tarcieri added a commit to RustCrypto/traits that referenced this pull request Apr 17, 2021
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.

6 participants