Skip to content

Commit

Permalink
simplify AKI parsing to only return keyIdentifier field
Browse files Browse the repository at this point in the history
  • Loading branch information
btoews committed Jun 27, 2019
1 parent 92e4c96 commit d63b835
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 74 deletions.
70 changes: 10 additions & 60 deletions lib/openssl/x509.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,28 +65,16 @@ module Helpers
def find_extension(oid)
extensions.find { |e| e.oid == oid }
end

def x509_name_from_general_name(gn_asn1)
unless gn_asn1.tag_class == :CONTEXT_SPECIFIC
raise ASN1::ASN1Error "invalid GeneralName"
end

if gn_asn1.tag == 4
Name.new(gn_asn1.value.first.to_der)
else
# TODO: raise error instead of returning nil?
# this is some other kind of general name.
nil
end
end
end

module SubjectKeyIdentifier
include Helpers

# Described in RFC5280 Section 4.2.1.2
# Get the subject's key identifier from the subjectKeyIdentifier
# exteension, as described in RFC5280 Section 4.2.1.2.
#
# Returns a binary String key identifier or nil.
# Returns the binary String key identifier or nil or raises
# ASN1::ASN1Error.
def subject_key_identifier
ext = find_extension("subjectKeyIdentifier")
return nil if ext.nil?
Expand All @@ -103,10 +91,12 @@ def subject_key_identifier
module AuthorityKeyIdentifier
include Helpers

# Described in RFC5280 Section 4.2.1.1
# Get the issuing certificate's key identifier from the
# authorityKeyIdentifier extension, as described in RFC5280
# Section 4.2.1.1
#
# Returns a Hash with keys :key_identifier, :authority_cert_issuer,
# and :authority_cert_serial_number.
# Returns the binary String keyIdentifier or nil or raises
# ASN1::ASN1Error.
def authority_key_identifier

This comment has been minimized.

Copy link
@ioquatix

ioquatix Jun 27, 2019

Member

If the functionality is changed, do you want to call it just key_identifier?

This comment has been minimized.

Copy link
@btoews

btoews Jul 29, 2019

Author Contributor

If we wanted to be totally correct, we'd call this authority_key_identifier_key_identifier, since this is the keyIdentifier field from the authority key identifier extension. I feel like authority_key_identifier might be also more clear, since there's also the subject key identifier.

ext = find_extension("authorityKeyIdentifier")
return nil if ext.nil?
Expand All @@ -116,51 +106,11 @@ def authority_key_identifier
raise ASN1::ASN1Error "invalid extension"
end

fields = {}

key_id = aki_asn1.value.find do |v|
v.tag_class == :CONTEXT_SPECIFIC && v.tag == 0
end

issuer = aki_asn1.value.find do |v|
v.tag_class == :CONTEXT_SPECIFIC && v.tag == 1
end

serial = aki_asn1.value.find do |v|
v.tag_class == :CONTEXT_SPECIFIC && v.tag == 2
end

# must have neither or both of issuer and serial
if (!issuer.nil? && serial.nil?) || (issuer.nil? && !serial.nil?)
raise ASN1::ASN1Error "invalid extension"
end

fields[:key_identifier] = if !key_id.nil?
key_id.value
else
nil
end

fields[:authority_cert_issuer] = if !issuer.nil?
# TODO raise if there are more than one values? GeneralNames is a
# SEQUENCE.
x509_name_from_general_name(issuer.value.first)
else
nil
end

fields[:authority_cert_serial_number] = if !serial.nil?
# encode/decode as integer to get value parsed as BN
ASN1.decode(ASN1::ASN1Data.new(
serial.value,
ASN1::INTEGER,
:UNIVERSAL
).to_der).value
else
nil
end

fields
key_id.nil? ? nil : key_id.value
end
end
end
Expand Down
5 changes: 1 addition & 4 deletions test/test_x509cert.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,8 @@ def test_extension
["authorityKeyIdentifier","issuer:always,keyid:always",false],
]
ca_cert = issue_cert(@ca, @rsa2048, 1, ca_exts, nil, nil)
aki_fields = ca_cert.authority_key_identifier
keyid = get_subject_key_id(ca_cert.to_der, hex: false)
assert_equal keyid, aki_fields[:key_identifier]
assert_equal ca_cert.subject, aki_fields[:authority_cert_issuer]
assert_equal ca_cert.serial, aki_fields[:authority_cert_serial_number]
assert_equal keyid, ca_cert.authority_key_identifier
assert_equal keyid, ca_cert.subject_key_identifier
ca_cert.extensions.each_with_index{|ext, i|
assert_equal(ca_exts[i].first, ext.oid)
Expand Down
11 changes: 1 addition & 10 deletions test/test_x509crl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -132,16 +132,7 @@ def test_extension
assert_equal(false, exts[0].critical?)

expected_keyid = OpenSSL::TestUtils.get_subject_key_id(cert, hex: false)
actual_keyid = crl.authority_key_identifier[:key_identifier]
assert_equal expected_keyid, actual_keyid

expected_issuer = cert.issuer
actual_issuer = crl.authority_key_identifier[:authority_cert_issuer]
assert_equal expected_issuer, actual_issuer

expected_serial = cert.serial
actual_serial = crl.authority_key_identifier[:authority_cert_serial_number]
assert_equal expected_serial, actual_serial
assert_equal expected_keyid, crl.authority_key_identifier

assert_equal("authorityKeyIdentifier", exts[1].oid)
keyid = OpenSSL::TestUtils.get_subject_key_id(cert)
Expand Down

2 comments on commit d63b835

@ioquatix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this change simplifies things which is good.

That being said, there is no reason why we can't add that functionality using other methods in the future? Would this be your preference e.g. authority_cert_issuer?

@btoews
Copy link
Contributor Author

@btoews btoews commented on d63b835 Jul 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. It would be fine to add helpers for getting the other AKI fields in the future. I'm just not sure how commonly those are used.

Please sign in to comment.