Skip to content

Commit

Permalink
Fix loading of Subject/Issuer-Alt-Name extensions. (#144)
Browse files Browse the repository at this point in the history
These were being treated specially and incorrectly when being loaded
from encoded values. A given extension may not occur more than once in
certificate or CRL, and hence this code could never be correct.

Fixed the erroneous test for this too.
  • Loading branch information
roadrunner2 authored and kares committed Oct 12, 2017
1 parent b852b51 commit 617ca56
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 27 deletions.
16 changes: 0 additions & 16 deletions src/main/java/org/jruby/ext/openssl/X509Extension.java
Original file line number Diff line number Diff line change
Expand Up @@ -141,22 +141,6 @@ static X509Extension[] newExtension(final ThreadContext context,
final ASN1ObjectIdentifier objectId = ASN1.getObjectID(runtime, oid);
final ASN1Encodable value = ASN1.readObject(extValue);

if ( oid.equals("2.5.29.17") || oid.equals("2.5.29.18") ) { // subjectAltName || issuerAltName
if ( value instanceof ASN1OctetString ) { // DEROctetString
final ASN1Encodable oct = ASN1.readObject( ((ASN1OctetString) value).getOctets() );
if ( oct instanceof ASN1Sequence ) {
final ASN1Sequence seq = (ASN1Sequence) oct;
final X509Extension[] ext = new X509Extension[ seq.size() ];
for ( int i = 0; i < ext.length; i++ ) {
ext[i] = newExtension(runtime, objectId, seq.getObjectAt(i), critical);
}
return ext;
}
// NOTE need to unwrap ((ASN1TaggedObject) oct).getObject() - likely not ?!?
return new X509Extension[] { newExtension(runtime, objectId, oct, critical) };
}
}

return new X509Extension[] { newExtension(runtime, objectId, value, critical) };
}

Expand Down
17 changes: 6 additions & 11 deletions src/test/ruby/x509/test_x509cert.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,25 +82,23 @@ def test_resolve_extensions
[ "keyUsage", "keyCertSign, cRLSign", true ],
[ "subjectKeyIdentifier", "hash", false ],
[ "authorityKeyIdentifier", "keyid:always", false ],
[ "subjectAltName", "email:[email protected]", false ],
[ "subjectAltName", "DNS:jruby.org", false ],
[ "subjectAltName", "email:[email protected], DNS:jruby.org", false ],
]

now = Time.now
ca_cert = issue_cert(ca, rsa2048, 1, now, now + 3600, ca_exts,
nil, nil, OpenSSL::Digest::SHA1.new)

assert_equal 6, ca_cert.extensions.size
assert_equal 5, ca_cert.extensions.size

cert = OpenSSL::X509::Certificate.new ca_cert.to_der
assert_equal 6, cert.extensions.size
assert_equal 5, cert.extensions.size

# Java 6/7 seems to maintain same order but Java 8 does definitely not :
# TODO there must be something going on under - maybe not BC parsing ?!?
if self.class.java6? || self.class.java7?
assert_equal '97:39:9D:C3:FB:CD:BA:8F:54:0C:90:7B:46:3F:EA:D6:43:75:B1:CB', cert.extensions[2].value
assert_equal 'email:[email protected]', cert.extensions[4].value
assert_equal 'DNS:jruby.org', cert.extensions[5].value
assert_equal 'email:[email protected], DNS:jruby.org', cert.extensions[4].value
end

exts = cert.extensions.dup
Expand All @@ -118,10 +116,7 @@ def test_resolve_extensions
assert ! ext.critical?

assert ext = exts.find { |e| e.oid == 'subjectAltName' }, "missing 'subjectAltName' among: #{exts.join(', ')}"
assert_equal 'email:[email protected]', ext.value
exts.delete(ext)
assert ext = exts.find { |e| e.oid == 'subjectAltName' }, "missing 'subjectAltName' among: #{exts.join(', ')}"
assert_equal 'DNS:jruby.org', ext.value
assert_equal 'email:[email protected], DNS:jruby.org', ext.value
end

def test_extensions
Expand Down Expand Up @@ -367,4 +362,4 @@ def test_cert_loading_regression
-----END RSA PRIVATE KEY-----
_end_of_pem_

end
end

0 comments on commit 617ca56

Please sign in to comment.