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

x-only ECDH support #994

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rustyrussell
Copy link
Contributor

Technically this isn't required: you can always implement an x-only hashfn yourself, and convert to a compressed pubkey to call secp256k1_ecdh(). But I'm not smart enough to figure that out myself (thanks @jonasnick !) so here we are.

API question: should the xonly version take a scalar or a keypair?

Impl question: should this implementation live in ecdh, extrakeys, or even a new ecdh_extrakeys module?

Thanks!

In a future world where xonly pubkeys rule, it seems strange to hash
the y-coordinate.  It also fails if you need to invert one of the
keys for the ECDH.

Suggested-by: Jonas Nick
Signed-off-by: Rusty Russell <[email protected]>
This is in the ecdh module, since that allows convenient inline access.
The alternative would be to expose ecdh_core() and put this inside
extrakeys, which would avoid open-coding secp256k1_xonly_pubkey_load().

Signed-off-by: Rusty Russell <[email protected]>
@real-or-random
Copy link
Contributor

Thanks! Unfortunately this may be more involved than you'd expect.

In a future world where xonly pubkeys rule, it seems strange to hash
the y-coordinate.

Hm, I don't think it's strange. So far, we use 33 bytes pubkeys but hash 64 bytes. Is that strange? I guess not.

So my real question is if there are other advantages to hashing only x. Here's one you may not have been aware of: The point multiplication for x-only can be made faster (#262).

It also fails if you need to invert one of the
keys for the ECDH.

That's exactly why people didn't like it back then. When "in doubt", crypto should rather fail than succeed. So the failure is a feature, and removing it adds a potential footgun. This is exactly why we didn't like #262 back then, see #262 (comment) for the relevant comments.

Now I don't think the footgun potential is very high, and if you read the comments there, people weren't strongly opposed to the idea. You could argue that BIP32 derivation and then converting the key to x-only is similar and BIP 340 accepted this fact (very explicitly even: https://github.com/sipa/bips/blob/bip-taproot/bip-0340.mediawiki#public-key-conversion). Moreover, X25519 (which is widely used, e.g., in Noise) uses x-only ECDH key and I never heard of any subtle issue, even though RFC7748 has a similar warning:

Designers using these curves should be aware that for each public
key, there are several publicly computable public keys that are
equivalent to it, i.e., they produce the same shared secrets. Thus
using a public key as an identifier and knowledge of a shared secret
as proof of ownership (without including the public keys in the key
derivation) might lead to subtle vulnerabilities.

(https://datatracker.ietf.org/doc/html/rfc7748#section-7)

Of course, all of this depends on the inputs to the ECDH function.
If it's an x-only -> x-only function, then formally speaking there's no issue because you simply can't negate the input pubkeys. On the other hand, that argument is mood if people effectively derive their x-only keys from full keys.

Taking a further step back:

In a future world where xonly pubkeys rule,

Is it really true that x-only pubkeys will rule? The way I see x-only pubkeys is that this is mostly an optimization when putting keys on chain. So you can use your normal BIP32/whatever key derivation step and only as the very last step when you're going to use the key on the chain, you drop a byte. But I admit that this is more a rule of thumb than a strict requirement, and I'm not sure if everyone will agree. I think this deserves a broader discussion. In general, I feel we have added a confusing element to BIP340 with x-only keys.

@rustyrussell
Copy link
Contributor Author

The way I see x-only pubkeys is that this is mostly an optimization when putting keys on chain.

I see things differently from you: compressed pubkeys are a vestigial tail, especially the 0x02/0x03 SEC1 encoding which is arbitrary.

As a simple crypto API user, 32-byte pubkeys are a minor but clear API improvement. They align nicely, they index ~uniformly, and I expect with encouragement and tooling they will become the standard over time.

So, I've started using x-only pubkeys everywhere in new greenfields projects. They're used in https://github.com/rustyrussell/centurymetadata (where I hit this ECDH issue) and in BOLT12 offers so far. I'm considering them for my gossip update proposal, too.,

I think future generations will thank us for encouraging and supporting their ubiquitous adoption.

@rustyrussell
Copy link
Contributor Author

Hm, I don't think it's strange. So far, we use 33 bytes pubkeys but hash 64 bytes. Is that strange? I guess not.

BTW, I think this is wrong: the default hashes 33 bytes, a-la a compressed key:

    unsigned char version = (y32[31] & 0x01) | 0x02;
    secp256k1_sha256 sha;
    (void)data;

    secp256k1_sha256_initialize(&sha);
    secp256k1_sha256_write(&sha, &version, 1);
    secp256k1_sha256_write(&sha, x32, 32);
    secp256k1_sha256_finalize(&sha, output);

@real-or-random
Copy link
Contributor

Hm, I don't think it's strange. So far, we use 33 bytes pubkeys but hash 64 bytes. Is that strange? I guess not.

BTW, I think this is wrong: the default hashes 33 bytes, a-la a compressed key:

Oh you're right! My point is if it were 64 bytes, it wouldn't be strange either because the two representations are equivalent (whereas 32 bytes are not equivalent). Of course everyone needs to agree on the choice but apart from that you don't even need to know as a user.

@real-or-random
Copy link
Contributor

As a simple crypto API user, 32-byte pubkeys are a minor but clear API improvement. They align nicely, they index ~uniformly, and I expect with encouragement and tooling they will become the standard over time.

But then pragmatically speaking, what you really care about is that there's an ECDH function which takes x-only pubkeys as input and it doesn't really matter what is hashed. Or does it matter?

@rustyrussell
Copy link
Contributor Author

As a simple crypto API user, 32-byte pubkeys are a minor but clear API improvement. They align nicely, they index ~uniformly, and I expect with encouragement and tooling they will become the standard over time.

But then pragmatically speaking, what you really care about is that there's an ECDH function which takes x-only pubkeys as input and it doesn't really matter what is hashed. Or does it matter?

Yes, this is how I got into this mess. The current scheme breaks: given S1->PK1, S2->PK2, ECDH(S1,PK2) != ECHD(S2,PK1) unless PK1 and PK2 are both even (or maybe both odd, not sure). I had to invert the secret if its pubkey was 03...

Further, interop matters, when setting standards (which is what libsecp256k1 is doing here, no doubt). We could also prepend a BIP-340 style tag, but prepending 02 or 03 is just a weird thing to lock in for future generations to ogle at?

I know @gmaxwell disliked the malleation which x-only hashing enabled, but that problem doesn't really exist in an x-only world. With that discounted, the fact that there's a further optimization possible in future is compelling IMHO.

@real-or-random
Copy link
Contributor

The current scheme breaks: given S1->PK1, S2->PK2, ECDH(S1,PK2) != ECHD(S2,PK1) unless PK1 and PK2 are both even (or maybe both odd, not sure). I had to invert the secret if its pubkey was 03...

Yes, ok, that's indeed a problem... I hadn't thought that through. Sorry that it took as that many posts to arrive at the core of the problem.

Here's some background: (as you probably know), the requirement to negate the secret key appears also with signatures. The problem is that you'd need to know the EC point/full pubkey in order to tell whether to negate or not. When we came up with x-only keys in BIP340, our thinking was:

  • We want to stay compatible with secret key generation, so we won't touch that part. (And you can stil generate secret keys without computing the public key.)
  • For BIP340 sigs, we can do the negation at signing time, because the signing algorithm anyway needs to full pubkey because it's put into the challenge hash. In secp256k1, this led to the keypair type in the API which avoids that you need to recompute the pubkey every time you create a signature, and then our signing function takes key pairs.

Now this is different in case of ECDH. There's no natural reason to for the ECDH function to take the own pubkey. So what could do? The only possibilities are:

  1. Stick with full keys
  2. Make the ECDH function take a key pair
  3. Hash only the x-coord (this PR)

I understand you don't like option 1. There's a tendency to re-purpose signature pubkeys (which represent identities) as static keys for key exchange. You can say the ECDH module has been built with this use case in mind, otherwise we'd probably have different datatypes for ECDSA keys and ECDH keys. It's useful in practice, and since BIP340 has x-only keys, people will want to repurpose those x-only keys for ECDH (this PR is an example). Careful crypto engineering tells us that repurposing keys is in general bad practice but in this case we even have a security proof that is very close to what's done in practice [1].

Option 2 is probably okay. It's a performance penalty but if this is a static key, you need to compute the full pubkey part of the key pair only once.

But if we accept that users want to re-purpose x-only signature keys for ECDH, then option 3 is just strictly better... In particular, option 2 does not protect against the malleability issue either (and we anyway don't have evidence that it does any harm in practice).

[1] https://crypto.stackexchange.com/a/3311

@w0xlt
Copy link

w0xlt commented Apr 3, 2022

I created an example of using x-only ECDH from this PR.
Implements the "Basic Scheme" mentioned in the article "Silent Payment".
https://github.com/w0xlt/secp256k1/blob/a9677ad9f064efd6c1f91afb9fa2f5d2ab43cd03/examples/spbs.c

More information at #1097 (comment)

@sipa
Copy link
Contributor

sipa commented Jan 27, 2023

I've opened an alternative: #1198, which uses more efficient x-only EC multiplication routines internally.

@real-or-random
Copy link
Contributor

@rustyrussell We're trying to gauge demand for this. Are you still interested in this feature? If yes, do you agree that this PR could be closed in favor of #1198?

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

Successfully merging this pull request may close these issues.

5 participants