search

sander / hierarchical-deterministic-keys

Hierarchical Deterministic Keys for the European Digital Identity Wallet
3 stars 0 forks source link

Review comments 2024-06-14 #29

Open emlun opened 3 weeks ago

emlun commented 3 weeks ago

Follow-up to #11. This review concerns the version as of commit 5f1a31d0.

Sorry, I didn't have much time to do an in depth review, but it definitely seems a bit clearer now. :slightly_smiling_face: And we probably should indeed extract and expose an ARKG-Derive-Blinding-Factor function in ARKG so that you don't need to do the "hack" here of extracting the blinding factor by using the identity scalar as the secret key argument to ARKG-Derive-Secret-Key. I'll try to get on that in July when I'm back from vacation.

the public key pk_bl associated with sk_device blinded by sk_bl0, and

Maybe call this pk_bl0 to emphasize the relationship with sk_bl0 (if I understood that correctly)?

H: SHA-256 [FIPS180-4] using hash_to_field from [RFC9380] Section 5 with:

I think this would rather be the other way around: "hash_to_field using SHA-256". The approach I took in ARKG was to just reference the hash_to_field function defined in the hash-to-curve suite for that curve, I'd suggest doing the same here:

H: hash_to_field with the parameters:

It's a bit awkward because the suite doesn't define the hash_to_field function directly, just its arguments, but I think this works well enough. At least this way you don't need to restate the L and expand_message arguments, and also outsource the rationale for those argument choices.

An instantiation based on ECDH and MAC

I don't see any MAC being used for proof of possession anymore? Is this implicit?

sander-cb commented 2 weeks ago

Thanks for this follow-up review @emlun!

This review concerns the version as of commit 06bf2131.

Note that 06bf2131eb9535d66ec5f7ec81b3d7bf52b9249c from 2024-05-26 is superseded by 5f1a31d0794cff73065a93a5696bebef6dd76633 which you linked to, which was updated 2024-06-08.

And we probably should indeed extract and expose an ARKG-Derive-Blinding-Factor function in ARKG so that you don't need to do the "hack" here of extracting the blinding factor by using the identity scalar as the secret key argument to ARKG-Derive-Secret-Key. I'll try to get on that in July when I'm back from vacation.

In the 2024-06-08 version, the hack was not needed anymore. Let’s look in July if the current way of applying ARKG is more idiomatic.

An instantiation based on ECDH and MAC

I don't see any MAC being used for proof of possession anymore? Is this implicit?

Indeed I have left this implicit, since the MAC generation seems out of scope for the interaction with the WSCD.

To resolve the comments, we need to:

emlun commented 2 weeks ago

Note that 06bf213 from 2024-05-26 is superseded by 5f1a31d which you linked to, which was updated 2024-06-08.

Oops, my bad - I copied the Markdown from my previous comment and updated the URL but not the display text. Fixed now.

In the 2024-06-08 version, the hack was not needed anymore

Oh? I saw this passage still in the text: "Note that the HDK scheme does not apply ARKG-Derive-Private-Key to the actual device private key [...]. Instead, HDK applies ARKG-Derive-Private-Key to the tree node’s key blinding private key [...]"

sander-cb commented 2 weeks ago

In the 2024-06-08 version, the hack was not needed anymore

Oh? I saw this passage still in the text: "Note that the HDK scheme does not apply ARKG-Derive-Private-Key to the actual device private key [...]. Instead, HDK applies ARKG-Derive-Private-Key to the tree node’s key blinding private key [...]"

Right, this is still in. But it’s way less hacky than it was a few weeks ago – indeed with an identity scalar as ARKG-Derive-Private-Key input. It seems like the way HDK 5f1a31d0794cff73065a93a5696bebef6dd76633 applies ARKG to the blinding scalar is quite idiomatic, and the issuer-wallet interface may be compatible with alternative ARKG applications.

emlun commented 2 weeks ago

Alright, I'll have another look when I'm back from vacation in two weeks!