BiagioFesta / wtransport

Async-friendly WebTransport implementation in Rust
Apache License 2.0
346 stars 19 forks source link

Add Clone to Identity and PrivateKey #159

Closed cBournhonesque closed 2 months ago

cBournhonesque commented 2 months ago

Context:

I'm storing the Identity in some configuration types that implement Debug and Clone; it would be very useful for me if I could just derive Clone on these types. This requires Identity to derive Clone as well.

BiagioFesta commented 2 months ago

Not strong opinion, but I'd follow the same approach of rustls. Avoid to impl Clone for private key where all methods are "explicit".

impl PrivateKey {
   /// TODO: docs
   pub fn clone_key(&self) -> Self {
     // ...
   }
}

after that, it is possible to "clone" an Identity by just invoking Identity::new

cBournhonesque commented 2 months ago

I'm not entirely sure what you mean; I have this big struct here that derives Clone, by I can't migrate to the latest version because of the missing Clone implementation.

Or are you suggesting that I store the PrivateKey and CertificateChain separately, and impl Clone manually without the derive macro (using clone_key inside my Clone implementation)?

BiagioFesta commented 2 months ago

Yeah, I was suggesting to avoid implementing Clone for Identity, and instead being explicit by keeping the same PrivateKey with a clone_key.

What do you think about:

diff --git a/wtransport/src/tls.rs b/wtransport/src/tls.rs
index d2df9d5..29894bb 100644
--- a/wtransport/src/tls.rs
+++ b/wtransport/src/tls.rs
@@ -99,6 +99,16 @@ impl PrivateKey {
     pub fn secret_der(&self) -> &[u8] {
         self.0.secret_der()
     }
+
+    /// Clones this private key.
+    ///
+    /// # Note
+    /// `PrivateKey` does not implement `Clone` directly to ensure that sensitive information
+    /// is not cloned inadvertently.
+    /// Implementing `Clone` directly could potentially lead to unintended cloning of sensitive data.
+    pub fn clone_key(&self) -> Self {
+        Self(self.0.clone_key())
+    }
 }

 /// A collection of [`Certificate`].
@@ -257,6 +267,16 @@ impl Identity {
     pub fn private_key(&self) -> &PrivateKey {
         &self.private_key
     }
+
+    /// Clones this identity.
+    ///
+    /// # Note
+    /// `Identity` does not implement `Clone` directly to ensure that sensitive information,
+    /// specifically the inner *private key*, is not cloned inadvertently.
+    /// Implementing `Clone` directly could potentially lead to unintended cloning of sensitive data.
+    pub fn clone_identity(&self) -> Self {
+        Self::new(self.certificate_chain.clone(), self.private_key.clone_key())
+    }
 }

 /// Represents the formatting options for displaying a SHA-256 digest.
cBournhonesque commented 2 months ago

This would work for me, thank you!

BiagioFesta commented 2 months ago

Can you please squash two commits together and rebase?

cBournhonesque commented 2 months ago

Does this work?

BiagioFesta commented 2 months ago

Perfect Thanks