[fix] format otherName and nameConstraints exts (#324)

Extension#value dumped BC's toString() for an otherName entry and
flagged as ';'-separated, ssl.rb's split(/,\s+/) then merged the DNS
entry that followed - verify_certificate_identity then rejected a valid
certificate

nameConstraints had no formatter at all and fell back to an ASN.1 dump

both now match what C OpenSSL prints

Author: kares

src/main/java/org/jruby/ext/openssl/X509Extension.java

@@ -61,6 +61,8 @@
 import org.bouncycastle.asn1.x509.AuthorityInformationAccess;
 import org.bouncycastle.asn1.x509.GeneralName;
 import org.bouncycastle.asn1.x509.GeneralNames;
+import org.bouncycastle.asn1.x509.GeneralSubtree;
+import org.bouncycastle.asn1.x509.NameConstraints;
 import org.bouncycastle.util.encoders.Hex;
 
 import org.jruby.Ruby;
@@ -315,9 +317,6 @@ public IRubyObject set_oid(final ThreadContext context, IRubyObject arg) {
     private static final byte[] Object_Signing_CA = {'O', 'b', 'j', 'e', 'c', 't', ' ', 'S', 'i', 'g', 'n', 'i', 'n', 'g', ' ', 'C', 'A'};
     private static final byte[] Unused = {'U', 'n', 'u', 's', 'e', 'd'};
     private static final byte[] Unspecified = {'U', 'n', 's', 'p', 'e', 'c', 'i', 'f', 'i', 'e', 'd'};
-    //private static final byte[] Key_Compromise = { 'K','e','y',' ','C','o','m','p','r','o','m','i','s','e' };
-    //private static final byte[] CA_Compromise = { 'C','A',' ','C','o','m','p','r','o','m','i','s','e' };
-    //private static final byte[] Affiliation_Changed = { 'A','f','f','i','l','i','a','t','i','o','n',' ','C','h','a','n','g','e','d' };
     private static final byte[] keyid_ = {'k', 'e', 'y', 'i', 'd', ':'};
 
     @JRubyMethod
@@ -329,6 +328,18 @@ public RubyString value(final ThreadContext context) {
         final Ruby runtime = context.runtime;
         final String oid = getRealObjectID().getId();
         try {
+            if ( oid.equals("2.5.29.30") ) { // nameConstraints
+                ASN1Encodable value = getRealValue();
+                if ( value instanceof ASN1OctetString ) {
+                    value = ASN1.readObject( ((ASN1OctetString) value).getOctets() );
+                }
+                final NameConstraints nameConstraints = NameConstraints.getInstance(value);
+                final ByteList val = new ByteList(64);
+                appendGeneralSubtrees(val, "Permitted", nameConstraints.getPermittedSubtrees());
+                appendGeneralSubtrees(val, "Excluded", nameConstraints.getExcludedSubtrees());
+                return runtime.newString( val );
+            }
+
             if ( oid.equals("2.5.29.19") ) { // basicConstraints
                 ASN1Sequence seq2 = (ASN1Sequence) ASN1.readObject( getRealValueEncoded() );
                 final ByteList val = new ByteList(32);
@@ -709,6 +720,50 @@ private static String accessDescriptionMethodName(final Ruby runtime, final Acce
         return method.getId();
     }
 
+    static final String MS_UPN_OID = "1.3.6.1.4.1.311.20.2.3"; // otherName type used by AD
+
+    // C: "Permitted:\n  DNS:example.com\n  DNS:other.com" (and/or an "Excluded:" block)
+    private static void appendGeneralSubtrees(final ByteList out, final String label,
+        final GeneralSubtree[] subtrees) {
+        if ( subtrees == null || subtrees.length == 0 ) return;
+
+        if ( out.length() > 0 ) out.append('\n');
+        out.append( ByteList.plain(label) );
+        out.append(':');
+        for ( int i = 0; i < subtrees.length; i++ ) {
+            out.append('\n').append(' ').append(' ');
+            final GeneralName base = subtrees[i].getBase();
+            // a constrained iPAddress carries address *and* mask (8 or 32 octets)
+            if ( base.getTagNo() == GeneralName.iPAddress ) {
+                final byte[] ip = ((ASN1OctetString) base.getName()).getOctets();
+                if ( ip.length == 8 || ip.length == 32 ) {
+                    out.append('I').append('P').append(':');
+                    final int half = ip.length / 2;
+                    appendIPAddress(out, ip, 0, half);
+                    out.append('/');
+                    appendIPAddress(out, ip, half, half);
+                    continue;
+                }
+            }
+            formatGeneralName(base, out, false);
+        }
+    }
+
+    private static void appendIPAddress(final ByteList out, final byte[] ip, final int off, final int len) {
+        if ( len == 4 ) {
+            for ( int i = 0; i < len; i++ ) {
+                out.append( ConvertBytes.intToCharBytes( ((int) ip[off + i]) & 0xff ) );
+                if ( i != len - 1 ) out.append('.');
+            }
+        }
+        else {
+            for ( int i = 0; i < len; i += 2 ) {
+                out.append( ConvertBytes.intToHexBytes( ((ip[off+i] & 0xff) << 8 | (ip[off+i+1] & 0xff)) ) );
+                if ( i != len - 2 ) out.append(':');
+            }
+        }
+    }
+
     @SuppressWarnings("unchecked")
     private static boolean formatGeneralName(final GeneralName name, final ByteList out, final boolean slashed) {
         final ASN1Encodable obj = name.getName();
@@ -762,11 +817,24 @@ private static boolean formatGeneralName(final GeneralName name, final ByteList
             }
             break;
         case GeneralName.otherName:
-            out.append('o').append('t').append('h').append('e').append('r').append('N').append('a').append('m').append('e').
-                append(':');
-            out.append( ByteList.plain( obj.toString() ) );
-            return true;
-            //tagged = true;
+            // OtherName ::= SEQUENCE { type-id OBJECT IDENTIFIER, value [0] EXPLICIT ANY }
+            // C: "othername: UPN:<value>" for the MS UPN type, "othername: <oid>:<value>" otherwise
+            out.append( ByteList.plain("othername: ") );
+            final ASN1Sequence otherName = ASN1Sequence.getInstance(obj);
+            final String typeId = ASN1ObjectIdentifier.getInstance(otherName.getObjectAt(0)).getId();
+            out.append( ByteList.plain( MS_UPN_OID.equals(typeId) ? "UPN" : typeId ) );
+            out.append(':');
+            ASN1Encodable otherValue = otherName.getObjectAt(1);
+            if ( otherValue instanceof ASN1TaggedObject ) { // [0] EXPLICIT
+                otherValue = ASN1Shim.getTaggedObject((ASN1TaggedObject) otherValue);
+            }
+            if ( otherValue instanceof ASN1String ) {
+                out.append( ByteList.plain( ((ASN1String) otherValue).getString() ) );
+            }
+            else {
+                out.append( ByteList.plain( otherValue.toString() ) );
+            }
+            break; // NOTE: not a ';' separated entry (OpenSSL separates every name with ", ")
         case GeneralName.registeredID:
             out.append('R').append('I').append('D').
                 append(':');

test/x509/test_x509ext.rb

@@ -3,11 +3,7 @@
 
 class TestX509Extension < TestCase
 
-  if defined? JRUBY_VERSION
-    def setup; require 'jopenssl/load' end
-  else
-    def setup; require 'openssl' end
-  end
+  def setup; require 'openssl' end
 
   def test_new
     assert_raise(ArgumentError) { OpenSSL::X509::Extension.new }
@@ -246,6 +242,68 @@ def test_subject_alt_name_sequence
     }
   end
 
+  # -- SAN with otherName + nameConstraints value formatting -------------------
+  # built straight from DER so both engines decode byte-identical input
+
+  MS_UPN_OID = '1.3.6.1.4.1.311.20.2.3' # otherName type used by Active Directory
+
+  # [0] IMPLICIT OtherName ::= SEQUENCE { type-id OBJECT IDENTIFIER, value [0] EXPLICIT ANY }
+  def other_name(oid, value)
+    seq = OpenSSL::ASN1::Sequence.new([
+        OpenSSL::ASN1::ObjectId.new(oid),
+        OpenSSL::ASN1::ASN1Data.new([ OpenSSL::ASN1::UTF8String.new(value) ], 0, :CONTEXT_SPECIFIC)
+    ])
+    OpenSSL::ASN1::ASN1Data.new(seq.value, 0, :CONTEXT_SPECIFIC)
+  end
+
+  def dns_name(name); OpenSSL::ASN1::ASN1Data.new(name, 2, :CONTEXT_SPECIFIC) end
+
+  def san_extension(*entries)
+    OpenSSL::X509::Extension.new('subjectAltName', OpenSSL::ASN1::Sequence.new(entries).to_der, false)
+  end
+
+  def test_subject_alt_name_value_with_other_name_upn
+    ext = san_of(cert_with_san(other_name(MS_UPN_OID, 'HOST1$@example.com'),
+                               dns_name('host1.example.com'), dns_name('example.com')))
+    assert_equal 'othername: UPN:HOST1$@example.com, DNS:host1.example.com, DNS:example.com', ext.value
+  end
+
+  def test_subject_alt_name_value_with_generic_other_name
+    ext = san_of(cert_with_san(other_name('1.2.3.4', 'xx'), dns_name('a.example.com')))
+    assert_equal 'othername: 1.2.3.4:xx, DNS:a.example.com', ext.value
+  end
+
+  def san_of(cert); cert.extensions.find { |e| e.oid == 'subjectAltName' } end
+
+  # GH-324: the otherName dump used to leak ", " (and a ';' separator) into the value,
+  # which made ssl.rb's split(/,\s+/) swallow the DNS entry that followed
+  def test_verify_certificate_identity_with_other_name
+    cert = cert_with_san(other_name(MS_UPN_OID, 'HOST1$@example.com'),
+                         dns_name('host1.example.com'), dns_name('example.com'))
+    assert_equal true, OpenSSL::SSL.verify_certificate_identity(cert, 'host1.example.com')
+    assert_equal true, OpenSSL::SSL.verify_certificate_identity(cert, 'example.com')
+    assert_equal false, OpenSSL::SSL.verify_certificate_identity(cert, 'other.example.com')
+  end
+
+  def test_name_constraints_value
+    ext = OpenSSL::X509::ExtensionFactory.new.
+        create_extension('nameConstraints', 'permitted;DNS:example.com,permitted;DNS:bla.example.net', true)
+    assert_equal "Permitted:\n  DNS:example.com\n  DNS:bla.example.net", ext.value
+  end
+
+  # signed + re-parsed (as received over the wire);
+  # subject CN deliberately does not match so the identity check can only pass through the SAN
+  def cert_with_san(*entries)
+    key = OpenSSL::PKey::RSA.new(2048)
+    cert = OpenSSL::X509::Certificate.new
+    cert.subject = cert.issuer = OpenSSL::X509::Name.parse('/CN=not.the.name')
+    cert.not_before = Time.now - 60; cert.not_after = Time.now + 3600
+    cert.public_key = key.public_key; cert.serial = 1; cert.version = 2
+    cert.add_extension(san_extension(*entries))
+    cert.sign(key, OpenSSL::Digest.new('SHA256'))
+    OpenSSL::X509::Certificate.new(cert.to_der)
+  end
+
   def test_authority_key_identifier
     cn = [ %w[CN localhost] ]
     # key = OpenSSL::PKey::RSA.new TEST_KEY_RSA2048