Ruben's Hash2 review

From: Ruben Perez (rubenperez038_at_[hidden])
Date: 2024-12-13 17:42:17

Hi all,

This is my review of the proposed Boost.Hash2. Thanks Peter and Chris
for submitting the library, and Matt for managing the review.

- What is your evaluation of the design?

Very good. This is how hashing C++ objects should work, as it properly
separates concerns between types and hash functions. I think usability
could be improved if the template class hash shown as an example was
included as part of the library and introduced much earlier in the
documentation.

In general, the HashAlgorithm concept makes sense. I'm not convinced
about the HashAlgorithm::result() interface, for two reasons:

* It makes integration of third-party hashes more difficult, as
already noted by other reviewers.
* The naming suggests an accessor, rather than a modifier. I'd
consider splitting it into two functions, maybe finalize_block() and
result().

But I can also see the benefits of the approach taken.

I miss some higher-level features when working with untyped byte
sequences. While this doesn't seem the main value proposition of the
library, it's listed as a valid use case. See below for more info.
These are nice-to-haves rather than requirements, though.

I was initially surprised to find out that the library was
header-only, as I didn't know that compile-time hashing was something
worth it. I was initially worried about compile-times. I built one of
the examples with compile-time profiling (-ftime-trace) and the
library is pretty lightweight, so I have no problems with the
header-only design.

Have you considered somehow supporting hashing for objects describable
via Boost.Pfr? I had it requested by some users in MySQL even when I
had built-in Boost.Describe support, so it may get requested here,
too.

- What is your evaluation of the implementation?

The parts where I've peaked in are good. Some comments would have
helped me, but in general I've been able to follow.

The test suite looks pretty comprehensive, too. Some fuzz tests for
the cryptographic algorithms would be appreciated (I can help if
required).

As other reviewers have commented, the algorithm for hashing unordered
ranges can use some improvements to make it more secure. I'm no
security expert and can't judge how important this is, but I'm pretty
sure the author will solve this promptly.

The implementation of cryptographic functions is currently slower than
solutions like OpenSSL [4], but the author has manifested the
possibility of closing this gap.

It would be nice having HashAlgorithm modelled as a C++20 concept,
although the required macro machinery required to perform the checking
only if concepts are supported (like BOOST_ASIO_COMPLETION_TOKEN_FOR
does) may be too much effort for the benefit it brings.

Regarding Boost integration, a build.jam should likely be added, so
the library integrates in the new modular Boost infrastructure
(although I've been able to use the library fine without it). This can
be done after acceptance.

As a matter of curiosity: which compilers do not support
__builtin_is_constant_evaluated? (as they will always get the slow
memcpy implementation, even at runtime).

- What is your evaluation of the documentation?

The documentation is good, and explains rationales pretty well, but
the order could be improved:

* Most users will likely want to use the existing algorithms with
their unordered containers, so this should be explained first.
* The next block of users will likely want to use the existing
cryptographic hash algorithms to untyped byte sequences, so this
should be next.
* The rest can be left as is.

I miss the motivation for the flavor construct in the docs. If I
understood correctly, it's only relevant if you plan to somehow
serialize your hashes. Maybe an example could help.

The library is also suitable for use cases that combine the C++ object
hashing and untyped byte sequence hashing parts, as explained by the
author here: https://lists.boost.org/Archives/boost/2024/12/258765.php.
Such cases are the rationale behind having cryptographic functions
within the same umbrella as non-cryptographic ones. I'd suggest adding
these to the docs.

In general, I dislike single-page docs (which unfortunately seem
pretty popular in Boost), as they're difficult to search. I also miss
linking (e.g. hash_append should contain a link to its description in
the reference).

It would also be beneficial to explain the library's relationship with
boost::hash / Boost.ContainerHash. In general, making it more
approachable for non-expert users would be good.

- What is your evaluation of the potential usefulness
of the library? Do you already use it in industry?

Robust, extensible C++ object hashing is very useful. My immediate use
case is related to hashing untyped byte sequences, though.

MySQL/MariaDB authentication involve a challenge-response protocol
that involves hashing the user-supplied password with a
server-supplied challenge, using either SHA1 (sigh) or SHA256 [1].

I'm currently using OpenSSL, as I use it for TLS, too. This library
would allow me to make the OpenSSL dependency optional (which I've
been requested). I'm also planning on writing a Postgres protocol
library. It will likely need to support SCRAM-SHA256 authentication,
similar to what I've described. This library could be handy for this.

- Did you try to use the library? With which compiler(s)? Did
you have any problems?

I've re-written the password hashing functionality in Boost.MySQL to
use the proposed library [2]. Everything has been very smooth. The
test suite passes for all the compilers I support [3], which is really
good.

I'd have liked some utilities on top of HashAlgorithm to make hashing
byte sequences safer and easier. These utilities would *not* imply any
change in the HashAlgorithm interface:

* Adding a contiguous range of bytes to an algorithm currently
supports only a pointer + size pair. This is the right choice for
HashAlgorithm, but having a helper function on top of it would be
great, as proposed by other reviewers. This would make my code go
from:

// password is a string_view
hash2::sha1_160 hasher1;
hasher1.update(password.data(), password.size());
auto password_sha1 = hasher1.result();

To:

hash2::sha1_160 hasher1;
hash2::hash_add_bytes(hasher1, password);
auto password_sha1 = hasher1.result();

* Adding a function that computes the hash of an entire byte sequence,
as a range, could be useful. OpenSSL has this as SHA1() and SHA256().
This would make my code go from:

hash2::sha1_160 hasher1;
hasher1.update(password.data(), password.size());
auto password_sha1 = hasher1.result();

To:

auto password_sha1 = hash2::compute_hash(hash2::sha1_160(), password);

These are just nice to have and trivially implemented in user-land, if
required. The code is already good as is today.

- How much effort did you put into your evaluation?
A glance? A quick reading? In-depth study?

I have dedicated around 8 hours to study the documentation, build the
library and examples, integrate it into Boost.MySQL, update my CIs to
run my tests with the library, and write this review.

- Are you knowledgeable about the problem domain?

I am a mere user of hash algorithms, as I usually write networking
code. I don't know about their internals. I haven't authored
associative containers, either.

- Affiliation disclosure

I am currently affiliated with the C++ Alliance.

- Final decision

My recommendation is to CONDITIONALLY ACCEPT the library into Boost.
My only acceptance condition is adding the hash template class
developed in the examples to the library. The benefit of having the
library in Boost is big and overcomes any concerns I might have.

Many thanks to both Peter and Matt.

Kind regards,
Ruben.

[1] https://dev.mysql.com/doc/dev/mysql-server/8.4.3/page_caching_sha2_authentication_exchanges.html
[2] https://github.com/boostorg/mysql/pull/389
[3] https://github.com/boostorg/mysql/blob/c438f26731e36c2db6457705ec5dbb9f7657db2a/.drone.star#L280-L325
[4] https://lists.boost.org/Archives/boost/2024/12/258687.php