do not der encode non-critical flag in X509::Extension (fixes jruby#389)

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

@@ -270,7 +270,7 @@ public IRubyObject to_text(final ThreadContext context) {
                 Extension ext = (Extension) extensions.entry(i);
                 ASN1ObjectIdentifier oiden = ext.getRealOid();
                 text.append(IND12).append( ASN1.o2a(runtime, oiden) ).append(": ");
-                if ( ext.getRealCritical() ) text.append("critical");
+                if ( ext.isRealCritical() ) text.append("critical");
                 text.append("\n");
                 text.append(IND16).append( ext.value(context) ).append("\n");
             }
@@ -421,7 +421,7 @@ public IRubyObject sign(final ThreadContext context, final IRubyObject key, IRub
         try {
             for ( int i = 0; i < extensions.size(); i++ ) {
                 Extension ext = (Extension) extensions.entry(i);
-                generator.addExtension(ext.getRealOid(), ext.getRealCritical(), ext.getRealValueBytes());
+                generator.addExtension(ext.getRealOid(), ext.isRealCritical(), ext.getRealValueBytes());
             }
         }
         catch (IOException e) { throw newCRLError(runtime, e); }

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

@@ -427,7 +427,7 @@ public IRubyObject sign(final ThreadContext context, final IRubyObject key, fina
         for ( X509Extensions.Extension ext : extensions ) {
             try {
                 final byte[] bytes = ext.getRealValueBytes();
-                generator.addExtension(ext.getRealOid(), ext.getRealCritical(), bytes);
+                generator.addExtension(ext.getRealOid(), ext.isRealCritical(), bytes);
             }
             catch (IOException ioe) {
                 throw runtime.newIOErrorFromException(ioe);

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

@@ -73,7 +73,6 @@
 import static org.jruby.ext.openssl.OpenSSLReal.debug;
 import static org.jruby.ext.openssl.OpenSSLReal.debugStackTrace;
 import static org.jruby.ext.openssl.OpenSSLReal.isDebug;
-import static org.jruby.ext.openssl.OpenSSLReal.warn;
 
 /**
  * @author <a href="mailto:ola.bini@ki.se">Ola Bini</a>
@@ -253,7 +252,7 @@ else if ( id.equals("2.5.29.37") ) { //extendedKeyUsage
 
             ext.setRealOid(objectId);
             ext.setRealValue(value);
-            ext.setRealCritical(critical.isTrue());
+            ext.setRealCritical(critical.isNil() ? null : critical.isTrue());
 
             return ext;
         }
@@ -513,28 +512,24 @@ public Extension(Ruby runtime, RubyClass type) {
 
         private ASN1ObjectIdentifier oid;
         private Object value;
-        private boolean critical;
-
-        void setRealOid(ASN1ObjectIdentifier oid) {
-            this.oid = oid;
-        }
-
-        void setRealValue(Object value) {
-            this.value = value;
-        }
-
-        void setRealCritical(boolean critical) {
-            this.critical = critical;
-        }
+        private Boolean critical;
 
         ASN1ObjectIdentifier getRealOid() {
             return oid;
         }
 
+        void setRealOid(ASN1ObjectIdentifier oid) {
+            this.oid = oid;
+        }
+
         Object getRealValue() {
             return value;
         }
 
+        void setRealValue(Object value) {
+            this.value = value;
+        }
+
         byte[] getRealValueBytes() throws IOException {
             if ( value instanceof RubyString ) {
                 return ((RubyString) value).getBytes();
@@ -550,8 +545,20 @@ byte[] getRealValueBytes() throws IOException {
             }
         }
 
-        boolean getRealCritical() {
-            return critical;
+        boolean isRealCritical() {
+            return critical == null ? Boolean.FALSE : critical.booleanValue();
+        }
+
+        //Boolean getRealCritical() {
+        //    return critical;
+        //}
+
+        void setRealCritical(boolean critical) {
+            this.critical = Boolean.valueOf(critical);
+        }
+
+        private void setRealCritical(Boolean critical) {
+            this.critical = critical;
         }
 
         @JRubyMethod(name = "initialize", rest = true, visibility = Visibility.PRIVATE)
@@ -593,8 +600,11 @@ public IRubyObject oid(final ThreadContext context) {
 
         @JRubyMethod(name="oid=")
         public IRubyObject set_oid(final ThreadContext context, IRubyObject arg) {
-            warn(context, "WARNING: unimplemented method called: Extension#oid=");
-            return false ? arg : context.runtime.getNil();
+            if ( arg instanceof RubyString ) {
+                setRealOid( ASN1.getObjectIdentifier( context.runtime, arg.toString() ) );
+                return arg;
+            }
+            throw context.runtime.newTypeError(arg, context.runtime.getString());
         }
 
         private static final byte[] CA_ = { 'C','A',':' };
@@ -806,30 +816,31 @@ else if ( name.getTagNo() == GeneralName.iPAddress ) {
 
         @JRubyMethod(name="value=")
         public IRubyObject set_value(final ThreadContext context, IRubyObject arg) {
-            //warn(context, "WARNING: unimplemented method called: Extension#value=");
-            //return false ? arg : context.runtime.getNil();
-            setRealValue(arg); return arg;
+            if ( arg instanceof RubyString ) {
+                setRealValue(arg); return arg;
+            }
+            throw context.runtime.newTypeError(arg, context.runtime.getString());
         }
 
         @JRubyMethod(name="critical?")
         public IRubyObject critical_p() {
-            return getRuntime().newBoolean(critical);
+            return getRuntime().newBoolean( isRealCritical() );
         }
 
         @JRubyMethod(name="critical=")
         public IRubyObject set_critical(final ThreadContext context, IRubyObject arg) {
-            //warn(context, "WARNING: unimplemented method called: Extension#critical=");
-            //return false ? arg : context.runtime.getNil();
-            setRealCritical(arg.isTrue()); return arg;
+            setRealCritical( arg.isTrue() ); return arg;
         }
 
         @JRubyMethod
         public IRubyObject to_der() {
             final ASN1EncodableVector vec = new ASN1EncodableVector();
             try {
-                vec.add(getRealOid());
-                vec.add(getRealCritical() ? DERBoolean.TRUE : DERBoolean.FALSE);
-                vec.add(new DEROctetString(getRealValueBytes()));
+                vec.add( getRealOid() );
+                if ( critical != null ) { // NOTE: likely a hack Boolean.FALSE should also get skipped
+                    vec.add(critical.booleanValue() ? DERBoolean.TRUE : DERBoolean.FALSE);
+                }
+                vec.add( new DEROctetString(getRealValueBytes()) );
                 return RubyString.newString(getRuntime(), new DLSequence(vec).getEncoded(ASN1Encoding.DER));
             }
             catch (IOException e) {

ext/openssl/src/test/ruby/asn1/test_x509ext.rb

@@ -0,0 +1,70 @@
+# coding: US-ASCII
+require File.expand_path('../test_helper', File.dirname(__FILE__))
+require 'jopenssl/load'
+
+class TestX509Extension < Test::Unit::TestCase
+
+  def test_attrs
+    ext = OpenSSL::X509::Extension.new('1.1.1.1.1.1', 'foo')
+    assert_equal false, ext.critical?
+
+    ext.critical = nil
+    assert_equal false, ext.critical?
+
+    ext.critical = true
+    assert_equal true, ext.critical?
+
+    assert_equal 'foo', ext.value
+
+    ext.value = 'bar'
+    assert_equal 'bar', ext.value
+
+    assert_equal '1.1.1.1.1.1', ext.oid
+
+    # TODO ASN1 needs to fallback to ASN1Registry
+    #ext.oid = '1.2'
+    #assert_equal 'member-body', ext.oid
+  end
+
+  def test_to_der # reproducing #389
+    ext = OpenSSL::X509::Extension.new('1.1.1.1.1.1', 'foo')
+
+    mri_to_der = "0\f\x06\x05)\x01\x01\x01\x01\x04\x03foo"
+    assert_equal mri_to_der, ext.to_der
+    # MRI 1.8  "0\f\006\005)\001\001\001\001\004\003foo"
+    # MRI 2.x  "0\f\x06\x05)\x01\x01\x01\x01\x04\x03foo"
+
+    dec = OpenSSL::ASN1.decode(ext.to_der)
+
+    assert_instance_of OpenSSL::ASN1::Sequence, dec
+    assert_equal 2, ( value = dec.value ).size
+
+    assert_instance_of OpenSSL::ASN1::ObjectId, value[0]
+    # assert_equal 4, value[0].tag
+    assert_equal '1.1.1.1.1.1', value[0].value
+
+    assert_instance_of OpenSSL::ASN1::OctetString, value[1]
+    # assert_equal 6, value[1].tag
+    assert_equal 'foo', value[1].value
+  end
+
+  def test_to_der_is_the_same_for_non_critical
+    ext1 = OpenSSL::X509::Extension.new('1.1.1.1.1.1', 'foo')
+    ext2 = OpenSSL::X509::Extension.new('1.1.1.1.1.1', 'foo', false)
+
+    assert_equal ext1.to_der, ext2.to_der
+
+    ext1.critical = nil
+
+    assert_equal ext1.to_der, ext2.to_der
+
+    ext1.critical = false
+
+    assert_equal ext1.to_der, ext2.to_der
+
+    ext1.critical = true
+
+    assert_not_equal ext1.to_der, ext2.to_der
+  end
+
+end