Allow DSA private keys to be initialized from parameters.

philr · philr · commit ee332db56afd · 2016-03-08T22:34:43.000Z
- OpenSSL::PKey::DSA was missing a priv_key= method.
- setKeySpecComponent() only handled the creation of the DSAPrivateKey.
diff --git a/src/main/java/org/jruby/ext/openssl/PKeyDSA.java b/src/main/java/org/jruby/ext/openssl/PKeyDSA.java
@@ -42,6 +42,7 @@
 import java.security.interfaces.DSAKey;
 import java.security.interfaces.DSAPrivateKey;
 import java.security.interfaces.DSAPublicKey;
+import java.security.spec.DSAPrivateKeySpec;
 import java.security.spec.DSAPublicKeySpec;
 import java.security.spec.InvalidKeySpecException;
 
@@ -111,6 +112,7 @@ public PKeyDSA(Ruby runtime, RubyClass type, DSAPrivateKey privKey, DSAPublicKey
     // a public key to be constructed incrementally, as required by the
     // current implementation of Net::SSH.
     // (see net-ssh-1.1.2/lib/net/ssh/transport/ossl/buffer.rb #read_keyblob)
+    private transient volatile BigInteger dsa_x;
     private transient volatile BigInteger dsa_y;
     private transient volatile BigInteger dsa_p;
     private transient volatile BigInteger dsa_q;
@@ -368,85 +370,96 @@ public IRubyObject sysverify(IRubyObject arg, IRubyObject arg2) {
         return getRuntime().getNil();
     }
 
-    @JRubyMethod(name = "p")
-    public synchronized IRubyObject get_p() {
-        // FIXME: return only for public?
-        DSAKey key; BigInteger param;
-        if ((key = this.publicKey) != null || (key = this.privateKey) != null) {
-            if ((param = key.getParams().getP()) != null) {
-                return BN.newBN(getRuntime(), param);
-            }
+    private DSAKey getDsaKey() {
+      DSAKey result;
+      return (result = publicKey) != null ? result : privateKey;
+    }
+
+    private IRubyObject toBN(BigInteger value) {
+      return value == null ? getRuntime().getNil() : BN.newBN(getRuntime(), value);
+    }
+
+    private synchronized BigInteger getP() {
+        DSAKey key = getDsaKey();
+        if (key != null) {
+            return key.getParams().getP();
         }
-        else if (dsa_p != null) {
-            return BN.newBN(getRuntime(), dsa_p);
+        else {
+            return dsa_p;
         }
-        return getRuntime().getNil();
+    }
+
+    @JRubyMethod(name = "p")
+    public IRubyObject get_p() {
+        return toBN(getP());
     }
 
     @JRubyMethod(name = "p=")
     public synchronized IRubyObject set_p(IRubyObject p) {
         return setKeySpecComponent(SPEC_P, p);
     }
 
-    @JRubyMethod(name = "q")
-    public synchronized IRubyObject get_q() {
-        // FIXME: return only for public?
-        DSAKey key; BigInteger param;
-        if ((key = this.publicKey) != null || (key = this.privateKey) != null) {
-            if ((param = key.getParams().getQ()) != null) {
-                return BN.newBN(getRuntime(), param);
-            }
+    private synchronized BigInteger getQ() {
+        DSAKey key = getDsaKey();
+        if (key != null) {
+            return key.getParams().getQ();
         }
-        else if (dsa_q != null) {
-            return BN.newBN(getRuntime(), dsa_q);
+        else {
+            return dsa_q;
         }
-        return getRuntime().getNil();
+    }
+
+    @JRubyMethod(name = "q")
+    public IRubyObject get_q() {
+        return toBN(getQ());
     }
 
     @JRubyMethod(name = "q=")
     public synchronized IRubyObject set_q(IRubyObject q) {
         return setKeySpecComponent(SPEC_Q, q);
     }
 
-    @JRubyMethod(name = "g")
-    public synchronized IRubyObject get_g() {
-        // FIXME: return only for public?
-        DSAKey key; BigInteger param;
-        if ((key = this.publicKey) != null || (key = this.privateKey) != null) {
-            if ((param = key.getParams().getG()) != null) {
-                return BN.newBN(getRuntime(), param);
-            }
+    private synchronized BigInteger getG() {
+        DSAKey key = getDsaKey();
+        if (key != null) {
+            return key.getParams().getG();
         }
-        else if (dsa_g != null) {
-            return BN.newBN(getRuntime(), dsa_g);
+        else {
+            return dsa_g;
         }
-        return getRuntime().getNil();
+    }
+
+    @JRubyMethod(name = "g")
+    public IRubyObject get_g() {
+        return toBN(getG());
     }
 
     @JRubyMethod(name = "g=")
     public synchronized IRubyObject set_g(IRubyObject g) {
         return setKeySpecComponent(SPEC_G, g);
     }
 
-    @JRubyMethod(name = "pub_key")
-    public synchronized IRubyObject get_pub_key() {
-        DSAPublicKey key;
-        if ( ( key = this.publicKey ) != null ) {
-            return BN.newBN(getRuntime(), key.getY());
-        }
-        else if (dsa_y != null) {
-            return BN.newBN(getRuntime(), dsa_y);
-        }
-        return getRuntime().getNil();
-    }
-
     @JRubyMethod(name = "priv_key")
     public synchronized IRubyObject get_priv_key() {
         DSAPrivateKey key;
         if ((key = this.privateKey) != null) {
-            return BN.newBN(getRuntime(), key.getX());
+            return toBN(key.getX());
         }
-        return getRuntime().getNil();
+        return toBN(dsa_x);
+    }
+
+    @JRubyMethod(name = "priv_key=")
+    public synchronized IRubyObject set_priv_key(IRubyObject priv_key) {
+        return setKeySpecComponent(SPEC_X, priv_key);
+    }
+
+    @JRubyMethod(name = "pub_key")
+    public synchronized IRubyObject get_pub_key() {
+        DSAPublicKey key;
+        if ( ( key = this.publicKey ) != null ) {
+            return toBN(key.getY());
+        }
+        return toBN(dsa_y);
     }
 
     @JRubyMethod(name = "pub_key=")
@@ -458,15 +471,38 @@ private IRubyObject setKeySpecComponent(final int index, final IRubyObject value
         final BigInteger val = BN.getBigInteger(value);
 
         switch (index) {
+            case SPEC_X: this.dsa_x = val; break;
             case SPEC_Y: this.dsa_y = val; break;
             case SPEC_P: this.dsa_p = val; break;
             case SPEC_Q: this.dsa_q = val; break;
             case SPEC_G: this.dsa_g = val; break;
         }
 
-        if ( dsa_y != null && dsa_p != null && dsa_q != null && dsa_g != null ) {
-            // we now have all components. create the key :
-            DSAPublicKeySpec spec = new DSAPublicKeySpec(dsa_y, dsa_p, dsa_q, dsa_g);
+        // Don't access the dsa_p, dsa_q and dsa_g fields directly. They may
+        // have already been consumed and cleared.
+        BigInteger _dsa_p = getP();
+        BigInteger _dsa_q = getQ();
+        BigInteger _dsa_g = getG();
+
+        if ( dsa_x != null && _dsa_p != null && _dsa_q != null && _dsa_g != null ) {
+            // we now have all private key components. create the key :
+            DSAPrivateKeySpec spec = new DSAPrivateKeySpec(dsa_x, _dsa_p, _dsa_q, _dsa_g);
+            try {
+                this.privateKey = (DSAPrivateKey) SecurityHelper.getKeyFactory("DSA").generatePrivate(spec);
+            }
+            catch (InvalidKeySpecException e) {
+                throw newDSAError(getRuntime(), "invalid keyspec", e);
+            }
+            catch (NoSuchAlgorithmException e) {
+                throw newDSAError(getRuntime(), "unsupported key algorithm (DSA)", e);
+            }
+            // clear out the specValues
+            this.dsa_x = this.dsa_p = this.dsa_q = this.dsa_g = null;
+        }
+
+        if ( dsa_y != null && _dsa_p != null && _dsa_q != null && _dsa_g != null ) {
+            // we now have all public key components. create the key :
+            DSAPublicKeySpec spec = new DSAPublicKeySpec(dsa_y, _dsa_p, _dsa_q, _dsa_g);
             try {
                 this.publicKey = (DSAPublicKey) SecurityHelper.getKeyFactory("DSA").generatePublic(spec);
             }
@@ -483,10 +519,11 @@ private IRubyObject setKeySpecComponent(final int index, final IRubyObject value
         return value;
     }
 
-    private static final int SPEC_Y = 0;
-    private static final int SPEC_P = 1;
-    private static final int SPEC_Q = 2;
-    private static final int SPEC_G = 3;
+    private static final int SPEC_X = 0;
+    private static final int SPEC_Y = 1;
+    private static final int SPEC_P = 2;
+    private static final int SPEC_Q = 3;
+    private static final int SPEC_G = 4;
 
     public static RaiseException newDSAError(Ruby runtime, String message) {
         return Utils.newError(runtime, _PKey(runtime).getClass("DSAError"), message);
diff --git a/src/test/ruby/dsa/private_key.pem b/src/test/ruby/dsa/private_key.pem
@@ -0,0 +1,12 @@
+-----BEGIN DSA PRIVATE KEY-----
+MIIBugIBAAKBgQDpdW60slBJrsrXrsputlqXFlT70CA0czpJZWbppiv4fed941TN
+/v/ICLjrNcsWXMbU5hb4faPrMZpAbUuIK+tMtJzz7sWMiINtso1FlQE/sUYBFqCv
+Tmkj52N0dPGsE7qmZ6ZknaJn6DbrAL569+5NIe9CR6cTtwL4IPWVXT3HQQIVAJqv
+o3Yrj85paNmGTIZfVt/oymAFAoGAb+S//7DQc6S/AK6r26BpQ/C4swSx1MTSl490
+hBJw0Czns5djqz9QB6ELufshGES1gcDGrYIncxQTGw1tPoJVrA+kefPVbRaYs2qM
+HasEfM1GfILfu4XDBB4xAoFryjKizOu8MwEXTsPLiTe9MdiT90NfcgSyIty1FgFP
+ZSz0JLMCgYBmAYli6D4DGB05NCVXBWPiu42c78gGCgibrbvCXpozB22TdMWA41ho
+7Oy7diBJLuJPUdmSsK++RE0bxlDl6QfmxTqfdb0ZUZ4u2bC9VeSM8ZtkbtxRpJGU
+p6znJdL83f05H2bhkKWI6a+vj894wRbtj+ube2UZPgKHFvkgdv732QIUKPp2Kkq+
+UQDoq2xu7v84G0sFIhc=
+-----END DSA PRIVATE KEY-----
diff --git a/src/test/ruby/dsa/test_dsa.rb b/src/test/ruby/dsa/test_dsa.rb
@@ -0,0 +1,64 @@
+# coding: US-ASCII
+require File.expand_path('../test_helper', File.dirname(__FILE__))
+
+class TestDsa < TestCase
+
+  def setup
+    super
+    self.class.disable_security_restrictions!
+    require 'base64'
+  end
+
+  def test_dsa_param_accessors
+    key_file = File.join(File.dirname(__FILE__), 'private_key.pem')
+    key = OpenSSL::PKey::DSA.new(File.read(key_file))
+
+    [:priv_key, :pub_key, :p, :q, :g].each do |param|
+      dsa = OpenSSL::PKey::DSA.new
+      assert_nil(dsa.send(param))
+      value = key.send(param)
+      dsa.send("#{param}=", value)
+      assert_equal(value, dsa.send(param), param)
+    end
+  end
+
+  def test_dsa_from_params_private_first
+    key_file = File.join(File.dirname(__FILE__), 'private_key.pem')
+    key = OpenSSL::PKey::DSA.new(File.read(key_file))
+
+    dsa = OpenSSL::PKey::DSA.new
+    dsa.priv_key, dsa.p, dsa.q, dsa.g = key.priv_key, key.p, key.q, key.g
+    assert(dsa.private?)
+    refute(dsa.public?)
+    [:priv_key, :p, :q, :g].each do |param|
+      assert_equal(key.send(param), dsa.send(param), param)
+    end
+
+    dsa.pub_key = key.pub_key
+    assert(dsa.private?)
+    assert(dsa.public?)
+    [:priv_key, :pub_key, :p, :q, :g].each do |param|
+      assert_equal(key.send(param), dsa.send(param), param)
+    end
+  end
+
+  def test_dsa_from_params_public_first
+    key_file = File.join(File.dirname(__FILE__), 'private_key.pem')
+    key = OpenSSL::PKey::DSA.new(File.read(key_file))
+
+    dsa = OpenSSL::PKey::DSA.new
+    dsa.pub_key, dsa.p, dsa.q, dsa.g = key.pub_key, key.p, key.q, key.g
+    refute(dsa.private?)
+    assert(dsa.public?)
+    [:pub_key, :p, :q, :g].each do |param|
+      assert_equal(key.send(param), dsa.send(param), param)
+    end
+
+    dsa.priv_key = key.priv_key
+    assert(dsa.private?)
+    assert(dsa.public?)
+    [:priv_key, :pub_key, :p, :q, :g].each do |param|
+      assert_equal(key.send(param), dsa.send(param), param)
+    end
+  end
+end
