Add fixed-key AES specification #193
Conversation
There was a problem hiding this comment.
I’m still working through the PR, but looking good so far. I should have a full pass done by the time we meet. In the meantime, three high-level things:
- In the markdown version of PrgSha3 I replaced the PyCryptodome APIs with the functions as defined in the SHA-3 spec. Could we do the same for PrgFixedKeyAes128?
- I think we should "fix" PrgFixedKeyAes128 as the only Prg used for IdpfPoplar. The main reason is that we want to generate test vectors for this, since we'll have to implement it elsewhere (in libprio-rs in particular).
- You mentioned you consulted with one of the authors of {{GKWWY?20}} -- it would be appropraite to acknowledge them in the Acknwledgements section.
cjpatton
force-pushed
the
schoppmp/fixed_key_aes
branch
from
f4cd43d to
c7a2885
Compare
schoppmp
force-pushed
the
schoppmp/fixed_key_aes
branch
from
c7a2885 to
4a6c2f9
Compare
cjpatton
force-pushed
the
schoppmp/fixed_key_aes
branch
from
ec4b4e0 to
de52a7d
Compare
There was a problem hiding this comment.
I've made addressed some of the comments and have pushed a few changes. I think this is in decent shape, but I would appreciate careful feedback before reviewing. And of course if there are any changes I made that you don't like, feel free to override me.
Non-blocking comments in-line.
|
Let's make sure to ACK whoever you worked with on this. |
Done. |
|
The current version looks great, thanks for the help! |
cjpatton
force-pushed
the
schoppmp/fixed_key_aes
branch
from
f8cdddb to
4ba234f
Compare
|
Squashed. |
| # Function `AES128(key, block)` is the AES-128 blockcipher. | ||
| def hash_block(self, block): | ||
| lo, hi = block[:8], block[8:] | ||
| sigma = hi + xor(hi, lo) |
There was a problem hiding this comment.
Is the '+' operator here a concatenation? It might be better to make that explicit because I came away with the distinct impression that you were losing the upper bits of the value here.
There was a problem hiding this comment.
sigma is a function, not a variable, so perhaps you could frame it that way
There was a problem hiding this comment.
- Yes, "+" means concatenation here. We can replace with "concat()" for clarity?
- Actually, sigma is meant to be a variable (it is input to AES, then xored with the output).
There was a problem hiding this comment.
https://eprint.iacr.org/2019/074.pdf Section 7.3 (last paragraph, "Instantiating
There was a problem hiding this comment.
Agreed, explicitly calling concat would be better here. We can call the variable sigma_block or so to emphasize that it is the function sigma evaluated on block?
| # The multi-instance tweakable circular correlation-robust hash function of | ||
| # [GKWWY20] (Section 4.2). |
There was a problem hiding this comment.
I don't see any tweaking going on here.
There was a problem hiding this comment.
If you look at Section 4.2 in https://eprint.iacr.org/2019/1168.pdf, the tweak i is passed as the key to the block cipher. So for a single client, the tweak stays the same, but it differs across clients.
| # | ||
| # Implementation note: This step can be cached across PRG | ||
| # evaluations with many different seeds. | ||
| self.fixed_key = cSHAKE128(binder, 16, b'', custom) |
There was a problem hiding this comment.
Does this really need to use a hash function that barely anyone has? I know that ... shiny... but this is a bit of a pain.
There was a problem hiding this comment.
Do you mean SHA-3 or cSHAKE in particular?
We're using SHA-3 because we needed an XOF. We looked at using HKDF-SHA-256 as an alternative, but the performance was pretty bad. Plus, we have some cases where we need longer outputs than HKDF permits.
We're using cSHAKE for PrgSha3 as well. What's nice about it is that we get distinct notions of "customization" and "binder" strings. (See https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-vdaf-05#section-6.2.3.)
There was a problem hiding this comment.
FWIW, @simon-friedberger may be planning to implement this in NSS :)
There was a problem hiding this comment.
We could use SHA-2 here if for example you waned PrgFixedKeyAes128 but not PrgSha3.
There was a problem hiding this comment.
Simon already reached out about the nasty complexity that this produces for us. I think that the current approach involves a bunch of non-NSS crypto, which is sort-of fine, but suboptimal.
I realize that there are advantages to using the new stuff, but that doesn't make it less awkward.
There was a problem hiding this comment.
Oh, I should say, why are you concerned about the performance of something that you instantiate so few times during a protocol run? We're using HKDF with SHA-256 in our prototypes of IPA and it's in the noise, even for tiny runs.
There was a problem hiding this comment.
The reason I chose cSHAKE here was to be consistent with PrgSha3 and not require yet another hash function dependency. If this is creating problems for implementations, I don't have a problem with switching to HKDF-SHA-256, or even simply SHA-256/128. We just want the AES key to be random for each client and don't require anything else from the hash function.
There was a problem hiding this comment.
It's reasonable to rely on the same primitive here. If we ever allow more options for the PrgSha3 this should be updated as well but it's fine for now.
There was a problem hiding this comment.
@martinthomson the performance issue with HKDF comes up when we need an XOF, like we need in Prio3. (See #106 (comment).) I agree the performance cost would be negligible, and I also think plain SHA-2 would be fine.
Closes #32.