rust-hpke icon indicating copy to clipboard operation
rust-hpke copied to clipboard

secp256k1 DHKEM

Open DanGould opened this issue 1 year ago • 4 comments

Close #50

Supersede #52 by filling in the test cases.

I found no k256 test vectors including a K256_DH_RES_XCOORD and so calculated one myself using a separate production ecdh library, libsecp256k1.

Test vector keys were taken from the draft's ChaCha20-Poly1305 first base test case. The test vector ecdh x coordinate was calculated using libsecp256k's rust bindings with this code since the "secp256k1-based DHKEM for HPKE" doc only displays the "shared_secret" which has been extracted and expanded, not a DH result x coordinate.

DanGould avatar Mar 19 '24 20:03 DanGould

Thank you! I'm sorry I'm so busy with school rn I will get around to this in 2-3 weeks. Two small things until then:

  1. Could you modify to merge to a new branch if possible? unstable-k256
  2. Could you write the code for generating your test vectors somewhere if it's not too long? Always nice to have as much documentation as possible

Thanks again, this looks great

rozbb avatar Mar 22 '24 01:03 rozbb

Could you modify to merge to a new branch if possible? unstable-k256

I don't appear to be able to create or target new branches on this repository. If you were to open an unstable-k256 branch from main then I could target that.

May you please share why you'd like a new branch as well? I think I could learn something from your development flow in understanding why.

Could you write the code for generating your test vectors somewhere if it's not too long? Always nice to have as much documentation as possible

The code for generating k256-ecdh test vectors can be found in this repository. Is this sufficient, or do you mean that you would like them to be committed into this repository?


best of luck with school

DanGould avatar Mar 27 '24 16:03 DanGould

Ok np I can do the branch stuff. The reason is just bc I don't want to put non-specc'd things in the main branch. Unless I misunderstood and K256 is an accepted IETF extension?

rozbb avatar Mar 27 '24 16:03 rozbb

I suppose it is still a draft https://datatracker.ietf.org/doc/draft-wahby-cfrg-hpke-kem-secp256k1/01/

but perhaps it could be gated behind an unstable-k256 feature?

DanGould avatar Mar 27 '24 17:03 DanGould

Any movement on this? I'm working on a project that needs to use hpke with secp256k1.

It would be fine, IMO, to see this gated behind a k256 feature flag.

erskingardner avatar Jul 05 '24 15:07 erskingardner

@erskingardner, Yes @nyonson and I are working on a version that uses the rust-secp256k1 bindings, since that's already in our dependency tree (as I imagine is in yours for nostr stuff), instead of k256, which seems less likely to be already in the dep tree for those actually using secp256k1.

You can find that branch Here: https://github.com/DanGould/rust-hpke/tree/secp

Since chacha20poly1305 is also a bitcoin-native dependency now, there's also an effort to get those primitives into rust-bitcoin so that HPKE (and eventually ohttp can depend entirely on bitcoin deps for all cryptography with a certain set of feature gates.

I'm curious what rozbb thinks of using that dependency for secp.

DanGould avatar Jul 05 '24 15:07 DanGould

Hi all. Apologies for the stall. I’m happy to merge now into a separate branch. I don’t see a strong reason not to put this in main beyond what I said above. I’ll discuss on the RustCrypto Zulip shortly and make a decision. In the meantime, merging. Thank you!!

rozbb avatar Jul 05 '24 15:07 rozbb

Re using libsecpk256, I’m much more inclined to keep it as-is, ie with k256. The main draw is that it’s pure Rust, and the diff here is very small. Is there a separate reason you wanted the C-based version besides that some ppl already have it in their dep tree?

rozbb avatar Jul 05 '24 16:07 rozbb

I plan to use the C-based version because that's already used across the rust-bitcoin and nostr ecosystems, which is where my use case (and presumably @erskingardner's) for HPKE lies. Some targets (like Cash App) would likely refuse to ship both the C-based libsecp256k1 and rust-k256 in one binary.

DanGould avatar Jul 05 '24 16:07 DanGould

Thanks for the quick turnaround here.

@DanGould My situation is a bit different. The OpenMLS library that I'm working in uses the RustCrypto k256 library which in turn uses this library.

However, OpenMLS has built the library to allow for library users to select which crypto library they want to use so I'm looking at whether it makes sense to try and add a simple wrapper there to use the rust-secp256k1 library instead of using the RustCrypto based version. From everyone I've talked to, the rust-secp256k1 library is the one that most in the bitcoin/nostr space are using so it'd be best to align there eventually.

Admittedly, I'm still learning my way around Rust and the Rust ecosystem of crates though so you guys shouldn't base any decisions on what I'm doing at this point. 😅 JS and Ruby to Rust has been a steep learning curve.

One other note. I emailed the guy who proposed adding secp256k1 to the HKPE RFC. I'm pasting his response below but tl;dr, sounds like IANA has approved and given a number to secp256k1 DHKEM in the 9180 RFC, it's just that the updates don't propagate back to the actual RFC doc. 🤷‍♂️ In any case, this change is actually official so we probably shouldn't refer to it as unstable or off-spec.

The short version: the contents of the draft are usable as-is, and IANA has allocated a code point for use with secp256k1. See here: https://www.iana.org/assignments/hpke/hpke.xhtml

The slightly longer version: IETF essentially never updates RFCs once they're published. So the usual process, and the one used by HPKE, is to use an IANA registry for code points and to have that registry point to the place where the code point's corresponding definitions live. (IANA first asks the RFC's authors to give the thumbs-up to those definitions, so you can rest assured that the HPKE authors have reviewed the contents of my draft.)

For IETF drafts, expiration doesn't mean they can't be used anymore, just that they're not under active development. So it's completely safe to use the draft. You may want to point to a specific version number, since IETF lets draft authors publish new versions that can in principle completely change things (but I have no plans to make any updates to the core specification, though I may go back and do a clarity edits pass if I find some time...)

Eventually it's possible that IETF would publish the secp256k1-kem draft as an RFC, but as far as I can tell that's somewhat rarely done for things like this because the overhead of publishing RFCs is high and this has already been vetted by the original RFC's authors.

erskingardner avatar Jul 06 '24 09:07 erskingardner

Ok understood. There are some ambiguities in the RFC though. ~~Eg what public key validation is done?~~(that's actually addressed) What shared secret validation is done? I suspect the answers are "both identical to the P-curves" but that's not written anywhere. I might send a PR when I get a chance.

rozbb avatar Jul 06 '24 15:07 rozbb

JFYI - I've created a PR on another HPKE crate hpke-rs to enable secp256k1 (using RustCrypto). I've also created a separate crate that implements the hkpe-rs crypto traits using secp256k1 library instead of RustCrypto. This is specifically for adding Nostr to MLS so it's deliberately simple.

erskingardner avatar Jul 15 '24 07:07 erskingardner

@erskingardner While I would like to ultimately land this feature into this crate so it can be used easily with ohttp, I have also forked this repository into bitcoin-hpke that implements the hpke traits using rust-secp256k1 instead of RustCrypto's k256. The change is feature gated there and could just as easily be feature gated here.

DanGould avatar Aug 11 '24 17:08 DanGould

Thanks for the heads up @DanGould. I guess we'll have multiple libraries doing hpke with secp256k1, no harm there since different downstream libraries are likely to use different hpke implementations. I'm having an issue currently with the test_vectors from the HPKE RFC, doesn't look like your tests try and test against those?

erskingardner avatar Aug 12 '24 10:08 erskingardner

I see your tests here and since the rust-hpke crate here wasn't doing using those test vectors, I wasn't either, but perhaps I should. Seems critical to ensure compatibility across multiple implementations.

DanGould avatar Aug 12 '24 14:08 DanGould

Yeah, I agree. We need to have some test vector coverage. The newer test vectors from the secp256k1 addition have a few issues.

The base vectors are ok but don't include any encryptions or exports so you're only testing key and secret generation. In any case, I've added those as is to the test_vectors.json file already.

The auth vectors don't work at all. The sender and receiver keys are the same which is wrong I think. I've been in touch with the author and I've been trying to debug/build new ones today. I'll keep you in the loop as soon as I have some that I think are accurate.

erskingardner avatar Aug 12 '24 15:08 erskingardner