Merge pull request #774 from rhenium/ky/pkey-get-params-nil

rhenium · web-flow · commit 6a48f7c0cc23 · 2025-01-23T01:29:58.000+09:00
pkey: change PKey::{RSA,DSA,DH}#params to use nil for missing parameters
diff --git a/ext/openssl/ossl_pkey_dh.c b/ext/openssl/ossl_pkey_dh.c
@@ -284,35 +284,6 @@ ossl_dh_to_der(VALUE self)
     return str;
 }
 
-/*
- *  call-seq:
- *     dh.params -> hash
- *
- * Stores all parameters of key to the hash
- * INSECURE: PRIVATE INFORMATIONS CAN LEAK OUT!!!
- * Don't use :-)) (I's up to you)
- */
-static VALUE
-ossl_dh_get_params(VALUE self)
-{
-    OSSL_3_const DH *dh;
-    VALUE hash;
-    const BIGNUM *p, *q, *g, *pub_key, *priv_key;
-
-    GetDH(self, dh);
-    DH_get0_pqg(dh, &p, &q, &g);
-    DH_get0_key(dh, &pub_key, &priv_key);
-
-    hash = rb_hash_new();
-    rb_hash_aset(hash, rb_str_new2("p"), ossl_bn_new(p));
-    rb_hash_aset(hash, rb_str_new2("q"), ossl_bn_new(q));
-    rb_hash_aset(hash, rb_str_new2("g"), ossl_bn_new(g));
-    rb_hash_aset(hash, rb_str_new2("pub_key"), ossl_bn_new(pub_key));
-    rb_hash_aset(hash, rb_str_new2("priv_key"), ossl_bn_new(priv_key));
-
-    return hash;
-}
-
 /*
  *  call-seq:
  *     dh.params_ok? -> true | false
@@ -443,8 +414,6 @@ Init_ossl_dh(void)
     DEF_OSSL_PKEY_BN(cDH, dh, priv_key);
     rb_define_method(cDH, "set_pqg", ossl_dh_set_pqg, 3);
     rb_define_method(cDH, "set_key", ossl_dh_set_key, 2);
-
-    rb_define_method(cDH, "params", ossl_dh_get_params, 0);
 }
 
 #else /* defined NO_DH */
diff --git a/ext/openssl/ossl_pkey_dsa.c b/ext/openssl/ossl_pkey_dsa.c
@@ -303,35 +303,6 @@ ossl_dsa_to_der(VALUE self)
 }
 
 
-/*
- *  call-seq:
- *    dsa.params -> hash
- *
- * Stores all parameters of key to the hash
- * INSECURE: PRIVATE INFORMATIONS CAN LEAK OUT!!!
- * Don't use :-)) (I's up to you)
- */
-static VALUE
-ossl_dsa_get_params(VALUE self)
-{
-    OSSL_3_const DSA *dsa;
-    VALUE hash;
-    const BIGNUM *p, *q, *g, *pub_key, *priv_key;
-
-    GetDSA(self, dsa);
-    DSA_get0_pqg(dsa, &p, &q, &g);
-    DSA_get0_key(dsa, &pub_key, &priv_key);
-
-    hash = rb_hash_new();
-    rb_hash_aset(hash, rb_str_new2("p"), ossl_bn_new(p));
-    rb_hash_aset(hash, rb_str_new2("q"), ossl_bn_new(q));
-    rb_hash_aset(hash, rb_str_new2("g"), ossl_bn_new(g));
-    rb_hash_aset(hash, rb_str_new2("pub_key"), ossl_bn_new(pub_key));
-    rb_hash_aset(hash, rb_str_new2("priv_key"), ossl_bn_new(priv_key));
-
-    return hash;
-}
-
 /*
  * Document-method: OpenSSL::PKey::DSA#set_pqg
  * call-seq:
@@ -396,8 +367,6 @@ Init_ossl_dsa(void)
     DEF_OSSL_PKEY_BN(cDSA, dsa, priv_key);
     rb_define_method(cDSA, "set_pqg", ossl_dsa_set_pqg, 3);
     rb_define_method(cDSA, "set_key", ossl_dsa_set_key, 2);
-
-    rb_define_method(cDSA, "params", ossl_dsa_get_params, 0);
 }
 
 #else /* defined NO_DSA */
diff --git a/ext/openssl/ossl_pkey_rsa.c b/ext/openssl/ossl_pkey_rsa.c
@@ -494,42 +494,6 @@ ossl_rsa_verify_pss(int argc, VALUE *argv, VALUE self)
     ossl_raise(eRSAError, NULL);
 }
 
-/*
- * call-seq:
- *   rsa.params => hash
- *
- * THIS METHOD IS INSECURE, PRIVATE INFORMATION CAN LEAK OUT!!!
- *
- * Stores all parameters of key to the hash.  The hash has keys 'n', 'e', 'd',
- * 'p', 'q', 'dmp1', 'dmq1', 'iqmp'.
- *
- * Don't use :-)) (It's up to you)
- */
-static VALUE
-ossl_rsa_get_params(VALUE self)
-{
-    OSSL_3_const RSA *rsa;
-    VALUE hash;
-    const BIGNUM *n, *e, *d, *p, *q, *dmp1, *dmq1, *iqmp;
-
-    GetRSA(self, rsa);
-    RSA_get0_key(rsa, &n, &e, &d);
-    RSA_get0_factors(rsa, &p, &q);
-    RSA_get0_crt_params(rsa, &dmp1, &dmq1, &iqmp);
-
-    hash = rb_hash_new();
-    rb_hash_aset(hash, rb_str_new2("n"), ossl_bn_new(n));
-    rb_hash_aset(hash, rb_str_new2("e"), ossl_bn_new(e));
-    rb_hash_aset(hash, rb_str_new2("d"), ossl_bn_new(d));
-    rb_hash_aset(hash, rb_str_new2("p"), ossl_bn_new(p));
-    rb_hash_aset(hash, rb_str_new2("q"), ossl_bn_new(q));
-    rb_hash_aset(hash, rb_str_new2("dmp1"), ossl_bn_new(dmp1));
-    rb_hash_aset(hash, rb_str_new2("dmq1"), ossl_bn_new(dmq1));
-    rb_hash_aset(hash, rb_str_new2("iqmp"), ossl_bn_new(iqmp));
-
-    return hash;
-}
-
 /*
  * Document-method: OpenSSL::PKey::RSA#set_key
  * call-seq:
@@ -617,8 +581,6 @@ Init_ossl_rsa(void)
     rb_define_method(cRSA, "set_factors", ossl_rsa_set_factors, 2);
     rb_define_method(cRSA, "set_crt_params", ossl_rsa_set_crt_params, 3);
 
-    rb_define_method(cRSA, "params", ossl_rsa_get_params, 0);
-
 /*
  * TODO: Test it
     rb_define_method(cRSA, "blinding_on!", ossl_rsa_blinding_on, 0);
diff --git a/lib/openssl/pkey.rb b/lib/openssl/pkey.rb
@@ -34,6 +34,18 @@ def public_key
       DH.new(to_der)
     end
 
+    # :call-seq:
+    #    dh.params -> hash
+    #
+    # Stores all parameters of key to a Hash.
+    #
+    # The hash has keys 'p', 'q', 'g', 'pub_key', and 'priv_key'.
+    def params
+      %w{p q g pub_key priv_key}.map { |name|
+        [name, send(name)]
+      }.to_h
+    end
+
     # :call-seq:
     #    dh.compute_key(pub_bn) -> string
     #
@@ -154,6 +166,18 @@ def public_key
       OpenSSL::PKey.read(public_to_der)
     end
 
+    # :call-seq:
+    #    dsa.params -> hash
+    #
+    # Stores all parameters of key to a Hash.
+    #
+    # The hash has keys 'p', 'q', 'g', 'pub_key', and 'priv_key'.
+    def params
+      %w{p q g pub_key priv_key}.map { |name|
+        [name, send(name)]
+      }.to_h
+    end
+
     class << self
       # :call-seq:
       #    DSA.generate(size) -> dsa
@@ -328,6 +352,18 @@ def public_key
       OpenSSL::PKey.read(public_to_der)
     end
 
+    # :call-seq:
+    #    rsa.params -> hash
+    #
+    # Stores all parameters of key to a Hash.
+    #
+    # The hash has keys 'n', 'e', 'd', 'p', 'q', 'dmp1', 'dmq1', and 'iqmp'.
+    def params
+      %w{n e d p q dmp1 dmq1 iqmp}.map { |name|
+        [name, send(name)]
+      }.to_h
+    end
+
     class << self
       # :call-seq:
       #    RSA.generate(size, exponent = 65537) -> RSA
diff --git a/test/openssl/test_pkey_dh.rb b/test/openssl/test_pkey_dh.rb
@@ -130,6 +130,25 @@ def test_params_ok?
     assert_equal(false, dh2.params_ok?)
   end
 
+  def test_params
+    dh = Fixtures.pkey("dh2048_ffdhe2048")
+    assert_kind_of(OpenSSL::BN, dh.p)
+    assert_equal(dh.p, dh.params["p"])
+    assert_kind_of(OpenSSL::BN, dh.g)
+    assert_equal(dh.g, dh.params["g"])
+    assert_nil(dh.pub_key)
+    assert_nil(dh.params["pub_key"])
+    assert_nil(dh.priv_key)
+    assert_nil(dh.params["priv_key"])
+
+    dhkey = OpenSSL::PKey.generate_key(dh)
+    assert_equal(dh.params["p"], dhkey.params["p"])
+    assert_kind_of(OpenSSL::BN, dhkey.pub_key)
+    assert_equal(dhkey.pub_key, dhkey.params["pub_key"])
+    assert_kind_of(OpenSSL::BN, dhkey.priv_key)
+    assert_equal(dhkey.priv_key, dhkey.params["priv_key"])
+  end
+
   def test_dup
     # Parameters only
     dh1 = Fixtures.pkey("dh2048_ffdhe2048")
diff --git a/test/openssl/test_pkey_dsa.rb b/test/openssl/test_pkey_dsa.rb
@@ -230,6 +230,27 @@ def test_read_DSAPublicKey_pem
     assert_equal(nil, key.priv_key)
   end
 
+  def test_params
+    key = Fixtures.pkey("dsa2048")
+    assert_kind_of(OpenSSL::BN, key.p)
+    assert_equal(key.p, key.params["p"])
+    assert_kind_of(OpenSSL::BN, key.q)
+    assert_equal(key.q, key.params["q"])
+    assert_kind_of(OpenSSL::BN, key.g)
+    assert_equal(key.g, key.params["g"])
+    assert_kind_of(OpenSSL::BN, key.pub_key)
+    assert_equal(key.pub_key, key.params["pub_key"])
+    assert_kind_of(OpenSSL::BN, key.priv_key)
+    assert_equal(key.priv_key, key.params["priv_key"])
+
+    pubkey = OpenSSL::PKey.read(key.public_to_der)
+    assert_equal(key.params["p"], pubkey.params["p"])
+    assert_equal(key.pub_key, pubkey.pub_key)
+    assert_equal(key.pub_key, pubkey.params["pub_key"])
+    assert_nil(pubkey.priv_key)
+    assert_nil(pubkey.params["priv_key"])
+  end
+
   def test_dup
     key = Fixtures.pkey("dsa1024")
     key2 = key.dup
diff --git a/test/openssl/test_pkey_rsa.rb b/test/openssl/test_pkey_rsa.rb
@@ -579,6 +579,26 @@ def test_private_encoding_encrypted
     assert_same_rsa rsa, OpenSSL::PKey.read(pem, "abcdef")
   end
 
+  def test_params
+    key = Fixtures.pkey("rsa2048")
+    assert_equal(2048, key.n.num_bits)
+    assert_equal(key.n, key.params["n"])
+    assert_equal(65537, key.e)
+    assert_equal(key.e, key.params["e"])
+    [:d, :p, :q, :dmp1, :dmq1, :iqmp].each do |name|
+      assert_kind_of(OpenSSL::BN, key.send(name))
+      assert_equal(key.send(name), key.params[name.to_s])
+    end
+
+    pubkey = OpenSSL::PKey.read(key.public_to_der)
+    assert_equal(key.n, pubkey.n)
+    assert_equal(key.e, pubkey.e)
+    [:d, :p, :q, :dmp1, :dmq1, :iqmp].each do |name|
+      assert_nil(pubkey.send(name))
+      assert_nil(pubkey.params[name.to_s])
+    end
+  end
+
   def test_dup
     key = Fixtures.pkey("rsa1024")
     key2 = key.dup
