search

cisco / go-hpke

Implementation of draft-irtf-cfrg-hpke
BSD 2-Clause "Simplified" License
30 stars 15 forks source link

Add DeriveKeyPair and apply other minor changes #30

Closed chris-wood closed 4 years ago

chris-wood commented 4 years ago

This change adds the DeriveKeyPair KEM interface in lieu of the GenerateKeyPair interface. It also updates the test vectors to store only DeriveKeyPair IKM seeds instead of serialized public and private key shares. (Should we keep the private and public keys in addition to the seeds?)

This change also adds the test vectors as an artifact in the repository.

cc @dmcardle, @rozbb, @raphaelrobert, @kjacobs-moz, @blipp to check against the new test vectors. I think I covered all the diffs from HPKE, but please shout if I missed something!

dmcardle commented 4 years ago

Success! I can verify that the test vectors pass on my end — that is, mode = base, kem = DHKEM(X25519, SHA256), and kdf = HKDF_SHA256.

chris-wood commented 4 years ago

Success! I can verify that the test vectors pass on my end — that is, mode = base, kem = DHKEM(X25519, SHA256), and kdf = HKDF_SHA256.

Sweet! Have you implemented support for any of the NIST curves?

dmcardle commented 4 years ago

Success! I can verify that the test vectors pass on my end — that is, mode = base, kem = DHKEM(X25519, SHA256), and kdf = HKDF_SHA256.

Sweet! Have you implemented support for any of the NIST curves?

No, I think we're only targeting X25519 until we have a need for another curve.

chris-wood commented 4 years ago

No, I think we're only targeting X25519 until we have a need for another curve.

Makes sense. Thanks for clarifying!

rozbb commented 4 years ago

Waiting on the dust to settle on the PR before I get to implementing things

chris-wood commented 4 years ago

@rozbb the spec PR landed. Can you please give these test vectors a shot?

dmcardle commented 4 years ago

OK, latest test vectors are passing for me!

rozbb commented 4 years ago

In the process of comparing and contrasting. One thing that I think needs to be re-added is serialized pubkeys/privkeys. It's way easier to check my DeriveKeyPair impl if I have actual keys to compare to. Also I think this diff wiped out all uses of serialize/deserialize, which is bad from a testing perspective.

rozbb commented 4 years ago

Oh, I take that back. It looks like seedE is explicitly compared to a pubkey, i.e., the encapped key. Still, secret key deserialization would be good too.

rozbb commented 4 years ago

Tests pass on all X25519 and P256 test vectors. Aside from my previous comments, this is good to go.

chris-wood commented 4 years ago

@rozbb I brought back the serialize key material. :)

chris-wood commented 4 years ago

Updated to take #127 into account. @bifurcation, please have a look.

chris-wood commented 4 years ago

@dmcardle and @rozbb, can you please give these new vectors a shot with your implementation?

rozbb commented 4 years ago

Can you update the test vectors to reflect the kemID change?

chris-wood commented 4 years ago

Can you update the test vectors to reflect the kemID change?

I forgot to push that. Stand by!

Update: @rozbb, @dmcardle: done!

dmcardle commented 4 years ago

Cool, test vectors are passing for me.

rozbb commented 4 years ago

All tests passing (X25519 and P-256). This helped me catch a ton of errors in my own codebase.