Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

secp256k1 DHKEM #59

Merged
merged 4 commits into from
Jul 5, 2024
Merged

secp256k1 DHKEM #59

merged 4 commits into from
Jul 5, 2024

Conversation

DanGould
Copy link

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 DanGould mentioned this pull request Mar 21, 2024
@rozbb
Copy link
Owner

rozbb commented Mar 22, 2024

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

@DanGould DanGould changed the base branch from main to unstable-k256-dev March 22, 2024 02:08
@DanGould DanGould changed the base branch from unstable-k256-dev to main March 22, 2024 02:08
@DanGould
Copy link
Author

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

@rozbb
Copy link
Owner

rozbb commented Mar 27, 2024

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?

@DanGould
Copy link
Author

DanGould commented Mar 27, 2024

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?

@erskingardner
Copy link

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.

@DanGould
Copy link
Author

DanGould commented Jul 5, 2024

@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.

@rozbb
Copy link
Owner

rozbb commented Jul 5, 2024

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 rozbb changed the base branch from main to unstable-k256 July 5, 2024 15:51
@rozbb rozbb merged commit 81a7782 into rozbb:unstable-k256 Jul 5, 2024
@DanGould DanGould mentioned this pull request Jul 5, 2024
@rozbb
Copy link
Owner

rozbb commented Jul 5, 2024

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?

@DanGould
Copy link
Author

DanGould commented Jul 5, 2024

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.

@erskingardner
Copy link

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.

@rozbb
Copy link
Owner

rozbb commented Jul 6, 2024

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.

@erskingardner
Copy link

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.

@DanGould
Copy link
Author

@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.

@erskingardner
Copy link

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?

@DanGould
Copy link
Author

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.

@erskingardner
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DHKEM over secp256k1
3 participants