From 4ee7e0d3a9398cf9deb9f72fd978fd3cab86db0e Mon Sep 17 00:00:00 2001
From: Steven Fackler <sfackler@gmail.com>
Date: Sun, 11 Mar 2018 14:06:57 -0700
Subject: [PATCH] Tweak verify_cert's signature

The call can fail either due to an invalid cert or an internal error,
and we should distinguish between the two.
---
 openssl/src/x509/mod.rs   | 50 +++++++++++++++++++--------------------
 openssl/src/x509/tests.rs | 26 ++++++++++++--------
 2 files changed, 40 insertions(+), 36 deletions(-)

diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs
index 146a77b0..ef4b57e5 100644
--- a/openssl/src/x509/mod.rs
+++ b/openssl/src/x509/mod.rs
@@ -122,22 +122,33 @@ impl X509StoreContextRef {
     ///
     /// [`X509_STORE_CTX_init`]:  https://www.openssl.org/docs/man1.0.2/crypto/X509_STORE_CTX_init.html
     /// [`X509_STORE_CTX_cleanup`]:  https://www.openssl.org/docs/man1.0.2/crypto/X509_STORE_CTX_cleanup.html
-    pub fn init<F, T>(&mut self, trust: &store::X509StoreRef, cert: &X509Ref,
-                      cert_chain: &StackRef<X509>, with_context: F) -> Result<T, ErrorStack>
+    pub fn init<F, T>(
+        &mut self,
+        trust: &store::X509StoreRef,
+        cert: &X509Ref,
+        cert_chain: &StackRef<X509>,
+        with_context: F,
+    ) -> Result<T, ErrorStack>
     where
-        F: FnOnce(&mut X509StoreContextRef) -> Result<T, ErrorStack>
+        F: FnOnce(&mut X509StoreContextRef) -> Result<T, ErrorStack>,
     {
         struct Cleanup<'a>(&'a mut X509StoreContextRef);
 
         impl<'a> Drop for Cleanup<'a> {
             fn drop(&mut self) {
-                self.0.cleanup();
+                unsafe {
+                    ffi::X509_STORE_CTX_cleanup(self.0.as_ptr());
+                }
             }
         }
 
         unsafe {
-            cvt(ffi::X509_STORE_CTX_init(self.as_ptr(), trust.as_ptr(),
-                                         cert.as_ptr(), cert_chain.as_ptr()))?;
+            cvt(ffi::X509_STORE_CTX_init(
+                self.as_ptr(),
+                trust.as_ptr(),
+                cert.as_ptr(),
+                cert_chain.as_ptr(),
+            ))?;
 
             let cleanup = Cleanup(self);
             with_context(cleanup.0)
@@ -145,30 +156,17 @@ impl X509StoreContextRef {
     }
 
     /// Verifies the stored certificate.
-    /// It is required to call `init` in beforehand, to initialize the required values.
+    ///
+    /// Returns `true` if verification succeeds. The `error` method will return the specific
+    /// validation error if the certificate was not valid.
+    ///
+    /// This will only work inside of a call to `init`.
     ///
     /// This corresponds to [`X509_verify_cert`].
     ///
     /// [`X509_verify_cert`]:  https://www.openssl.org/docs/man1.0.2/crypto/X509_verify_cert.html
-    ///
-    /// # Result
-    /// 
-    /// The Result must be `Ok(())` to be a valid certificate, otherwise the cert is not valid.
-    pub fn verify_cert(&mut self) -> Result<(), ErrorStack> {
-        unsafe {
-            cvt(ffi::X509_verify_cert(self.as_ptr())).map(|_| ())
-        }
-    }
-
-    /// Cleans-up the context.
-    ///
-    /// This corresponds to [`X509_STORE_CTX_cleanup`].
-    ///
-    /// [`X509_STORE_CTX_cleanup`]:  https://www.openssl.org/docs/man1.0.2/crypto/X509_STORE_CTX_cleanup.html
-    fn cleanup(&mut self) {
-        unsafe {
-            ffi::X509_STORE_CTX_cleanup(self.as_ptr());
-        }
+    pub fn verify_cert(&mut self) -> Result<bool, ErrorStack> {
+        unsafe { cvt_n(ffi::X509_verify_cert(self.as_ptr())).map(|n| n != 0) }
     }
 
     /// Set the error code of the context.
diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs
index e3c726ae..ecc7f7de 100644
--- a/openssl/src/x509/tests.rs
+++ b/openssl/src/x509/tests.rs
@@ -7,7 +7,7 @@ use nid::Nid;
 use pkey::{PKey, Private};
 use rsa::Rsa;
 use stack::Stack;
-use x509::{X509, X509Name, X509Req, X509VerifyResult, X509StoreContext};
+use x509::{X509, X509Name, X509Req, X509StoreContext, X509VerifyResult};
 use x509::extension::{AuthorityKeyIdentifier, BasicConstraints, ExtendedKeyUsage, KeyUsage,
                       SubjectAlternativeName, SubjectKeyIdentifier};
 use x509::store::X509StoreBuilder;
@@ -242,15 +242,11 @@ fn test_stack_from_pem() {
 
     assert_eq!(certs.len(), 2);
     assert_eq!(
-        hex::encode(certs[0]
-            .fingerprint(MessageDigest::sha1())
-            .unwrap()),
+        hex::encode(certs[0].fingerprint(MessageDigest::sha1()).unwrap()),
         "59172d9313e84459bcff27f967e79e6e9217e584"
     );
     assert_eq!(
-        hex::encode(certs[1]
-            .fingerprint(MessageDigest::sha1())
-            .unwrap()),
+        hex::encode(certs[1].fingerprint(MessageDigest::sha1()).unwrap()),
         "c0cbdf7cdd03c9773e5468e1f6d2da7d5cbb1875"
     );
 }
@@ -306,8 +302,16 @@ fn test_verify_cert() {
     let store = store_bldr.build();
 
     let mut context = X509StoreContext::new().unwrap();
-    assert!(context.init(&store, &cert, &chain, |c| c.verify_cert()).is_ok());
-    assert!(context.init(&store, &cert, &chain, |c| c.verify_cert()).is_ok());
+    assert!(
+        context
+            .init(&store, &cert, &chain, |c| c.verify_cert())
+            .unwrap()
+    );
+    assert!(
+        context
+            .init(&store, &cert, &chain, |c| c.verify_cert())
+            .unwrap()
+    );
 }
 
 #[test]
@@ -323,5 +327,7 @@ fn test_verify_fails() {
     let store = store_bldr.build();
 
     let mut context = X509StoreContext::new().unwrap();
-    assert!(context.init(&store, &cert, &chain, |c| c.verify_cert()).is_err());
+    assert!(!context
+        .init(&store, &cert, &chain, |c| c.verify_cert())
+        .unwrap());
 }