Bug #4421

[ext/openssl] Fix RSA public key encoding

Added by Martin Bosslet about 3 years ago. Updated 4 months ago.

[ruby-core:35327]
Status:Closed
Priority:Normal
Assignee:Martin Bosslet
Category:ext
Target version:1.9.3
ruby -v:- Backport:

Description

=begin
When calling RSA#toder and RSA#topem on RSA public keys, they currently
get encoded using i2dRSAPublicKey and PEMwritebioRSAPublicKey. This encoding
was specified in PKCS#1 and is specific to RSA. It is also not the default
encoding used by OpenSSL itself, which rather uses the generic format generated
by i2dRSAPUBKEY and PEMwritebioRSAPUBKEY. This format is the same that is
used in a certificate's SubjectPublicKeyInfo, the advantage being that the format
is generic and can be used to represent public keys of all kinds, including RSA,
DSA and Elliptic Curve.

The attached patch will make use of the generic format for encoding RSA keys. The
change should not cause compatibility problems, since RSA#initialize uses several
fallback scenarios that cover public keys of both formats.

The fallbacks are also re-prioritized according to these changes.

Regards,
Martin
=end

fix_rsa_pub_encoding.diff Magnifier (1.65 KB) Martin Bosslet, 02/22/2011 08:18 AM

noname (500 Bytes) Aaron Patterson, 05/10/2011 09:29 AM

Associated revisions

Revision 31520
Added by emboss almost 3 years ago

Thu May 12 07:27:31 2011 Martin Bosslet Martin.Bosslet@googlemail.com

* ext/openssl/ossl_pkey_rsa.c: Use generic X.509 SubjectPublicKeyInfo
  format for encoding RSA public keys. 
   [Bug #4421]

Previous revision: 31507

M ChangeLog
M ext/openssl/osslpkeyrsa.c

Revision 31554
Added by emboss almost 3 years ago

Sat May 14 04:19:06 2011 Martin Bosslet Martin.Bosslet@googlemail.com

* NEWS: Describe altered behaviour for RSA and DSA public key
encoding. [Ruby 1.9 - Bug #4421, Bug #4422]
[ruby-core:35327,35328]

Previous revision: 31553

Revision 31560
Added by emboss almost 3 years ago

Sat May 14 10:32:36 2011 Martin Bosslet Martin.Bosslet@googlemail.com

* test/openssl/test_pkey_rsa.rb: Add tests that ensure new public key
encoding behavior and also ensure backward compatibility.
[Ruby 1.9 - Bug #4421, Bug #4422] [ruby-core:35327,35328]

previous revision: 31559

History

#1 Updated by Yui NARUSE about 3 years ago

  • Status changed from Open to Assigned
  • Assignee set to Hiroshi Nakamura

=begin

=end

#2 Updated by Yui NARUSE almost 3 years ago

=begin
Hi, Martin,

We don't have enough resource and knowledge about ext/openssl. So do you need a commit bit?

If you want, please express it; and after the approval of matz, you can commit to our repo.
(of course, before a commit you need to reach a consensus)

See also DeveloperHowTo and CommitterHowTo.
=end

#3 Updated by Martin Bosslet almost 3 years ago

=begin
Hi Yui,

Yes, I'd love to contribute and I would really appreciate it!
=end

#4 Updated by Martin Bosslet almost 3 years ago

Hi all,

now that I have SVN access - would it be fine if I assigned the issues that I reported and that are still open to myself?
What about those already assigned?

Regards,
Martin

#5 Updated by Yui NARUSE almost 3 years ago

Welcome to committers!
Yeah you can change the assignee and commit the patch.

#6 Updated by Martin Bosslet almost 3 years ago

  • Assignee changed from Hiroshi Nakamura to Martin Bosslet

Great :) Thanks for all the help so far!

#7 Updated by Aaron Patterson almost 3 years ago

  • ruby -v changed from ruby 1.9.2p180 (2011-02-18 revision 30909) [i686-linux] to -

On Tue, May 10, 2011 at 07:37:13AM +0900, Martin Bosslet wrote:

Issue #4421 has been updated by Martin Bosslet.

Hi all,

now that I have SVN access - would it be fine if I assigned the issues that I reported and that are still open to myself?

Yay! Congrats Martin!

--
Aaron Patterson
http://tenderlovemaking.com/

#8 Updated by Anonymous almost 3 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r31520.
Martin, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


Thu May 12 07:27:31 2011 Martin Bosslet Martin.Bosslet@googlemail.com

* ext/openssl/ossl_pkey_rsa.c: Use generic X.509 SubjectPublicKeyInfo
  format for encoding RSA public keys. 
   [Bug #4421]

Previous revision: 31507

M ChangeLog
M ext/openssl/osslpkeyrsa.c

#9 Updated by Hiroshi Nakamura almost 3 years ago

Martin: Congrats! Go ahead.

Some comments;
* Please add a test for each commit to express the intent of the change. It must help us in the future.
* Please add some description about PKey format change to NEWS file. I think this change is OK (our older ruby should be able to read new format) but DSA might have some incompat behavior. DSA#p could be nil? Test it!

Regards,
// NaHi

#10 Updated by Martin Bosslet almost 3 years ago

First of all thanks for the kind words!

Some comments;
 * Please add a test for each commit to express the intent of the change. It must help us in the future.

The real benefit of applying this patch and
http://redmine.ruby-lang.org/issues/4422 is that
http://redmine.ruby-lang.org/issues/4424
becomes possible. You can create a PKey without knowing what kind of
key it actually is, much like what EVP_PKEY allows in OpenSSL itself.
I have included tests in 4424 that would test the changes in this
patch and 4422. If there is no objection, maybe I could commit 4424 to
add the tests and make the intent more clear? I was reluctant about
the naming in 4424, maybe you could help me out?

 * Please add some description about PKey format change to NEWS file. I think this change is OK (our older ruby should be able to read new format) but DSA might have some incompat behavior. DSA#p could be nil? Test it!

I'll add explicit tests for DSA and RSA that will use the old format
to "prove" backwards compatibility. Thanks for the hint with DSA. I'll
add the tests and fix it should any incompatibility show up. I also
added a few words to NEWS!

Regards,
Martin

#11 Updated by Yui NARUSE almost 3 years ago

Hi,

thank you for your contributions!

I have another comment: nahi and emboss's commit message has extra
header and indent like:

Mon May 16 05:13:20 2011 Martin Bosslet Martin.Bosslet@googlemail.com

 * ext/openssl/ossl_asn1.c: Add documentation.

Previous revision: 31583

It should be

#12 Updated by Hiroshi Nakamura almost 3 years ago

On Mon, May 16, 2011 at 11:47, NARUSE, Yui naruse@airemix.jp wrote:

I have another comment: nahi and emboss's commit message has extra
header and indent like:

Mon May 16 05:13:20 2011  Martin Bosslet  Martin.Bosslet@googlemail.com

   * ext/openssl/ossl_asn1.c: Add documentation.

Previous revision: 31583

It should be

  • ext/openssl/ossl_asn1.c: Add documentation.

OK. I copied ChangeLog and left it. I changed my style.

Thanks for your suggestion!

Regards,
// NaHi

#13 Updated by Nathaniel Talbott about 2 years ago

FYI, this does seem to break compatibility if you're using a fingerprint of the key for something, since (I just learned) key fingerprints are dependent on the format the key is stored in (since the fingerprint is based on the MD5 of the der-encoded key). Not sure there's much that can be done, but figured I'd leave this note here for anyone else that comes along later and encounters breakage due to this.

Our solution is to do a temporary hack to continue to spit out the old fingerprint, and migrate to a fingerprint based on the new format. Tricky, but doable.

#14 Updated by Martin Bosslet about 2 years ago

Nathaniel Talbott wrote:

FYI, this does seem to break compatibility if you're using a fingerprint of the key for something, since (I just learned) key fingerprints are dependent on the format the key is stored in (since the fingerprint is based on the MD5 of the der-encoded key). Not sure there's much that can be done, but figured I'd leave this note here for anyone else that comes along later and encounters breakage due to this.

Our solution is to do a temporary hack to continue to spit out the old fingerprint, and migrate to a fingerprint based on the new format. Tricky, but doable.

Yes, unfortunately this broke things like fingerprints - someone approached me about a similar issue not that long ago. What might help in the process of migration is the fact that it is not too hard to port the pre-1.9.3 format to the newer X.509 public key format [1]. Similarly, you could also do the "downgrade" from the 1.9.3 format to the PKCS#1 format used by pre-1.9.3, allowing you to keep the old fingerprints.

If this would help you in your migration process and you need the latter instead of the former, let me know, I could also provide a code sample for that case.

-Martin

[1] https://gist.github.com/1470287

#15 Updated by Wolfgang Woehl about 2 years ago

MartinBosslet (Martin Bosslet) wrote:

[...] and you need the latter instead of the former, let me know, I could also provide a code sample for that case.

Martin, I'd appreciate an example for the latter, yes, thanks in advance. In my code I need to reach public key digests which correspond to info embedded in X.509v3 certs' CNs (generated elsewhere, not with some ruby/openssl) and 1.9.3(-p125) breaks this.

Also I'm wondering: With 1.8.7-p352 and 1.9.2-p290 I can reproduce values computed by openssl 0.9.8k. With 1.9.3-p125 I cannot. So did openssl's default encoding change?

Here's what I'm doing:

$ openssl x509 -pubkey -noout -in x509v3.pem | openssl base64 -d | dd bs=1 skip=24 2> /dev/null | openssl sha1 -binary | openssl base64
NPq2kOXj9wUCE/Q/L+YSWm8Es9k=

$ irb

RUBY_VERSION
=> "1.9.2"

cert = OpenSSL::X509::Certificate.new( open 'x509v3.pem' )

=> #

puts cert.publickey.topem
-----BEGIN RSA PUBLIC KEY-----
MIIBCgKCAQEA7YZMQoS91QPXD1lMbyJQlK7jPidOMZ2hCsuq6UaJQsIyqDuu3RkJ
3Byl2xayvFmt7NSAwUEQvaCC0hoUPASB9GJJ9G/nAk4kPP1vbSmnyEjeWe+deb+m
6BB9/4GvRnacoHYw2MoOXScqrLVBJ2JoNvSXcPjqxZ266bb8b0mznuDubGACOH8L
luATgTdomeBmh80hl+Kpb4mfFKoyNGoIQPSybwoFDzTxgDo1YHD/rUgCF8Djim9W
c+/Rllz5q+Fhxsz9VgxlY0E2yV6vdUML+n4fqK9QmM9Z0e9X5TOz5Ntj6lZFCjmE
Hot18W+HNhncghiPkfEMDwyldP+/797ruwIDAQAB
-----END RSA PUBLIC KEY-----
=> nil

asn1 = Base64.decode64( cert.publickey.topem.split( "\n" )[ 1 .. -2 ].join )
=> "0\x82\x01\n\x02\x82\x01\x01\x00\xED\x86LB\x84\xBD\xD5\x03\xD7\x0FYLo"P\x94\xAE\xE3>'N1\x9D\xA1\n\xCB\xAA\xE9F\x89B\xC22\xA8;\xAE\xDD\x19\t\xDC\x1C\xA5\xDB\x16\xB2\xBCY\xAD\xEC\xD4\x80\xC1A\x10\xBD\xA0\x82\xD2\x1A\x14<\x04\x81\xF4bI\xF4o\xE7\x02N$<\xFDom)\xA7\xC8H\xDEY\xEF\x9Dy\xBF\xA6\xE8\x10}\xFF\x81\xAFFv\x9C\xA0v0\xD8\xCA\x0E]'*\xAC\xB5A'bh6\xF4\x97p\xF8\xEA\xC5\x9D\xBA\xE9\xB6\xFCoI\xB3\x9E\xE0\xEEl\x028\x7F\v\x96\xE0\x13\x817h\x99\xE0f\x87\xCD!\x97\xE2\xA9o\x89\x9F\x14\xAA24j\b@\xF4\xB2o\n\x05\x0F4\xF1\x80:5p\xFF\xADH\x02\x17\xC0\xE3\x8AoVs\xEF\xD1\x96\\xF9\xAB\xE1a\xC6\xCC\xFDV\fecA6\xC9\xAFuC\v\xFA~\x1F\xA8\xAFP\x98\xCFY\xD1\xEFW\xE53\xB3\xE4\xDBc\xEAVE\n9\x84\x1E\x8Bu\xF1o\x876\x19\xDC\x82\x18\x8F\x91\xF1\f\x0F\f\xA5t\xFF\xBF\xEF\xDE\xEB\xBB\x02\x03\x01\x00\x01"

dnq_calc = Base64.encode64( OpenSSL::Digest.new( 'sha1', asn1 ).digest ).chomp
=> "NPq2kOXj9wUCE/Q/L+YSWm8Es9k="
exit

$ rbenv global 1.9.3-p125
$ irb

RUBY_VERSION
=> "1.9.3"
cert = OpenSSL::X509::Certificate.new( open 'x509v3.pem' )

=> #

puts cert.publickey.topem
-----BEGIN PUBLIC KEY-----
MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA7YZMQoS91QPXD1lMbyJQ
lK7jPidOMZ2hCsuq6UaJQsIyqDuu3RkJ3Byl2xayvFmt7NSAwUEQvaCC0hoUPASB
9GJJ9G/nAk4kPP1vbSmnyEjeWe+deb+m6BB9/4GvRnacoHYw2MoOXScqrLVBJ2Jo
NvSXcPjqxZ266bb8b0mznuDubGACOH8LluATgTdomeBmh80hl+Kpb4mfFKoyNGoI
QPSybwoFDzTxgDo1YHD/rUgCF8Djim9Wc+/Rllz5q+Fhxsz9VgxlY0E2yV6vdUML
+n4fqK9QmM9Z0e9X5TOz5Ntj6lZFCjmEHot18W+HNhncghiPkfEMDwyldP+/797r
uwIDAQAB
-----END PUBLIC KEY-----
=> nil

asn1 = Base64.decode64( cert.publickey.topem.split( "\n" )[ 1 .. -2 ].join )
=> "0\x82\x01\"0\r\x06\t\x86H\x86\xF7\r\x01\x01\x01\x05\x00\x03\x82\x01\x0F\x000\x82\x01\n\x02\x82\x01\x01\x00\xED\x86LB\x84\xBD\xD5\x03\xD7\x0FYLo\"P\x94\xAE\xE3>'N1\x9D\xA1\n\xCB\xAA\xE9F\x89B\xC22\xA8;\xAE\xDD\x19\t\xDC\x1C\xA5\xDB\x16\xB2\xBCY\xAD\xEC\xD4\x80\xC1A\x10\xBD\xA0\x82\xD2\x1A\x14<\x04\x81\xF4bI\xF4o\xE7\x02N$<\xFDom)\xA7\xC8H\xDEY\xEF\x9Dy\xBF\xA6\xE8\x10}\xFF\x81\xAFFv\x9C\xA0v0\xD8\xCA\x0E]'\xAC\xB5A'bh6\xF4\x97p\xF8\xEA\xC5\x9D\xBA\xE9\xB6\xFCoI\xB3\x9E\xE0\xEEl\x028\x7F\v\x96\xE0\x13\x817h\x99\xE0f\x87\xCD!\x97\xE2\xA9o\x89\x9F\x14\xAA24j\b@\xF4\xB2o\n\x05\x0F4\xF1\x80:5p\xFF\xADH\x02\x17\xC0\xE3\x8AoVs\xEF\xD1\x96\\xF9\xAB\xE1a\xC6\xCC\xFDV\fecA6\xC9\xAFuC\v\xFA~\x1F\xA8\xAFP\x98\xCFY\xD1\xEFW\xE53\xB3\xE4\xDBc\xEAVE\n9\x84\x1E\x8Bu\xF1o\x876\x19\xDC\x82\x18\x8F\x91\xF1\f\x0F\f\xA5t\xFF\xBF\xEF\xDE\xEB\xBB\x02\x03\x01\x00\x01"

dnq_calc = Base64.encode64( OpenSSL::Digest.new( 'sha1', asn1 ).digest ).chomp
=> "7pxBugGtDPy/CAbe8IDHuj4LUY4="

#16 Updated by Martin Bosslet almost 2 years ago

Sorry Wolfgang, I just saw your request. Better late than never, here's the example for what you asked for: https://gist.github.com/2902696

To reproduce the same digests as you got in pre-1.9.3, you need to create the format as in the gist, then compute the hash on that value. The default format has indeed changed with 1.9.3 (note the "BEGIN RSA PUBLIC KEY" vs. "BEGIN PUBLIC KEY"). We now use the more generic X.509 encoding of public keys instead of the RSA-specific PKCS#1 encoding.

#17 Updated by David Welton 4 months ago

Hi,

It certainly would have been useful to have the code available in gist https://gist.github.com/2902696 available in some form, because this patch broke backwards compatibility for a system I was working on.

Also available in: Atom PDF