Fix clippy::cmp_owned for (sapling, orchard)::keys with `ConstantTimeEq` (#2184)

* Impl subtle::ConstantTimeEq for orchard SpendingKey, use that in Eq/PartialEq

* Use constant time comparisons for secret key data where applicable

This also makes Clippy happier so that we aren't creating types just to compare.

* Fix clippy::cmp_owned for orchard::keys Eq/PartialEq

By impl'ing ConstantTimeEq for those types where leaks of the value
would compromise access or privacy.

* Make clippy::cmp_owned happy for some sapling::keys

Cargo.lock

@@ -3407,9 +3407,9 @@ dependencies = [
 
 [[package]]
 name = "subtle"
-version = "2.3.0"
+version = "2.4.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "343f3f510c2915908f155e94f17220b19ccfacf2a64a2a5d8004f2c3e311e7fd"
+checksum = "1e81da0851ada1f3e9d4312c704aa4f8806f0f9d69faaf8df2f3464b4a9437c2"
 
 [[package]]
 name = "syn"
@@ -4442,6 +4442,7 @@ dependencies = [
  "serde-big-array",
  "sha2",
  "spandoc",
+ "subtle",
  "thiserror",
  "tracing",
  "x25519-dalek",

zebra-chain/Cargo.toml

@@ -41,6 +41,7 @@ secp256k1 = { version = "0.20.2", features = ["serde"] }
 serde = { version = "1", features = ["serde_derive", "rc"] }
 serde-big-array = "0.3.2"
 sha2 = { version = "0.9.5", features=["compress"] }
+subtle = "2.4"
 thiserror = "1"
 x25519-dalek = { version = "1.1", features = ["serde"] }
 

zebra-chain/src/orchard/keys.rs

@@ -23,6 +23,7 @@ use halo2::{
     pasta::pallas,
 };
 use rand_core::{CryptoRng, RngCore};
+use subtle::{Choice, ConstantTimeEq};
 
 use crate::{
     parameters::Network,
@@ -133,7 +134,7 @@ mod sk_hrp {
 /// key types derive from the [`SpendingKey`] value.
 ///
 /// [ps]: https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents
-#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+#[derive(Copy, Clone, Debug)]
 #[cfg_attr(
     any(test, feature = "proptest-impl"),
     derive(proptest_derive::Arbitrary)
@@ -143,9 +144,20 @@ pub struct SpendingKey {
     bytes: [u8; 32],
 }
 
-impl From<SpendingKey> for [u8; 32] {
+impl ConstantTimeEq for SpendingKey {
-    fn from(sk: SpendingKey) -> Self {
+    /// Check whether two `SpendingKey`s are equal, runtime independent of the
-        sk.bytes
+    /// value of the secret.
+    ///
+    /// # Note
+    ///
+    /// This function short-circuits if the networks of the keys are different.
+    /// Otherwise, it should execute in time independent of the `bytes` value.
+    fn ct_eq(&self, other: &Self) -> Choice {
+        if self.network != other.network {
+            return Choice::from(0);
+        }
+
+        self.bytes.ct_eq(&other.bytes)
     }
 }
 
@@ -160,6 +172,20 @@ impl fmt::Display for SpendingKey {
     }
 }
 
+impl From<SpendingKey> for [u8; 32] {
+    fn from(sk: SpendingKey) -> Self {
+        sk.bytes
+    }
+}
+
+impl Eq for SpendingKey {}
+
+impl PartialEq for SpendingKey {
+    fn eq(&self, other: &Self) -> bool {
+        self.ct_eq(other).unwrap_u8() == 1u8
+    }
+}
+
 impl SpendingKey {
     /// Generate a new `SpendingKey`.
     ///
@@ -195,12 +221,20 @@ impl SpendingKey {
 /// A Spend authorizing key (_ask_), as described in [protocol specification
 /// §4.2.3][orchardkeycomponents].
 ///
-/// Used to generate _spend authorization randomizers_ to sign each _Spend
+/// Used to generate _spend authorization randomizers_ to sign each _Action
-/// Description_, proving ownership of notes.
+/// Description_ that spends notes, proving ownership of notes.
 ///
 /// [orchardkeycomponents]: https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents
-#[derive(Copy, Clone, Eq, PartialEq)]
+#[derive(Copy, Clone)]
-pub struct SpendAuthorizingKey(pub pallas::Scalar);
+pub struct SpendAuthorizingKey(pub(crate) pallas::Scalar);
+
+impl ConstantTimeEq for SpendAuthorizingKey {
+    /// Check whether two `SpendAuthorizingKey`s are equal, runtime independent
+    /// of the value of the secret.
+    fn ct_eq(&self, other: &Self) -> Choice {
+        self.0.to_bytes().ct_eq(&other.0.to_bytes())
+    }
+}
 
 impl fmt::Debug for SpendAuthorizingKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -232,11 +266,19 @@ impl From<SpendingKey> for SpendAuthorizingKey {
     }
 }
 
+impl Eq for SpendAuthorizingKey {}
+
+impl PartialEq for SpendAuthorizingKey {
+    fn eq(&self, other: &Self) -> bool {
+        self.ct_eq(other).unwrap_u8() == 1u8
+    }
+}
+
 impl PartialEq<[u8; 32]> for SpendAuthorizingKey {
     // TODO: do we want to use constant-time comparison here?
     #[allow(clippy::cmp_owned)]
     fn eq(&self, other: &[u8; 32]) -> bool {
-        <[u8; 32]>::from(*self) == *other
+        self.0.to_bytes().ct_eq(other).unwrap_u8() == 1u8
     }
 }
 
@@ -248,7 +290,7 @@ impl PartialEq<[u8; 32]> for SpendAuthorizingKey {
 ///
 /// [orchardkeycomponents]: https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents
 #[derive(Copy, Clone, Debug)]
-pub struct SpendValidatingKey(pub redpallas::VerificationKey<SpendAuth>);
+pub struct SpendValidatingKey(pub(crate) redpallas::VerificationKey<SpendAuth>);
 
 impl Eq for SpendValidatingKey {}
 
@@ -275,14 +317,16 @@ impl From<SpendAuthorizingKey> for SpendValidatingKey {
 impl PartialEq for SpendValidatingKey {
     #[allow(clippy::cmp_owned)]
     fn eq(&self, other: &Self) -> bool {
-        <[u8; 32]>::from(self.0) == <[u8; 32]>::from(other.0)
+        // XXX: These redpallas::VerificationKey(Bytes) fields are pub(crate)
+        self.0.bytes.bytes == other.0.bytes.bytes
     }
 }
 
 impl PartialEq<[u8; 32]> for SpendValidatingKey {
     #[allow(clippy::cmp_owned)]
     fn eq(&self, other: &[u8; 32]) -> bool {
-        <[u8; 32]>::from(self.0) == *other
+        // XXX: These redpallas::VerificationKey(Bytes) fields are pub(crate)
+        self.0.bytes.bytes == *other
     }
 }
 
@@ -292,8 +336,16 @@ impl PartialEq<[u8; 32]> for SpendValidatingKey {
 /// Used to create a _Nullifier_ per note.
 ///
 /// [orchardkeycomponents]: https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents
-#[derive(Copy, Clone, PartialEq)]
+#[derive(Copy, Clone)]
-pub struct NullifierDerivingKey(pub pallas::Base);
+pub struct NullifierDerivingKey(pub(crate) pallas::Base);
+
+impl ConstantTimeEq for NullifierDerivingKey {
+    /// Check whether two `NullifierDerivingKey`s are equal, runtime independent
+    /// of the value of the secret.
+    fn ct_eq(&self, other: &Self) -> Choice {
+        self.0.to_bytes().ct_eq(&other.0.to_bytes())
+    }
+}
 
 impl fmt::Debug for NullifierDerivingKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -303,6 +355,8 @@ impl fmt::Debug for NullifierDerivingKey {
     }
 }
 
+impl Eq for NullifierDerivingKey {}
+
 impl From<NullifierDerivingKey> for [u8; 32] {
     fn from(nk: NullifierDerivingKey) -> [u8; 32] {
         nk.0.to_bytes()
@@ -339,12 +393,16 @@ impl From<SpendingKey> for NullifierDerivingKey {
     }
 }
 
-impl Eq for NullifierDerivingKey {}
+impl PartialEq for NullifierDerivingKey {
+    fn eq(&self, other: &Self) -> bool {
+        self.ct_eq(other).unwrap_u8() == 1u8
+    }
+}
 
 impl PartialEq<[u8; 32]> for NullifierDerivingKey {
     #[allow(clippy::cmp_owned)]
     fn eq(&self, other: &[u8; 32]) -> bool {
-        <[u8; 32]>::from(*self) == *other
+        self.0.to_bytes().ct_eq(other).unwrap_u8() == 1u8
     }
 }
 
@@ -352,8 +410,16 @@ impl PartialEq<[u8; 32]> for NullifierDerivingKey {
 ///
 /// https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents
 // XXX: Should this be replaced by commitment::CommitmentRandomness?
-#[derive(Copy, Clone, Eq, PartialEq)]
+#[derive(Copy, Clone)]
-pub struct IvkCommitRandomness(pallas::Scalar);
+pub struct IvkCommitRandomness(pub(crate) pallas::Scalar);
+
+impl ConstantTimeEq for IvkCommitRandomness {
+    /// Check whether two `IvkCommitRandomness`s are equal, runtime independent
+    /// of the value of the secret.
+    fn ct_eq(&self, other: &Self) -> Choice {
+        self.0.to_bytes().ct_eq(&other.0.to_bytes())
+    }
+}
 
 impl fmt::Debug for IvkCommitRandomness {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -363,6 +429,8 @@ impl fmt::Debug for IvkCommitRandomness {
     }
 }
 
+impl Eq for IvkCommitRandomness {}
+
 impl From<SpendingKey> for IvkCommitRandomness {
     /// rivk = ToScalar^Orchard(PRF^expand_sk ([8]))
     ///
@@ -386,6 +454,18 @@ impl From<IvkCommitRandomness> for pallas::Scalar {
     }
 }
 
+impl PartialEq for IvkCommitRandomness {
+    fn eq(&self, other: &Self) -> bool {
+        self.ct_eq(other).unwrap_u8() == 1u8
+    }
+}
+
+impl PartialEq<[u8; 32]> for IvkCommitRandomness {
+    fn eq(&self, other: &[u8; 32]) -> bool {
+        self.0.to_bytes().ct_eq(other).unwrap_u8() == 1u8
+    }
+}
+
 impl TryFrom<[u8; 32]> for IvkCommitRandomness {
     type Error = &'static str;
 
@@ -415,12 +495,29 @@ mod ivk_hrp {
 /// Used to decrypt incoming notes without spending them.
 ///
 /// [ps]: https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents
-#[derive(Copy, Clone, Eq, PartialEq)]
+#[derive(Copy, Clone)]
 pub struct IncomingViewingKey {
     network: Network,
     scalar: pallas::Scalar,
 }
 
+impl ConstantTimeEq for IncomingViewingKey {
+    /// Check whether two `IncomingViewingKey`s are equal, runtime independent
+    /// of the value of the secret.
+    ///
+    /// # Note
+    ///
+    /// This function short-circuits if the networks of the keys are different.
+    /// Otherwise, it should execute in time independent of the `bytes` value.
+    fn ct_eq(&self, other: &Self) -> Choice {
+        if self.network != other.network {
+            return Choice::from(0);
+        }
+
+        self.scalar.to_bytes().ct_eq(&other.scalar.to_bytes())
+    }
+}
+
 impl fmt::Debug for IncomingViewingKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         f.debug_tuple("IncomingViewingKey")
@@ -440,6 +537,8 @@ impl fmt::Display for IncomingViewingKey {
     }
 }
 
+impl Eq for IncomingViewingKey {}
+
 impl From<FullViewingKey> for IncomingViewingKey {
     /// Commit^ivk_rivk(ak, nk) :=
     ///    SinsemillaShortCommit_rcm (︁"z.cash:Orchard-CommitIvk", I2LEBSP_l(ak) || I2LEBSP_l(nk)︁) mod r_P
@@ -472,9 +571,15 @@ impl From<FullViewingKey> for IncomingViewingKey {
     }
 }
 
+impl PartialEq for IncomingViewingKey {
+    fn eq(&self, other: &Self) -> bool {
+        self.ct_eq(other).unwrap_u8() == 1u8
+    }
+}
+
 impl PartialEq<[u8; 32]> for IncomingViewingKey {
     fn eq(&self, other: &[u8; 32]) -> bool {
-        self.scalar.to_bytes() == *other
+        self.scalar.to_bytes().ct_eq(other).unwrap_u8() == 1u8
     }
 }
 
@@ -515,34 +620,6 @@ pub struct FullViewingKey {
     ivk_commit_randomness: IvkCommitRandomness,
 }
 
-impl fmt::Debug for FullViewingKey {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        f.debug_struct("FullViewingKey")
-            .field("network", &self.network)
-            .field("spend_validating_key", &self.spend_validating_key)
-            .field("nullifier_deriving_key", &self.nullifier_deriving_key)
-            .field("ivk_commit_randomness", &self.ivk_commit_randomness)
-            .finish()
-    }
-}
-
-impl fmt::Display for FullViewingKey {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        let mut bytes = io::Cursor::new(Vec::new());
-
-        let _ = bytes.write_all(&<[u8; 32]>::from(self.spend_validating_key));
-        let _ = bytes.write_all(&<[u8; 32]>::from(self.nullifier_deriving_key));
-        let _ = bytes.write_all(&<[u8; 32]>::from(self.ivk_commit_randomness));
-
-        let hrp = match self.network {
-            Network::Mainnet => fvk_hrp::MAINNET,
-            Network::Testnet => fvk_hrp::TESTNET,
-        };
-
-        bech32::encode_to_fmt(f, hrp, bytes.get_ref().to_base32(), Variant::Bech32).unwrap()
-    }
-}
-
 impl FullViewingKey {
     /// [4.2.3]: https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents
     #[allow(non_snake_case)]
@@ -578,14 +655,50 @@ impl FullViewingKey {
     }
 }
 
+impl fmt::Debug for FullViewingKey {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        f.debug_struct("FullViewingKey")
+            .field("network", &self.network)
+            .field("spend_validating_key", &self.spend_validating_key)
+            .field("nullifier_deriving_key", &self.nullifier_deriving_key)
+            .field("ivk_commit_randomness", &self.ivk_commit_randomness)
+            .finish()
+    }
+}
+
+impl fmt::Display for FullViewingKey {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        let mut bytes = io::Cursor::new(Vec::new());
+
+        let _ = bytes.write_all(&<[u8; 32]>::from(self.spend_validating_key));
+        let _ = bytes.write_all(&<[u8; 32]>::from(self.nullifier_deriving_key));
+        let _ = bytes.write_all(&<[u8; 32]>::from(self.ivk_commit_randomness));
+
+        let hrp = match self.network {
+            Network::Mainnet => fvk_hrp::MAINNET,
+            Network::Testnet => fvk_hrp::TESTNET,
+        };
+
+        bech32::encode_to_fmt(f, hrp, bytes.get_ref().to_base32(), Variant::Bech32).unwrap()
+    }
+}
+
 /// An outgoing viewing key, as described in [protocol specification
 /// §4.2.3][ps].
 ///
 /// Used to decrypt outgoing notes without spending them.
 ///
 /// [ps]: https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents
-#[derive(Copy, Clone, Eq, PartialEq)]
+#[derive(Copy, Clone)]
-pub struct OutgoingViewingKey(pub [u8; 32]);
+pub struct OutgoingViewingKey(pub(crate) [u8; 32]);
+
+impl ConstantTimeEq for OutgoingViewingKey {
+    /// Check whether two `OutgoingViewingKey`s are equal, runtime independent
+    /// of the value of the secret.
+    fn ct_eq(&self, other: &Self) -> Choice {
+        self.0.ct_eq(&other.0)
+    }
+}
 
 impl fmt::Debug for OutgoingViewingKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -595,6 +708,8 @@ impl fmt::Debug for OutgoingViewingKey {
     }
 }
 
+impl Eq for OutgoingViewingKey {}
+
 impl From<[u8; 32]> for OutgoingViewingKey {
     /// Generate an `OutgoingViewingKey` from existing bytes.
     fn from(bytes: [u8; 32]) -> Self {
@@ -623,9 +738,15 @@ impl From<FullViewingKey> for OutgoingViewingKey {
     }
 }
 
+impl PartialEq for OutgoingViewingKey {
+    fn eq(&self, other: &Self) -> bool {
+        self.ct_eq(other).unwrap_u8() == 1u8
+    }
+}
+
 impl PartialEq<[u8; 32]> for OutgoingViewingKey {
     fn eq(&self, other: &[u8; 32]) -> bool {
-        self.0 == *other
+        self.0.ct_eq(other).unwrap_u8() == 1u8
     }
 }
 
@@ -685,7 +806,7 @@ impl From<DiversifierKey> for [u8; 32] {
     any(test, feature = "proptest-impl"),
     derive(proptest_derive::Arbitrary)
 )]
-pub struct Diversifier(pub [u8; 11]);
+pub struct Diversifier(pub(crate) [u8; 11]);
 
 impl fmt::Debug for Diversifier {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -776,7 +897,7 @@ impl Diversifier {
 /// [concretediversifyhash]: https://zips.z.cash/protocol/nu5.pdf#concretediversifyhash
 /// https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents
 #[derive(Copy, Clone, PartialEq)]
-pub struct TransmissionKey(pub pallas::Affine);
+pub struct TransmissionKey(pub(crate) pallas::Affine);
 
 impl fmt::Debug for TransmissionKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -833,7 +954,7 @@ impl From<(IncomingViewingKey, Diversifier)> for TransmissionKey {
 impl PartialEq<[u8; 32]> for TransmissionKey {
     #[allow(clippy::cmp_owned)]
     fn eq(&self, other: &[u8; 32]) -> bool {
-        <[u8; 32]>::from(*self) == *other
+        &self.0.to_bytes() == other
     }
 }
 
@@ -842,7 +963,7 @@ impl PartialEq<[u8; 32]> for TransmissionKey {
 /// https://zips.z.cash/protocol/nu5.pdf#concreteorchardkeyagreement
 /// https://zips.z.cash/protocol/nu5.pdf#saplingandorchardencrypt
 #[derive(Copy, Clone, Deserialize, PartialEq, Serialize)]
-pub struct EphemeralPublicKey(#[serde(with = "serde_helpers::Affine")] pub pallas::Affine);
+pub struct EphemeralPublicKey(#[serde(with = "serde_helpers::Affine")] pub(crate) pallas::Affine);
 
 impl fmt::Debug for EphemeralPublicKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -874,7 +995,7 @@ impl From<&EphemeralPublicKey> for [u8; 32] {
 impl PartialEq<[u8; 32]> for EphemeralPublicKey {
     #[allow(clippy::cmp_owned)]
     fn eq(&self, other: &[u8; 32]) -> bool {
-        <[u8; 32]>::from(self) == *other
+        &self.0.to_bytes() == other
     }
 }
 

zebra-chain/src/sapling/keys.rs

@@ -24,6 +24,7 @@ use std::{
 
 use bech32::{self, FromBase32, ToBase32, Variant};
 use rand_core::{CryptoRng, RngCore};
+use subtle::{Choice, ConstantTimeEq};
 
 use crate::{
     parameters::Network,
@@ -194,10 +195,10 @@ mod sk_hrp {
 /// §4.2.2][ps].
 ///
 /// Our root secret key of the Sapling key derivation tree. All other
-/// Sapling key types derive from the SpendingKey value.
+/// Sapling key types derive from the `SpendingKey` value.
 ///
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#saplingkeycomponents
-#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+#[derive(Copy, Clone, Debug)]
 #[cfg_attr(
     any(test, feature = "proptest-impl"),
     derive(proptest_derive::Arbitrary)
@@ -207,10 +208,42 @@ pub struct SpendingKey {
     bytes: [u8; 32],
 }
 
+impl SpendingKey {
+    /// Generate a new _SpendingKey_.
+    pub fn new<T>(csprng: &mut T) -> Self
+    where
+        T: RngCore + CryptoRng,
+    {
+        let mut bytes = [0u8; 32];
+        csprng.fill_bytes(&mut bytes);
+
+        Self::from(bytes)
+    }
+}
+
+impl ConstantTimeEq for SpendingKey {
+    /// Check whether two `SpendingKey`s are equal, runtime independent of the
+    /// value of the secret.
+    ///
+    /// # Note
+    ///
+    /// This function short-circuits if the networks of the keys are different.
+    /// Otherwise, it should execute in time independent of the `bytes` value.
+    fn ct_eq(&self, other: &Self) -> Choice {
+        if self.network != other.network {
+            return Choice::from(0);
+        }
+
+        self.bytes.ct_eq(&other.bytes)
+    }
+}
+
+impl Eq for SpendingKey {}
+
 // TODO: impl a From that accepts a Network?
 
 impl From<[u8; 32]> for SpendingKey {
-    /// Generate a _SpendingKey_ from existing bytes.
+    /// Generate a `SpendingKey` from existing bytes.
     fn from(bytes: [u8; 32]) -> Self {
         Self {
             network: Network::default(),
@@ -254,16 +287,9 @@ impl FromStr for SpendingKey {
     }
 }
 
-impl SpendingKey {
+impl PartialEq for SpendingKey {
-    /// Generate a new _SpendingKey_.
+    fn eq(&self, other: &Self) -> bool {
-    pub fn new<T>(csprng: &mut T) -> Self
+        self.ct_eq(other).unwrap_u8() == 1u8
-    where
-        T: RngCore + CryptoRng,
-    {
-        let mut bytes = [0u8; 32];
-        csprng.fill_bytes(&mut bytes);
-
-        Self::from(bytes)
     }
 }
 
@@ -274,8 +300,16 @@ impl SpendingKey {
 /// _Spend Description_, proving ownership of notes.
 ///
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#saplingkeycomponents
-#[derive(Copy, Clone, Eq, PartialEq)]
+#[derive(Copy, Clone)]
-pub struct SpendAuthorizingKey(pub Scalar);
+pub struct SpendAuthorizingKey(pub(crate) Scalar);
+
+impl ConstantTimeEq for SpendAuthorizingKey {
+    /// Check whether two `SpendAuthorizingKey`s are equal, runtime independent
+    /// of the value of the secret.
+    fn ct_eq(&self, other: &Self) -> Choice {
+        self.0.to_bytes().ct_eq(&other.0.to_bytes())
+    }
+}
 
 impl fmt::Debug for SpendAuthorizingKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -285,6 +319,8 @@ impl fmt::Debug for SpendAuthorizingKey {
     }
 }
 
+impl Eq for SpendAuthorizingKey {}
+
 impl From<SpendAuthorizingKey> for [u8; 32] {
     fn from(sk: SpendAuthorizingKey) -> Self {
         sk.0.to_bytes()
@@ -304,11 +340,15 @@ impl From<SpendingKey> for SpendAuthorizingKey {
     }
 }
 
+impl PartialEq for SpendAuthorizingKey {
+    fn eq(&self, other: &Self) -> bool {
+        self.ct_eq(other).unwrap_u8() == 1u8
+    }
+}
+
 impl PartialEq<[u8; 32]> for SpendAuthorizingKey {
-    // TODO: do we want to use constant-time comparison here?
-    #[allow(clippy::cmp_owned)]
     fn eq(&self, other: &[u8; 32]) -> bool {
-        <[u8; 32]>::from(*self) == *other
+        self.0.to_bytes().ct_eq(other).unwrap_u8() == 1u8
     }
 }
 
@@ -318,8 +358,16 @@ impl PartialEq<[u8; 32]> for SpendAuthorizingKey {
 /// Used in the _Spend Statement_ to prove nullifier integrity.
 ///
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#saplingkeycomponents
-#[derive(Copy, Clone, Eq, PartialEq)]
+#[derive(Copy, Clone)]
-pub struct ProofAuthorizingKey(pub Scalar);
+pub struct ProofAuthorizingKey(pub(crate) Scalar);
+
+impl ConstantTimeEq for ProofAuthorizingKey {
+    /// Check whether two `ProofAuthorizingKey`s are equal, runtime independent
+    /// of the value of the secret.
+    fn ct_eq(&self, other: &Self) -> Choice {
+        self.0.to_bytes().ct_eq(&other.0.to_bytes())
+    }
+}
 
 impl fmt::Debug for ProofAuthorizingKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -329,6 +377,8 @@ impl fmt::Debug for ProofAuthorizingKey {
     }
 }
 
+impl Eq for ProofAuthorizingKey {}
+
 impl From<ProofAuthorizingKey> for [u8; 32] {
     fn from(nsk: ProofAuthorizingKey) -> Self {
         nsk.0.to_bytes()
@@ -347,11 +397,15 @@ impl From<SpendingKey> for ProofAuthorizingKey {
     }
 }
 
+impl PartialEq for ProofAuthorizingKey {
+    fn eq(&self, other: &Self) -> bool {
+        self.ct_eq(other).unwrap_u8() == 1u8
+    }
+}
+
 impl PartialEq<[u8; 32]> for ProofAuthorizingKey {
-    // TODO: do we want to use constant-time comparison here?
-    #[allow(clippy::cmp_owned)]
     fn eq(&self, other: &[u8; 32]) -> bool {
-        <[u8; 32]>::from(*self) == *other
+        self.0.to_bytes().ct_eq(other).unwrap_u8() == 1u8
     }
 }
 
@@ -362,7 +416,7 @@ impl PartialEq<[u8; 32]> for ProofAuthorizingKey {
 ///
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#saplingkeycomponents
 #[derive(Copy, Clone, Eq, PartialEq)]
-pub struct OutgoingViewingKey(pub [u8; 32]);
+pub struct OutgoingViewingKey(pub(crate) [u8; 32]);
 
 impl fmt::Debug for OutgoingViewingKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -414,7 +468,7 @@ impl PartialEq<[u8; 32]> for OutgoingViewingKey {
 ///
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#saplingkeycomponents
 #[derive(Copy, Clone, Debug)]
-pub struct AuthorizingKey(pub redjubjub::VerificationKey<SpendAuth>);
+pub struct AuthorizingKey(pub(crate) redjubjub::VerificationKey<SpendAuth>);
 
 impl Eq for AuthorizingKey {}
 
@@ -439,24 +493,32 @@ impl From<SpendAuthorizingKey> for AuthorizingKey {
 
 impl PartialEq for AuthorizingKey {
     fn eq(&self, other: &Self) -> bool {
-        <[u8; 32]>::from(self.0) == <[u8; 32]>::from(other.0)
+        self == &<[u8; 32]>::from(*other)
     }
 }
 
 impl PartialEq<[u8; 32]> for AuthorizingKey {
     fn eq(&self, other: &[u8; 32]) -> bool {
-        <[u8; 32]>::from(self.0) == *other
+        &<[u8; 32]>::from(self.0) == other
     }
 }
 
 /// A _Nullifier Deriving Key_, as described in [protocol
 /// specification §4.2.2][ps].
 ///
-/// Used to create a _Nullifier_ per note.
+/// Used to create a `Nullifier` per note.
 ///
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#saplingkeycomponents
-#[derive(Copy, Clone, PartialEq)]
+#[derive(Copy, Clone)]
-pub struct NullifierDerivingKey(pub jubjub::AffinePoint);
+pub struct NullifierDerivingKey(pub(crate) jubjub::AffinePoint);
+
+impl ConstantTimeEq for NullifierDerivingKey {
+    /// Check whether two `NullifierDerivingKey`s are equal, runtime independent
+    /// of the value of the secret.
+    fn ct_eq(&self, other: &Self) -> Choice {
+        self.0.to_bytes().ct_eq(&other.0.to_bytes())
+    }
+}
 
 impl fmt::Debug for NullifierDerivingKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -508,11 +570,15 @@ impl From<ProofAuthorizingKey> for NullifierDerivingKey {
     }
 }
 
+impl PartialEq for NullifierDerivingKey {
+    fn eq(&self, other: &Self) -> bool {
+        self.ct_eq(other).unwrap_u8() == 1u8
+    }
+}
+
 impl PartialEq<[u8; 32]> for NullifierDerivingKey {
-    // TODO: do we want to use constant-time comparison here?
-    #[allow(clippy::cmp_owned)]
     fn eq(&self, other: &[u8; 32]) -> bool {
-        <[u8; 32]>::from(*self) == *other
+        self.0.to_bytes().ct_eq(other).unwrap_u8() == 1u8
     }
 }
 
@@ -638,7 +704,7 @@ impl PartialEq<[u8; 32]> for IncomingViewingKey {
     any(test, feature = "proptest-impl"),
     derive(proptest_derive::Arbitrary)
 )]
-pub struct Diversifier(pub [u8; 11]);
+pub struct Diversifier(pub(crate) [u8; 11]);
 
 impl fmt::Debug for Diversifier {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -758,7 +824,7 @@ impl Diversifier {
 ///
 /// [ps]: https://zips.z.cash/protocol/protocol.pdf#concretediversifyhash
 #[derive(Copy, Clone, PartialEq)]
-pub struct TransmissionKey(pub jubjub::AffinePoint);
+pub struct TransmissionKey(pub(crate) jubjub::AffinePoint);
 
 impl fmt::Debug for TransmissionKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -802,10 +868,8 @@ impl From<(IncomingViewingKey, Diversifier)> for TransmissionKey {
 }
 
 impl PartialEq<[u8; 32]> for TransmissionKey {
-    // TODO: do we want to use constant-time comparison here?
-    #[allow(clippy::cmp_owned)]
     fn eq(&self, other: &[u8; 32]) -> bool {
-        <[u8; 32]>::from(*self) == *other
+        &self.0.to_bytes() == other
     }
 }
 
@@ -899,7 +963,7 @@ impl FromStr for FullViewingKey {
 /// https://zips.z.cash/protocol/protocol.pdf#concretesaplingkeyagreement
 #[derive(Copy, Clone, Deserialize, PartialEq, Serialize)]
 pub struct EphemeralPublicKey(
-    #[serde(with = "serde_helpers::AffinePoint")] pub jubjub::AffinePoint,
+    #[serde(with = "serde_helpers::AffinePoint")] pub(crate) jubjub::AffinePoint,
 );
 
 impl fmt::Debug for EphemeralPublicKey {
@@ -926,10 +990,8 @@ impl From<&EphemeralPublicKey> for [u8; 32] {
 }
 
 impl PartialEq<[u8; 32]> for EphemeralPublicKey {
-    // TODO: do we want to use constant-time comparison here?
-    #[allow(clippy::cmp_owned)]
     fn eq(&self, other: &[u8; 32]) -> bool {
-        <[u8; 32]>::from(self) == *other
+        &self.0.to_bytes() == other
     }
 }