From 376c96fc1db3aa35669fc9913bb25a87eb8a1ffc Mon Sep 17 00:00:00 2001
From: Tony Arcieri <bascule@gmail.com>
Date: Wed, 16 Dec 2020 13:41:20 -0800
Subject: [PATCH] elliptic-curve: make `ToEncodedPoint` fallible

Closes #399.

As discussed in the aforementioned issue, this changes `ToEncodedPoint`
to be fallible, returning an `Option<EncodedPoint<C>>` with the
expectation that `None` is returned for the additive identity
(a.k.a. point-at-infinity)

Brief background: originally it was not expected for `AffinePoint` to be
able to encode the additive identity, however supporting it allowed for
infallible conversions between `AffinePoint` <-> `ProjectivePoint`.

Technically, according to SEC1 Section 2.3.3, the identity can be
represented by the `Elliptic-Curve-Point-to-Octet-String` encoding by
using a single NULL byte (i.e. `[0u8]`).

However, this would have the disadvantage of complicating
`EncodedPoint`, making APIs that are presently infallible fallible, and
may be unexpected by end users of the traits.

If there's a legitimate use case for supporting SEC1-encoded identity
points, we can revisit this tradeoff, adding support for encoding the
identity element to `EncodedPoint`, and making `ToEncodedPoint`
infallible once again.
---
 elliptic-curve/src/ecdh.rs       |  7 ++++++-
 elliptic-curve/src/public_key.rs | 18 +++++++++++-------
 elliptic-curve/src/sec1.rs       | 18 ++++++++++++++----
 3 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/elliptic-curve/src/ecdh.rs b/elliptic-curve/src/ecdh.rs
index e9a717771..c1f9272ba 100644
--- a/elliptic-curve/src/ecdh.rs
+++ b/elliptic-curve/src/ecdh.rs
@@ -78,7 +78,12 @@ where
         #[allow(clippy::op_ref)]
         let shared_secret = public_key.to_projective() * &*self.scalar;
         // SharedSecret::new expects an uncompressed point
-        SharedSecret::new(shared_secret.to_affine().to_encoded_point(false))
+        SharedSecret::new(
+            shared_secret
+                .to_affine()
+                .to_encoded_point(false)
+                .expect("secret is identity"),
+        )
     }
 }
 
diff --git a/elliptic-curve/src/public_key.rs b/elliptic-curve/src/public_key.rs
index 607acb576..c0292d883 100644
--- a/elliptic-curve/src/public_key.rs
+++ b/elliptic-curve/src/public_key.rs
@@ -152,7 +152,7 @@ where
     }
 }
 
-impl<C> From<PublicKey<C>> for EncodedPoint<C>
+impl<C> TryFrom<PublicKey<C>> for EncodedPoint<C>
 where
     C: Curve + ProjectiveArithmetic + point::Compression,
     FieldBytes<C>: From<Scalar<C>> + for<'r> From<&'r Scalar<C>>,
@@ -162,12 +162,14 @@ where
     UntaggedPointSize<C>: Add<U1> + ArrayLength<u8>,
     UncompressedPointSize<C>: ArrayLength<u8>,
 {
-    fn from(public_key: PublicKey<C>) -> EncodedPoint<C> {
-        EncodedPoint::<C>::from(&public_key)
+    type Error = Error;
+
+    fn try_from(public_key: PublicKey<C>) -> Result<EncodedPoint<C>, Error> {
+        EncodedPoint::<C>::try_from(&public_key)
     }
 }
 
-impl<C> From<&PublicKey<C>> for EncodedPoint<C>
+impl<C> TryFrom<&PublicKey<C>> for EncodedPoint<C>
 where
     C: Curve + ProjectiveArithmetic + point::Compression,
     FieldBytes<C>: From<Scalar<C>> + for<'r> From<&'r Scalar<C>>,
@@ -177,8 +179,10 @@ where
     UntaggedPointSize<C>: Add<U1> + ArrayLength<u8>,
     UncompressedPointSize<C>: ArrayLength<u8>,
 {
-    fn from(public_key: &PublicKey<C>) -> EncodedPoint<C> {
-        public_key.to_encoded_point(C::COMPRESS_POINTS)
+    type Error = Error;
+
+    fn try_from(public_key: &PublicKey<C>) -> Result<EncodedPoint<C>, Error> {
+        public_key.to_encoded_point(C::COMPRESS_POINTS).ok_or(Error)
     }
 }
 
@@ -210,7 +214,7 @@ where
 {
     /// Serialize this [`PublicKey`] as a SEC1 [`EncodedPoint`], optionally applying
     /// point compression
-    fn to_encoded_point(&self, compress: bool) -> EncodedPoint<C> {
+    fn to_encoded_point(&self, compress: bool) -> Option<EncodedPoint<C>> {
         self.point.to_encoded_point(compress)
     }
 }
diff --git a/elliptic-curve/src/sec1.rs b/elliptic-curve/src/sec1.rs
index 577d46eb2..4d8b947f3 100644
--- a/elliptic-curve/src/sec1.rs
+++ b/elliptic-curve/src/sec1.rs
@@ -141,6 +141,7 @@ where
         (C::ProjectivePoint::generator() * secret_key.secret_scalar())
             .to_affine()
             .to_encoded_point(compress)
+            .expect("secret is identity")
     }
 
     /// Get the length of the encoded point in bytes
@@ -204,7 +205,7 @@ where
         match self.coordinates() {
             Coordinates::Compressed { x, y_is_odd } => {
                 AffinePoint::<C>::decompress(x, Choice::from(y_is_odd as u8))
-                    .map(|s| s.to_encoded_point(false))
+                    .map(|s| s.to_encoded_point(false).expect("point is identity"))
                     .into()
             }
             Coordinates::Uncompressed { .. } => Some(self.clone()),
@@ -212,11 +213,11 @@ where
     }
 
     /// Encode an [`EncodedPoint`] from the desired type
-    pub fn encode<T>(encodable: T, compress: bool) -> Self
+    pub fn encode<T>(encodable: T, compress: bool) -> Result<Self, Error>
     where
         T: ToEncodedPoint<C>,
     {
-        encodable.to_encoded_point(compress)
+        encodable.to_encoded_point(compress).ok_or(Error)
     }
 
     /// Decode this [`EncodedPoint`] into the desired type
@@ -366,7 +367,16 @@ where
 {
     /// Serialize this value as a SEC1 [`EncodedPoint`], optionally applying
     /// point compression.
-    fn to_encoded_point(&self, compress: bool) -> EncodedPoint<C>;
+    ///
+    /// # Returns
+    ///
+    /// `None` if this point is the additive identity (a.k.a. point-at-infinity).
+    ///
+    /// Though the additive identity can technically be encoded using the
+    /// `Elliptic-Curve-Point-to-Octet-String` encoding specified in SEC1
+    /// Section 2.3.3, this encoding is not supported by [`EncodedPoint`] and
+    /// implementers of this trait should return `None` for this case instead.
+    fn to_encoded_point(&self, compress: bool) -> Option<EncodedPoint<C>>;
 }
 
 /// Tag byte used by the `Elliptic-Curve-Point-to-Octet-String` encoding.