ASN1#to_der in pure ruby #777
Conversation
|
I think the tests outside
What breaking change do you expect? |
HoneyryderChuck
force-pushed
the
asn1-in-ruby
branch
from
09c3fbf to
e92f837
Compare
|
@rhenium made a few adjustments to the ruby code. I believe it got cleaner. The same point stands regarding I broke down Didn't start cutting C code just yet. |
HoneyryderChuck
force-pushed
the
asn1-in-ruby
branch
from
e92f837 to
c4823d9
Compare
I think so, unfortunately.
Encoding the value should be trivial. I added in an inline comment.
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
force-pushed
the
asn1-in-ruby
branch
4 times, most recently
from
95e2d8d to
005d32b
Compare
HoneyryderChuck
force-pushed
the
asn1-in-ruby
branch
2 times, most recently
from
2f32854 to
1e35012
Compare
|
@rhenium did UTCTime and GeneralizedTime, lmk what you think. |
HoneyryderChuck
force-pushed
the
asn1-in-ruby
branch
3 times, most recently
from
13e200e to
5785230
Compare
lib/openssl/asn1.rb
Outdated
| xclass = take_asn1_tag_class(tag_class) | ||
|
|
||
| i = constructed ? V_ASN1_CONSTRUCTED : 0 | ||
| i |= (xclass & V_ASN1_PRIVATE) |
There was a problem hiding this comment.
& V_ASN1_PRIVATE doesn't change the value, as a tag class can only be one of the 4 classes.
There was a problem hiding this comment.
I ported this from here
HoneyryderChuck
force-pushed
the
asn1-in-ruby
branch
3 times, most recently
from
47e6cf5 to
cfcbe9e
Compare
|
@rhenium anything still pending? |
HoneyryderChuck
force-pushed
the
asn1-in-ruby
branch
2 times, most recently
from
faa1dc0 to
6f22cbd
Compare
|
@rhenium it's been a while since I worked on this, but got a buffer to do some benchmark / optimizations while doing similar work in # benchmarks/asn1.rb
require "benchmark/ips"
require "openssl"
OBJS = {
"integer" => OpenSSL::ASN1::Integer.new(128),
"negative integer" => OpenSSL::ASN1::Integer.new(-128),
"oid" => OpenSSL::ASN1::ObjectId.new("1.2.34.56789".b),
"nid" => OpenSSL::ASN1::ObjectId.new("sha256"),
"emptyseq" => OpenSSL::ASN1::Sequence.new([]),
"seq" => OpenSSL::ASN1::Sequence.new([OpenSSL::ASN1::EndOfContent.new]),
"emptyset" => OpenSSL::ASN1::Set.new([]),
"set" => OpenSSL::ASN1::Set.new([OpenSSL::ASN1::EndOfContent.new]),
"utctime" => OpenSSL::ASN1::UTCTime.new(Time.utc(2016, 9, 8, 23, 43, 39)),
"gentime" => OpenSSL::ASN1::GeneralizedTime.new(Time.utc(2016, 12, 8, 19, 34, 29))
}
RubyVM::YJIT.enable
def test_name(name)
"#{name} (#{OpenSSL::VERSION})"
end
Benchmark.ips do |x|
# x.config(warmup: 2, time: 5)
name = ENV.fetch("TEST", "integer")
obj = OBJS.fetch(name)
x.report(test_name(name)) { obj.to_der }
x.save! "asn1-results.json"
x.compare!
endThe measurements I got locally are below. The tl;dr:
|
| def to_der | ||
| if @value.is_a?(Array) | ||
| cons_to_der | ||
| elsif @indefinite_length |
There was a problem hiding this comment.
Shouldn't this check go to prim_to_der?
There was a problem hiding this comment.
this follows what the C version is doing: indefinite length can be used only with "collections", i.e. arrays (constructive will always be arrays, and raw asn1data can be initialized with arrays).
lib/openssl/asn1.rb
Outdated
|
|
||
| return "\x00".b if @value.empty? | ||
|
|
||
| @unused_bits.chr << super |
There was a problem hiding this comment.
Integer#chr produces a US-ASCII String if the value is lower than 128.
I think we should avoid using Integer#chr in OpenSSL::ASN1 at all since automatic encoding promotion with ascii-only strings can be very tricky, for example:
(1.chr << "a".b).encoding #=> #<Encoding:US-ASCII>
There was a problem hiding this comment.
I've kept .chr followed by .force_encoding. I don't know of a more efficient way to perform the op in ruby (using pack would involve allocating an array). If you have a better alternative, let me know.
lib/openssl/asn1.rb
Outdated
|
|
||
| class Null < Primitive | ||
| def der_value | ||
| "".b |
There was a problem hiding this comment.
Please keep the @value == nil check because our C implementation had it.
I think we should test it too.
lib/openssl/asn1.rb
Outdated
| class UTCTime < Primitive | ||
| FORMAT = "%y%m%d%H%M%SZ".freeze | ||
|
|
||
| def der_value | ||
| value = if @value.is_a?(Time) | ||
| @value | ||
| else | ||
| Time.at(Integer(@value)) | ||
| end | ||
|
|
||
| value.utc.strftime(FORMAT) | ||
| end | ||
| end | ||
|
|
||
| class GeneralizedTime < Primitive | ||
| FORMAT = "%Y%m%d%H%M%SZ".freeze | ||
| def der_value | ||
| value = if @value.is_a?(Time) | ||
| @value | ||
| else | ||
| Time.at(Integer(@value)) | ||
| end | ||
|
|
||
| value.utc.strftime(FORMAT) | ||
| end | ||
| end |
There was a problem hiding this comment.
Time can express more than UTCTime or GeneralizedTime can do, so a range check is needed.
Also, new constants should be marked as private unless we intend to expose it to users. But I feel it could simply be inlined in this case.
There was a problem hiding this comment.
Added the range check to UTCTime, but at least specwise, GeneralizedTime does not seem to have known limitations. openssl does seem to limit to numbers convertible to long, not sure whether this should be ported to ruby though. WDYT?
| if constructed && indefinite_length | ||
| str << "\x80" | ||
| else | ||
| str << put_length(length) |
There was a problem hiding this comment.
I think this can be refactored since this is the only location using put_length and integer_to_octets.
integer_to_octets currently considers the case the number is negative, but it never happens because the length here is of a String object.
There was a problem hiding this comment.
by refactored, you mean inlined?
integer_to_octets currently considers the case the number is negative
it's true, length is always positive 👍
|
Sorry for the slow response. I've added some additional inline comments. My motivation for this series is that we can stop relying on those OpenSSL C API for BER encoding/decoding, which are screaming that it's not intended to be used outside OpenSSL, are completely undocumented, often have inconsistent semantics, and have undergone a lot of unexpected breaking changes. We also want to avoid performing string manipulation in C in general, especially when handling untrusted input. I think our top priority should be to make the code more understandable and maintainable, while keeping backwards compatibility. Good performance is nice to have, but I feel it's too early to focus on that. Some of the current optimizations feel a bit too aggressive. Using As for the speed of the OID encoding (or of anything else), I wouldn't rule out adding a specialized helper method implemented in C if it's necessary. However, I'm not yet sure if the current performance is an actual issue. I think it's too early to tell it. For measuring performance, I think using a complex and realistic structure like an X.509 certificate might provide a more useful result. |
this allows (when value is an integer) the runtime and JIT to optimize the call, instead of relying on C-extension openssl
…ppends) the former is a data structure that the runtime can improve processing of, and avoids realloc'ing the string on each call (as well as packing one byte at a time)
which means that it'll be nil; this improves encoding of asn1 nulls and cons with no elements
it's US-ASCII for integers < 128
utctime should not take dates into account with a year below 1950 and above 2049
HoneyryderChuck
force-pushed
the
asn1-in-ruby
branch
from
6f22cbd to
a46b07b
Compare
Wholeheartedly agree. FWIW the main motivation for this work is the belif that ruby code is more maintainable/readable than C code, and the ASN1 benefits little of being in C.
I agree, I just want to make sure that any regression is within a certain margin. There are people using this in production, after all. I don't think that some of the optimizations done here make the code less readable though:
Agree with the edge cases, but just to clarify, the goal of this MR is not to touch parsing (that can come later).
Agreed. It'd be great to have some input from someone that knows about YJIT or ZJIT whether these operations are something that future versions will be able to optimize though. The less C, the more maintainable it is.
Anything in the repo that I could use? |
This is a draft request-for-comment proposal for implementing
OpenSSL::ASN1::ASN1Data#to_derin 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_deris 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.