"salt" option property does not work

[Jazza-231]

I have two guesses: it is a bug, or it was not marked as deprecated in the types. Here is my testing:

This is one I can't explain too well.

    const salt = generateRandomString(
      {
        read(bytes) {
          crypto.getRandomValues(bytes);
        },
      },
      alphabet,
      16,
    );

This creates a salt using generateRandomString from @oslojs/crypto/random.

It then uses hash from @node-rs/argon2:

    const passwordHash = await hash(password, {
      memoryCost: 19456,
      timeCost: 2,
      outputLen: 32,
      parallelism: 1,
      salt: Buffer.from(salt),
    });

password: testtest
salt: ojq_1kIXDG5zpGx0
salt buffer: <Buffer 6f 6a 71 5f 31 6b 49 58 44 47 35 7a 70 47 78 30>

hash: $argon2id$v=19$m=19456,t=2,p=1$ZgWgG5c6yUxJWjp3QSRMmw$JJmgej/ScGPbq+RozS2zKyWZLMZixL3MV1JVsu03WI4

According to the types, salt is a correct property of Options, and is NOT marked as deprecated.

I then use the verify function from the same package, giving it the same password, and the hash created earlier, BUT i modofy the salt being passed.

    const validPassword = await verify(existingUser.passwordHash, password, {
      memoryCost: 19456,
      timeCost: 2,
      outputLen: 32,
      parallelism: 1,
      salt: Buffer.from(existingUser.salt),
    });

hash: $argon2id$v=19$m=19456,t=2,p=1$ZgWgG5c6yUxJWjp3QSRMmw$JJmgej/ScGPbq+RozS2zKyWZLMZixL3MV1JVsu03WI4
password: testtest
salt: "This has been set and should not match anything"
salt buffer: <Buffer 54 68 69 73 20 68 61 73 20 62 65 65 6e 20 73 65 74 20 61 6e 64 20 73 68 6f 75 6c 64 20 6e 6f 74 20 6d 61 74 63 68 20 61 6e 79 74 68 69 6e 67>

validPassword = true

This shows that the verify function that verifies the password against the hashed password, with a completely different salt input still passes.
Back to the fact I said it was not marked as deprecated, nowhere in the documentation nor the types does it say this. It also returns no error. However, using the secret prop instead of the salt prop now properly fails the verify function when the fake salt is given.

[ainsly]

@Jazza-231 what version of the package are you using?

This should have been addressed by #899 raised by #893 and fixed in v2+

[Jazza-231]

I couldn't tell you I'm sorry, I have no version history on the project that used it, and that has been since deleted as it was just a test.

[ainsly]

Just ran a quick test, it seems the original issue of the hash function ignoring salt was fixed, BUT, the verify function still does not respect the provided salt. So it seems you are correct.

A quick workaround would be to use hash again instead of verify using the same argon params and comparing them manually - but that shouldn't ever be required or used as a long term solution, and totally negates the point of having a verify function in the first place.

@Brooooooklyn could you take another look please?

[ainsly]

Actually, my last post was incorrect. I just remembered the salt is included in the encoded password hash

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

So it doesn't need to be supplied to the verify function. And as per the Rust implementation's docs

// Verify password against PHC string.
//
// NOTE: hash params from `parsed_hash` are used instead of what is configured in the
// `Argon2` instance.
let parsed_hash = PasswordHash::new(&password_hash)?;
assert!(Argon2::default().verify_password(password, &parsed_hash).is_ok());

So this is actually working as expected.. similar to any other password hasher that is encoded to the PHC format, which includes the salt in the output string. It simplifies having to store all parameters of the underlying hashing function manually and enables you to change your argon2 options at any time, without breaking previously generated passwords.

So the real solution to @Jazza-231's intended usage of the salt, would be to use it as the 'secret' param instead. But it would still be possible to use your own generated salt AND secret as options when generating password hashes, and then supplying the secret when verifying.

@Brooooooklyn perhaps it would be a good idea to change the TS types of the options parameter for the verify/verifySync methods to Pick<Options, 'secret'>. And reflect that change in the README. Hopefully that should prevent any confusion.

Edit: Pick<Options, 'secret'> & Options would prevent any breaking changes for existing usages including other options, but should be noted somewhere that only the secret should be supplied and the intersection with Options is only there to prevent breaking changes.