From 32369c44dc79befa9a229d912aeba208fb8af82b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ronald=20Tschal=C3=A4r?= <ronald@innovation.ch>
Date: Fri, 4 Aug 2017 19:53:40 -0700
Subject: [PATCH] Fix subjectAltName parsing to always generate GeneralNames.

It now handles arbitrary lists of names and always produces a GeneralNames
as required by the definition. Additionally, leading and trailing whitespace
around separators and labels is properly removed.

This fixes #134.
---
 .../ext/openssl/X509ExtensionFactory.java     | 67 +++++++++----------
 src/test/ruby/x509/test_x509ext.rb            | 38 +++++++++--
 2 files changed, 62 insertions(+), 43 deletions(-)

diff --git a/src/main/java/org/jruby/ext/openssl/X509ExtensionFactory.java b/src/main/java/org/jruby/ext/openssl/X509ExtensionFactory.java
index cf691704..53920cca 100644
--- a/src/main/java/org/jruby/ext/openssl/X509ExtensionFactory.java
+++ b/src/main/java/org/jruby/ext/openssl/X509ExtensionFactory.java
@@ -490,69 +490,62 @@ private ASN1Encodable parseIssuerAltName(final ThreadContext context, final Stri
 
     private static final String DNS_ = "DNS:";
     private static final String DNS_Name_ = "DNS Name:";
+    private static final String IP_ = "IP:";
+    private static final String IP_Address_ = "IP Address:";
     private static final String URI_ = "URI:";
     private static final String RID_ = "RID:";
     private static final String email_ = "email:";
     private static final String dirName_ = "dirName:";
     private static final String otherName_ = "otherName:";
 
-    private static ASN1Encodable parseSubjectAltName(final String valuex) throws IOException {
+    private static GeneralNames parseSubjectAltName(final String valuex) throws IOException {
+        final String[] vals = valuex.split("(?<!\\\\),");   // allow one level of escaping of ','
+        final GeneralName[] names = new GeneralName[vals.length];
+        for ( int i = 0; i < vals.length; i++ ) {
+            names[i] = parseGeneralName(vals[i].replace("\\,", ",").trim());
+        }
+        return new GeneralNames(names);
+    }
+
+    private static GeneralName parseGeneralName(final String valuex) throws IOException {
         if ( valuex.startsWith(DNS_) ) {
-            final String[] vals = valuex.split(",");
-            final GeneralName[] names = new GeneralName[vals.length];
-            for ( int i = 0; i < vals.length; i++ ) {
-                final String dns = vals[i].substring(DNS_.length());
-                names[i] = new GeneralName(GeneralName.dNSName, dns);
-            }
-            return new GeneralNames(names);
+            final String dns = valuex.substring(DNS_.length()).trim();
+            return new GeneralName(GeneralName.dNSName, dns);
         }
         if ( valuex.startsWith(DNS_Name_) ) {
-            final String dns = valuex.substring(DNS_Name_.length());
+            final String dns = valuex.substring(DNS_Name_.length()).trim();
             return new GeneralName(GeneralName.dNSName, dns);
         }
         if ( valuex.startsWith(URI_) ) {
-            final String uri = valuex.substring(URI_.length());
+            final String uri = valuex.substring(URI_.length()).trim();
             return new GeneralName(GeneralName.uniformResourceIdentifier, uri);
         }
         if ( valuex.startsWith(RID_) ) {
-            final String rid = valuex.substring(RID_.length());
+            final String rid = valuex.substring(RID_.length()).trim();
             return new GeneralName(GeneralName.registeredID, rid);
         }
         if ( valuex.startsWith(email_) ) {
-            final String[] vals = valuex.split(",");
-            final GeneralName[] names = new GeneralName[vals.length];
-            for ( int i = 0; i < vals.length; i++ ) {
-                if (vals[i].startsWith(email_)) {
-                    String mail = vals[i].substring(email_.length());
-                    names[i] = new GeneralName(GeneralName.rfc822Name, mail);
-                }
-                else {
-                    ASN1Encodable name = parseSubjectAltName(vals[i]);
-                    names[i] = name instanceof GeneralNames ? ((GeneralNames) name).getNames()[0] : (GeneralName) name;
-                }
-            }
-            return new GeneralNames(names);
-        }
-        if ( valuex.startsWith("IP:") || valuex.startsWith("IP Address:") ) {
-            final int idx = valuex.charAt(2) == ':' ? 3 : 11;
-            String[] vals = valuex.substring(idx).split("\\.|::");
-            final byte[] ip = new byte[vals.length];
-            for ( int i = 0; i < vals.length; i++ ) {
-                ip[i] = (byte) (Integer.parseInt(vals[i]) & 0xff);
-            }
-            return new GeneralName(GeneralName.iPAddress, new DEROctetString(ip));
+            String mail = valuex.substring(email_.length()).trim();
+            return new GeneralName(GeneralName.rfc822Name, mail);
+        }
+        if ( valuex.startsWith(IP_) ) {
+            final String ip = valuex.substring(IP_.length()).trim();
+            return new GeneralName(GeneralName.iPAddress, ip);
+        }
+        if ( valuex.startsWith(IP_Address_) ) {
+            final String ip = valuex.substring(IP_Address_.length()).trim();
+            return new GeneralName(GeneralName.iPAddress, ip);
         }
         if ( valuex.startsWith("other") ) { // otherName || othername
-            final String other = valuex.substring(otherName_.length());
+            final String other = valuex.substring(otherName_.length()).trim();
             return new GeneralName(GeneralName.otherName, other);
         }
         if ( valuex.startsWith("dir") ) { // dirName || dirname
-            final String dir = valuex.substring(dirName_.length());
+            final String dir = valuex.substring(dirName_.length()).trim();
             return new GeneralName(GeneralName.directoryName, dir);
         }
 
-        throw new IOException("could not parse SubjectAltName: " + valuex);
-
+        throw new IOException("could not parse SubjectAltName part: " + valuex);
     }
 
     private DEROctetString parseSubjectKeyIdentifier(final ThreadContext context, final String oid, final String valuex) {
diff --git a/src/test/ruby/x509/test_x509ext.rb b/src/test/ruby/x509/test_x509ext.rb
index 8b1812af..6a5629fd 100644
--- a/src/test/ruby/x509/test_x509ext.rb
+++ b/src/test/ruby/x509/test_x509ext.rb
@@ -151,12 +151,38 @@ def test_subject_alt_name_sign_to_pem
   end
 
   def test_subject_alt_name_sequence
+    tests = [
+        {
+            :input => "email:foo@bar.com,DNS:a.b.com,email:baz@bar.com",
+            :output => "email:foo@bar.com, DNS:a.b.com, email:baz@bar.com",
+            :der => "0,\x06\x03U\x1D\x11\x04%0#\x81\vfoo@bar.com\x82\aa.b.com\x81\vbaz@bar.com",
+        },
+        {
+            :input => "DNS:a.b.com, email:foo@bar.com",
+            :der => "0\x1f\x06\x03U\x1d\x11\x04\x180\x16\x82\x07a.b.com\x81\x0bfoo@bar.com",
+        },
+        {
+            :input => "URI:https://a.b.com/, DNS:a.b.com",
+            :der => "0$\x06\x03U\x1d\x11\x04\x1d0\x1b\x86\x10https://a.b.com/\x82\x07a.b.com",
+        },
+        {
+            :input => "IP:1.2.3.4,IP: fe80::12:345:5678, email:foo@bar.com, dirName: CN=John Doe\\,O=Acme",
+            :output => "IP:1.2.3.4, IP:fe80:0:0:0:0:12:345:5678, email:foo@bar.com, DirName:CN=John Doe,O=Acme",
+            :der => "0T\x06\x03U\x1d\x11\x04M0K\x87\x04\x01\x02\x03\x04\x87\x10\xfe\x80\x00\x00\x00\x00\x00\x00\x00\x00\x00\x12\x03EVx\x81\x0bfoo@bar.com\xa4$0\"1\x110\x0f\x06\x03U\x04\x03\x0c\x08John Doe1\x0d0\x0b\x06\x03U\x04\x0a\x0c\x04Acme",
+        },
+        {
+            :input => "RID:1.3.6.1.3.100.200",
+            :der => "0\x12\x06\x03U\x1d\x11\x04\x0b0\x09\x88\x07+\x06\x01\x03d\x81H",
+        },
+    ]
+
     extensions = OpenSSL::X509::ExtensionFactory.new
-    ext = extensions.create_extension("subjectAltName", "email:foo@bar.com,DNS:a.b.com,email:baz@bar.com")
-    assert_equal 'subjectAltName', ext.oid
-    assert_equal 'email:foo@bar.com, DNS:a.b.com, email:baz@bar.com', ext.value
-    mri_der = "0,\x06\x03U\x1D\x11\x04%0#\x81\vfoo@bar.com\x82\aa.b.com\x81\vbaz@bar.com"
-    assert_equal mri_der, ext.to_der
+    tests.each { |test|
+        ext = extensions.create_extension("subjectAltName", test[:input])
+        assert_equal 'subjectAltName', ext.oid
+        assert_equal (test[:output] || test[:input]), ext.value
+        assert_equal test[:der], ext.to_der
+    }
   end
 
   def subject_alt_name(domains)
@@ -165,4 +191,4 @@ def subject_alt_name(domains)
   end
   private :subject_alt_name
 
-end
\ No newline at end of file
+end