Add support for BLAKE2 algorithms#339
Conversation
gilles-peskine-arm
left a comment
There was a problem hiding this comment.
Mostly LGTM, with a couple of questions and suggestions.
doc/crypto/api/ops/mac.rst
Outdated
| To select a non-default tag length ``tag_len``, use :code:`PSA_ALG_TRUNCATED_MAC(PSA_ALG_BLAKE2B_MAC, tag_len)` as the algorithm. | ||
|
|
||
| .. note:: | ||
| When BLAKE2b is used as general MAC algorithm, it is recommended to use an output tag of at least 64 bits (8 bytes). |
There was a problem hiding this comment.
Why are we stating 64 bits as a minimum recommended MAC length? We aren't doing that for other MAC algorithms.
There was a problem hiding this comment.
It's the diection of travel for official MAC recommendations - NIST SP 800-227 will say this for HMAC.
We could say nothing, or we could put general recommendations/guidelines like this into the MAC chapter preamble instead, as this is not really BLAKE2 specific guidance?
There was a problem hiding this comment.
Conclusion: if anywhere, guidance around choosing adequate tag lengths should be in PSA_ALG_TRUNCATED_MAC.
|
|
||
| A call to `psa_key_derivation_output_key()` will draw :math:`m/8` bytes of output and use these as the key data, where :math:`m` is the bit-size of the key. | ||
|
|
||
| .. macro:: PSA_KEY_TYPE_BLAKE2 |
There was a problem hiding this comment.
Does it make sense to have a single BLAKE2 key type for both BLAKE2s and BLAKE2b MAC algorithms?
I have no strong feelings about this. Technically BLAKE2s-MAC and BLAKE2b-MAC are families of algorithms that use different families of keys, since they have different length constraints. But since they're just an array of bytes, it doesn't really matter.
There was a problem hiding this comment.
Might be better to be separate, except that if we implement this idea, then a single key type for the family of PSA_ALG_BLAKE2_MAC() algorithms actually makes more sense.
There was a problem hiding this comment.
Kept a single key type in the updated PR
|
|
||
| It is recommended that these compound algorithms are not supported with `PSA_ALG_ASCON_HASH256`. | ||
|
|
||
| .. macro:: PSA_ALG_BLAKE2S_HASH256 |
There was a problem hiding this comment.
I assume nobody is using the parallel versions, Blake2bp and Blake2sp?
The only thing I can find on the web (other than software that supports a lot of different hash functions) is Winrar using BLAKE2sp. And p7zip also supports BLAKE2sp but no other BLAKE variant.
According to BLAKE2 §2.11, “parallel hashes have exactly the same interfaces as their sequential counterparts”. So specifying them in PSA would be very cheap, we just need to give them an algorithm encoding. So I lean towards doing it.
There was a problem hiding this comment.
Without an explicit request for these variants, with specific use case(s), we might not specify the right algorithms, as BLAKE2 does not map neatly onto our hash vs xof vs MAC taxonomy.
There was a problem hiding this comment.
True, but the only use I've found of a p variant is the s hash, with its nominal length.
There was a problem hiding this comment.
So perhaps we can add PSA_ALG_BLAKE2SP_HASH256 and PSA_ALG_BLAKE2BP_HASH512 and the associated MACs, as the specification is less effort if done now with the non-parallel variants.
doc/crypto/appendix/encodings.rst
Outdated
| BLAKE2s-MAC, 0, ``0x03``, `PSA_ALG_BLAKE2S_MAC`, ``0x03800300`` :sup:`a` | ||
| BLAKE2b-MAC, 0, ``0x04``, `PSA_ALG_BLAKE2B_MAC`, ``0x03800400`` :sup:`a` |
There was a problem hiding this comment.
Especially if we add BLAKE2sp and BLAKE2bp, could we have an encoding where (PSA_ALG_BLAKE2S_HASH256 & 3) == (PSA_ALG_BLAKE2S_MAC & 3) etc.?
There was a problem hiding this comment.
Are you suggesting that we use [some of] bits 7:0 in the MAC identifier to create a link to the related hash identifier? Given that BLAKE2s-MAC is not parameterized by hash (unlike HMAC), we could instead have BLAKE2s-MAC and BLAKE2b-MAC use the same MAC-TYPE value, and then reuse the HASH identifer bits 7:0? e.g. BLAKE2S_HASH256 == 0x02000019, BLAKE2B_HASH512 == 0x0200001A so BLAKE2S_MAC == 0x03800319; BLAKE2B_MAC == 0x0380031A?
This stops being 'neat' if we ever need to add BLAKE2s hash variants with shorter outputs, which cannot share the same Hash identifier value....
There was a problem hiding this comment.
Oh, no, I'd missed that we're not using the low-order bits in MAC and other non-hash algorithms. I didn't mean to deviate from that rule. So what I'd like is for bits 8..9 (or 8, or 8..10, depending on how many BLAKE variants we specify) of the MAC to indicate the BLAKE variant with the same encoding as bits 0..1 of the hash.
There was a problem hiding this comment.
If we also add the parallel hashes, then it would make sense to define PSA_ALG_BLAKE2_MAC() which accepts one of the four base BLAKE2 hash algorithms as a parameter.
We can also set the BLAKE2 hash algorithms to be consecutive identifiers starting with a value that has bits[1:0] == 2b00.
doc/crypto/api/ops/hash.rst
Outdated
|
|
||
| .. versionadded:: 1.5 | ||
|
|
||
| The BLAKE2s-256 hash is BLAKE2s with a zero-length key, and a 256 bit (32 byte) output. |
There was a problem hiding this comment.
Nit (same for BLAKE2b):
| The BLAKE2s-256 hash is BLAKE2s with a zero-length key, and a 256 bit (32 byte) output. | |
| The BLAKE2s-256 hash is BLAKE2s with a zero-length key, and a 256-bit (32-byte) output. |
One aspect of the PR that I would like feedback on:
Does it make sense to have a single BLAKE2 key type for both BLAKE2s and BLAKE2b MAC algorithms?
Fixes #333