maj: sémantique, révision vérification password #3

Merged
cmsassot merged 6 commits from maj into develop 2023-01-05 14:09:41 +01:00
Owner

Ajout d'un customHasher pour le password qui implémente un salt éventuellement récupéré si configuré et si présent
ainsi qu'un pepper si non null dans les variables d'environnement.

Révision sémantique namespace et noms de classe
PDO pour SQLLogin

Ajout d'un customHasher pour le password qui implémente un salt éventuellement récupéré si configuré et si présent ainsi qu'un pepper si non null dans les variables d'environnement. Révision sémantique namespace et noms de classe PDO pour SQLLogin
rmasson added 1 commit 2022-12-14 16:46:00 +01:00
Cadoles/hydra-sql/pipeline/head This commit is unstable Details
Cadoles/hydra-sql/pipeline/pr-develop This commit is unstable Details
441c0f563c
maj: sémantique, révision vérification ppassword
Owner

Symfony Security Check Report

1 package has known vulnerabilities.

twig/twig (v3.3.10)

  • CVE-2022-39261: Possibility to load a template outside a configured directory when using the filesystem loader

Note that this checker can only detect vulnerabilities that are referenced in the security advisories database.
Execute this command regularly to check the newly discovered vulnerabilities.

Symfony Security Check Report ============================= 1 package has known vulnerabilities. twig/twig (v3.3.10) ------------------- * [CVE-2022-39261][]: Possibility to load a template outside a configured directory when using the filesystem loader [CVE-2022-39261]: https://symfony.com/blog/twig-security-release-possibility-to-load-a-template-outside-a-configured-directory-when-using-the-filesystem-loader Note that this checker can only detect vulnerabilities that are referenced in the security advisories database. Execute this command regularly to check the newly discovered vulnerabilities.
Owner

Test report

PHP CS Fixer

Overview

State Total
Passed 0
Skipped 0
Failed 5
Error 0

Total duration: 0s

See details
Status Name Class
Cadoles_hydra-sql_PR-3/src/Service/SQLLoginService
Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLPasswordException
Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLLoginException
Cadoles_hydra-sql_PR-3/src/SQLLogin/SQLLoginConnect
Cadoles_hydra-sql_PR-3/src/Security/SQLLoginUserAuthenticator

Errors

`Cadoles_hydra-sql_PR-3/src/Service/SQLLoginService`

Output

applied fixers:
---------------
* global_namespace_import
* no_unused_imports```

</details>


<details>
  <summary>`Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLPasswordException`</summary>

**Output** 

applied fixers:

  • global_namespace_import
  • no_unused_imports
  • no_extra_blank_lines```
`Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLLoginException`

Output

applied fixers:
---------------
* global_namespace_import
* no_unused_imports
* no_extra_blank_lines```

</details>


<details>
  <summary>`Cadoles_hydra-sql_PR-3/src/SQLLogin/SQLLoginConnect`</summary>

**Output** 

applied fixers:

  • global_namespace_import
  • phpdoc_separation
  • no_unused_imports```
`Cadoles_hydra-sql_PR-3/src/Security/SQLLoginUserAuthenticator`

Output

applied fixers:
---------------
* global_namespace_import
* no_unused_imports```

</details>





# Test report ## PHP CS Fixer ### Overview | State | Total | |-------|-------| | Passed | 0 | | Skipped | 0 | | Failed | 5 | | Error | 0 | **Total duration**: 0s <details> <summary>See details</summary> | Status | Name | Class | |--------|------|-------| | &#10799; | `Cadoles_hydra-sql_PR-3/src/Service/SQLLoginService` || | &#10799; | `Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLPasswordException` || | &#10799; | `Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLLoginException` || | &#10799; | `Cadoles_hydra-sql_PR-3/src/SQLLogin/SQLLoginConnect` || | &#10799; | `Cadoles_hydra-sql_PR-3/src/Security/SQLLoginUserAuthenticator` || </details> <br /> #### Errors <details> <summary>`Cadoles_hydra-sql_PR-3/src/Service/SQLLoginService`</summary> **Output** ``` applied fixers: --------------- * global_namespace_import * no_unused_imports``` </details> <details> <summary>`Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLPasswordException`</summary> **Output** ``` applied fixers: --------------- * global_namespace_import * no_unused_imports * no_extra_blank_lines``` </details> <details> <summary>`Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLLoginException`</summary> **Output** ``` applied fixers: --------------- * global_namespace_import * no_unused_imports * no_extra_blank_lines``` </details> <details> <summary>`Cadoles_hydra-sql_PR-3/src/SQLLogin/SQLLoginConnect`</summary> **Output** ``` applied fixers: --------------- * global_namespace_import * phpdoc_separation * no_unused_imports``` </details> <details> <summary>`Cadoles_hydra-sql_PR-3/src/Security/SQLLoginUserAuthenticator`</summary> **Output** ``` applied fixers: --------------- * global_namespace_import * no_unused_imports``` </details>
Owner

Rapport PHPStan

 ------ ---------------------------------------------- 
  Line   Security/SQLLoginUserProvider.php             
 ------ ---------------------------------------------- 
  19     Access to an undefined property               
         App\Security\SQLLoginUserProvider::$session.  
 ------ ---------------------------------------------- 


 [ERROR] Found 1 error                                                          


## Rapport PHPStan ``` ------ ---------------------------------------------- Line Security/SQLLoginUserProvider.php ------ ---------------------------------------------- 19 Access to an undefined property App\Security\SQLLoginUserProvider::$session. ------ ---------------------------------------------- [ERROR] Found 1 error ```
wpetit requested changes 2022-12-15 21:34:58 +01:00
@ -0,0 +24,4 @@
if ($this->isPasswordTooLong($plainPassword)) {
throw new InvalidPasswordException();
}
$hash = hash($this->hashAlgo, $plainPassword.$salt.$this->pepper);
Owner

Je pense que ce code ne sera pas compatible avec les applications ayant choisies d'utiliser les algorithmes argon2id, scrypt et bcrypt (qui sont d'ailleurs les recommandations OWASP 1 aujourd'hui).

Normalement, en PHP on utilise les méthodes password_hash() 2 et password_verify() 3 pour utiliser ces algorithmes (et d'ailleurs le salt est directement stocké dans le hash final 4).

Autre chose: la méthode assume un certain format pour la concaténation des différents éléments à hacher ($plainPassword.$salt.$this->pepper). Il est fort peu probable que cette séquence soit toujours respectée dans toutes les applications. À mon avis il serait certainement préférable d'utiliser un patron (avec la méthode strstr() 5 par exemple) pour permettre à l'utilisateur de spécifiquer le format de concaténation.

Je pense que ce code ne sera pas compatible avec les applications ayant choisies d'utiliser les algorithmes `argon2id`, `scrypt` et `bcrypt` (qui sont d'ailleurs les recommandations OWASP [^1] aujourd'hui). Normalement, en PHP on utilise les méthodes `password_hash()` [^2] et `password_verify()` [^3] pour utiliser ces algorithmes (et d'ailleurs le salt est directement stocké dans le hash final [^4]). Autre chose: la méthode assume un certain format pour la concaténation des différents éléments à hacher (`$plainPassword.$salt.$this->pepper`). Il est fort peu probable que cette séquence soit toujours respectée dans toutes les applications. À mon avis il serait certainement préférable d'utiliser un patron (avec la méthode `strstr()` [^5] par exemple) pour permettre à l'utilisateur de spécifiquer le format de concaténation. [^1]: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html [^2]: https://www.php.net/manual/en/function.password-hash.php [^3]: https://www.php.net/manual/en/function.password-verify.php [^4]: https://stackoverflow.com/questions/40993645/understanding-bcrypt-salt-as-used-by-php-password-hash [^5]: https://www.php.net/manual/en/function.strtr.php
Author
Owner

j'ai utilisé les fonction password_algos() et hash_algos() pour faire la différence entre les algos, pour utiliser hash() ou password_hash()

Mais effectivement le pattern est pas pris en compte

j'ai utilisé les fonction password_algos() et hash_algos() pour faire la différence entre les algos, pour utiliser hash() ou password_hash() Mais effectivement le pattern est pas pris en compte
rmasson marked this conversation as resolved
@ -0,0 +35,4 @@
return false;
}
if ($this->hash($plainPassword, $salt) === $hashedPassword) {
Owner

Il ne faut pas utiliser l'opérateur === pour faire des comparaisons de mots de passe (ça ouvre à des failles du type "Timing attack" 1).

Il serait mieux d'utiliser https://www.php.net/manual/en/function.hash-equals.php je pense dans ce cas.

Il ne faut pas utiliser l'opérateur `===` pour faire des comparaisons de mots de passe (ça ouvre à des failles du type "Timing attack" [^1]). Il serait mieux d'utiliser https://www.php.net/manual/en/function.hash-equals.php je pense dans ce cas. [^1]: https://www.chosenplaintext.ca/articles/beginners-guide-constant-time-cryptography.html
wpetit changed title from maj: sémantique, révision vérification ppassword to maj: sémantique, révision vérification password 2022-12-15 21:35:10 +01:00
wpetit requested changes 2022-12-15 21:41:43 +01:00
@ -25,2 +25,4 @@
locales: '%env(APP_LOCALES)%'
app.supported_locales: ~
env(PEPPER): "257d62c24cd352c21b51c26dba678c8ff05011a89022aec106185bf67c69aa8b"
Owner

Hmm, je pense que par défaut le pepper devrait être vide.

Hmm, je pense que par défaut le pepper devrait être vide.
@ -0,0 +28,4 @@
public const ERROR_PDO = 'error_pdo';
protected string $baseUrl;
private SQLLoginService $pdoService;
Owner

Je pense que la variable n'a pas été renommée $pdoService en accord avec le changement de nom du service.

Je pense que la variable n'a pas été renommée `$pdoService` en accord avec le changement de nom du service.
@ -0,0 +32,4 @@
private UrlGeneratorInterface $router;
private PasswordEncoder $passwordHasher;
public function __construct(string $baseUrl, SQLLoginService $pdoService, UrlGeneratorInterface $router, PasswordEncoder $passwordHasher)
Owner
Idem, cf. [commentaire précédent](https://forge.cadoles.com/Cadoles/hydra-sql/pulls/3/files#issuecomment-55979)
@ -0,0 +35,4 @@
public function __construct(string $baseUrl, SQLLoginService $pdoService, UrlGeneratorInterface $router, PasswordEncoder $passwordHasher)
{
$this->baseUrl = $baseUrl;
$this->pdoService = $pdoService;
Owner
Idem, cf. [commentaire précédent](https://forge.cadoles.com/Cadoles/hydra-sql/pulls/3/files#issuecomment-55979)
@ -0,0 +72,4 @@
public function getConnection()
{
// Appel du singleton
$pdo = SQLLoginConnect::getInstance();
Owner
Idem, cf. [commentaire précédent](https://forge.cadoles.com/Cadoles/hydra-sql/pulls/3/files#issuecomment-55979)
@ -1 +1 @@
123
134
Owner

Ce fichier doit il vraiment être commit dans le dépôt ? Attention avec les git add -A :P

Ce fichier doit il vraiment être commit dans le dépôt ? Attention avec les `git add -A` :P
rmasson added 1 commit 2022-12-16 15:00:22 +01:00
Cadoles/hydra-sql/pipeline/pr-develop This commit is unstable Details
bd1b035f1e
ajout de l'updatde hashage selon algo indiqué en ver env, fix typo
Owner

Symfony Security Check Report

1 package has known vulnerabilities.

twig/twig (v3.3.10)

  • CVE-2022-39261: Possibility to load a template outside a configured directory when using the filesystem loader

Note that this checker can only detect vulnerabilities that are referenced in the security advisories database.
Execute this command regularly to check the newly discovered vulnerabilities.

Symfony Security Check Report ============================= 1 package has known vulnerabilities. twig/twig (v3.3.10) ------------------- * [CVE-2022-39261][]: Possibility to load a template outside a configured directory when using the filesystem loader [CVE-2022-39261]: https://symfony.com/blog/twig-security-release-possibility-to-load-a-template-outside-a-configured-directory-when-using-the-filesystem-loader Note that this checker can only detect vulnerabilities that are referenced in the security advisories database. Execute this command regularly to check the newly discovered vulnerabilities.
Owner

Test report

PHP CS Fixer

Overview

State Total
Passed 0
Skipped 0
Failed 5
Error 0

Total duration: 0s

See details
Status Name Class
Cadoles_hydra-sql_PR-3/src/Service/SQLLoginService
Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLLoginAlgoException
Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLLoginConfigurationException
Cadoles_hydra-sql_PR-3/src/SQLLogin/SQLLoginConnect
Cadoles_hydra-sql_PR-3/src/Security/SQLLoginUserAuthenticator

Errors

`Cadoles_hydra-sql_PR-3/src/Service/SQLLoginService`

Output

applied fixers:
---------------
* global_namespace_import
* no_unused_imports```

</details>


<details>
  <summary>`Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLLoginAlgoException`</summary>

**Output** 

applied fixers:

  • global_namespace_import
  • no_unused_imports
  • no_extra_blank_lines```
`Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLLoginConfigurationException`

Output

applied fixers:
---------------
* global_namespace_import
* no_unused_imports
* no_extra_blank_lines```

</details>


<details>
  <summary>`Cadoles_hydra-sql_PR-3/src/SQLLogin/SQLLoginConnect`</summary>

**Output** 

applied fixers:

  • global_namespace_import
  • phpdoc_separation
  • no_unused_imports```
`Cadoles_hydra-sql_PR-3/src/Security/SQLLoginUserAuthenticator`

Output

applied fixers:
---------------
* global_namespace_import
* no_unused_imports```

</details>





# Test report ## PHP CS Fixer ### Overview | State | Total | |-------|-------| | Passed | 0 | | Skipped | 0 | | Failed | 5 | | Error | 0 | **Total duration**: 0s <details> <summary>See details</summary> | Status | Name | Class | |--------|------|-------| | &#10799; | `Cadoles_hydra-sql_PR-3/src/Service/SQLLoginService` || | &#10799; | `Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLLoginAlgoException` || | &#10799; | `Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLLoginConfigurationException` || | &#10799; | `Cadoles_hydra-sql_PR-3/src/SQLLogin/SQLLoginConnect` || | &#10799; | `Cadoles_hydra-sql_PR-3/src/Security/SQLLoginUserAuthenticator` || </details> <br /> #### Errors <details> <summary>`Cadoles_hydra-sql_PR-3/src/Service/SQLLoginService`</summary> **Output** ``` applied fixers: --------------- * global_namespace_import * no_unused_imports``` </details> <details> <summary>`Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLLoginAlgoException`</summary> **Output** ``` applied fixers: --------------- * global_namespace_import * no_unused_imports * no_extra_blank_lines``` </details> <details> <summary>`Cadoles_hydra-sql_PR-3/src/SQLLogin/Exception/InvalidSQLLoginConfigurationException`</summary> **Output** ``` applied fixers: --------------- * global_namespace_import * no_unused_imports * no_extra_blank_lines``` </details> <details> <summary>`Cadoles_hydra-sql_PR-3/src/SQLLogin/SQLLoginConnect`</summary> **Output** ``` applied fixers: --------------- * global_namespace_import * phpdoc_separation * no_unused_imports``` </details> <details> <summary>`Cadoles_hydra-sql_PR-3/src/Security/SQLLoginUserAuthenticator`</summary> **Output** ``` applied fixers: --------------- * global_namespace_import * no_unused_imports``` </details>
Owner

Rapport PHPStan

 ------ ----------------------------------------------------------------------- 
  Line   Security/Hasher/PasswordEncoder.php                                    
 ------ ----------------------------------------------------------------------- 
  107    Method App\Security\Hasher\PasswordEncoder::getValidAlgo() should      
         return App\SQLLogin\Exception\InvalidSQLLoginAlgoException|string but  
         return statement is missing.                                           
 ------ ----------------------------------------------------------------------- 


 [ERROR] Found 1 error                                                          


## Rapport PHPStan ``` ------ ----------------------------------------------------------------------- Line Security/Hasher/PasswordEncoder.php ------ ----------------------------------------------------------------------- 107 Method App\Security\Hasher\PasswordEncoder::getValidAlgo() should return App\SQLLogin\Exception\InvalidSQLLoginAlgoException|string but return statement is missing. ------ ----------------------------------------------------------------------- [ERROR] Found 1 error ```
rmasson added 1 commit 2022-12-16 15:39:50 +01:00
Cadoles/hydra-sql/pipeline/pr-develop There was a failure building this commit Details
906d8edf82
fix faille twig, coding standard
rmasson added 1 commit 2022-12-16 16:49:32 +01:00
Cadoles/hydra-sql/pipeline/pr-develop There was a failure building this commit Details
422e97d329
supression des fonctions liées à l'update du hashage de mot de passe
rmasson requested review from wpetit 2022-12-16 16:49:48 +01:00
rmasson added 1 commit 2023-01-05 11:31:35 +01:00
Cadoles/hydra-sql/pipeline/pr-develop There was a failure building this commit Details
73eae4176e
maj: suppression commentaire inutile
rmasson force-pushed maj from 73eae4176e to b8fb01da5c 2023-01-05 12:13:24 +01:00 Compare
Owner

Symfony Security Check Report

No packages have known vulnerabilities.

Note that this checker can only detect vulnerabilities that are referenced in the security advisories database.
Execute this command regularly to check the newly discovered vulnerabilities.

Symfony Security Check Report ============================= No packages have known vulnerabilities. Note that this checker can only detect vulnerabilities that are referenced in the security advisories database. Execute this command regularly to check the newly discovered vulnerabilities.
Owner

Test report

PHP CS Fixer

Overview

State Total
Passed 0
Skipped 0
Failed 1
Error 0

Total duration: 0s

See details
Status Name Class
Cadoles_hydra-sql_PR-3/src/Security/SQLLoginUserAuthenticator

Errors

`Cadoles_hydra-sql_PR-3/src/Security/SQLLoginUserAuthenticator`

Output

applied fixers:
---------------
* global_namespace_import
* no_unused_imports```

</details>





# Test report ## PHP CS Fixer ### Overview | State | Total | |-------|-------| | Passed | 0 | | Skipped | 0 | | Failed | 1 | | Error | 0 | **Total duration**: 0s <details> <summary>See details</summary> | Status | Name | Class | |--------|------|-------| | &#10799; | `Cadoles_hydra-sql_PR-3/src/Security/SQLLoginUserAuthenticator` || </details> <br /> #### Errors <details> <summary>`Cadoles_hydra-sql_PR-3/src/Security/SQLLoginUserAuthenticator`</summary> **Output** ``` applied fixers: --------------- * global_namespace_import * no_unused_imports``` </details>
Owner

Rapport PHPStan

 ------ ---------------------------------------------------------------------- 
  Line   SQLLogin/SQLLoginRequest.php                                          
 ------ ---------------------------------------------------------------------- 
  19     Deprecated in PHP 8.0: Required parameter $dsn follows optional       
         parameter $config.                                                    
  19     Deprecated in PHP 8.0: Required parameter $password follows optional  
         parameter $config.                                                    
  19     Deprecated in PHP 8.0: Required parameter $user follows optional      
         parameter $config.                                                    
 ------ ---------------------------------------------------------------------- 


 [ERROR] Found 3 errors                                                         


## Rapport PHPStan ``` ------ ---------------------------------------------------------------------- Line SQLLogin/SQLLoginRequest.php ------ ---------------------------------------------------------------------- 19 Deprecated in PHP 8.0: Required parameter $dsn follows optional parameter $config. 19 Deprecated in PHP 8.0: Required parameter $password follows optional parameter $config. 19 Deprecated in PHP 8.0: Required parameter $user follows optional parameter $config. ------ ---------------------------------------------------------------------- [ERROR] Found 3 errors ```
rmasson added 1 commit 2023-01-05 13:22:41 +01:00
Cadoles/hydra-sql/pipeline/pr-develop This commit looks good Details
Cadoles/hydra-sql/pipeline/head This commit is unstable Details
03e15da862
maj: correction phpstan, optional param config must be behind
Owner

Symfony Security Check Report

No packages have known vulnerabilities.

Note that this checker can only detect vulnerabilities that are referenced in the security advisories database.
Execute this command regularly to check the newly discovered vulnerabilities.

Symfony Security Check Report ============================= No packages have known vulnerabilities. Note that this checker can only detect vulnerabilities that are referenced in the security advisories database. Execute this command regularly to check the newly discovered vulnerabilities.
Owner

Test report

PHP CS Fixer

Overview

State Total
Passed 1
Skipped 0
Failed 0
Error 0

Total duration: 0s

See details
Status Name Class
All OK

# Test report ## PHP CS Fixer ### Overview | State | Total | |-------|-------| | Passed | 1 | | Skipped | 0 | | Failed | 0 | | Error | 0 | **Total duration**: 0s <details> <summary>See details</summary> | Status | Name | Class | |--------|------|-------| | &#10003; | `All OK` || </details> <br />
Owner

Rapport PHPStan


 [OK] No errors                                                                 


## Rapport PHPStan ``` [OK] No errors ```
cmsassot merged commit 84b72ccf18 into develop 2023-01-05 14:09:41 +01:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Cadoles/hydra-sql#3
No description provided.