Add OpenSSL::X509::Extension#value_der method

[btoews]

The #value method provides a weird stringification of the extension value that can't be parsed and isn't very useful. The new #value_der method provides the raw value, allowing users to decode the value and use it as needed.

For example, I'm wanting to use the Authority Key Identifier extension from a certificate. To do this currently, I do

ext = cert.extensions.find { |c| c.oid == "authorityKeyIdentifier" }
if ext.nil?
  raise "missing extension"
end

# OpenSSL's weird stringification of the extension value
ext.value
#=> "keyid:E7:02:23:80:00:4F:D8:D7:BC:94:0B:D9:3F:74:39:49:32:3C:8A:79\n"

ext_asn1 = OpenSSL::ASN1.decode(ext.to_der)
unless ext_asn1.tag == OpenSSL::ASN1::SEQUENCE && [2, 3].include?(ext_asn1.value.length)
  raise GitSigning::OCSPError, "invalid x.509 extension"
end

ext_value = ext_asn1.value.last
unless ext_value.tag == OpenSSL::ASN1::OCTET_STRING
  raise "bad extension value"
end

aki_asn1 = OpenSSL::ASN1.decode(ext_value.value)
unless aki_asn1.tag == OpenSSL::ASN1::SEQUENCE
  raise "bad aki value"
end

key_id_value = ak1_asn1.value.find { |v| v.tag_class == :CONTEXT_SPECIFIC &&  v.tag == 0 }

# The actual AKI digest value we care about
key_id_value.value
#=> "\xE7\x02#\x80\x00O\xD8\xD7\xBC\x94\v\xD9?t9I2<\x8Ay"

The change in this PR lets us simplify this to

ext = cert.extensions.find { |c| c.oid == "authorityKeyIdentifier" }
if ext.nil?
  raise "missing extension"
end

aki_asn1 = OpenSSL::ASN1.decode(ext.value_der)
unless aki_asn1.tag == OpenSSL::ASN1::SEQUENCE
  raise "bad aki value"
end

key_id_value = ak1_asn1.value.find { |v| v.tag_class == :CONTEXT_SPECIFIC &&  v.tag == 0 }

key_id_value.value

While this still isn't great, I think it's as good as we can get. Since each extension has a different ASN.1 profile, we can't do much to parse the actual extension value for the user unless we wanted to add a bunch of logic for several popular extensions.

[bdewater]

👍 I was seeing something similar with cedarcode/webauthn-ruby#127 - in this case, it straight up mangles the binary data.

authenticator_data.aaguid.bytes # expected
[248, 160, 17, 243, 140, 10, 77, 21, 128, 6, 23, 17, 31, 158, 220, 125]

extension.value.bytes # actual
[46, 46, 46, 46, 46, 46, 46, 10, 77, 46, 46, 46, 46, 46, 46, 46, 46, 125]
extension.to_h
{"oid"=>"1.3.6.1.4.1.45724.1.1.4", "value"=>".......\nM........}", "critical"=>false}

Fortunately my use case is a little simpler and I know the expected byte length, so I don't need the ASN1 parsing:

extension.to_der[-16..-1] == authenticator_data.aaguid

Edit: this closes #163

[ioquatix]

LGTM.

[ioquatix]

@mastahyeti Just wondering if value_der is the right name. Do you think maybe something like raw_value might be more logical?

[btoews]

I like value_der, just because it makes it more clear how the return value is encoded. I don't feel strongly though.

[ioquatix]

What does der mean?

[btoews]

Distinguished encoding rules. It's one of the binary encodings for ASN.1 data.

…

On Sat, Jun 15, 2019 at 1:17 AM Samuel Williams ***@***.***> wrote: What does der mean? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#234?email_source=notifications&email_token=AAIXLBMAA7A7EQDJC6CFUMTP2SJQVA5CNFSM4GJRSUUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXYSFVI#issuecomment-502342357>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIXLBLKH6BUMMQZ57TDEZTP2SJQVANCNFSM4GJRSUUA> .

[ioquatix]

I wonder if we should have a more complex "value" object, e.g.

class Value
  def to_der
    # ... same as .value_der
  end
  
  def to_s
    # same as current .value
  end
end

Are there any other interesting properties/formats of value we should expose?

[btoews]

DER encoded and parsed (OpenSSL::ASN1 objects) values are the only forms that can be used programmatically today. The #to_s value might be useful for displaying an extension to a human, but it isn't consistently useful. The main problem is that different extensions are used for conveying different information. One might be an list of strings while another might be a set of key value pairs. It would be hard for this library to know how to parse all of these and present them to the user in a meaningful way.

I think the biggest improvement we could make to how extensions are treated in this library would be to pick the most common extensions and expose their data in a useful way on the parent object. For example X509 certificates often have Subject Alternative Name (SAN), Extended Key Usage (EKU), Authority Key Identifier (AKI), and Subject Key Identifier (SKI) extensions. Instead of making the user parse these themselves, we could expose their values directly on the X509::Certificate class. This is the approach taken by Go's crypto/x509#Certificate. I've found this approach to work quite well in Go.

For less common extensions, I think providing individual Extension#value/value_der/value_parsed methods should be fine. Moving those methods to an Extension::Value class would be fine also, but I'm not sure it would be a big improvement.

[ioquatix]

That sounds really good to me, do you think you have time to implement it? I will review it and merge it.

[btoews]

I don't have time to implement it right now, but can add it to my TODO list.