ruby / openssl

Provides SSL, TLS and general purpose cryptography.
Other
241 stars 163 forks source link

ASN1#to_der in pure ruby #777

Open HoneyryderChuck opened 2 months ago

HoneyryderChuck commented 2 months ago

This is a draft request-for-comment proposal for implementing OpenSSL::ASN1::ASN1Data#to_der in ruby, as per suggestion on the previous MR. It already passes the test, although there is a part which hasn't been ported yet, and seems not be covered in the tests. I guess that the ruby code can benefit from being broke down in multiple files, but I'll keep it here just for easy of review, ,for now. I also didn't delete C code, not yet.

Currently, the whole of #to_der is implemented in the ASN1Data class. The reason is, one can build a primitive or constructive object "manually" (if tag and/or tag_class is known), or can just use arrays directly. It would benefit from moving specific implementations into the respective child classes, but then it'd be a breaking change because of what I said. Any suggestions?

I also didn't do any benchmarks yet. Is it relevant?

LMK what you think.

rhenium commented 2 months ago

I think the tests outside test_basic_asn1data are still using the C implementation because #to_der is redefined on Primitive and Constructive.

Currently, the whole of #to_der is implemented in the ASN1Data class. The reason is, one can build a primitive or constructive object "manually" (if tag and/or tag_class is known), or can just use arrays directly. It would benefit from moving specific implementations into the respective child classes, but then it'd be a breaking change because of what I said. Any suggestions?

ossl_asn1_get_asn1type() in the C impl can definitely be broken down and moved to each class. It's currently done this way just because it's simpler to do so in C.

What breaking change do you expect?

HoneyryderChuck commented 2 months ago

@rhenium made a few adjustments to the ruby code. I believe it got cleaner. The same point stands regarding ASN1Data class having to own to_der implementations for cons and prim though, not much one can do (?).

I broke down ossl_asn1_get_asn1type(), but not completely, as I didn't manage to port OID / UTCTime / GeneralizedTime impls to ruby. Not sure if worth pursuing.

Didn't start cutting C code just yet.

rhenium commented 2 months ago

The same point stands regarding ASN1Data class having to own to_der implementations for cons and prim though, not much one can do (?).

I think so, unfortunately.

OpenSSL::ASN1.decode produces an instance of OpenSSL::ASN1::ASN1Data when the data doesn't have a known universal class tag number, so ASN1Data#to_der has to be able to handle both. It would have been simpler if .decode produced Primitive/Constructed instance, and ASN1Data#to_der didn't exist, but I feel it's too late to make such an API change.

OID

Encoding the value should be trivial. I added in an inline comment.

UTCTime / GeneralizedTime

Please check X.690 for exact details, but encoding these types should be relatively simple too, since they are just encoded into ISO 8601 (except UTCTime uses 2-digit years).

HoneyryderChuck commented 2 months ago

@rhenium did UTCTime and GeneralizedTime, lmk what you think.

HoneyryderChuck commented 1 month ago

@rhenium anything still pending?