bb-10.4-danielblack-MDEV-24207-accept-m57-forms-of-invalid-password is there for review.

The goal is to provide transportability of MYSQL native password's invalid forms while ensuring that the invalid password really is invalid.

cvicentiu can you have a review of this one please.

Is removing compatibility of 11111111111111111111111111111111111111111 and insisting on *1111111111111111111111111111111111111111 too much of a change?

danblack After carefully looking at this, here is my assessment.

1. The main goal of this patch is to prevent MariaDB's native_password_plugin from "parsing" the hex (or non hex) authentication_string. Due to how the current code is written, we convert any string (within native_password_get_salt) that has the appropriate length to a "binary" representation, that can potentially match a real password.

More specifically,
"*THISISNOTAVALIDPASSWORDTHATCANBEUSEDHERE" produces the same results as "*d13c3c78dafa52d9bce09bdd1adcb7befced1ebe".

2. Please reword the commit message to highlight this fact first. It took a long time to understand why the check was necessary and I had to read the review of MDEV-22974 to understand the problem in more detail.
Also, the commit message has the MDEV-22974 and the following title is not related to MDEV-22974, nor to MDEV-24207's title.

3. I see no reason to require the specific * character at the start. It is a breaking change for users in a stable version so it's a definite *no* there.

4. I like checking for non-hex characters and if any are found, shortcut and set the password to be "invalid password". However I think the while loop is very hard to read. Here's an alternative that I suggest to use, replace the whole if-else clause with this:

  const char *hash_end= hash + hash_length;

  for (const char *c= hash + 1; c < hash_end; c++)

  {

    /* If any non-hex characters are found, mark the password as invalid. */

    if (!(*c >= '0' && *c <= '9') &&

        !(*c >= 'A' && *c <= 'F') &&

        !(*c >= 'a' && *c <= 'f'))

    {

      memcpy(out, invalid_password, sizeof(invalid_password));

      *out_length= sizeof(invalid_password);

      return 0;

    }

  }

With points 1-4, the patch fixes both the "invalid-password-could-match-to-a-real-password" problem and also is compatible with MySQL's invalid password definition. I think that covers all cases.

Assuming buildbot doesn't highlight any problems, I'm ok with pushing the patch with the review changes.

And actually, one more idea that came to mind, why not also check hash[0] to see if it is in fact the "*" character. If it is not, memcpy the invalid_password directly. This will also allow for the following use-cases:

To quickly "disable" a user's password, change the "*" to "!". To "re-enable" the password, change the "!" to "*".

cvicentiu, I presume, you meant "if hash[0] is not /[a-z0-9*]/

On account locking via password 10.4 has `ALTER USER xx ACCOUNT LOCK/ UNLOCK` so no need to add another mechanism.

Given there's no current restriction on the first character I guess its breaking an compatibility to give it any meaning.

Thanks for the other review comments cvicentiu, I'll take those in the rework.

serg I don't follow https://github.com/MariaDB/server/commit/778cb28230b42d1abdc388c3d5e4d2807f4ea198#r45005968 comment literally or objective wise. The password length is the main criteria for it being invalid (per MDEV-22974).