diff --git a/src/main/java/org/jruby/ext/openssl/x509store/CRL.java b/src/main/java/org/jruby/ext/openssl/x509store/CRL.java index 7effd1b8..99b7cb26 100644 --- a/src/main/java/org/jruby/ext/openssl/x509store/CRL.java +++ b/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 diff --git a/src/main/java/org/jruby/ext/openssl/x509store/Certificate.java b/src/main/java/org/jruby/ext/openssl/x509store/Certificate.java index c06ac1f6..b132bef5 100644 --- a/src/main/java/org/jruby/ext/openssl/x509store/Certificate.java +++ b/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; } diff --git a/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java b/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java index f63c7687..a53658b7 100644 --- a/src/main/java/org/jruby/ext/openssl/x509store/StoreContext.java +++ b/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 {} Store.LookupCerts lookup_certs; - //private boolean isValid; - private int num_untrusted; // last_untrusted (OpenSSL 1.0.2) in the chain private ArrayList 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; diff --git a/src/main/java/org/jruby/ext/openssl/x509store/X509AuxCertificate.java b/src/main/java/org/jruby/ext/openssl/x509store/X509AuxCertificate.java index 27f99e12..1ed7e1aa 100644 --- a/src/main/java/org/jruby/ext/openssl/x509store/X509AuxCertificate.java +++ b/src/main/java/org/jruby/ext/openssl/x509store/X509AuxCertificate.java @@ -368,9 +368,4 @@ public Integer getNsCertType() throws CertificateException { } } - static boolean equalSubjects(final X509AuxCertificate cert1, final X509AuxCertificate cert2) { - if ( cert1.cert == cert2.cert ) return true; - return cert1.getSubjectX500Principal().equals( cert2.getSubjectX500Principal() ); - } - }// X509AuxCertificate diff --git a/test/x509/test_x509store.rb b/test/x509/test_x509store.rb index e12a8ac6..ad2c5f4f 100644 --- a/test/x509/test_x509store.rb +++ b/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