Problem/Motivation

PHP 8.1 adds support for xxHash - a very fast, collision-resistant, non-crytographic hash.

We use a lot of hashes for non-crypto purposes, and our approach flip-flops between using weak non-crypto hashes like crc32, or mis-using cryptographic hashes to be 'correct'. xxHash means we don't need to flip-flop any more.

Steps to reproduce

Proposed resolution

Remaining tasks

Open sub-issues for each hash usage we want to change, including the recently added one in #2531564: Fix leaky and brittle container serialization solution.

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3307718

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

neclimdul made their first commit to this issue’s fork.

neclimdul’s picture

neclimdul
He/Him
commented

Used some downtime to do the busywork to get a first pass at some of the obvious crc/md5/sha1 changes out of the way. The only remaining uses should be legacy stuff like d6/d7 password migration and composer compatibility. They all seem pretty straight forward not touching anything that would really be visible or a BC break.

The places where we use sha256 will be a bit trickier but I expect almost everything outside of hmac usage is probably not actually cryptographic and fair game if its not a BC break.

Speaking of sha256 uses, look at this odd ball method. \Drupal\Core\Cache\Cache::keyFromQuery. No usage in core and one usage in contrib on a project with no releases. Seems like something to just deprecate and remove. :-D.

catch’s picture

catch
he/him
Primary language English
commented

That was added in 2010 factored out of forum module, it's been rendered (see what I did there) irrelevant by render caching. Should definitely deprecate and remove.

neclimdul’s picture

neclimdul
He/Him
commented

split that off here #3308507: Remove Cache::keyFromQuery

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs work

#3032078: Multiple webheads can cause infinite growth of Twig cache introduces another crc32.

cilefen’s picture

cilefen commented
Title: Start using xxHash » Implement xxHash for non-cryptographic use-cases

catch opened merge request !4350

Version: 10.0.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost made their first commit to this issue’s fork.

catch closed merge request !4350

catch opened merge request !12441

catch’s picture

catch
he/him
Primary language English
commented

The fork was too far behind (early 10.x branch) to be able to rebase on, so cherry-picked the one commit that still applied to 11.x, recreated one more of @neclimdul's and pushed a new branch.

Also adding PermissionsHashGenerator here after discovering it makes debugging test cache changes unhelpfully difficult. We probably need to split that to its own issue though because it'll likely need constructor deprecations and a double check that it's definitely not security sensitive (I don't think it is though, it's a hash of permissions, not account information otherwise).

nicxvan made their first commit to this issue’s fork.

nicxvan’s picture

nicxvan commented

Applied some suggestions for cs and stan.

nicxvan’s picture

nicxvan commented

Ah core services needs to be updated too.

nicxvan’s picture

nicxvan commented

Is there a reason not to use xxh3?

https://xxhash.com/

It seems to be supported and faster, but I'll admit I'm not super familiar with this so I may be missing something.

catch’s picture

catch
he/him
Primary language English
commented

Can't see any reason not to use xxh3, probably got thrown off by it being 'new' but it was included in PHP 8.1

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

The MR is back to green, but we need to figure out scope here. There are other sha256 and other hash usages in core we can probably convert.

Also not sure whether we want to spin-off PermissionsHashGenerator and any other logic changes to their own issues. Also noticed in the twig change that we're using Crypt::base64Encode() purely to base64 encode a hash but I think that's already the case with xxh3 so we could probably use xxh3 for that case too.

smustgrave’s picture

smustgrave commented

Any suggestion on how to review this?

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Appears to need a rebase

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.