[fix] X509 store issuer lookup with duplicate subject DNs

src/main/java/org/jruby/ext/openssl/x509store/CRL.java

@@ -36,10 +36,10 @@
  */
 public class CRL extends X509Object {
 
-    public final java.security.cert.CRL crl;
+    public final java.security.cert.X509CRL crl;
 
     public CRL(java.security.cert.CRL crl) {
-        this.crl = crl;
+        this.crl = (X509CRL) crl;
     }
 
     @Override
@@ -49,15 +49,13 @@ public int type() {
 
     @Override
     public boolean isName(final Name name) {
-        return name.equalTo( ((X509CRL) crl).getIssuerX500Principal() );
+        return name.equalTo( crl.getIssuerX500Principal() );
     }
 
     @Override
     public boolean matches(final X509Object other) {
         if (other instanceof CRL) {
-            final X509CRL thisCRL = (X509CRL) crl;
-            final X509CRL thatCRL = (X509CRL)((CRL) other).crl;
-            return thisCRL.getIssuerX500Principal().equals( thatCRL.getIssuerX500Principal() );
+            return this.hashCode() == other.hashCode() && this.crl.equals(((CRL) other).crl);
         }
         return false;
     }
@@ -69,4 +67,14 @@ public int compareTo(final X509Object other) {
         return crl.equals( ((CRL) other).crl ) ? 0 : -1;
     }
 
+    private transient int hash = -1;
+
+    @Override
+    public int hashCode() {
+        if (hash == -1) {
+            hash = crl.hashCode(); // X509CRL based on encoded bytes
+        }
+        return hash;
+    }
+
 }// X509_OBJECT_CRL

src/main/java/org/jruby/ext/openssl/x509store/Certificate.java

@@ -54,9 +54,8 @@ public boolean isName(final Name name) {
     public boolean matches(final X509Object other) {
         if (other instanceof Certificate) {
             final Certificate that = (Certificate) other;
-            if (X509AuxCertificate.equalSubjects(this.cert, that.cert)) {
-                return this.cert.hashCode() == that.cert.hashCode();
-            };
+            return this.cert.cert.hashCode() == that.cert.cert.hashCode()
+                && this.cert.cert.equals(that.cert.cert);
         }
         return false;
     }

src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java

@@ -30,7 +30,6 @@
 import java.io.IOException;
 import java.security.GeneralSecurityException;
 import java.security.PublicKey;
-import java.security.cert.CertificateException;
 import java.security.cert.CRLReason;
 import java.security.cert.X509CRL;
 import java.security.cert.X509CRLEntry;
@@ -111,8 +110,6 @@ interface CheckPolicyFunction extends Function1<StoreContext> {}
 
     Store.LookupCerts lookup_certs;
 
-    //private boolean isValid;
-
     private int num_untrusted; // last_untrusted (OpenSSL 1.0.2) in the chain
 
     private ArrayList<X509AuxCertificate> chain;
@@ -202,13 +199,13 @@ else if ( ok != X509Utils.X509_LU_FAIL ) {
         /* Look through all matching certificates for a suitable issuer */
         for ( int i = idx; i < objects.size(); i++ ) {
             final X509Object pobj = objects.get(i);
-            /* See if we've run past the matches */
             if (pobj.type() != X509_LU_X509) {
-                break; // return 0
+                continue;
             }
             final X509AuxCertificate x509 = ((Certificate) pobj).cert;
             if ( ! xn.equalTo( x509.getSubjectX500Principal() ) ) {
-                break; // return 0
+                // NOTE: unlike OpenSSL our store.objects aren't sorted by subject (insertion order)
+                continue; // same-DN certs may be separated by other entries - keep scanning
             }
             if ( checkIssued.call(this, x, x509) != 0 ) {
                 _issuer[0] = x509;
@@ -1556,7 +1553,7 @@ private int check_chain_extensions() throws Exception {
     }
 
     /* Return 1 is a certificate is self signed */
-    private boolean cert_self_signed(X509AuxCertificate x) throws CertificateException, IOException {
+    private boolean cert_self_signed(X509AuxCertificate x) throws IOException {
         // Purpose.checkPurpose(x, -1, 0);
         if ((x.getExFlags() & EXFLAG_SI) != 0) { // TODO EXFLAG_SS
             return true;

src/main/java/org/jruby/ext/openssl/x509store/X509AuxCertificate.java

@@ -159,12 +159,12 @@
                     akid = AuthorityKeyIdentifier.getInstance(seq);
                 }
 
                 if (akid.getKeyIdentifier() != null) {
                     if (getExtensionValue("2.5.29.14") != null) {
                         DEROctetString der = (DEROctetString) X509Utils.get(getExtensionValue("2.5.29.14"));
                         SubjectKeyIdentifier skid = SubjectKeyIdentifier.getInstance(X509Utils.get(der.getOctets()));
                         if (skid.getKeyIdentifier() != null) {
                             if (Arrays.equals(akid.getKeyIdentifier(), skid.getKeyIdentifier())) { /* SKID matches AKID */
                                 /* .. and the signature alg matches the PUBKEY alg: */
                                 if (getSigAlgName().equals(getPublicKey().getAlgorithm())) {
                                     flags |= X509Utils.EXFLAG_SS; /* indicate self-signed */
@@ -368,9 +368,4 @@
         }
     }
 
-    static boolean equalSubjects(final X509AuxCertificate cert1, final X509AuxCertificate cert2) {
-        if ( cert1.cert == cert2.cert ) return true;
-        return cert1.getSubjectX500Principal().equals( cert2.getSubjectX500Principal() );
-    }
-
 }// X509AuxCertificate

test/x509/test_x509store.rb

@@ -1183,3 +1183,118 @@ def test_verify_at_exact_not_after_is_expired
   end
 
 end
+
+# GH#370: X509Store must try every CA matching the issuer DN
+# (e.g. CA rotation where old and new chains share a subject)
+class TestX509StoreMultiCASameDN < TestCase
+
+  def setup
+    @old_root_cert, @old_root_key = make_root_ca('Test-Root', serial: 1)
+    @old_inter_cert, @old_inter_key = make_intermediate_ca('Test-Intermediate', @old_root_cert, @old_root_key, serial: 100)
+    @new_root_cert, @new_root_key = make_root_ca('Test-Root', serial: 2)
+    @new_inter_cert, @new_inter_key = make_intermediate_ca('Test-Intermediate', @new_root_cert, @new_root_key, serial: 200)
+    @server_cert, @server_key = make_leaf('localhost', @new_inter_cert, @new_inter_key, serial: 1000)
+  end
+
+  private
+
+  def make_root_ca(cn, serial:)
+    key = OpenSSL::PKey::RSA.new(2048)
+    name = OpenSSL::X509::Name.new([['CN', cn], ['O', 'TestOrg'], ['C', 'US']])
+
+    cert = OpenSSL::X509::Certificate.new
+    cert.version = 2
+    cert.serial = serial
+    cert.subject = name
+    cert.issuer = name
+    cert.public_key = key.public_key
+    cert.not_before = Time.now - 3600
+    cert.not_after = Time.now + 86400 * 365
+
+    ef = OpenSSL::X509::ExtensionFactory.new
+    ef.subject_certificate = cert
+    ef.issuer_certificate = cert
+    cert.add_extension(ef.create_extension('basicConstraints', 'CA:TRUE', true))
+    cert.add_extension(ef.create_extension('keyUsage', 'keyCertSign,cRLSign', true))
+    cert.add_extension(ef.create_extension('subjectKeyIdentifier', 'hash'))
+    cert.add_extension(ef.create_extension('authorityKeyIdentifier', 'keyid:always'))
+
+    cert.sign(key, OpenSSL::Digest.new('SHA256'))
+    [cert, key]
+  end
+
+  def make_intermediate_ca(cn, parent_cert, parent_key, serial:)
+    key = OpenSSL::PKey::RSA.new(2048)
+    name = OpenSSL::X509::Name.new([['CN', cn], ['O', 'TestOrg'], ['C', 'US']])
+
+    cert = OpenSSL::X509::Certificate.new
+    cert.version = 2
+    cert.serial = serial
+    cert.subject = name
+    cert.issuer = parent_cert.subject
+    cert.public_key = key.public_key
+    cert.not_before = Time.now - 3600
+    cert.not_after = Time.now + 86400 * 365
+
+    ef = OpenSSL::X509::ExtensionFactory.new
+    ef.subject_certificate = cert
+    ef.issuer_certificate = parent_cert
+    cert.add_extension(ef.create_extension('basicConstraints', 'CA:TRUE', true))
+    cert.add_extension(ef.create_extension('keyUsage', 'keyCertSign,cRLSign', true))
+    cert.add_extension(ef.create_extension('subjectKeyIdentifier', 'hash'))
+    cert.add_extension(ef.create_extension('authorityKeyIdentifier', 'keyid:always'))
+
+    cert.sign(parent_key, OpenSSL::Digest.new('SHA256'))
+    [cert, key]
+  end
+
+  def make_leaf(cn, parent_cert, parent_key, serial:)
+    key = OpenSSL::PKey::RSA.new(2048)
+    name = OpenSSL::X509::Name.new([['CN', cn], ['O', 'TestOrg'], ['C', 'US']])
+
+    cert = OpenSSL::X509::Certificate.new
+    cert.version = 2
+    cert.serial = serial
+    cert.subject = name
+    cert.issuer = parent_cert.subject
+    cert.public_key = key.public_key
+    cert.not_before = Time.now - 3600
+    cert.not_after = Time.now + 86400 * 365
+
+    ef = OpenSSL::X509::ExtensionFactory.new
+    ef.subject_certificate = cert
+    ef.issuer_certificate = parent_cert
+    cert.add_extension(ef.create_extension('basicConstraints', 'CA:FALSE'))
+    cert.add_extension(ef.create_extension('keyUsage', 'digitalSignature,keyEncipherment'))
+    cert.add_extension(ef.create_extension('extendedKeyUsage', 'serverAuth'))
+    cert.add_extension(ef.create_extension('subjectKeyIdentifier', 'hash'))
+    cert.add_extension(ef.create_extension('authorityKeyIdentifier', 'keyid:always'))
+    cert.add_extension(ef.create_extension('subjectAltName', "DNS:localhost,DNS:#{cn}"))
+
+    cert.sign(parent_key, OpenSSL::Digest.new('SHA256'))
+    [cert, key]
+  end
+
+  public
+
+  def test_verify_with_old_chain_first
+    store = OpenSSL::X509::Store.new
+    store.add_cert(@old_inter_cert)
+    store.add_cert(@old_root_cert)
+    store.add_cert(@new_inter_cert)
+    store.add_cert(@new_root_cert)
+
+    assert store.verify(@server_cert), "expected verify to pass, got: #{store.error_string} (error #{store.error})"
+  end
+
+  def test_verify_with_new_chain_first
+    store = OpenSSL::X509::Store.new
+    store.add_cert(@new_inter_cert)
+    store.add_cert(@new_root_cert)
+    store.add_cert(@old_inter_cert)
+    store.add_cert(@old_root_cert)
+
+    assert store.verify(@server_cert), "expected verify to pass, got: #{store.error_string} (error #{store.error})"
+  end
+
+end