Merge pull request #392 from rhenium/ky/x509store-review

Review OpenSSL::X509::Store

2 files changed, 417 insertions, 207 deletions

diff --git a/ext/openssl/ossl_x509store.c b/ext/openssl/ossl_x509store.c
index 61543d44..1eaaf4b3 100644
--- a/ext/openssl/ossl_x509store.c
+++ b/ext/openssl/ossl_x509store.c
@@ -157,9 +157,8 @@ ossl_x509store_alloc(VALUE klass)
     VALUE obj;
 
     obj = NewX509Store(klass);
-    if((store = X509_STORE_new()) == NULL){
-        ossl_raise(eX509StoreError, NULL);
-    }
+    if ((store = X509_STORE_new()) == NULL)
+        ossl_raise(eX509StoreError, "X509_STORE_new");
     SetX509Store(obj, store);
 
     return obj;
@@ -192,8 +191,9 @@ ossl_x509store_initialize(int argc, VALUE *argv, VALUE self)
 {
     X509_STORE *store;
 
-/* BUG: This method takes any number of arguments but appears to ignore them. */
     GetX509Store(self, store);
+    if (argc != 0)
+        rb_warn("OpenSSL::X509::Store.new does not take any arguments");
 #if !defined(HAVE_OPAQUE_OPENSSL)
     /* [Bug #405] [Bug #1678] [Bug #3000]; already fixed? */
     store->ex_data.sk = NULL;
@@ -214,8 +214,16 @@ ossl_x509store_initialize(int argc, VALUE *argv, VALUE self)
  * call-seq:
  *   store.flags = flags
  *
- * Sets _flags_ to the Store. _flags_ consists of zero or more of the constants
- * defined in with name V_FLAG_* or'ed together.
+ * Sets the default flags used by certificate chain verification performed with
+ * the Store.
+ *
+ * _flags_ consists of zero or more of the constants defined in OpenSSL::X509
+ * with name V_FLAG_* or'ed together.
+ *
+ * OpenSSL::X509::StoreContext#flags= can be used to change the flags for a
+ * single verification operation.
+ *
+ * See also the man page X509_VERIFY_PARAM_set_flags(3).
  */
 static VALUE
 ossl_x509store_set_flags(VALUE self, VALUE flags)
@@ -233,9 +241,9 @@ ossl_x509store_set_flags(VALUE self, VALUE flags)
  * call-seq:
  *   store.purpose = purpose
  *
- * Sets the store's purpose to _purpose_. If specified, the verifications on
- * the store will check every untrusted certificate's extensions are consistent
- * with the purpose. The purpose is specified by constants:
+ * Sets the store's default verification purpose. If specified,
+ * the verifications on the store will check every certificate's extensions are
+ * consistent with the purpose. The purpose is specified by constants:
  *
  * * X509::PURPOSE_SSL_CLIENT
  * * X509::PURPOSE_SSL_SERVER
@@ -246,6 +254,11 @@ ossl_x509store_set_flags(VALUE self, VALUE flags)
  * * X509::PURPOSE_ANY
  * * X509::PURPOSE_OCSP_HELPER
  * * X509::PURPOSE_TIMESTAMP_SIGN
+ *
+ * OpenSSL::X509::StoreContext#purpose= can be used to change the value for a
+ * single verification operation.
+ *
+ * See also the man page X509_VERIFY_PARAM_set_purpose(3).
  */
 static VALUE
 ossl_x509store_set_purpose(VALUE self, VALUE purpose)
@@ -262,6 +275,14 @@ ossl_x509store_set_purpose(VALUE self, VALUE purpose)
 /*
  * call-seq:
  *   store.trust = trust
+ *
+ * Sets the default trust settings used by the certificate verification with
+ * the store.
+ *
+ * OpenSSL::X509::StoreContext#trust= can be used to change the value for a
+ * single verification operation.
+ *
+ * See also the man page X509_VERIFY_PARAM_set_trust(3).
  */
 static VALUE
 ossl_x509store_set_trust(VALUE self, VALUE trust)
@@ -279,7 +300,13 @@ ossl_x509store_set_trust(VALUE self, VALUE trust)
  * call-seq:
  *   store.time = time
  *
- * Sets the time to be used in verifications.
+ * Sets the time to be used in the certificate verifications with the store.
+ * By default, if not specified, the current system time is used.
+ *
+ * OpenSSL::X509::StoreContext#time= can be used to change the value for a
+ * single verification operation.
+ *
+ * See also the man page X509_VERIFY_PARAM_set_time(3).
  */
 static VALUE
 ossl_x509store_set_time(VALUE self, VALUE time)
@@ -295,23 +322,23 @@ ossl_x509store_set_time(VALUE self, VALUE time)
  * Adds the certificates in _file_ to the certificate store. _file_ is the path
  * to the file, and the file contains one or more certificates in PEM format
  * concatenated together.
+ *
+ * See also the man page X509_LOOKUP_file(3).
  */
 static VALUE
 ossl_x509store_add_file(VALUE self, VALUE file)
 {
     X509_STORE *store;
     X509_LOOKUP *lookup;
-    char *path = NULL;
+    const char *path;
 
-    if(file != Qnil){
-	path = StringValueCStr(file);
-    }
     GetX509Store(self, store);
+    path = StringValueCStr(file);
     lookup = X509_STORE_add_lookup(store, X509_LOOKUP_file());
-    if(lookup == NULL) ossl_raise(eX509StoreError, NULL);
-    if(X509_LOOKUP_load_file(lookup, path, X509_FILETYPE_PEM) != 1){
-        ossl_raise(eX509StoreError, NULL);
-    }
+    if (!lookup)
+        ossl_raise(eX509StoreError, "X509_STORE_add_lookup");
+    if (X509_LOOKUP_load_file(lookup, path, X509_FILETYPE_PEM) != 1)
+        ossl_raise(eX509StoreError, "X509_LOOKUP_load_file");
 #if OPENSSL_VERSION_NUMBER < 0x10101000 || defined(LIBRESSL_VERSION_NUMBER)
     /*
      * X509_load_cert_crl_file() which is called from X509_LOOKUP_load_file()
@@ -330,23 +357,23 @@ ossl_x509store_add_file(VALUE self, VALUE file)
  *   store.add_path(path) -> self
  *
  * Adds _path_ as the hash dir to be looked up by the store.
+ *
+ * See also the man page X509_LOOKUP_hash_dir(3).
  */
 static VALUE
 ossl_x509store_add_path(VALUE self, VALUE dir)
 {
     X509_STORE *store;
     X509_LOOKUP *lookup;
-    char *path = NULL;
+    const char *path;
 
-    if(dir != Qnil){
-	path = StringValueCStr(dir);
-    }
     GetX509Store(self, store);
+    path = StringValueCStr(dir);
     lookup = X509_STORE_add_lookup(store, X509_LOOKUP_hash_dir());
-    if(lookup == NULL) ossl_raise(eX509StoreError, NULL);
-    if(X509_LOOKUP_add_dir(lookup, path, X509_FILETYPE_PEM) != 1){
-        ossl_raise(eX509StoreError, NULL);
-    }
+    if (!lookup)
+        ossl_raise(eX509StoreError, "X509_STORE_add_lookup");
+    if (X509_LOOKUP_add_dir(lookup, path, X509_FILETYPE_PEM) != 1)
+        ossl_raise(eX509StoreError, "X509_LOOKUP_add_dir");
 
     return self;
 }
@@ -361,6 +388,8 @@ ossl_x509store_add_path(VALUE self, VALUE dir)
  *
  * * OpenSSL::X509::DEFAULT_CERT_FILE
  * * OpenSSL::X509::DEFAULT_CERT_DIR
+ *
+ * See also the man page X509_STORE_set_default_paths(3).
  */
 static VALUE
 ossl_x509store_set_default_paths(VALUE self)
@@ -368,18 +397,19 @@ ossl_x509store_set_default_paths(VALUE self)
     X509_STORE *store;
 
     GetX509Store(self, store);
-    if (X509_STORE_set_default_paths(store) != 1){
-        ossl_raise(eX509StoreError, NULL);
-    }
+    if (X509_STORE_set_default_paths(store) != 1)
+        ossl_raise(eX509StoreError, "X509_STORE_set_default_paths");
 
     return Qnil;
 }
 
 /*
  * call-seq:
- *   store.add_cert(cert)
+ *   store.add_cert(cert) -> self
  *
  * Adds the OpenSSL::X509::Certificate _cert_ to the certificate store.
+ *
+ * See also the man page X509_STORE_add_cert(3).
  */
 static VALUE
 ossl_x509store_add_cert(VALUE self, VALUE arg)
@@ -389,9 +419,8 @@ ossl_x509store_add_cert(VALUE self, VALUE arg)
 
     cert = GetX509CertPtr(arg); /* NO NEED TO DUP */
     GetX509Store(self, store);
-    if (X509_STORE_add_cert(store, cert) != 1){
-        ossl_raise(eX509StoreError, NULL);
-    }
+    if (X509_STORE_add_cert(store, cert) != 1)
+        ossl_raise(eX509StoreError, "X509_STORE_add_cert");
 
     return self;
 }
@@ -401,6 +430,8 @@ ossl_x509store_add_cert(VALUE self, VALUE arg)
  *   store.add_crl(crl) -> self
  *
  * Adds the OpenSSL::X509::CRL _crl_ to the store.
+ *
+ * See also the man page X509_STORE_add_crl(3).
  */
 static VALUE
 ossl_x509store_add_crl(VALUE self, VALUE arg)
@@ -410,9 +441,8 @@ ossl_x509store_add_crl(VALUE self, VALUE arg)
 
     crl = GetX509CRLPtr(arg); /* NO NEED TO DUP */
     GetX509Store(self, store);
-    if (X509_STORE_add_crl(store, crl) != 1){
-        ossl_raise(eX509StoreError, NULL);
-    }
+    if (X509_STORE_add_crl(store, crl) != 1)
+        ossl_raise(eX509StoreError, "X509_STORE_add_crl");
 
     return self;
 }
@@ -491,9 +521,8 @@ ossl_x509stctx_alloc(VALUE klass)
     VALUE obj;
 
     obj = NewX509StCtx(klass);
-    if((ctx = X509_STORE_CTX_new()) == NULL){
-        ossl_raise(eX509StoreError, NULL);
-    }
+    if ((ctx = X509_STORE_CTX_new()) == NULL)
+        ossl_raise(eX509StoreError, "X509_STORE_CTX_new");
     SetX509StCtx(obj, ctx);
 
     return obj;
@@ -518,6 +547,8 @@ static VALUE ossl_x509stctx_set_time(VALUE, VALUE);
 /*
  * call-seq:
  *   StoreContext.new(store, cert = nil, chain = nil)
+ *
+ * Sets up a StoreContext for a verification of the X.509 certificate _cert_.
  */
 static VALUE
 ossl_x509stctx_initialize(int argc, VALUE *argv, VALUE self)
@@ -548,6 +579,10 @@ ossl_x509stctx_initialize(int argc, VALUE *argv, VALUE self)
 /*
  * call-seq:
  *   stctx.verify -> true | false
+ *
+ * Performs the certificate verification using the parameters set to _stctx_.
+ *
+ * See also the man page X509_verify_cert(3).
  */
 static VALUE
 ossl_x509stctx_verify(VALUE self)
@@ -560,48 +595,45 @@ ossl_x509stctx_verify(VALUE self)
 
     switch (X509_verify_cert(ctx)) {
       case 1:
-	return Qtrue;
+        return Qtrue;
       case 0:
-	ossl_clear_error();
-	return Qfalse;
+        ossl_clear_error();
+        return Qfalse;
       default:
-	ossl_raise(eX509CertError, NULL);
+        ossl_raise(eX509CertError, "X509_verify_cert");
     }
 }
 
 /*
  * call-seq:
- *   stctx.chain -> Array of X509::Certificate
+ *   stctx.chain -> nil | Array of X509::Certificate
+ *
+ * Returns the verified chain.
+ *
+ * See also the man page X509_STORE_CTX_set0_verified_chain(3).
  */
 static VALUE
 ossl_x509stctx_get_chain(VALUE self)
 {
     X509_STORE_CTX *ctx;
-    STACK_OF(X509) *chain;
-    X509 *x509;
-    int i, num;
-    VALUE ary;
+    const STACK_OF(X509) *chain;
 
     GetX509StCtx(self, ctx);
-    if((chain = X509_STORE_CTX_get0_chain(ctx)) == NULL){
-        return Qnil;
-    }
-    if((num = sk_X509_num(chain)) < 0){
-	OSSL_Debug("certs in chain < 0???");
-	return rb_ary_new();
-    }
-    ary = rb_ary_new2(num);
-    for(i = 0; i < num; i++) {
-	x509 = sk_X509_value(chain, i);
-	rb_ary_push(ary, ossl_x509_new(x509));
-    }
-
-    return ary;
+    chain = X509_STORE_CTX_get0_chain(ctx);
+    if (!chain)
+        return Qnil; /* Could be an empty array instead? */
+    return ossl_x509_sk2ary(chain);
 }
 
 /*
  * call-seq:
  *   stctx.error -> Integer
+ *
+ * Returns the error code of _stctx_. This is typically called after #verify
+ * is done, or from the verification callback set to
+ * OpenSSL::X509::Store#verify_callback=.
+ *
+ * See also the man page X509_STORE_CTX_get_error(3).
  */
 static VALUE
 ossl_x509stctx_get_err(VALUE self)
@@ -616,6 +648,11 @@ ossl_x509stctx_get_err(VALUE self)
 /*
  * call-seq:
  *   stctx.error = error_code
+ *
+ * Sets the error code of _stctx_. This is used by the verification callback
+ * set to OpenSSL::X509::Store#verify_callback=.
+ *
+ * See also the man page X509_STORE_CTX_set_error(3).
  */
 static VALUE
 ossl_x509stctx_set_error(VALUE self, VALUE err)
@@ -632,7 +669,10 @@ ossl_x509stctx_set_error(VALUE self, VALUE err)
  * call-seq:
  *   stctx.error_string -> String
  *
- * Returns the error string corresponding to the error code retrieved by #error.
+ * Returns the human readable error string corresponding to the error code
+ * retrieved by #error.
+ *
+ * See also the man page X509_verify_cert_error_string(3).
  */
 static VALUE
 ossl_x509stctx_get_err_string(VALUE self)
@@ -649,6 +689,10 @@ ossl_x509stctx_get_err_string(VALUE self)
 /*
  * call-seq:
  *   stctx.error_depth -> Integer
+ *
+ * Returns the depth of the chain. This is used in combination with #error.
+ *
+ * See also the man page X509_STORE_CTX_get_error_depth(3).
  */
 static VALUE
 ossl_x509stctx_get_err_depth(VALUE self)
@@ -663,6 +707,10 @@ ossl_x509stctx_get_err_depth(VALUE self)
 /*
  * call-seq:
  *   stctx.current_cert -> X509::Certificate
+ *
+ * Returns the certificate which caused the error.
+ *
+ * See also the man page X509_STORE_CTX_get_current_cert(3).
  */
 static VALUE
 ossl_x509stctx_get_curr_cert(VALUE self)
@@ -677,6 +725,10 @@ ossl_x509stctx_get_curr_cert(VALUE self)
 /*
  * call-seq:
  *   stctx.current_crl -> X509::CRL
+ *
+ * Returns the CRL which caused the error.
+ *
+ * See also the man page X509_STORE_CTX_get_current_crl(3).
  */
 static VALUE
 ossl_x509stctx_get_curr_crl(VALUE self)
@@ -696,7 +748,10 @@ ossl_x509stctx_get_curr_crl(VALUE self)
  * call-seq:
  *   stctx.flags = flags
  *
- * Sets the verification flags to the context. See Store#flags=.
+ * Sets the verification flags to the context. This overrides the default value
+ * set by Store#flags=.
+ *
+ * See also the man page X509_VERIFY_PARAM_set_flags(3).
  */
 static VALUE
 ossl_x509stctx_set_flags(VALUE self, VALUE flags)
@@ -714,7 +769,10 @@ ossl_x509stctx_set_flags(VALUE self, VALUE flags)
  * call-seq:
  *   stctx.purpose = purpose
  *
- * Sets the purpose of the context. See Store#purpose=.
+ * Sets the purpose of the context. This overrides the default value set by
+ * Store#purpose=.
+ *
+ * See also the man page X509_VERIFY_PARAM_set_purpose(3).
  */
 static VALUE
 ossl_x509stctx_set_purpose(VALUE self, VALUE purpose)
@@ -731,6 +789,11 @@ ossl_x509stctx_set_purpose(VALUE self, VALUE purpose)
 /*
  * call-seq:
  *   stctx.trust = trust
+ *
+ * Sets the trust settings of the context. This overrides the default value set
+ * by Store#trust=.
+ *
+ * See also the man page X509_VERIFY_PARAM_set_trust(3).
  */
 static VALUE
 ossl_x509stctx_set_trust(VALUE self, VALUE trust)
@@ -749,6 +812,8 @@ ossl_x509stctx_set_trust(VALUE self, VALUE trust)
  *   stctx.time = time
  *
  * Sets the time used in the verification. If not set, the current time is used.
+ *
+ * See also the man page X509_VERIFY_PARAM_set_time(3).
  */
 static VALUE
 ossl_x509stctx_set_time(VALUE self, VALUE time)
@@ -824,23 +889,37 @@ Init_ossl_x509store(void)
     cX509Store = rb_define_class_under(mX509, "Store", rb_cObject);
     /*
      * The callback for additional certificate verification. It is invoked for
-     * each untrusted certificate in the chain.
+     * each certificate in the chain and can be used to implement custom
+     * certificate verification conditions.
      *
      * The callback is invoked with two values, a boolean that indicates if the
      * pre-verification by OpenSSL has succeeded or not, and the StoreContext in
-     * use. The callback must return either true or false.
+     * use.
+     *
+     * The callback can use StoreContext#error= to change the error code as
+     * needed. The callback must return either true or false.
+     *
+     * NOTE: any exception raised within the callback will be ignored.
+     *
+     * See also the man page X509_STORE_CTX_set_verify_cb(3).
      */
     rb_attr(cX509Store, rb_intern("verify_callback"), 1, 0, Qfalse);
     /*
      * The error code set by the last call of #verify.
+     *
+     * See also StoreContext#error.
      */
     rb_attr(cX509Store, rb_intern("error"), 1, 0, Qfalse);
     /*
      * The description for the error code set by the last call of #verify.
+     *
+     * See also StoreContext#error_string.
      */
     rb_attr(cX509Store, rb_intern("error_string"), 1, 0, Qfalse);
     /*
      * The certificate chain constructed by the last call of #verify.
+     *
+     * See also StoreContext#chain.
      */
     rb_attr(cX509Store, rb_intern("chain"), 1, 0, Qfalse);
     rb_define_alloc_func(cX509Store, ossl_x509store_alloc);
diff --git a/test/openssl/test_x509store.rb b/test/openssl/test_x509store.rb
index e9602e34..7bbbc662 100644
--- a/test/openssl/test_x509store.rb
+++ b/test/openssl/test_x509store.rb
@@ -4,37 +4,27 @@ require_relative "utils"
 if defined?(OpenSSL)
 
 class OpenSSL::TestX509Store < OpenSSL::TestCase
-  def setup
-    super
-    @rsa1024 = Fixtures.pkey("rsa1024")
-    @rsa2048 = Fixtures.pkey("rsa2048")
-    @dsa256  = Fixtures.pkey("dsa256")
-    @dsa512  = Fixtures.pkey("dsa512")
-    @ca1 = OpenSSL::X509::Name.parse("/DC=org/DC=ruby-lang/CN=CA1")
-    @ca2 = OpenSSL::X509::Name.parse("/DC=org/DC=ruby-lang/CN=CA2")
-    @ee1 = OpenSSL::X509::Name.parse("/DC=org/DC=ruby-lang/CN=EE1")
-    @ee2 = OpenSSL::X509::Name.parse("/DC=org/DC=ruby-lang/CN=EE2")
-  end
-
-  def test_nosegv_on_cleanup
-    cert  = OpenSSL::X509::Certificate.new
-    store = OpenSSL::X509::Store.new
-    ctx   = OpenSSL::X509::StoreContext.new(store, cert, [])
-    EnvUtil.suppress_warning do
-      ctx.cleanup
-    end
-    ctx.verify
+  def test_store_new
+    # v2.3.0 emits explicit warning
+    assert_warning(/new does not take any arguments/) {
+      OpenSSL::X509::Store.new(123)
+    }
   end
 
-  def test_add_file
+  def test_add_file_path
     ca_exts = [
       ["basicConstraints", "CA:TRUE", true],
       ["keyUsage", "cRLSign,keyCertSign", true],
     ]
-    cert1 = issue_cert(@ca1, @rsa1024, 1, ca_exts, nil, nil)
-    cert2 = issue_cert(@ca2, @rsa2048, 1, ca_exts, nil, nil)
-    tmpfile = Tempfile.open { |f| f << cert1.to_pem << cert2.to_pem; f }
+    cert1_subj = OpenSSL::X509::Name.parse_rfc2253("CN=Cert 1")
+    cert1_key = Fixtures.pkey("rsa-1")
+    cert1 = issue_cert(cert1_subj, cert1_key, 1, ca_exts, nil, nil)
+    cert2_subj = OpenSSL::X509::Name.parse_rfc2253("CN=Cert 2")
+    cert2_key = Fixtures.pkey("rsa-2")
+    cert2 = issue_cert(cert2_subj, cert2_key, 1, ca_exts, nil, nil)
 
+    # X509::Store#add_file reads concatenated PEM file
+    tmpfile = Tempfile.open { |f| f << cert1.to_pem << cert2.to_pem; f }
     store = OpenSSL::X509::Store.new
     assert_equal false, store.verify(cert1)
     assert_equal false, store.verify(cert2)
@@ -42,188 +32,321 @@ class OpenSSL::TestX509Store < OpenSSL::TestCase
     assert_equal true, store.verify(cert1)
     assert_equal true, store.verify(cert2)
 
+    # X509::Store#add_path
+    Dir.mktmpdir do |dir|
+      hash1 = "%08x.%d" % [cert1_subj.hash, 0]
+      File.write(File.join(dir, hash1), cert1.to_pem)
+      store = OpenSSL::X509::Store.new
+      store.add_path(dir)
+
+      assert_equal true, store.verify(cert1)
+      assert_equal false, store.verify(cert2)
+    end
+
     # OpenSSL < 1.1.1 leaks an error on a duplicate certificate
     assert_nothing_raised { store.add_file(tmpfile.path) }
     assert_equal [], OpenSSL.errors
+
+    # Non-String is given
+    assert_raise(TypeError) { store.add_file(nil) }
   ensure
     tmpfile and tmpfile.close!
   end
 
-  def test_verify
-    # OpenSSL uses time(2) while Time.now uses clock_gettime(CLOCK_REALTIME),
-    # and there may be difference.
-    now = Time.now - 3
+  def test_verify_simple
     ca_exts = [
-      ["basicConstraints","CA:TRUE",true],
-      ["keyUsage","cRLSign,keyCertSign",true],
+      ["basicConstraints", "CA:TRUE", true],
+      ["keyUsage", "cRLSign,keyCertSign", true],
     ]
+    ca1 = OpenSSL::X509::Name.parse_rfc2253("CN=Root CA")
+    ca1_key = Fixtures.pkey("rsa-1")
+    ca1_cert = issue_cert(ca1, ca1_key, 1, ca_exts, nil, nil)
+    ca2 = OpenSSL::X509::Name.parse_rfc2253("CN=Intermediate CA")
+    ca2_key = Fixtures.pkey("rsa-2")
+    ca2_cert = issue_cert(ca2, ca2_key, 2, ca_exts, ca1_cert, ca1_key)
+
     ee_exts = [
-      ["keyUsage","keyEncipherment,digitalSignature",true],
+      ["keyUsage", "keyEncipherment,digitalSignature", true],
     ]
-    ca1_cert = issue_cert(@ca1, @rsa2048, 1, ca_exts, nil, nil)
-    ca2_cert = issue_cert(@ca2, @rsa1024, 2, ca_exts, ca1_cert, @rsa2048,
-                          not_after: now+1800)
-    ee1_cert = issue_cert(@ee1, @dsa256, 10, ee_exts, ca2_cert, @rsa1024)
-    ee2_cert = issue_cert(@ee2, @dsa512, 20, ee_exts, ca2_cert, @rsa1024)
-    ee3_cert = issue_cert(@ee2, @dsa512, 30,  ee_exts, ca2_cert, @rsa1024,
-                          not_before: now-100, not_after: now-1)
-    ee4_cert = issue_cert(@ee2, @dsa512, 40, ee_exts, ca2_cert, @rsa1024,
-                          not_before: now+1000, not_after: now+2000,)
+    ee1 = OpenSSL::X509::Name.parse_rfc2253("CN=EE 1")
+    ee1_key = Fixtures.pkey("rsa-3")
+    ee1_cert = issue_cert(ee1, ee1_key, 10, ee_exts, ca2_cert, ca2_key)
 
-    revoke_info = []
-    crl1   = issue_crl(revoke_info, 1, now, now+1800, [],
-                       ca1_cert, @rsa2048, OpenSSL::Digest.new('SHA1'))
-    revoke_info = [ [2, now, 1], ]
-    crl1_2 = issue_crl(revoke_info, 2, now, now+1800, [],
-                       ca1_cert, @rsa2048, OpenSSL::Digest.new('SHA1'))
-    revoke_info = [ [20, now, 1], ]
-    crl2   = issue_crl(revoke_info, 1, now, now+1800, [],
-                       ca2_cert, @rsa1024, OpenSSL::Digest.new('SHA1'))
-    revoke_info = []
-    crl2_2 = issue_crl(revoke_info, 2, now-100, now-1, [],
-                       ca2_cert, @rsa1024, OpenSSL::Digest.new('SHA1'))
+    # Nothing trusted
+    store = OpenSSL::X509::Store.new
+    assert_equal(false, store.verify(ee1_cert, [ca2_cert, ca1_cert]))
+    assert_equal(OpenSSL::X509::V_ERR_SELF_SIGNED_CERT_IN_CHAIN, store.error)
+    assert_match(/self.signed/i, store.error_string)
 
-    assert_equal(true, ca1_cert.verify(ca1_cert.public_key))   # self signed
-    assert_equal(true, ca2_cert.verify(ca1_cert.public_key))   # issued by ca1
-    assert_equal(true, ee1_cert.verify(ca2_cert.public_key))   # issued by ca2
-    assert_equal(true, ee2_cert.verify(ca2_cert.public_key))   # issued by ca2
-    assert_equal(true, ee3_cert.verify(ca2_cert.public_key))   # issued by ca2
-    assert_equal(true, crl1.verify(ca1_cert.public_key))       # issued by ca1
-    assert_equal(true, crl1_2.verify(ca1_cert.public_key))     # issued by ca1
-    assert_equal(true, crl2.verify(ca2_cert.public_key))       # issued by ca2
-    assert_equal(true, crl2_2.verify(ca2_cert.public_key))     # issued by ca2
+    # CA1 trusted, CA2 missing
+    store = OpenSSL::X509::Store.new
+    store.add_cert(ca1_cert)
+    assert_equal(false, store.verify(ee1_cert))
+    assert_equal(OpenSSL::X509::V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY, store.error)
 
+    # CA1 trusted, CA2 supplied
     store = OpenSSL::X509::Store.new
-    assert_equal(false, store.verify(ca1_cert))
-    assert_not_equal(OpenSSL::X509::V_OK, store.error)
+    store.add_cert(ca1_cert)
+    assert_equal(true, store.verify(ee1_cert, [ca2_cert]))
+    assert_match(/ok/i, store.error_string)
+    assert_equal(OpenSSL::X509::V_OK, store.error)
+    assert_equal([ee1_cert, ca2_cert, ca1_cert], store.chain)
+  end
 
-    assert_equal(false, store.verify(ca2_cert))
-    assert_not_equal(OpenSSL::X509::V_OK, store.error)
+  def test_verify_callback
+    ca_exts = [
+      ["basicConstraints", "CA:TRUE", true],
+      ["keyUsage", "cRLSign,keyCertSign", true],
+    ]
+    ca1 = OpenSSL::X509::Name.parse_rfc2253("CN=Root CA")
+    ca1_key = Fixtures.pkey("rsa-1")
+    ca1_cert = issue_cert(ca1, ca1_key, 1, ca_exts, nil, nil)
+    ca2 = OpenSSL::X509::Name.parse_rfc2253("CN=Intermediate CA")
+    ca2_key = Fixtures.pkey("rsa-2")
+    ca2_cert = issue_cert(ca2, ca2_key, 2, ca_exts, ca1_cert, ca1_key)
 
+    ee_exts = [
+      ["keyUsage", "keyEncipherment,digitalSignature", true],
+    ]
+    ee1 = OpenSSL::X509::Name.parse_rfc2253("CN=EE 1")
+    ee1_key = Fixtures.pkey("rsa-3")
+    ee1_cert = issue_cert(ee1, ee1_key, 10, ee_exts, ca2_cert, ca2_key)
+
+    # verify_callback on X509::Store is called with proper arguments
+    cb_calls = []
+    store = OpenSSL::X509::Store.new
+    store.verify_callback = -> (preverify_ok, sctx) {
+      cb_calls << [preverify_ok, sctx.current_cert]
+      preverify_ok
+    }
     store.add_cert(ca1_cert)
-    assert_equal(true, store.verify(ca2_cert))
-    assert_equal(OpenSSL::X509::V_OK, store.error)
-    assert_equal("ok", store.error_string)
-    chain = store.chain
-    assert_equal(2, chain.size)
-    assert_equal(@ca2.to_der, chain[0].subject.to_der)
-    assert_equal(@ca1.to_der, chain[1].subject.to_der)
+    assert_equal(true, store.verify(ee1_cert, [ca2_cert]))
+    assert_equal(3, cb_calls.size)
+    assert_equal([true, ca1_cert], cb_calls[0])
+    assert_equal([true, ca2_cert], cb_calls[1])
+    assert_equal([true, ee1_cert], cb_calls[2])
 
-    store.purpose = OpenSSL::X509::PURPOSE_SSL_CLIENT
-    assert_equal(false, store.verify(ca2_cert))
-    assert_not_equal(OpenSSL::X509::V_OK, store.error)
+    # verify_callback can change verification result
+    store = OpenSSL::X509::Store.new
+    store.verify_callback = -> (preverify_ok, sctx) {
+      next preverify_ok if sctx.current_cert != ee1_cert
+      sctx.error = OpenSSL::X509::V_ERR_APPLICATION_VERIFICATION
+      false
+    }
+    store.add_cert(ca1_cert)
+    assert_equal(false, store.verify(ee1_cert, [ca2_cert]))
+    assert_equal(OpenSSL::X509::V_ERR_APPLICATION_VERIFICATION, store.error)
 
-    store.purpose = OpenSSL::X509::PURPOSE_CRL_SIGN
-    assert_equal(true, store.verify(ca2_cert))
-    assert_equal(OpenSSL::X509::V_OK, store.error)
+    # Exception raised by verify_callback is currently suppressed, and is
+    # treated as a non-truthy return (with warning)
+    store = OpenSSL::X509::Store.new
+    store.verify_callback = -> (preverify_ok, sctx) {
+      raise "suppressed"
+    }
+    store.add_cert(ca1_cert)
+    assert_warning(/exception in verify_callback/) {
+      assert_equal(false, store.verify(ee1_cert, [ca2_cert]))
+    }
 
-    store.add_cert(ca2_cert)
-    store.purpose = OpenSSL::X509::PURPOSE_SSL_CLIENT
-    assert_equal(true, store.verify(ee1_cert))
-    assert_equal(true, store.verify(ee2_cert))
-    assert_equal(OpenSSL::X509::V_OK, store.error)
-    assert_equal("ok", store.error_string)
-    chain = store.chain
-    assert_equal(3, chain.size)
-    assert_equal(@ee2.to_der, chain[0].subject.to_der)
-    assert_equal(@ca2.to_der, chain[1].subject.to_der)
-    assert_equal(@ca1.to_der, chain[2].subject.to_der)
-    assert_equal(false, store.verify(ee3_cert))
-    assert_equal(OpenSSL::X509::V_ERR_CERT_HAS_EXPIRED, store.error)
-    assert_match(/expire/i, store.error_string)
-    assert_equal(false, store.verify(ee4_cert))
-    assert_equal(OpenSSL::X509::V_ERR_CERT_NOT_YET_VALID, store.error)
-    assert_match(/not yet valid/i, store.error_string)
+    # The block given to X509::Store#verify replaces it
+    called = nil
+    store = OpenSSL::X509::Store.new
+    store.verify_callback = -> (preverify_ok, sctx) { called = :store; preverify_ok }
+    store.add_cert(ca1_cert)
+    blk = proc { |preverify_ok, sctx| called = :block; preverify_ok }
+    assert_equal(true, store.verify(ee1_cert, [ca2_cert], &blk))
+    assert_equal(:block, called)
+  end
+
+  def test_verify_purpose
+    ca_exts = [
+      ["basicConstraints", "CA:TRUE", true],
+      ["keyUsage", "cRLSign,keyCertSign", true],
+    ]
+    ca1 = OpenSSL::X509::Name.parse_rfc2253("CN=Root CA")
+    ca1_key = Fixtures.pkey("rsa-1")
+    ca1_cert = issue_cert(ca1, ca1_key, 1, ca_exts, nil, nil)
 
+    ee_exts = [
+      ["keyUsage", "keyEncipherment,digitalSignature", true],
+    ]
+    ee1 = OpenSSL::X509::Name.parse_rfc2253("CN=EE 1")
+    ee1_key = Fixtures.pkey("rsa-3")
+    ee1_cert = issue_cert(ee1, ee1_key, 10, ee_exts, ca1_cert, ca1_key)
+
+    # Purpose not set
     store = OpenSSL::X509::Store.new
     store.add_cert(ca1_cert)
-    store.add_cert(ca2_cert)
-    store.time = now + 1500
     assert_equal(true, store.verify(ca1_cert))
-    assert_equal(true, store.verify(ca2_cert))
-    assert_equal(true, store.verify(ee4_cert))
-    store.time = now + 1900
+    assert_equal(true, store.verify(ee1_cert))
+
+    # Purpose set to X509::PURPOSE_SSL_CLIENT; keyUsage is checked
+    store = OpenSSL::X509::Store.new
+    store.purpose = OpenSSL::X509::PURPOSE_CRL_SIGN
+    store.add_cert(ca1_cert)
     assert_equal(true, store.verify(ca1_cert))
-    assert_equal(false, store.verify(ca2_cert))
-    assert_equal(OpenSSL::X509::V_ERR_CERT_HAS_EXPIRED, store.error)
-    assert_equal(false, store.verify(ee4_cert))
-    assert_equal(OpenSSL::X509::V_ERR_CERT_HAS_EXPIRED, store.error)
-    store.time = now + 4000
     assert_equal(false, store.verify(ee1_cert))
-    assert_equal(OpenSSL::X509::V_ERR_CERT_HAS_EXPIRED, store.error)
-    assert_equal(false, store.verify(ee4_cert))
-    assert_equal(OpenSSL::X509::V_ERR_CERT_HAS_EXPIRED, store.error)
+  end
+
+  def test_verify_validity_period
+    # Creating test certificates with validity periods:
+    #
+    #  now-5000                 now-1000    now+1000                  now+5000
+    # CA1:|---------------------------------------------------------------|
+    # EE1:|---------------------------------------------------------------|
+    # EE2:|-------------------------|
+    # EE3:                                      |-------------------------|
+    now = Time.now
+    ca_exts = [
+      ["basicConstraints", "CA:TRUE", true],
+      ["keyUsage", "cRLSign,keyCertSign", true],
+    ]
+    ca1 = OpenSSL::X509::Name.parse_rfc2253("CN=Root CA")
+    ca1_key = Fixtures.pkey("rsa-1")
+    ca1_cert = issue_cert(ca1, ca1_key, 1, ca_exts, nil, nil,
+                          not_before: now - 5000, not_after: now + 5000)
 
-    # the underlying X509 struct caches the result of the last
-    # verification for signature and not-before. so the following code
-    # rebuilds new objects to avoid site effect.
-    store.time = Time.now - 4000
-    assert_equal(false, store.verify(OpenSSL::X509::Certificate.new(ca2_cert)))
+    ee_exts = [
+      ["keyUsage", "keyEncipherment,digitalSignature", true],
+    ]
+    ee1 = OpenSSL::X509::Name.parse_rfc2253("CN=EE 1")
+    ee1_key = Fixtures.pkey("rsa-1")
+    ee1_cert = issue_cert(ee1, ee1_key, 11, ee_exts, ca1_cert, ca1_key,
+                          not_before: now - 5000, not_after: now + 5000)
+    ee2 = OpenSSL::X509::Name.parse_rfc2253("CN=EE 2")
+    ee2_key = Fixtures.pkey("rsa-2")
+    ee2_cert = issue_cert(ee2, ee2_key, 12, ee_exts, ca1_cert, ca1_key,
+                          not_before: now - 5000, not_after: now - 1000)
+    ee3 = OpenSSL::X509::Name.parse_rfc2253("CN=EE 3")
+    ee3_key = Fixtures.pkey("rsa-3")
+    ee3_cert = issue_cert(ee3, ee3_key, 13, ee_exts, ca1_cert, ca1_key,
+                          not_before: now + 1000, not_after: now + 5000)
+
+    # Using system time
+    store = OpenSSL::X509::Store.new
+    store.add_cert(ca1_cert)
+    assert_equal(true, store.verify(ee1_cert))
+    assert_equal(false, store.verify(ee2_cert))
+    assert_equal(OpenSSL::X509::V_ERR_CERT_HAS_EXPIRED, store.error)
+    assert_equal(false, store.verify(ee3_cert))
     assert_equal(OpenSSL::X509::V_ERR_CERT_NOT_YET_VALID, store.error)
-    assert_equal(false, store.verify(OpenSSL::X509::Certificate.new(ee1_cert)))
+
+    # Time set to now-2000; EE2 is still valid
+    store = OpenSSL::X509::Store.new
+    store.time = now - 2000
+    store.add_cert(ca1_cert)
+    assert_equal(true, store.verify(ee1_cert))
+    assert_equal(true, store.verify(ee2_cert))
+    assert_equal(false, store.verify(ee3_cert))
     assert_equal(OpenSSL::X509::V_ERR_CERT_NOT_YET_VALID, store.error)
+  end
+
+  def test_verify_with_crl
+    ca_exts = [
+      ["basicConstraints", "CA:TRUE", true],
+      ["keyUsage", "cRLSign,keyCertSign", true],
+    ]
+    ca1 = OpenSSL::X509::Name.parse_rfc2253("CN=Root CA")
+    ca1_key = Fixtures.pkey("rsa-1")
+    ca1_cert = issue_cert(ca1, ca1_key, 1, ca_exts, nil, nil)
+    ca2 = OpenSSL::X509::Name.parse_rfc2253("CN=Intermediate CA")
+    ca2_key = Fixtures.pkey("rsa-2")
+    ca2_cert = issue_cert(ca2, ca2_key, 2, ca_exts, ca1_cert, ca1_key)
+
+    ee_exts = [
+      ["keyUsage", "keyEncipherment,digitalSignature", true],
+    ]
+    ee1 = OpenSSL::X509::Name.parse_rfc2253("CN=EE 1")
+    ee1_key = Fixtures.pkey("rsa-3")
+    ee1_cert = issue_cert(ee1, ee1_key, 10, ee_exts, ca2_cert, ca2_key)
+    ee2 = OpenSSL::X509::Name.parse_rfc2253("CN=EE 2")
+    ee2_key = Fixtures.pkey("rsa-3")
+    ee2_cert = issue_cert(ee2, ee2_key, 20, ee_exts, ca2_cert, ca2_key)
+
+    # OpenSSL uses time(2) while Time.now uses clock_gettime(CLOCK_REALTIME),
+    # and there may be difference, so giving 50 seconds margin.
+    now = Time.now - 50
+    revoke_info = []
+    ca1_crl1 = issue_crl(revoke_info, 1, now, now+1800, [], ca1_cert, ca1_key, "sha256")
+    revoke_info = [ [2, now, 1], ]
+    ca1_crl2 = issue_crl(revoke_info, 2, now, now+1800, [], ca1_cert, ca1_key, "sha256")
+
+    revoke_info = [ [20, now, 1], ]
+    ca2_crl1 = issue_crl(revoke_info, 1, now, now+1800, [], ca2_cert, ca2_key, "sha256")
+    revoke_info = []
+    ca2_crl2 = issue_crl(revoke_info, 2, now-100, now-1, [], ca2_cert, ca2_key, "sha256")
 
+    # CRL check required, but no CRL supplied
     store = OpenSSL::X509::Store.new
-    store.purpose = OpenSSL::X509::PURPOSE_ANY
     store.flags = OpenSSL::X509::V_FLAG_CRL_CHECK
     store.add_cert(ca1_cert)
-    store.add_crl(crl1)   # revoke no cert
-    store.add_crl(crl2)   # revoke ee2_cert
-    assert_equal(true,  store.verify(ca1_cert))
-    assert_equal(true,  store.verify(ca2_cert))
-    assert_equal(true,  store.verify(ee1_cert, [ca2_cert]))
+    assert_equal(false, store.verify(ca2_cert))
+    assert_equal(OpenSSL::X509::V_ERR_UNABLE_TO_GET_CRL, store.error)
+
+    # Intermediate CA revoked EE2
+    store = OpenSSL::X509::Store.new
+    store.flags = OpenSSL::X509::V_FLAG_CRL_CHECK
+    store.add_cert(ca1_cert)
+    store.add_crl(ca1_crl1) # revoke no cert
+    store.add_crl(ca2_crl1) # revoke ee2_cert
+    assert_equal(true, store.verify(ca2_cert))
+    assert_equal(true, store.verify(ee1_cert, [ca2_cert]))
     assert_equal(false, store.verify(ee2_cert, [ca2_cert]))
 
+    # Root CA revoked Intermediate CA; Intermediate CA revoked EE2
     store = OpenSSL::X509::Store.new
-    store.purpose = OpenSSL::X509::PURPOSE_ANY
     store.flags = OpenSSL::X509::V_FLAG_CRL_CHECK
     store.add_cert(ca1_cert)
-    store.add_crl(crl1_2) # revoke ca2_cert
-    store.add_crl(crl2)   # revoke ee2_cert
-    assert_equal(true,  store.verify(ca1_cert))
+    store.add_crl(ca1_crl2) # revoke ca2_cert
+    store.add_crl(ca2_crl1) # revoke ee2_cert
     assert_equal(false, store.verify(ca2_cert))
-    assert_equal(true,  store.verify(ee1_cert, [ca2_cert]),
-      "This test is expected to be success with OpenSSL 0.9.7c or later.")
+    # Validity of intermediate CAs is not checked by default
+    assert_equal(true, store.verify(ee1_cert, [ca2_cert]))
     assert_equal(false, store.verify(ee2_cert, [ca2_cert]))
 
-    store.flags =
-      OpenSSL::X509::V_FLAG_CRL_CHECK|OpenSSL::X509::V_FLAG_CRL_CHECK_ALL
-    assert_equal(true,  store.verify(ca1_cert))
+    # Same as above, but with OpenSSL::X509::V_FLAG_CRL_CHECK_ALL
+    store = OpenSSL::X509::Store.new
+    store.flags = OpenSSL::X509::V_FLAG_CRL_CHECK|OpenSSL::X509::V_FLAG_CRL_CHECK_ALL
+    store.add_cert(ca1_cert)
+    store.add_crl(ca1_crl2) # revoke ca2_cert
+    store.add_crl(ca2_crl1) # revoke ee2_cert
     assert_equal(false, store.verify(ca2_cert))
     assert_equal(false, store.verify(ee1_cert, [ca2_cert]))
     assert_equal(false, store.verify(ee2_cert, [ca2_cert]))
 
+    # Expired CRL supplied
     store = OpenSSL::X509::Store.new
-    store.purpose = OpenSSL::X509::PURPOSE_ANY
-    store.flags =
-      OpenSSL::X509::V_FLAG_CRL_CHECK|OpenSSL::X509::V_FLAG_CRL_CHECK_ALL
+    store.flags = OpenSSL::X509::V_FLAG_CRL_CHECK|OpenSSL::X509::V_FLAG_CRL_CHECK_ALL
     store.add_cert(ca1_cert)
     store.add_cert(ca2_cert)
-    store.add_crl(crl1)
-    store.add_crl(crl2_2) # issued by ca2 but expired.
-    assert_equal(true, store.verify(ca1_cert))
+    store.add_crl(ca1_crl1)
+    store.add_crl(ca2_crl2) # issued by ca2 but expired
     assert_equal(true, store.verify(ca2_cert))
     assert_equal(false, store.verify(ee1_cert))
     assert_equal(OpenSSL::X509::V_ERR_CRL_HAS_EXPIRED, store.error)
     assert_equal(false, store.verify(ee2_cert))
   end
 
-  def test_set_errors
+  def test_add_cert_duplicate
+    # Up until OpenSSL 1.1.0, X509_STORE_add_{cert,crl}() returned an error
+    # if the given certificate is already in the X509_STORE
     return if openssl?(1, 1, 0) || libressl?
-    now = Time.now
-    ca1_cert = issue_cert(@ca1, @rsa2048, 1, [], nil, nil)
+    ca1 = OpenSSL::X509::Name.parse_rfc2253("CN=Root CA")
+    ca1_key = Fixtures.pkey("rsa-1")
+    ca1_cert = issue_cert(ca1, ca1_key, 1, [], nil, nil)
     store = OpenSSL::X509::Store.new
     store.add_cert(ca1_cert)
     assert_raise(OpenSSL::X509::StoreError){
       store.add_cert(ca1_cert)  # add same certificate twice
     }
 
+    now = Time.now
     revoke_info = []
     crl1 = issue_crl(revoke_info, 1, now, now+1800, [],
-                     ca1_cert, @rsa2048, OpenSSL::Digest.new('SHA1'))
+                     ca1_cert, ca1_key, "sha256")
     revoke_info = [ [2, now, 1], ]
     crl2 = issue_crl(revoke_info, 2, now+1800, now+3600, [],
-                     ca1_cert, @rsa2048, OpenSSL::Digest.new('SHA1'))
+                     ca1_cert, ca1_key, "sha256")
     store.add_crl(crl1)
     assert_raise(OpenSSL::X509::StoreError){
       store.add_crl(crl2) # add CRL issued by same CA twice.
@@ -236,6 +359,14 @@ class OpenSSL::TestX509Store < OpenSSL::TestCase
     ctx = OpenSSL::X509::StoreContext.new(store)
     assert_raise(NoMethodError) { ctx.dup }
   end
+
+  def test_ctx_cleanup
+    # Deprecated in Ruby 1.9.3
+    cert  = OpenSSL::X509::Certificate.new
+    store = OpenSSL::X509::Store.new
+    ctx   = OpenSSL::X509::StoreContext.new(store, cert, [])
+    assert_warning(/cleanup/) { ctx.cleanup }
+  end
 end
 
 end