From eb6296e892b168ef5f0908271443b646d742d724 Mon Sep 17 00:00:00 2001 From: Benjamin Fry Date: Sun, 19 Mar 2017 00:25:45 -0700 Subject: [PATCH 01/16] add verify_cert and store_context_builder --- openssl-sys/src/lib.rs | 4 ++++ openssl/src/x509/tests.rs | 17 +++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/openssl-sys/src/lib.rs b/openssl-sys/src/lib.rs index 77f69188..85ab03f7 100644 --- a/openssl-sys/src/lib.rs +++ b/openssl-sys/src/lib.rs @@ -2605,6 +2605,8 @@ extern "C" { pub fn X509_sign(x: *mut X509, pkey: *mut EVP_PKEY, md: *const EVP_MD) -> c_int; pub fn X509_get_pubkey(x: *mut X509) -> *mut EVP_PKEY; pub fn X509_to_X509_REQ(x: *mut X509, pkey: *mut EVP_PKEY, md: *const EVP_MD) -> *mut X509_REQ; + #[cfg(not(any(ossl101, libressl)))] + pub fn X509_verify_cert(ctx: *mut X509_STORE_CTX) -> c_int; pub fn X509_verify_cert_error_string(n: c_long) -> *const c_char; pub fn X509_get1_ocsp(x: *mut X509) -> *mut stack_st_OPENSSL_STRING; pub fn X509_check_issued(issuer: *mut X509, subject: *mut X509) -> c_int; @@ -2638,6 +2640,8 @@ extern "C" { pub fn X509_STORE_add_cert(store: *mut X509_STORE, x: *mut X509) -> c_int; pub fn X509_STORE_set_default_paths(store: *mut X509_STORE) -> c_int; + pub fn X509_STORE_CTX_new() -> *mut X509_STORE_CTX; + pub fn X509_STORE_CTX_init(ctx: *mut X509_STORE_CTX, store: *mut X509_STORE, x509: *mut X509, chain: *mut stack_st_X509) -> c_int; pub fn X509_STORE_CTX_free(ctx: *mut X509_STORE_CTX); pub fn X509_STORE_CTX_get_current_cert(ctx: *mut X509_STORE_CTX) -> *mut X509; pub fn X509_STORE_CTX_get_error(ctx: *mut X509_STORE_CTX) -> c_int; diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index 6f6b430a..e91a039b 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -291,3 +291,20 @@ fn clone_x509() { let cert = X509::from_pem(cert).unwrap(); cert.clone(); } + +#[test] +fn test_verify_cert() { + let cert = include_bytes!("../../test/cert.pem"); + let cert = X509::from_pem(cert).unwrap(); + let ca = include_bytes!("../../test/root-ca.pem"); + let ca = X509::from_pem(ca).unwrap(); + + let mut store_bldr = X509StoreBuilder::new().unwrap(); + store_bldr.add_cert(ca); + let store = store_bldr.build(); + + let store_ctx_bldr = X509StoreContext::builder().unwrap(); + let store_ctx = store_ctx_bldr.build(store, cert, Stack::new().unwrap()).unwrap(); + + store_ctx.verify_cert().unwrap(); +} From 3595ff9e5170f6ef396f579e7e366ebe6ecb0a1f Mon Sep 17 00:00:00 2001 From: Benjamin Fry Date: Sun, 19 Mar 2017 00:47:22 -0700 Subject: [PATCH 02/16] Fix memory mgmt --- openssl/src/x509/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index e91a039b..a94c40bd 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -304,7 +304,7 @@ fn test_verify_cert() { let store = store_bldr.build(); let store_ctx_bldr = X509StoreContext::builder().unwrap(); - let store_ctx = store_ctx_bldr.build(store, cert, Stack::new().unwrap()).unwrap(); + let store_ctx = store_ctx_bldr.build(&store, &cert, &Stack::new().unwrap()).unwrap(); store_ctx.verify_cert().unwrap(); } From 847fac25f8c85a6cb0327f361cb310a93e2bada7 Mon Sep 17 00:00:00 2001 From: Benjamin Fry Date: Sun, 19 Mar 2017 00:55:42 -0700 Subject: [PATCH 03/16] properly version library functions --- openssl/src/x509/mod.rs | 7 +++++++ openssl/src/x509/tests.rs | 1 + 2 files changed, 8 insertions(+) diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index 9638e6a3..1e7c4448 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -86,6 +86,13 @@ impl X509StoreContextRef { } } + #[cfg(any(all(feature = "v102", ossl102), all(feature = "v110", ossl110)))] + pub fn verify_cert(self) -> Result, ErrorStack> { + unsafe { + cvt(ffi::X509_verify_cert(self.as_ptr())).map(|_| ()) + } + } + /// Returns the error code of the context. /// /// This corresponds to [`X509_STORE_CTX_get_error`]. diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index a94c40bd..2b9d5dd3 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -285,6 +285,7 @@ fn signature() { assert_eq!(algorithm.object().to_string(), "sha256WithRSAEncryption"); } +#[cfg(any(all(feature = "v102", ossl102), all(feature = "v110", ossl110)))] #[test] fn clone_x509() { let cert = include_bytes!("../../test/cert.pem"); From 35cad33d518f3b6ecff1a01a4707b35ab834342e Mon Sep 17 00:00:00 2001 From: Benjamin Fry Date: Sun, 19 Mar 2017 12:06:41 -0700 Subject: [PATCH 04/16] fix error check --- openssl-sys/src/lib.rs | 1 - openssl/src/x509/mod.rs | 4 ++-- openssl/src/x509/tests.rs | 5 ++--- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/openssl-sys/src/lib.rs b/openssl-sys/src/lib.rs index 85ab03f7..c29b60e8 100644 --- a/openssl-sys/src/lib.rs +++ b/openssl-sys/src/lib.rs @@ -2605,7 +2605,6 @@ extern "C" { pub fn X509_sign(x: *mut X509, pkey: *mut EVP_PKEY, md: *const EVP_MD) -> c_int; pub fn X509_get_pubkey(x: *mut X509) -> *mut EVP_PKEY; pub fn X509_to_X509_REQ(x: *mut X509, pkey: *mut EVP_PKEY, md: *const EVP_MD) -> *mut X509_REQ; - #[cfg(not(any(ossl101, libressl)))] pub fn X509_verify_cert(ctx: *mut X509_STORE_CTX) -> c_int; pub fn X509_verify_cert_error_string(n: c_long) -> *const c_char; pub fn X509_get1_ocsp(x: *mut X509) -> *mut stack_st_OPENSSL_STRING; diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index 1e7c4448..6133e1a3 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -86,11 +86,11 @@ impl X509StoreContextRef { } } - #[cfg(any(all(feature = "v102", ossl102), all(feature = "v110", ossl110)))] pub fn verify_cert(self) -> Result, ErrorStack> { unsafe { - cvt(ffi::X509_verify_cert(self.as_ptr())).map(|_| ()) + try!(cvt(ffi::X509_verify_cert(self.as_ptr())).map(|_| ())) } + Ok(self.error()) } /// Returns the error code of the context. diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index 2b9d5dd3..b6303ade 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -285,7 +285,6 @@ fn signature() { assert_eq!(algorithm.object().to_string(), "sha256WithRSAEncryption"); } -#[cfg(any(all(feature = "v102", ossl102), all(feature = "v110", ossl110)))] #[test] fn clone_x509() { let cert = include_bytes!("../../test/cert.pem"); @@ -301,11 +300,11 @@ fn test_verify_cert() { let ca = X509::from_pem(ca).unwrap(); let mut store_bldr = X509StoreBuilder::new().unwrap(); - store_bldr.add_cert(ca); + store_bldr.add_cert(ca).unwrap(); let store = store_bldr.build(); let store_ctx_bldr = X509StoreContext::builder().unwrap(); let store_ctx = store_ctx_bldr.build(&store, &cert, &Stack::new().unwrap()).unwrap(); - store_ctx.verify_cert().unwrap(); + assert!(store_ctx.verify_cert().unwrap().is_none()); } From 910386027dea776c3b5034d216169cefde912e45 Mon Sep 17 00:00:00 2001 From: Benjamin Fry Date: Sun, 19 Mar 2017 12:12:57 -0700 Subject: [PATCH 05/16] add comment about consuming self in verify_cert --- openssl/src/x509/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index 6133e1a3..cb5eca40 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -86,6 +86,9 @@ impl X509StoreContextRef { } } + /// Verifies the certificate associated in the `build()` method + /// + /// This consumes self as the `X509StoreContext` must be reinitialized subsequent to any cally to verify. pub fn verify_cert(self) -> Result, ErrorStack> { unsafe { try!(cvt(ffi::X509_verify_cert(self.as_ptr())).map(|_| ())) From d8a11973e2c9ccc5a806936edb2cccf28332bc5e Mon Sep 17 00:00:00 2001 From: Benjamin Fry Date: Mon, 20 Mar 2017 22:28:15 -0700 Subject: [PATCH 06/16] convert to raw pass-through methods --- openssl/src/x509/mod.rs | 26 +++++++++++++++++++++----- openssl/src/x509/tests.rs | 4 ++-- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index cb5eca40..52907110 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -86,14 +86,20 @@ impl X509StoreContextRef { } } - /// Verifies the certificate associated in the `build()` method + /// Initializes the store context to verify the certificate. /// - /// This consumes self as the `X509StoreContext` must be reinitialized subsequent to any cally to verify. - pub fn verify_cert(self) -> Result, ErrorStack> { + /// This Context can only be used once, subsequent to any validation, the context must be reinitialized. + /// + /// # Arguments + /// + /// * `trust` - a store of the trusted chain of certificates, or CAs, to validated the certificate + /// * `cert` - certificate to validate + /// * `cert_chain` - the certificates chain + pub fn init(&self, trust: &store::X509StoreRef, cert: &X509Ref, cert_chain: &StackRef) -> Result<(), ErrorStack> { unsafe { - try!(cvt(ffi::X509_verify_cert(self.as_ptr())).map(|_| ())) + cvt(ffi::X509_STORE_CTX_init(self.as_ptr(), trust.as_ptr(), cert.as_ptr(), cert_chain.as_ptr())) + .map(|_| ()) } - Ok(self.error()) } /// Returns the error code of the context. @@ -105,6 +111,16 @@ impl X509StoreContextRef { unsafe { X509VerifyResult::from_raw(ffi::X509_STORE_CTX_get_error(self.as_ptr())) } } + /// Verifies the certificate associated in the `init()` method + /// + /// This consumes self as the `X509StoreContext` must be reinitialized subsequent to any cally to verify. + pub fn verify_cert(&self) -> Result, ErrorStack> { + unsafe { + try!(cvt(ffi::X509_verify_cert(self.as_ptr())).map(|_| ())) + } + Ok(self.error()) + } + /// Set the error code of the context. /// /// This corresponds to [`X509_STORE_CTX_set_error`]. diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index b6303ade..6ef4f18e 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -303,8 +303,8 @@ fn test_verify_cert() { store_bldr.add_cert(ca).unwrap(); let store = store_bldr.build(); - let store_ctx_bldr = X509StoreContext::builder().unwrap(); - let store_ctx = store_ctx_bldr.build(&store, &cert, &Stack::new().unwrap()).unwrap(); + let store_ctx = X509StoreContext::new().unwrap(); + store_ctx.init(&store, &cert, &Stack::new().unwrap()).unwrap(); assert!(store_ctx.verify_cert().unwrap().is_none()); } From 2251a6f2b6da97a2e07f230787cb63c549e6940c Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Tue, 21 Mar 2017 08:59:56 +0000 Subject: [PATCH 07/16] Little tweaks --- openssl/src/x509/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index 52907110..dcd4296a 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -88,14 +88,14 @@ impl X509StoreContextRef { /// Initializes the store context to verify the certificate. /// - /// This Context can only be used once, subsequent to any validation, the context must be reinitialized. + /// The context must be re-initialized before each call to `verify_cert`. /// /// # Arguments /// /// * `trust` - a store of the trusted chain of certificates, or CAs, to validated the certificate /// * `cert` - certificate to validate - /// * `cert_chain` - the certificates chain - pub fn init(&self, trust: &store::X509StoreRef, cert: &X509Ref, cert_chain: &StackRef) -> Result<(), ErrorStack> { + /// * `cert_chain` - the certificate's chain + pub fn init(&mut self, trust: &store::X509StoreRef, cert: &X509Ref, cert_chain: &StackRef) -> Result<(), ErrorStack> { unsafe { cvt(ffi::X509_STORE_CTX_init(self.as_ptr(), trust.as_ptr(), cert.as_ptr(), cert_chain.as_ptr())) .map(|_| ()) @@ -113,7 +113,7 @@ impl X509StoreContextRef { /// Verifies the certificate associated in the `init()` method /// - /// This consumes self as the `X509StoreContext` must be reinitialized subsequent to any cally to verify. + /// The context must be re-initialized before each call to this method. pub fn verify_cert(&self) -> Result, ErrorStack> { unsafe { try!(cvt(ffi::X509_verify_cert(self.as_ptr())).map(|_| ())) From 3187366cc5fb8619dd496b9bfccaba8c66c6923f Mon Sep 17 00:00:00 2001 From: Benjamin Fry Date: Thu, 23 Mar 2017 21:37:42 -0700 Subject: [PATCH 08/16] restructure to self contained function --- openssl/src/x509/mod.rs | 15 +++++++++++---- openssl/src/x509/tests.rs | 5 +---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index dcd4296a..5dd12b0e 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -112,13 +112,20 @@ impl X509StoreContextRef { } /// Verifies the certificate associated in the `init()` method + /// * `cert_chain` - the certificates chain /// - /// The context must be re-initialized before each call to this method. - pub fn verify_cert(&self) -> Result, ErrorStack> { + /// # Result + /// + /// The Result must be `Some(None)` to be a valid certificate, otherwise the cert is not valid. + pub fn verify_cert(trust: &store::X509StoreRef, cert: &X509Ref, cert_chain: &StackRef) -> Result, ErrorStack> { unsafe { - try!(cvt(ffi::X509_verify_cert(self.as_ptr())).map(|_| ())) + ffi::init(); + let context = try!(cvt_p(ffi::X509_STORE_CTX_new()).map(|p| X509StoreContext(p))); + try!(cvt(ffi::X509_STORE_CTX_init(context.as_ptr(), trust.as_ptr(), cert.as_ptr(), cert_chain.as_ptr())) + .map(|_| ())); + try!(cvt(ffi::X509_verify_cert(context.as_ptr())).map(|_| ())); + Ok(context.error()) } - Ok(self.error()) } /// Set the error code of the context. diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index 6ef4f18e..05baac12 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -303,8 +303,5 @@ fn test_verify_cert() { store_bldr.add_cert(ca).unwrap(); let store = store_bldr.build(); - let store_ctx = X509StoreContext::new().unwrap(); - store_ctx.init(&store, &cert, &Stack::new().unwrap()).unwrap(); - - assert!(store_ctx.verify_cert().unwrap().is_none()); + assert!(X509StoreContext::verify_cert(&store, &cert, &Stack::new().unwrap()).unwrap().is_none()); } From a1cfde765a2a63798411ce4d518b8d32c085ffbf Mon Sep 17 00:00:00 2001 From: Benjamin Fry Date: Thu, 23 Mar 2017 22:11:23 -0700 Subject: [PATCH 09/16] add cleanup ffi to store context --- openssl-sys/src/lib.rs | 1 + openssl/src/x509/mod.rs | 8 ++++++-- openssl/src/x509/tests.rs | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/openssl-sys/src/lib.rs b/openssl-sys/src/lib.rs index c29b60e8..3e5b3dd6 100644 --- a/openssl-sys/src/lib.rs +++ b/openssl-sys/src/lib.rs @@ -2640,6 +2640,7 @@ extern "C" { pub fn X509_STORE_set_default_paths(store: *mut X509_STORE) -> c_int; pub fn X509_STORE_CTX_new() -> *mut X509_STORE_CTX; + pub fn X509_STORE_CTX_cleanup(ctx: *mut X509_STORE_CTX); pub fn X509_STORE_CTX_init(ctx: *mut X509_STORE_CTX, store: *mut X509_STORE, x509: *mut X509, chain: *mut stack_st_X509) -> c_int; pub fn X509_STORE_CTX_free(ctx: *mut X509_STORE_CTX); pub fn X509_STORE_CTX_get_current_cert(ctx: *mut X509_STORE_CTX) -> *mut X509; diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index 5dd12b0e..0cfa8ada 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -117,14 +117,18 @@ impl X509StoreContextRef { /// # Result /// /// The Result must be `Some(None)` to be a valid certificate, otherwise the cert is not valid. - pub fn verify_cert(trust: &store::X509StoreRef, cert: &X509Ref, cert_chain: &StackRef) -> Result, ErrorStack> { + pub fn verify_cert(trust: store::X509Store, cert: X509, cert_chain: Stack) -> Result, ErrorStack> { unsafe { ffi::init(); let context = try!(cvt_p(ffi::X509_STORE_CTX_new()).map(|p| X509StoreContext(p))); try!(cvt(ffi::X509_STORE_CTX_init(context.as_ptr(), trust.as_ptr(), cert.as_ptr(), cert_chain.as_ptr())) .map(|_| ())); try!(cvt(ffi::X509_verify_cert(context.as_ptr())).map(|_| ())); - Ok(context.error()) + + let result = Ok(context.error()); + ffi::X509_STORE_CTX_cleanup(context.as_ptr()); + + result } } diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index 05baac12..96d45742 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -303,5 +303,5 @@ fn test_verify_cert() { store_bldr.add_cert(ca).unwrap(); let store = store_bldr.build(); - assert!(X509StoreContext::verify_cert(&store, &cert, &Stack::new().unwrap()).unwrap().is_none()); + assert!(X509StoreContext::verify_cert(store, cert, Stack::new().unwrap()).unwrap().is_none()); } From 6abac82f13c80a2726fc2e9a0f6913357e93a985 Mon Sep 17 00:00:00 2001 From: Benjamin Fry Date: Sun, 26 Mar 2017 00:16:27 -0700 Subject: [PATCH 10/16] cleanup and add negative test --- openssl/src/x509/mod.rs | 13 ++++++++----- openssl/src/x509/tests.rs | 16 +++++++++++++++- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index 0cfa8ada..6bb58dbd 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -117,18 +117,21 @@ impl X509StoreContextRef { /// # Result /// /// The Result must be `Some(None)` to be a valid certificate, otherwise the cert is not valid. - pub fn verify_cert(trust: store::X509Store, cert: X509, cert_chain: Stack) -> Result, ErrorStack> { + pub fn verify_cert(trust: store::X509Store, cert: X509, cert_chain: Stack) -> Result<(), ErrorStack> { unsafe { ffi::init(); let context = try!(cvt_p(ffi::X509_STORE_CTX_new()).map(|p| X509StoreContext(p))); try!(cvt(ffi::X509_STORE_CTX_init(context.as_ptr(), trust.as_ptr(), cert.as_ptr(), cert_chain.as_ptr())) .map(|_| ())); + + mem::forget(trust); + mem::forget(cert); + mem::forget(cert_chain); + + // verify_cert returns an error `<= 0` if there was a validation error try!(cvt(ffi::X509_verify_cert(context.as_ptr())).map(|_| ())); - let result = Ok(context.error()); - ffi::X509_STORE_CTX_cleanup(context.as_ptr()); - - result + Ok(()) } } diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index 96d45742..7ea91432 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -303,5 +303,19 @@ fn test_verify_cert() { store_bldr.add_cert(ca).unwrap(); let store = store_bldr.build(); - assert!(X509StoreContext::verify_cert(store, cert, Stack::new().unwrap()).unwrap().is_none()); + assert!(X509StoreContext::verify_cert(store, cert, Stack::new().unwrap()).is_ok()); +} + +#[test] +fn test_verify_fails() { + let cert = include_bytes!("../../test/cert.pem"); + let cert = X509::from_pem(cert).unwrap(); + let ca = include_bytes!("../../test/alt_name_cert.pem"); + let ca = X509::from_pem(ca).unwrap(); + + let mut store_bldr = X509StoreBuilder::new().unwrap(); + store_bldr.add_cert(ca).unwrap(); + let store = store_bldr.build(); + + assert!(X509StoreContext::verify_cert(store, cert, Stack::new().unwrap()).is_err()); } From 53adf0e6a4975c775dc1f18875d6ebc972650b92 Mon Sep 17 00:00:00 2001 From: Benjamin Fry Date: Sun, 26 Mar 2017 00:20:49 -0700 Subject: [PATCH 11/16] delay return until after forgets --- openssl/src/x509/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index 6bb58dbd..6023b5a9 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -121,13 +121,15 @@ impl X509StoreContextRef { unsafe { ffi::init(); let context = try!(cvt_p(ffi::X509_STORE_CTX_new()).map(|p| X509StoreContext(p))); - try!(cvt(ffi::X509_STORE_CTX_init(context.as_ptr(), trust.as_ptr(), cert.as_ptr(), cert_chain.as_ptr())) - .map(|_| ())); + let init_result = cvt(ffi::X509_STORE_CTX_init(context.as_ptr(), trust.as_ptr(), cert.as_ptr(), cert_chain.as_ptr())) + .map(|_| ()); mem::forget(trust); mem::forget(cert); mem::forget(cert_chain); + try!(init_result); + // verify_cert returns an error `<= 0` if there was a validation error try!(cvt(ffi::X509_verify_cert(context.as_ptr())).map(|_| ())); From 888f4ccaabafd8eed7cdc69be127dc700c7bbf54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 7 Mar 2018 15:07:55 +0100 Subject: [PATCH 12/16] Fixes the implementation of `X509StoreContextRef::verify_cert` The certificate, the store and the certificates chain does not need to be consumed by `verify_cert` and instead are taken as references. We also call `X509_STORE_CTX_cleanup`, after the verification succeeded. --- openssl/src/x509/mod.rs | 61 +++++++++++++++++++-------------------- openssl/src/x509/tests.rs | 11 +++++-- 2 files changed, 38 insertions(+), 34 deletions(-) diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index 6023b5a9..e2c87731 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -67,6 +67,18 @@ impl X509StoreContext { pub fn ssl_idx() -> Result, ErrorStack> { unsafe { cvt_n(ffi::SSL_get_ex_data_X509_STORE_CTX_idx()).map(|idx| Index::from_raw(idx)) } } + + /// Creates a new `X509StoreContext` instance. + /// + /// This corresponds to [`X509_STORE_CTX_new`]. + /// + /// [`X509_STORE_CTX_new`]: https://www.openssl.org/docs/man1.1.0/crypto/X509_STORE_CTX_new.html + pub fn new() -> Result { + unsafe { + ffi::init(); + cvt_p(ffi::X509_STORE_CTX_new()).map(|p| X509StoreContext(p)) + } + } } impl X509StoreContextRef { @@ -86,22 +98,6 @@ impl X509StoreContextRef { } } - /// Initializes the store context to verify the certificate. - /// - /// The context must be re-initialized before each call to `verify_cert`. - /// - /// # Arguments - /// - /// * `trust` - a store of the trusted chain of certificates, or CAs, to validated the certificate - /// * `cert` - certificate to validate - /// * `cert_chain` - the certificate's chain - pub fn init(&mut self, trust: &store::X509StoreRef, cert: &X509Ref, cert_chain: &StackRef) -> Result<(), ErrorStack> { - unsafe { - cvt(ffi::X509_STORE_CTX_init(self.as_ptr(), trust.as_ptr(), cert.as_ptr(), cert_chain.as_ptr())) - .map(|_| ()) - } - } - /// Returns the error code of the context. /// /// This corresponds to [`X509_STORE_CTX_get_error`]. @@ -111,27 +107,30 @@ impl X509StoreContextRef { unsafe { X509VerifyResult::from_raw(ffi::X509_STORE_CTX_get_error(self.as_ptr())) } } - /// Verifies the certificate associated in the `init()` method - /// * `cert_chain` - the certificates chain + /// Verifies a certificate with the given certificate store. + /// * `trust` - The certificate store with the trusted certificates. + /// * `cert` - The certificate that should be verified. + /// * `cert_chain` - The certificates chain. + /// + /// This corresponds to [`X509_STORE_CTX_init`] followed by [`X509_verify_cert`] and + /// [`X509_STORE_CTX_cleanup`]. + /// + /// [`X509_STORE_CTX_init`]: https://www.openssl.org/docs/man1.0.2/crypto/X509_STORE_CTX_init.html + /// [`X509_verify_cert`]: https://www.openssl.org/docs/man1.0.2/crypto/X509_verify_cert.html + /// [`X509_STORE_CTX_cleanup`]: https://www.openssl.org/docs/man1.0.2/crypto/X509_STORE_CTX_cleanup.html /// /// # Result /// - /// The Result must be `Some(None)` to be a valid certificate, otherwise the cert is not valid. - pub fn verify_cert(trust: store::X509Store, cert: X509, cert_chain: Stack) -> Result<(), ErrorStack> { + /// The Result must be `Ok(())` to be a valid certificate, otherwise the cert is not valid. + pub fn verify_cert(&mut self, trust: &store::X509StoreRef, cert: &X509Ref, + cert_chain: &StackRef) -> Result<(), ErrorStack> { unsafe { - ffi::init(); - let context = try!(cvt_p(ffi::X509_STORE_CTX_new()).map(|p| X509StoreContext(p))); - let init_result = cvt(ffi::X509_STORE_CTX_init(context.as_ptr(), trust.as_ptr(), cert.as_ptr(), cert_chain.as_ptr())) - .map(|_| ()); + cvt(ffi::X509_STORE_CTX_init(self.as_ptr(), trust.as_ptr(), + cert.as_ptr(), cert_chain.as_ptr()))?; - mem::forget(trust); - mem::forget(cert); - mem::forget(cert_chain); + cvt(ffi::X509_verify_cert(self.as_ptr()))?; - try!(init_result); - - // verify_cert returns an error `<= 0` if there was a validation error - try!(cvt(ffi::X509_verify_cert(context.as_ptr())).map(|_| ())); + ffi::X509_STORE_CTX_cleanup(self.as_ptr()); Ok(()) } diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index 7ea91432..31805956 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -7,9 +7,10 @@ use nid::Nid; use pkey::{PKey, Private}; use rsa::Rsa; use stack::Stack; -use x509::{X509, X509Name, X509Req, X509VerifyResult}; +use x509::{X509, X509Name, X509Req, X509VerifyResult, X509StoreContext}; use x509::extension::{AuthorityKeyIdentifier, BasicConstraints, ExtendedKeyUsage, KeyUsage, SubjectAlternativeName, SubjectKeyIdentifier}; +use x509::store::X509StoreBuilder; fn pkey() -> PKey { let rsa = Rsa::generate(2048).unwrap(); @@ -298,12 +299,14 @@ fn test_verify_cert() { let cert = X509::from_pem(cert).unwrap(); let ca = include_bytes!("../../test/root-ca.pem"); let ca = X509::from_pem(ca).unwrap(); + let chain = Stack::new().unwrap(); let mut store_bldr = X509StoreBuilder::new().unwrap(); store_bldr.add_cert(ca).unwrap(); let store = store_bldr.build(); - assert!(X509StoreContext::verify_cert(store, cert, Stack::new().unwrap()).is_ok()); + let mut context = X509StoreContext::new().unwrap(); + assert!(context.verify_cert(&store, &cert, &chain).is_ok()); } #[test] @@ -312,10 +315,12 @@ fn test_verify_fails() { let cert = X509::from_pem(cert).unwrap(); let ca = include_bytes!("../../test/alt_name_cert.pem"); let ca = X509::from_pem(ca).unwrap(); + let chain = Stack::new().unwrap(); let mut store_bldr = X509StoreBuilder::new().unwrap(); store_bldr.add_cert(ca).unwrap(); let store = store_bldr.build(); - assert!(X509StoreContext::verify_cert(store, cert, Stack::new().unwrap()).is_err()); + let mut context = X509StoreContext::new().unwrap(); + assert!(context.verify_cert(&store, &cert, &chain).is_err()); } From 810ddeb4cad73b5f44d2faa88e3439e45fe3bf66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 8 Mar 2018 12:08:39 +0100 Subject: [PATCH 13/16] Moves `cleanup` into its own function --- openssl/src/x509/mod.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index e2c87731..107bfe69 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -108,16 +108,16 @@ impl X509StoreContextRef { } /// Verifies a certificate with the given certificate store. + /// For successive calls to this function, it is required to call `cleanup` in beforehand. + /// /// * `trust` - The certificate store with the trusted certificates. /// * `cert` - The certificate that should be verified. /// * `cert_chain` - The certificates chain. /// - /// This corresponds to [`X509_STORE_CTX_init`] followed by [`X509_verify_cert`] and - /// [`X509_STORE_CTX_cleanup`]. + /// This corresponds to [`X509_STORE_CTX_init`] followed by [`X509_verify_cert`]. /// /// [`X509_STORE_CTX_init`]: https://www.openssl.org/docs/man1.0.2/crypto/X509_STORE_CTX_init.html /// [`X509_verify_cert`]: https://www.openssl.org/docs/man1.0.2/crypto/X509_verify_cert.html - /// [`X509_STORE_CTX_cleanup`]: https://www.openssl.org/docs/man1.0.2/crypto/X509_STORE_CTX_cleanup.html /// /// # Result /// @@ -129,13 +129,22 @@ impl X509StoreContextRef { cert.as_ptr(), cert_chain.as_ptr()))?; cvt(ffi::X509_verify_cert(self.as_ptr()))?; - - ffi::X509_STORE_CTX_cleanup(self.as_ptr()); Ok(()) } } + /// 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 + pub fn cleanup(&mut self) { + unsafe { + ffi::X509_STORE_CTX_cleanup(self.as_ptr()); + } + } + /// Set the error code of the context. /// /// This corresponds to [`X509_STORE_CTX_set_error`]. From 1a0b085377815c7377f63434bbd18d6da7903ad3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 8 Mar 2018 12:10:29 +0100 Subject: [PATCH 14/16] Extends the test to verify the certificate two times --- openssl/src/x509/tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index 31805956..8e89c3a0 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -307,6 +307,8 @@ fn test_verify_cert() { let mut context = X509StoreContext::new().unwrap(); assert!(context.verify_cert(&store, &cert, &chain).is_ok()); + context.cleanup(); + assert!(context.verify_cert(&store, &cert, &chain).is_ok()); } #[test] From a5d7f8a718bc16d8c7986ea13b8585af8f20a648 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sat, 10 Mar 2018 00:15:03 +0100 Subject: [PATCH 15/16] Moves store context init into its own function --- openssl/src/x509/mod.rs | 28 ++++++++++++++++++---------- openssl/src/x509/tests.rs | 9 ++++++--- 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index 107bfe69..55e5c75d 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -107,30 +107,38 @@ impl X509StoreContextRef { unsafe { X509VerifyResult::from_raw(ffi::X509_STORE_CTX_get_error(self.as_ptr())) } } - /// Verifies a certificate with the given certificate store. + /// Initializes this context with the given certificate, certificates chain and certificate + /// store. /// For successive calls to this function, it is required to call `cleanup` in beforehand. /// /// * `trust` - The certificate store with the trusted certificates. /// * `cert` - The certificate that should be verified. /// * `cert_chain` - The certificates chain. /// - /// This corresponds to [`X509_STORE_CTX_init`] followed by [`X509_verify_cert`]. + /// This corresponds to [`X509_STORE_CTX_init`]. /// /// [`X509_STORE_CTX_init`]: https://www.openssl.org/docs/man1.0.2/crypto/X509_STORE_CTX_init.html + pub fn init(&mut self, trust: &store::X509StoreRef, cert: &X509Ref, + cert_chain: &StackRef) -> Result<(), ErrorStack> { + unsafe { + cvt(ffi::X509_STORE_CTX_init(self.as_ptr(), trust.as_ptr(), + cert.as_ptr(), cert_chain.as_ptr())).map(|_| ()) + } + } + + /// Verifies the stored certificate. + /// It is required to call `init` in beforehand, to initialize the required values. + /// + /// 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, trust: &store::X509StoreRef, cert: &X509Ref, - cert_chain: &StackRef) -> Result<(), ErrorStack> { + pub fn verify_cert(&mut self) -> Result<(), ErrorStack> { unsafe { - cvt(ffi::X509_STORE_CTX_init(self.as_ptr(), trust.as_ptr(), - cert.as_ptr(), cert_chain.as_ptr()))?; - - cvt(ffi::X509_verify_cert(self.as_ptr()))?; - - Ok(()) + cvt(ffi::X509_verify_cert(self.as_ptr())).map(|_| ()) } } diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index 8e89c3a0..9b630d0e 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -306,9 +306,11 @@ fn test_verify_cert() { let store = store_bldr.build(); let mut context = X509StoreContext::new().unwrap(); - assert!(context.verify_cert(&store, &cert, &chain).is_ok()); + assert!(context.init(&store, &cert, &chain).is_ok()); + assert!(context.verify_cert().is_ok()); context.cleanup(); - assert!(context.verify_cert(&store, &cert, &chain).is_ok()); + assert!(context.init(&store, &cert, &chain).is_ok()); + assert!(context.verify_cert().is_ok()); } #[test] @@ -324,5 +326,6 @@ fn test_verify_fails() { let store = store_bldr.build(); let mut context = X509StoreContext::new().unwrap(); - assert!(context.verify_cert(&store, &cert, &chain).is_err()); + assert!(context.init(&store, &cert, &chain).is_ok()); + assert!(context.verify_cert().is_err()); } From d7a7c379a85965fa2a49849d9e424c2c11035d89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Sun, 11 Mar 2018 11:29:45 +0100 Subject: [PATCH 16/16] Changes `init` to take a closure which is called with the initialized context After calling the closure, we automatically cleanup the context. This is required, because otherwise we could have dangling references in the context. --- openssl/src/x509/mod.rs | 32 +++++++++++++++++++++++++------- openssl/src/x509/tests.rs | 10 +++------- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/openssl/src/x509/mod.rs b/openssl/src/x509/mod.rs index 55e5c75d..146a77b0 100644 --- a/openssl/src/x509/mod.rs +++ b/openssl/src/x509/mod.rs @@ -108,21 +108,39 @@ impl X509StoreContextRef { } /// Initializes this context with the given certificate, certificates chain and certificate - /// store. - /// For successive calls to this function, it is required to call `cleanup` in beforehand. + /// store. After initializing the context, the `with_context` closure is called with the prepared + /// context. As long as the closure is running, the context stays initialized and can be used + /// to e.g. verify a certificate. The context will be cleaned up, after the closure finished. /// /// * `trust` - The certificate store with the trusted certificates. /// * `cert` - The certificate that should be verified. /// * `cert_chain` - The certificates chain. + /// * `with_context` - The closure that is called with the initialized context. /// - /// This corresponds to [`X509_STORE_CTX_init`]. + /// This corresponds to [`X509_STORE_CTX_init`] before calling `with_context` and to + /// [`X509_STORE_CTX_cleanup`] after calling `with_context`. /// /// [`X509_STORE_CTX_init`]: https://www.openssl.org/docs/man1.0.2/crypto/X509_STORE_CTX_init.html - pub fn init(&mut self, trust: &store::X509StoreRef, cert: &X509Ref, - cert_chain: &StackRef) -> Result<(), ErrorStack> { + /// [`X509_STORE_CTX_cleanup`]: https://www.openssl.org/docs/man1.0.2/crypto/X509_STORE_CTX_cleanup.html + pub fn init(&mut self, trust: &store::X509StoreRef, cert: &X509Ref, + cert_chain: &StackRef, with_context: F) -> Result + where + F: FnOnce(&mut X509StoreContextRef) -> Result + { + struct Cleanup<'a>(&'a mut X509StoreContextRef); + + impl<'a> Drop for Cleanup<'a> { + fn drop(&mut self) { + self.0.cleanup(); + } + } + unsafe { cvt(ffi::X509_STORE_CTX_init(self.as_ptr(), trust.as_ptr(), - cert.as_ptr(), cert_chain.as_ptr())).map(|_| ()) + cert.as_ptr(), cert_chain.as_ptr()))?; + + let cleanup = Cleanup(self); + with_context(cleanup.0) } } @@ -147,7 +165,7 @@ impl X509StoreContextRef { /// 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 - pub fn cleanup(&mut self) { + fn cleanup(&mut self) { unsafe { ffi::X509_STORE_CTX_cleanup(self.as_ptr()); } diff --git a/openssl/src/x509/tests.rs b/openssl/src/x509/tests.rs index 9b630d0e..e3c726ae 100644 --- a/openssl/src/x509/tests.rs +++ b/openssl/src/x509/tests.rs @@ -306,11 +306,8 @@ fn test_verify_cert() { let store = store_bldr.build(); let mut context = X509StoreContext::new().unwrap(); - assert!(context.init(&store, &cert, &chain).is_ok()); - assert!(context.verify_cert().is_ok()); - context.cleanup(); - assert!(context.init(&store, &cert, &chain).is_ok()); - assert!(context.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()).is_ok()); } #[test] @@ -326,6 +323,5 @@ fn test_verify_fails() { let store = store_bldr.build(); let mut context = X509StoreContext::new().unwrap(); - assert!(context.init(&store, &cert, &chain).is_ok()); - assert!(context.verify_cert().is_err()); + assert!(context.init(&store, &cert, &chain, |c| c.verify_cert()).is_err()); }