Fix loading of Subject/Issuer-Alt-Name extensions. (#144)

These were being treated specially and incorrectly when being loaded from encoded values. A given extension may not occur more than once in certificate or CRL, and hence this code could never be correct. Fixed the erroneous test for this too.
jruby · Oct 12, 2017 · 617ca56 · 617ca56
1 parent b852b51
commit 617ca56
Showing 2 changed files with 6 additions and 27 deletions.
diff --git a/src/main/java/org/jruby/ext/openssl/X509Extension.java b/src/main/java/org/jruby/ext/openssl/X509Extension.java
@@ -141,22 +141,6 @@ static X509Extension[] newExtension(final ThreadContext context,
         final ASN1ObjectIdentifier objectId = ASN1.getObjectID(runtime, oid);
         final ASN1Encodable value = ASN1.readObject(extValue);
 
-        if ( oid.equals("2.5.29.17") || oid.equals("2.5.29.18") ) { // subjectAltName || issuerAltName
-            if ( value instanceof ASN1OctetString ) { // DEROctetString
-                final ASN1Encodable oct = ASN1.readObject( ((ASN1OctetString) value).getOctets() );
-                if ( oct instanceof ASN1Sequence ) {
-                    final ASN1Sequence seq = (ASN1Sequence) oct;
-                    final X509Extension[] ext = new X509Extension[ seq.size() ];
-                    for ( int i = 0; i < ext.length; i++ ) {
-                        ext[i] = newExtension(runtime, objectId, seq.getObjectAt(i), critical);
-                    }
-                    return ext;
-                }
-                // NOTE need to unwrap ((ASN1TaggedObject) oct).getObject() - likely not ?!?
-                return new X509Extension[] { newExtension(runtime, objectId, oct, critical) };
-            }
-        }
-
         return new X509Extension[] { newExtension(runtime, objectId, value, critical) };
     }
 

diff --git a/src/test/ruby/x509/test_x509cert.rb b/src/test/ruby/x509/test_x509cert.rb
@@ -82,25 +82,23 @@ def test_resolve_extensions
       [ "keyUsage", "keyCertSign, cRLSign", true ],
       [ "subjectKeyIdentifier", "hash", false ],
       [ "authorityKeyIdentifier", "keyid:always", false ],
-      [ "subjectAltName", "email:self@jruby.org", false ],
-      [ "subjectAltName", "DNS:jruby.org", false ],
+      [ "subjectAltName", "email:self@jruby.org, DNS:jruby.org", false ],
     ]
 
     now = Time.now
     ca_cert = issue_cert(ca, rsa2048, 1, now, now + 3600, ca_exts,
                          nil, nil, OpenSSL::Digest::SHA1.new)
 
-    assert_equal 6, ca_cert.extensions.size
+    assert_equal 5, ca_cert.extensions.size
 
     cert = OpenSSL::X509::Certificate.new ca_cert.to_der
-    assert_equal 6, cert.extensions.size
+    assert_equal 5, cert.extensions.size
 
     # Java 6/7 seems to maintain same order but Java 8 does definitely not :
     # TODO there must be something going on under - maybe not BC parsing ?!?
     if self.class.java6? || self.class.java7?
       assert_equal '97:39:9D:C3:FB:CD:BA:8F:54:0C:90:7B:46:3F:EA:D6:43:75:B1:CB', cert.extensions[2].value
-      assert_equal 'email:self@jruby.org', cert.extensions[4].value
-      assert_equal 'DNS:jruby.org', cert.extensions[5].value
+      assert_equal 'email:self@jruby.org, DNS:jruby.org', cert.extensions[4].value
     end
 
     exts = cert.extensions.dup
@@ -118,10 +116,7 @@ def test_resolve_extensions
     assert ! ext.critical?
 
     assert ext = exts.find { |e| e.oid == 'subjectAltName' }, "missing 'subjectAltName' among: #{exts.join(', ')}"
-    assert_equal 'email:self@jruby.org', ext.value
-    exts.delete(ext)
-    assert ext = exts.find { |e| e.oid == 'subjectAltName' }, "missing 'subjectAltName' among: #{exts.join(', ')}"
-    assert_equal 'DNS:jruby.org', ext.value
+    assert_equal 'email:self@jruby.org, DNS:jruby.org', ext.value
   end
 
   def test_extensions
@@ -367,4 +362,4 @@ def test_cert_loading_regression
 -----END RSA PRIVATE KEY-----
   _end_of_pem_
 
-end
+end