Add support for secp256k1 curve

[erskingardner]

This adds support for the secp256k1 curve.

The curve now has an official IANA HPKE KEM identifier.

p.s. I'm pretty new to Rust so any feedback is very welcome!

[erskingardner]

@franziskuskiefer anything I can do here to help get this PR reviewed?

[franziskuskiefer]

Thanks for the contribution! (I just had to find the time to look at it ...)
It generally looks good to me. Only a few nits.

[erskingardner]

@franziskuskiefer I've put all the k256 vectors in a single file and added a few of the auth vectors (while changing the init values so we get passing tests there). For now I've left the kat tests pointing to this k256.json file, but I'll merge these to the main test_vectors.json file when we're ready to merge.

A few questions for you:

1. Do we think testing just the key generation and values of the other context fields is enough or do we need to have encryptions and exports for each of the k256 tests as well?
2. This probably depends on the answer to 1 but, right now the sender and receiver keys in the auth tests are the same. do you think we should change those to be different?

[franziskuskiefer]

1. Do we think testing just the key generation and values of the other context fields is enough or do we need to have encryptions and exports for each of the k256 tests as well?

I think it would be nice to have everything covered by tests. Since we have the code already, this shouldn't be too much work to run for k256 as well I'd hope?

2. This probably depends on the answer to 1 but, right now the sender and receiver keys in the auth tests are the same. do you think we should change those to be different?

It shouldn't make a difference really, but it looks a little odd of course. If it's not a big hassle, having different keys would be nice.

[erskingardner]

@franziskuskiefer I've just pushed an update with new test vectors (courtesy of DanGould) that include encryptions and exports. I'm still getting some odd behavior with them though.

1. The encryptions and exports all pass the tests
2. The direct_ctx check against the key, nonce, and exporter_secret are failing still. (lines 155-157)

I'd really appreciate if you could have a look at that and let me know if you think that's an issue with my changes to add k256 or if potentially it's an issue with the test vectors.

[franziskuskiefer]

2. The direct_ctx check against the key, nonce, and exporter_secret are failing still. (lines 155-157)

Hm, when you enable the hpke-test-prng feature, you see those tests fail as well. It looks like the code doesn't agree with the test vectors on the key. Looking at some intermediate values it looks like the shared_secret doesn't seem to match.

Can you post a run where all the intermediate values are printed (just like the test vector)? Then we should see where the discrepancy is and whether the code here or the test vector is wrong.

[franziskuskiefer]

I had another look. The key schedule context doesn't seem to be right.

[DanGould]

You're right @franziskuskiefer, the test vectors were not right. I generated new ones which seem to now pass on all 3 implementations I have available: the one here, my rust-hpke/k256 version depending on RustCrypto k256, and my rust-hpke/secp version depending on rust bindings to the C version of libsecp256k1

I've tested that this commit runs here in hpke-rs with the hpke-test-prng feature enabled. let me know what you think. If we have well generated and reproducible test vectors, I'd like to update the RFC draft so that others can find them.

e3a8c8e

risking over communication for clarity: both the way I was deriving keys in my secp256k1 bindings implementation and the key_schedule_context test vector generation were wrong. Cross checking the 3 implementations I was able to find them and generate what I believe to be correct implementations and vectors.

[franziskuskiefer]

Great! @erskingardner can you update the PR and check if all the tests are passing now?

[erskingardner]

@DanGould you're a legend! thanks! I'll have a look at this first thing tomorrow.

[erskingardner]

Actually, I couldn't wait! :) I'm getting passing tests all around 🟢

If you run the tests with RUST_LOG=trace cargo test --test test_hpke_kat you'll see the trace output go through all the vectors (including the k256 vectors that I added to the main test_vectors.json file.

@franziskuskiefer I think this looks good.

Super well done @DanGould 👏 So good to have a set of vectors that pass on three separate implementations.

[DanGould]

@erskingardner thanks for the quick turnaround. note that the original test_vectors.json are reproducible from the go-hpke repository which is why I originally committed it as a separate file. No problem combining them just noting the original rationale.

[franziskuskiefer]

note that the original test_vectors.json are reproducible from the go-hpke repository which is why I originally committed it as a separate file.

I agree, let's use two files.

Otherwise this looks good to me.

The fuzzer is failing, but that should be easy to fix.

[erskingardner]

Moved those tests back to individual files @franziskuskiefer and adjusted the test file to run both sets of vectors.

I have no idea how the fuzzer works. Happy to try and fix it if you want to explain the basics of how to run it.

[erskingardner]

fixed the fuzzer.

[franziskuskiefer]

CI was hanging. I hope it goes through now. Then we can get this merged.

[franziskuskiefer]

We need to update CI first in #69. Looks like we didn't update macos CI runners in a while.

[erskingardner]

Nice! 👍 thanks for sticking through this one with me @franziskuskiefer

[franziskuskiefer]

👍🏻 let me know if you need a new release soonish. I plan to do one, but there's no timeline for it yet.

[erskingardner]

No rush on the release from my side.

…

On 4 Sep 2024, at 08:51, Franziskus Kiefer ***@***.***> wrote: 👍🏻 let me know if you need a new release soonish. I plan to do one, but there's no timeline for it yet. — Reply to this email directly, view it on GitHub <#66 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABRRAHXKPCMCJTMVPUYV73ZU2UYBAVCNFSM6AAAAABKPGZS7CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRYGA2TINJUHA>. You are receiving this because you were mentioned.