elliptic-curve: ToEncodedPoint should be fallible

[rozbb]

The point at infinity has no SEC1 bytestring representation. If an AffinePoint is the point at infinity, this method should fail. Currently, it will just output whatever garbage is in the x,y coordinates of the point.

https://github.com/RustCrypto/elliptic-curves/blob/bd7c9ea91a2e6bdb2f3fd71290708459b8aabc5f/p256/src/arithmetic/affine.rs#L150-L154

This might be difficult to fix, since ToEncodedPoint::to_encoded_point is an infallible method. Options are:

1. Make the point at infinity non-representable again in AffinePoint (i.e., remove the infinity field)
2. Change ToEncodedPoint to have an associated ErrorType which can be set to !
3. Ignore this and just make a big warning on the method

[tarcieri]

Aah yes, good catch. Note that this is actually a problem with the sec1::ToEncodedPoint trait in the elliptic-curve crate (and as such, I might consider moving the issue over there).

When the ToEncodedPoint trait was originally created, the assumption was that AffinePoint was always a valid point on the curve and could not represent the identity. We changed this later to make conversions from ProjectivePoint to AffinePoint infallible, but it seems ToEncodedPoint was never updated to reflect this.

[tarcieri]

Note: moved this to https://github.com/RustCrypto/traits/

We just cut another breaking release of the elliptic-curve crate. It seems important to address, but it might be nice to wait a bit for some more breaking changes.

That said, I'd also like to bump the pkcs8 crate dependency, so perhaps I can cut another breaking release.

[rozbb]

Gotcha, so I assume the plan is to make ToEncodedPoint fallible. No rush on my behalf. I keep an invariant that my pubkeys are never O, so the infallibility is appropriate in my case.

[tarcieri]

I double checked this as I thought there was a conversion defined:

The point at infinity has no SEC1 bytestring representation.

There does appear to be one. From SEC1 section 2.3.3:

https://www.secg.org/sec1-v2.pdf

I believe that translates from the wacky SEC1 notation to a single null byte b"\0" in Rust syntax.

A separate question is: do we actually want to support this? Though sec1::EncodedPoint already supports variable-length encodings and could theoretically support this, I would vote against it.

My reasoning is I don't think there's a practical use case, and it would require making several parts of the sec1::EncodedPoint API fallible where they presently aren't.

I also think users may not be expecting that a SEC1-encoded point maps to the identity, so it may introduce a surprising edge case where there presently isn't one.

I think it makes sense to do what this proposal originally suggested and make to_encoded_point() fallible.

[rozbb]

Oh wow I really should have double checked before making the claim. Good catch. I'm actually inclined to do the opposite. Having total functions is nice, and since the standard supports it, it's no extra work on our part.

I agree though that a user might expect a SEC1 encoded point to be nonzero. I actually need this to be true for rust-hpke. So what I do is just check that the input bytes have length commensurate with an uncompressed point (HPKE uses uncompressed point repr). Maybe we can keep the infallibility if we document from_sec1 enough, or provide another function like from_nonzero_sec1.

[tarcieri]

So perhaps there's a middle ground option: add support to EncodedPoint for the identity encoding, but enforce the invariant that PublicKey is not the identity, which should prevent it from popping up unexpectedly in high-level cases.

[rozbb]

That sounds like a good compromise to me. It's more or less what rust-hpke already does