-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace hash_to_base PRG with HKDF-Expand. #141
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great.
It wasn't clear to me that it would be quite this easy to invoke HKDF, because I was thinking that the size of the PRK
argument to HKDF was restricted to the output length of H, rather than the block size of H. But as far as I can tell, this mostly works for the hash functions we'd expect people to use (but see below).
We will have to stage this and #139 because the definition of m'
changes in that PR. (EDIT: #139 is now merged, so we'll have to do some conflict resolution here.)
Assuming we go with the string "HASH-TO-CURVE" as in #139, the length of m'
is 14 + H_output_len bytes. Just for sanity, let's see whether this works with the hash functions we might care to use:
- 224-bit hashes: len(
m'
) = 42 bytes- SHA2-224 block size = 64 bytes ✔️
- SHA3-224 block size = 144 bytes ✔️
- BLAKE2s224 block size = 64 bytes ✔️
- 256-bit hashes: len(
m'
) = 46 bytes- SHA2-256 block size = 64 bytes ✔️
- SHA3-256 block size = 136 bytes ✔️
- BLAKE2s256 block size = 64 bytes ✔️
- BLAKE2b256 block size = 128 bytes ✔️
- 384-bit hashes: len(
m'
) = 62 bytes- SHA2-384 block size = 128 bytes ✔️
- SHA3-384 block size = 104 bytes ✔️
- BLAKE2b384 block size = 128 bytes ✔️
- 512-bit hashes: len(
m'
) = 78 bytes- SHA2-512 block size = 128 bytes ✔️
- SHA3-512 block size = 72 bytes ❌ hmmmm
- BLAKE2b512 block size = 128 bytes ✔️
So the only one that's a little weird is SHA3-512. Maybe we should consider replacing "HASH-TO-CURVE" with "H2CURVE", which is 6 bytes shorter and thus works with SHA3-512, or even just "H2C" as @samscott89 has suggested elsewhere.
(Note that len(m'
) > block size isn't fatal---it just requires, per RFC2104, hashing m'
again to give an HMAC key that is shorter than the block length. We should probably avoid this.)
An alternative suggestion: What if we did
This guarantees that the HMAC key ( Also, aesthetically this is slightly nicer, since it moves all of the hash-to-curve--specific domain separation pieces ("H2C", ctr, i) into one place. Finally, this change sort of anticipates the suggestion in my next comment. Let's check to make sure that HMAC uses the minimum number of H invocations in all cases. Recall that HKDF-Expand(k, info, L) in the worst case invokes
where b is 1 byte long. So what we need to check is that
So it looks like "H2C" is preferred if we want to avoid another compression function invocation in the absolute worst case, which is SHA3-512. |
Another question to consider: should we use HKDF-Extract to compute (Just spitballing here, not sure whether I like it or not. Also, I'm going to assume for concreteness that we're going with the suggested change in my prior comment. This could work either way, though.) HKDF-Extract takes two arguments,
If DSS is fixed, H(DSS) can be precomputed to save one invocation of H. Also, this lets people use domain separation strings of arbitrary length with effectively no performance penalty. |
As an aside, STROBE would handle this role fairly cleanly too. |
Great! Since this is a very general framework, is there a specific STROBE-related hash function that you have in mind here? (My guess is that our initial ciphersuite specs will all use hashes in the SHA2 family, but I'm certain that other people will eventually want to use, e.g., BLAKE. So probably the action item with respect to STROBE is just to make sure that we're not accidentally specifying something that's incredibly inefficient.) |
That is good to know, though I don't think we could adopt it so easily at the moment. |
I'm fine with this change, though I think I'd remove the initial hash computation of DSS. My reasoning being that HKDF will compute this hash anyway if |DSS| > H's output size anyway. Thanks for the suggestion!
|
This seems to resolve itself by just using Extract() before Expand(). :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
These are pretty small nits, even though they look like a bunch of comments...
One other small thing: should we add a forward ref from {{domain-separation}} to here?
Maybe a standalone paragraph before the one that starts "Care is required..." that says something like
{{hashtobase}} specifies how to apply a domain separation tag.
I think only the keccak-f(1600) based STROBE variant has any implementations right now. @chris-wood Are you saying the hash-to-field functions call extract in a tree like way? It's true STROBE does not add so much for trees where you'd clone the state all the time. You'd need to impose an ordering on the extractions to exploit STROBE optimally. And doing so encurages constraints on the order in which developers extract field elements. It's actually common to clone STROBE states, which may still save some stack space over HKDF, but not much, and maybe worse with hand optimizations. I suppose the most efficient scheme for extracting a tree is to simply use ChaCha20, assigning nonces in a tree-like way using "heap addressing". All this is moot because BLS is really for consensus protocols, not "accounts", so nobody will ever run BLS on ridiculously constrained devices anyways, like say a Ledger device. |
Co-Authored-By: Riad S. Wahby <[email protected]>
Co-Authored-By: Riad S. Wahby <[email protected]>
Co-Authored-By: Riad S. Wahby <[email protected]>
No, sadly, my comment was more reflective about IETF than it was about anything technical. (We'd need to fully specify STROBE here or elsewhere prior to adopting it.) |
Right, I'm actually not convinced STROBE is optimal anyways. I'd think the |
Awesome! I just realized there's one more bit of inconsistency that this PR should fix: the description in {{hashtobase-perf}}. I opened #143 against the PR branch because suggested edits can't yet do multiline as far as I can tell. |
draft-irtf-cfrg-hash-to-curve.md
Outdated
parameter of the cryptosystem (e.g., k = 128). | ||
- HKDF-Extract-H is the HKDF-Extract function of RFC5869 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this syntax a little confusing. Is the idea that the H in HKDF-Extract-H
should be expanded to SHA2, etc, in each case?
I suggest we stick to the same notation used in the original draft, and others like the TLS 1.3 draft. So, just use "HKDF-Extract" and specify under that the hash function used is given by the ciphersuite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we just called it "Extract"?
- Extract is the HKDF-Extract function of RFC5869 instantiated with
hash function H.
Is that clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
- HKDF-Expand and HKDF-Extract are as defined in {{rfc5869}}, instantiated with the hash function H
Again, keeping it closer to notation used elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I preferred the -H notation since it made clear that H determined Extract, though the expansion issue is a valid concern. I'm fine with the proposal!
update description in {{hashtobase-perf}}
…-to-curve into caw/hkdf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Just one tiny annoyance, sorry...
Co-Authored-By: Riad S. Wahby <[email protected]>
Nits are always appreciated! No need to apologize. |
Addresses #137.