Merge branch 'ticket_759' into 'main'

tor-cert: Replace the KeyUnknownCert::check_key API

Closes #759

See merge request tpo/core/arti!1184

crates/tor-cert/semver.md

@@ -0,0 +1,2 @@
+ADDED: New methods to replace check_key.
+

crates/tor-cert/src/encode.rs

@@ -191,7 +191,7 @@ mod test {
 
         let decoded = Ed25519Cert::decode(&encoded).unwrap(); // Well-formed?
         let validated = decoded
-            .check_key(Some(&keypair.public.into()))
+            .should_be_signed_with(&keypair.public.into())
             .unwrap()
             .check_signature()
             .unwrap(); // Well-signed?

crates/tor-cert/src/lib.rs

@@ -419,6 +419,12 @@ impl Ed25519Cert {
 
 /// A parsed Ed25519 certificate. Maybe it includes its signing key;
 /// maybe it doesn't.
+///
+/// To validate this cert, either it must contain its signing key,
+/// or the caller must know the signing key.  In the first case, call
+/// [`should_have_signing_key`](KeyUnknownCert::should_have_signing_key);
+/// in the latter, call
+/// [`should_be_signed_with`](KeyUnknownCert::should_be_signed_with).
 #[derive(Clone, Debug)]
 pub struct KeyUnknownCert {
     /// The certificate whose signing key might not be known.
@@ -438,16 +444,57 @@ impl KeyUnknownCert {
     /// Check whether a given pkey is (or might be) a key that has correctly
     /// signed this certificate.
     ///
+    /// If pkey is None, this certificate must contain its signing key.
+    ///
     /// On success, we can check whether the certificate is well-signed;
     /// otherwise, we can't check the certificate.
+    #[deprecated(
+        since = "0.7.1",
+        note = "Use should_have_signing_key or should_be_signed_with instead."
+    )]
     pub fn check_key(self, pkey: Option<&ed25519::Ed25519Identity>) -> CertResult<UncheckedCert> {
-        let real_key = match (pkey, self.cert.cert.signed_with) {
-            (Some(a), Some(b)) if a == &b => b,
-            (Some(_), Some(_)) => return Err(CertError::KeyMismatch),
-            (Some(a), None) => *a,
-            (None, Some(b)) => b,
-            (None, None) => return Err(CertError::MissingPubKey),
+        match pkey {
+            Some(wanted) => self.should_be_signed_with(wanted),
+            None => self.should_have_signing_key(),
+        }
+    }
+
+    /// Declare that this should be a self-contained certificate that contains its own
+    /// signing key.
+    ///
+    /// On success, this certificate did indeed turn out to be self-contained, and so
+    /// we can validate it.
+    /// On failure, this certificate was not self-contained.
+    pub fn should_have_signing_key(self) -> CertResult<UncheckedCert> {
+        let real_key = match &self.cert.cert.signed_with {
+            Some(a) => *a,
+            None => return Err(CertError::MissingPubKey),
         };
+
+        Ok(UncheckedCert {
+            cert: Ed25519Cert {
+                signed_with: Some(real_key),
+                ..self.cert.cert
+            },
+            ..self.cert
+        })
+    }
+
+    /// Declare that this should be a certificate signed with a given key.
+    ///
+    /// On success, this certificate either listed the provided key, or did not
+    /// list any key: in either case, we can validate it.
+    /// On failure, this certificate claims to be signed with a different key.
+    pub fn should_be_signed_with(
+        self,
+        pkey: &ed25519::Ed25519Identity,
+    ) -> CertResult<UncheckedCert> {
+        let real_key = match &self.cert.cert.signed_with {
+            Some(a) if a == pkey => *pkey,
+            None => *pkey,
+            Some(_) => return Err(CertError::KeyMismatch),
+        };
+
         Ok(UncheckedCert {
             cert: Ed25519Cert {
                 signed_with: Some(real_key),

crates/tor-cert/tests/invalid_certs.rs

@@ -69,7 +69,7 @@ fn mismatched_signing_key() {
     // We give the wrong key to check_key, so it will tell us that
     // wasn't what the cert contained.
     assert_eq!(
-        cert.check_key(Some(&not_that_key)).err().unwrap(),
+        cert.should_be_signed_with(&not_that_key).err().unwrap(),
         CertError::KeyMismatch
     );
 
@@ -86,7 +86,7 @@ fn mismatched_signing_key() {
     // We give no key to check_key, which will tell us that there wasn't
     // a signing-key extension in the cert.
     assert_eq!(
-        cert.check_key(None).err().unwrap(),
+        cert.should_have_signing_key().err().unwrap(),
         CertError::MissingPubKey
     );
 }

crates/tor-cert/tests/testvec_certs.rs

@@ -29,7 +29,7 @@ fn test_valid_ed() {
     assert_eq!(cert.peek_cert_type(), 4.into());
     assert_eq!(cert.peek_subject_key().as_ed25519(), Some(&signing_key));
     let cert = cert
-        .check_key(None)
+        .should_have_signing_key()
         .unwrap()
         .check_signature()
         .unwrap()
@@ -58,7 +58,7 @@ fn test_valid_ed() {
     assert_eq!(cert.peek_cert_type(), 5.into());
     assert_eq!(cert.peek_subject_key().as_bytes(), &tls_cert_digest[..]);
     let cert = cert
-        .check_key(Some(&signing_key))
+        .should_be_signed_with(&signing_key)
         .unwrap()
         .check_signature()
         .unwrap()

crates/tor-netdoc/src/doc/hsdesc/inner.rs

@@ -133,7 +133,7 @@ fn handle_inner_certificate(
 
     // These certs have to include a signing key.
     let cert = cert
-        .check_key(None) // TODO arti#759
+        .should_have_signing_key()
         .map_err(|e| make_err(e, "Certificate was not self-signed"))?;
 
     // Peel off the signature.

crates/tor-netdoc/src/doc/hsdesc/outer.rs

@@ -222,7 +222,7 @@ impl HsDescOuter {
                 .parse_obj::<UnvalidatedEdCert>("ED25519 CERT")?
                 .check_cert_type(tor_cert::CertType::HS_BLINDED_ID_V_SIGNING)?
                 .into_unchecked()
-                .check_key(None) // require that the cert contains its signing key.
+                .should_have_signing_key()
                 .map_err(|err| {
                     EK::BadObjectVal
                         .err()

crates/tor-netdoc/src/doc/routerdesc.rs

@@ -479,7 +479,7 @@ impl RouterDesc {
                 .parse_obj::<UnvalidatedEdCert>("ED25519 CERT")?
                 .check_cert_type(tor_cert::CertType::IDENTITY_V_SIGNING)?
                 .into_unchecked()
-                .check_key(None)
+                .should_have_signing_key()
                 .map_err(|err| {
                     EK::BadObjectVal
                         .err()
@@ -607,7 +607,7 @@ impl RouterDesc {
                 .check_cert_type(tor_cert::CertType::NTOR_CC_IDENTITY)?
                 .check_subject_key_is(identity_cert.peek_signing_key())?
                 .into_unchecked()
-                .check_key(Some(&ntor_as_ed.into()))
+                .should_be_signed_with(&ntor_as_ed.into())
                 .map_err(|err| EK::BadSignature.err().with_source(err))?
         };
 

crates/tor-proto/src/channel/handshake.rs

@@ -425,7 +425,7 @@ impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static, S: SleepProvider> Unver
 
         // Check the identity->signing cert
         let (id_sk, id_sk_sig) = id_sk
-            .check_key(None)
+            .should_have_signing_key()
             .map_err(Error::HandshakeCertErr)?
             .dangerously_split()
             .map_err(Error::HandshakeCertErr)?;
@@ -445,7 +445,7 @@ impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static, S: SleepProvider> Unver
         // Now look at the signing->TLS cert and check it against the
         // peer certificate.
         let (sk_tls, sk_tls_sig) = sk_tls
-            .check_key(Some(signing_key))
+            .should_be_signed_with(signing_key)
             .map_err(Error::HandshakeCertErr)?
             .dangerously_split()
             .map_err(Error::HandshakeCertErr)?;