Apply SensitiveParameter attribute

Is there anything preventing #[\SensitiveParameter] from being added now, in 10.x, rather than waiting until 8.2 is required? It wouldn't do anything on 8.1, but could be a nice little security enhancement for those using 8.2

> Is there anything preventing #[\SensitiveParameter] from being added now, in 10.x, rather than waiting until 8.2 is required? It wouldn't do anything on 8.1, but could be a nice little security enhancement for those using 8.2

The RFC states:

> 1. The \SensitiveParameter and \SensitiveParameterValue class name will no longer be available to userland code.

which, the other way round, says Yes, we can do it now. So let's do it.

Retitling then. As far as "explore where this attribute could be applied," to start with this could be helpful for $password parameter; also the unhashed $sid which is functionally similar to password. That stuff is truly sensitive vs. other PII which I'd call less sensitive.

I am not sure how I messed up, I was using DrupalPod to give it a try but I guess somehow It started a new PR. Well initial change is here https://git.drupalcode.org/issue/drupal-3296293/-/commit/615123b7ae06b98...

A traditional patch

FIxed CCF for #8.
Please review.

Patch needs some work for CI failures

Removed #[\SensitiveParameter] from the function calls as they appeared to be causing errors in those locations.

random ckeditor5 failure.

Presumably the PasswordInterface $password argument wouldn't be sensitive, that's just the password hashing service

Ah I gotcha. Changes look good.

Could we use #[SensitiveParameter] on the $connection_options of Drupal\mysql\Driver\Database\mysql\Connection::open() to mitigate 3061567?

If that would be an acceptable addition to this patch, I would be happy to add it. It would probably be later this evening though (Pacific time).

I think adding the attribute there would be a good addition.

It seems that 3061567 will/is already resolved by SensitiveParameters in PHP 8.2. The actual backtrace logged is PDO::construct(), and it seems that this is already using SensitiveParameter properly. We can disregard my comment in #19. Thank you.

PDOException: SQLSTATE[HY000] [2002] php_network_getaddresses: getaddrinfo for 8b9fb7fd9a14 failed: Name or service not known in /opt/drupal/web/core/lib/Drupal/Component/DependencyInjection/PhpArrayContainer.php on line 79 #0 /opt/drupal/web/core/modules/mysql/src/Driver/Database/mysql/Connection.php(165): PDO->__construct('mysql:host=8b9f...', 'root', Object(SensitiveParameterValue), Array)

Steps to test

Pre-step

1. Create a local Drupal (drupal:latest) & MariaDB (mariadb:10.3) docker environment using the official images

Positive test case

1. Apply my patch that adds the #[SensitiveParameter] attribute to the $connection_options parameters of Drupal\mysql\Driver\Database\mysql\Connection::__construct() and Drupal\mysql\Driver\Database\mysql\Connection::open()
2. Stop the MariaDB container
3. Verify that a PDOException is thrown BUT it should NOT include the plain-text connection_options

Negative test case

FAILED test: This did NOT include the plain-text connection_options, but instead Object(SensitiveParameterValue).

verify that issue 3061567 still throws the PDOException that includes the plain text connection options

1. Stop the MariaDB container
2. Verify that a PDOException still thrown and includes the plain-text connection_options

Adding to the unhashed $sid as I suggested in #4

1. +++ b/core/lib/Drupal/Core/Password/PasswordInterface.php
   @@ -34,7 +34,7 @@ public function hash($password);
   +  public function check(#[\SensitiveParameter] $password, #[\SensitiveParameter] $hash);
   

   Is the $hash parameter sensitive?

2. +++ b/core/lib/Drupal/Core/Session/PermissionsHashGenerator.php
   @@ -43,7 +43,7 @@ class PermissionsHashGenerator implements PermissionsHashGeneratorInterface {
   +  public function __construct(#[\SensitiveParameter] PrivateKey $private_key, CacheBackendInterface $cache, CacheBackendInterface $static) {
   

   This is the PrivateKey service, not an actual private key.

I would say it's only necessary to match what php and symfony are doing with their password hashing functions, which is that just the password itself is tagged as SensitiveParameter, not the hash. Of course a hashed password, like lots of data, is still sensitive and there are procedures if there's a breach, just saying it doesn't need to be a "SensitiveParameter".

@andypost The actual string private key is certainly a SensitiveParameter (as would be the hash salt setting if that is a parameter anywhere), but I don't see a need for the PrivateKey service to be a SensitiveParameter; can you explain? That's just a service which doesn't contain any data.

Symfony will now apply SensitiveParameter to $sessionId as of https://github.com/symfony/symfony/pull/49016 so now we just need to get this issue committed :) I'm still ambivalent about adding it to hashes since php and symfony do not, but leaked password hashes are both unfortunate and embarrassing so, works for me.

Thanks for your work on this. Attributes is a cool feature to see finally in PHP (as well as this attribute specifically). I'm excited every time I read the release notes for a new PHP version.

Only moving back to NW for the change record

The change it self looks good!

I made a first draft/attempt at a change record. https://www.drupal.org/node/3339665

Not sure what more the change record should include

Committed and pushed 90c51b9047 to 10.1.x. Thanks!

Also tweaked the change record a little and published it.