Fix Argon2 options checks

[MichaIng]

As of: #19023 (comment)

The minimum for memory cost is 8 KiB per thread. Threads must be checked and set first to allow checking against the correct memory cost mimimum.
Options are now applied the following way:

• If config.php contains the setting with an integer higher or equal to the minimum, it is applied.
• If config.php contains the setting with an integer lower than the minimum, the minimum is applied.
• If config.php does not contain the setting or with no integer value, the PHP default is applied.

[kesselb]

Probably we should explicitly set the default (if PASSWORD_ARGON2_DEFAULT_THREADS is assured to be always set valid?) or the minimum 1 then, so options['threads'] is assure to be set and can be used in the memory cost arithmetic check?

I think so.

$this->options['threads'] = max($this->config->getSystemValueInt('hashingThreads', PASSWORD_ARGON2_DEFAULT_THREADS), 1);
$this->options['memory_cost'] = max($this->config->getSystemValueInt('hashingMemoryCost', PASSWORD_ARGON2_DEFAULT_MEMORY_COST), $this->options['threads'] * 8);
$this->options['time_cost'] = max($this->config->getSystemValueInt('hashingTimeCost', PASSWORD_ARGON2_DEFAULT_TIME_COST), 1);

Probably something like that. As long as we use the PASSWORD_ARGON2_DEFAULT* constants as fallback values we are good. But I don't know much about ARGON2 ;)

[MichaIng]

@kesselb
That looks actually like a great clean solution. I am no PHP code/syntax specialist but would just take it exactly like this. If the PASSWORD_ARGON2_DEFAULT* would not be present or invalid, the minimum values are the second fallback. The max() function takes the largest integer and does not fail in case one argument is empty or no integer, doesn't it?

[kesselb]

The max() function takes the largest integer and does not fail in case one argument is empty or no integer, doesn't it?

I'm not sure but getSystemValueInt should always return an integer.

[MichaIng]

@kesselb
Makes sense to expect/enforce a valid integer as default value. I'll go through the code and PHP docs, just to be sure 😉.

EDIT:

• getSystemValueInt returns 0 as last resort fallback:

  server/lib/private/AllConfig.php

  Lines 153 to 155 in 1762a40

  	public function getSystemValueInt(string $key, int $default = 0): int {
  	return (int) $this->getSystemValue($key, $default);
  	}

• max treats non-numeric values as 0 as long as there is a numeric value (the minimum value in our case) as well: https://www.php.net/manual/en/function.max.php

Okay so all this is pretty failsafe.

[MichaIng]

@kesselb
I added the PASSWORD_ARGON2_DEFAULT_* variables separatly to max() arguments as well. This way, if the config.php setting is set, but contains an invalid value or too low integer, the PASSWORD_ARGON2_DEFAULT_* (if set and valid) will be applied. Only if PASSWORD_ARGON2_DEFAULT_* is not set or invalid, the minimum will be applied.

[kesselb]

@kesselb
I added the PASSWORD_ARGON2_DEFAULT_* variables separatly to max() arguments as well. This way, if the config.php setting is set, but contains an invalid value or too low integer, the PASSWORD_ARGON2_DEFAULT_* (if set and valid) will be applied. Only if PASSWORD_ARGON2_DEFAULT_* is not set or invalid, the minimum will be applied.

That makes it impossible to pick a lower value than PASSWORD_ARGON2_DEFAULT_* 🤔

[kesselb]

1. getSystemValueInt will always return a integer. PASSWORD_ARGON2_DEFAULT_* as default or the value set in config.php.
2. Make sure to use at least X as value.

That should cover most cases. Lets keep it simple ;)

[MichaIng]

@kesselb

That makes it impossible to pick a lower value than PASSWORD_ARGON2_DEFAULT_* 🤔

Ah damn you're right. Also: https://www.php.net/manual/en/password.constants.php

The constants below are always available as part of the PHP core.

So no chance those are not set or invalid.

[rullzer]

Makes sense.

[rullzer]

CI says no...
I'll have a look

[rullzer]

Fixed the tests

[MichaIng]

/backport to stable18

[MichaIng]

/backport to stable17

[MichaIng]

/backport to stable16