to_der on ASN1Data should convert ruby strings into java strings before encoding

[HoneyryderChuck]

OpenSSL::ASN1::ASN1Data encoding and decoding hasn't worked well for
some time:

• it was decoding the value always as a Sequence (when values can be strings as well);
• after decoding, the .tag_class was always :CONTEXT_SPECIFIC (it
  can be :UNIVERSAL as well);
• it wasn't correctly encoding :APPLICATION-tagged structures;

The tests added verify that the behaviour of these operations now
matchws what CRuby's openssl gem does.

Closes #264
Closes #119

[kares]

thanks, could you look into the CI failures ... there seems to be compilation errors

[HoneyryderChuck]

Hi @kares ,

Just reverted the main change. I think I will need some guidance here.

The issue is pretty clear, ((ASN1Data) val) breaks if val is a RubyString. However, this cast is being used in several other places of the code base. so I assume this cast does not break, p.ex. for Integers? My java's also a bit rusty, so I can't figure out how to enable ASN1Data to cast ruby strings, or any other ruby object for that matter. Do you know of a reasonable way to implement this?

[HoneyryderChuck]

@kares can you help figure out what's going on? The CI suite fails for 3 errors that are passing locally to me. I've been testing my patch against jruby 9.4.2.0 and java 18 (don't know if it's because of that).

[kares]

some (test) certificates expired causing test failures, updated them on master - you could try rebasing

[HoneyryderChuck]

@kares thx, managed to fix the remainder of the tests, I think it's ready for review.

[HoneyryderChuck]

@kares any updates?

[HoneyryderChuck]

@kares any feedback you want to give regarding this MR? I believe it addresses the significant part of ASN1 parsing issues, and is currently making rodauth-oauth mtls support unusable under jruby.

[skunkworker]

@HoneyryderChuck does this example have the correct output for your branch? #282

Edit: I tested the patches ASN1.java and it did not fix the problem. I'll dive into it now.

[kares]

sorry I didn't have time to look into this deeply, due limited free time, during the last JOSSL (release) updates.

I did take a look superficially and doing so the current state of changes seemed incomplete (potentially causing regressions)...

[HoneyryderChuck]

@skunkworker I've pushed a commit to address your issue.

I did take a look superficially and doing so the current state of changes seemed incomplete (potentially causing regressions)...

@kares do you mind expanding which issues do you foresee causing regressions? Not claiming there aren't any bugs, but the current decoding/encoding logic hasn't worked for years, so barring any crash concerns, should be a net positive?

[skunkworker]

@skunkworker I've pushed a commit to address your issue.

I did take a look superficially and doing so the current state of changes seemed incomplete (potentially causing regressions)...

@HoneyryderChuck Thanks for adding that.

I would also add another test around ::OpenSSL::ASN1::Set.

data_set = ::OpenSSL::ASN1::Set([::OpenSSL::ASN1::Integer(0)])
asn1 = ::OpenSSL::ASN1::Set(data_sequence)
assert_equal "1\x03\x02\x01\x00" , asn1.to_der

[kares]

I've included a commit from this PR (+ added back the test removal) in JOSSL 0.14.4

I've been looking into the PR several times (latest example) and tried to work on top of it but failed to get it into an acceptable state. The changes look good and I am thankful for them but some of the test related changes are actually regressions (when run on MRI some of the changed tests start failing).

Due very limited time on my part I am not sure how to proceed here, might take a final look although primarily in the context of an updated BC version as some parts related to tagging are quite opaque in BC 1.74.

Sorry, if the experience has been frustrating. I certainly appreciate the effort here but unfortunately can not merge the MR in its current form as it will likely cause regression reports...

[kares]

⚠️ a lot of the .value wrapping changes (such as in this test) seem to me like they are regressions as this has and still (using OpenSSL 3.0) passes on MRI.

[HoneyryderChuck]

Hey @kares , thx for the feedback. Understand you're very limited on time, but it's hard to keep momentum on my side as well. In the interest of making the most of your and my time, what do you think needs to be done? Shall I rebase, cherry pick the commit from the other MR, and make them pass? Are those enough to ensure compatibility?

[kares]

In the interest of making the most of your and my time, what do you think needs to be done?

as I already pointed out we should not merge code where existing tests are changed in ways that do not pass on MRI.
there might be an exception to this with edge cases, but in general if we are changing an existing ASN.1 test than it should be in a way that is more compatible with the C OpenSSL behavior not less.

Shall I rebase, cherry pick the commit from #296, and make them pass?

that would be the long term goal yes - to pass the test_asn1.rb the same way as MRI.
for starters I would suggest to look at the test changes made here in this PR - whether they were changed in way that is no longer passing on MRI:

The changes look good and I am thankful for them but some of the test related changes are actually regressions (when run on MRI some of the changed tests start failing).

---

as a side note code such as decodeObject(context, ASN1, taggedBaseObj).callMethod(context, "value") feels weird and might be an indication of the wrong direction or that there needs to be deeper refactoring...

[kares]

also maybe wait for the BC update, it might be easier to start looking at this again...

[HoneyryderChuck]

@kares I've rebased and resumed the work left off last time. I've followed your recommendation and deleted the test tweaks commit which was hiding the API breaking changes. I've now reached a standstill, and would like to get your input/assistance.

The main issue right now is, when decoding to an ASN1Data object, the java/BC API seems insufficient to determine whether its value should be wrapped in an array or not. I've left my last attempt there for you to see (force array, then fallback on class cast exception), but this does not work.

Essentially, an API is missing to branch that logic, which you can find for CRuby here. This relies on the return value of openssl ASN1_get_object, which I couldn't find a corresponding BC API for.

jruby-openssl does have internal logic to get the constructive tag from the bytestream, and this logic seems to have apparently been adapted from BC source code, however this only works for the top-most asn1 structure; once .readObject is called, then we're dealing with BC java objects all the way down, none of which seems to have an API to determine whether the taggedobject is "constructed".

Does this make sense? Is this something you've stumbled before on, which you can see a migration path out of?

[kares]

Thanks Tiago, appreciate the effort.

Been also busy and pushed some "low-hanging" fruits in terms of ASN.1 compatibility (change set: kares/jruby-openssl#4). Seems like I can handle the tag fixes from this PR, even though there are now conflicts, which I should be able to manage picking the relevant parts (unless you beat me to it of course 😉).

Essentially, an API is missing to branch that logic, which you can find for CRuby here. This relies on the return value of openssl ASN1_get_object, which I couldn't find a corresponding BC API for.

In terms of ASN.1 I haven't really checked the C sources so far, but the tagged object decoding part if hard to figure out so might eventually need to dive in there as well, at this point I have no idea - would have assumed decoding logic would be straightforward to figure with BC.

jruby-openssl does have internal logic to get the constructive tag from the bytestream, and this logic seems to have apparently been adapted from BC source code, however this only works for the top-most asn1 structure; once .readObject is called

that might be the silver lining - we should attempt to get rid of the custom logic, if possible...

BTW. the PR's concern is now only one ticket: #264 (#119 is resolved with the changes mentioned on master), for that bug I do not understand the context and whether there's library code using similar code. Because, as far I checked what OpenSSL::ASN1::ASN1Data.new("bla", 0, :APPLICATION).to_der produces is not standard DER content, pretty much nothing can read that except for C OpenSSL itself. If you recall where code like that has been coming from please update the bug report.

[kares]

looked into this, and there's a bunch of regressions as shown by the 🔴 tests on CI.

tried picking it upon master (there's conflicts) but that gets us even more failures (checked and those failing tests pass on MRI so the current state is correct):

E
========================================================================================================================================================================================================================================
Error: test_decode(TestASN1): NoMethodError: undefined method `size' for #<OpenSSL::BN:0x2ebcbf9d>
src/test/ruby/test_asn1.rb:977:in `test_decode'
     974:     version = tbs_cert.value[0]
     975:     assert_equal(:CONTEXT_SPECIFIC, version.tag_class)
     976:     assert_equal(0, version.tag)
  => 977:     assert_equal(1, version.value.size)
     978:     assert_equal(OpenSSL::ASN1::Integer, version.value[0].class)
     979:     assert_equal(2, version.value[0].value)
     980: 
org/jruby/RubyKernel.java:1423:in `catch'
org/jruby/RubyKernel.java:1418:in `catch'
org/jruby/RubyKernel.java:1423:in `catch'
org/jruby/RubyKernel.java:1418:in `catch'
========================================================================================================================================================================================================================================
E
========================================================================================================================================================================================================================================
Error: test_decode_application_specific(TestASN1): RuntimeError: object implicit - explicit expected.
org/jruby/ext/openssl/ASN1.java:1135:in `decode'
src/test/ruby/test_asn1.rb:1244:in `test_decode_application_specific'
     1241: 
     1242:   def test_decode_application_specific
     1243:     raw = "0\x18\x02\x01\x01`\x13\x02\x01\x03\x04\to=Telstra\x80\x03ess"
  => 1244:     asn1 = OpenSSL::ASN1.decode(raw)
     1245:     pp asn1 if false
     1246: 
     1247:     assert_equal OpenSSL::ASN1::Sequence, asn1.class
org/jruby/RubyKernel.java:1423:in `catch'
org/jruby/RubyKernel.java:1418:in `catch'
org/jruby/RubyKernel.java:1423:in `catch'
org/jruby/RubyKernel.java:1418:in `catch'
========================================================================================================================================================================================================================================
E
========================================================================================================================================================================================================================================
Error: test_decode_x509_certificate(TestASN1): NoMethodError: undefined method `size' for #<OpenSSL::BN:0x70b630d>
src/test/ruby/test_asn1.rb:31:in `test_decode_x509_certificate'
     28:     version = tbs_cert.value[0]
     29:     assert_equal(:CONTEXT_SPECIFIC, version.tag_class)
     30:     assert_equal(0, version.tag)
  => 31:     assert_equal(1, version.value.size)
     32:     assert_equal(OpenSSL::ASN1::Integer, version.value[0].class)
     33:     assert_equal(2, version.value[0].value)
     34: 
org/jruby/RubyKernel.java:1423:in `catch'
org/jruby/RubyKernel.java:1418:in `catch'
org/jruby/RubyKernel.java:1423:in `catch'
org/jruby/RubyKernel.java:1418:in `catch'
========================================================================================================================================================================================================================================
F
========================================================================================================================================================================================================================================
Failure: test_encode_data_integer(TestASN1)
src/test/ruby/test_asn1.rb:278:in `test_encode_data_integer'
     275:     assert_equal :CONTEXT_SPECIFIC, dec.tag_class
     276:     assert_equal 1, dec.tag
     277: 
  => 278:     assert_equal Array, dec.value.class
     279:     assert_equal 1, dec.value.size
     280:     int = dec.value[0]
     281:     assert_equal OpenSSL::ASN1::Integer, int.class
org/jruby/RubyKernel.java:1423:in `catch'
org/jruby/RubyKernel.java:1418:in `catch'
org/jruby/RubyKernel.java:1423:in `catch'
org/jruby/RubyKernel.java:1418:in `catch'
<Array> expected but was
<OpenSSL::BN>
========================================================================================================================================================================================================================================
F
========================================================================================================================================================================================================================================
Failure: test_prim_explicit_tagging(TestASN1)
src/test/ruby/test_asn1.rb:621:in `test_prim_explicit_tagging'
     618:     assert_equal 1, decoded.tag
     619:     assert_equal 1, decoded.value.size
     620:     inner = decoded.value[0]
  => 621:     assert_equal OpenSSL::ASN1::OctetString, inner.class
     622:     assert_equal B(%w{ 61 }), inner.value
     623:   end
     624: 
org/jruby/RubyKernel.java:1423:in `catch'
org/jruby/RubyKernel.java:1418:in `catch'
org/jruby/RubyKernel.java:1423:in `catch'
org/jruby/RubyKernel.java:1418:in `catch'
<OpenSSL::ASN1::OctetString> expected but was
<String>

diff:
? OpenSSL::ASN1::OctetString

[HoneyryderChuck]

I've reverted my commit, and most of the weird cases I was seeing aren't there anymore. It also seems that you were able to tackle the constructed tagged object case (? at least encoding well, must do further testing).

So now, the only thing missing is dealing with the tests introduced in the first commit. I've left "handle string" block there in the last commit, but I don't know which ASN1Encodable object to use to just pass the raw string bytes there, as they're not supposed to be wrapped inside another ASN1 object:

ai = OpenSSL::ASN1::ASN1Data.new(i = "bla", 0, :APPLICATION)
ai.to_der #=> must be "@\x03bla"
#=> failing in jruby

[HoneyryderChuck]

@kares I've managed to pull of a change to support the first case of the test I introduced in the first commit.

Sadly it gets more difficult from there. Take the second test:

ai = OpenSSL::ASN1::ASN1Data.new(i = "bla", 4, :UNIVERSAL)
ai.to_der #=> "\x04\x03bla", same as OpenSSL::ASN1::OctetString.new("bla").to_der

This test fails in JRuby; It seems that I can't instantiate a plain DERTaggedObject with universal tag class and correct tag number, see this check. Although one could pull of a big switch to support all tag cases (i.e. for universal, in case of tag number 4 use DERGeneralString), ruby openssl support even invalid tag numbers, and that's unlikely to be supported by BC.

Also, in ruby-openssl, This case:

ai = OpenSSL::ASN1::ASN1Data.new(i = ["bla"], 0, :APPLICATION)
ai2 = OpenSSL::ASN1.decode(ai.to_der)

encodes correctly, but fails to decode (decode': too long (OpenSSL::ASN1::ASN1Error)), which seems like a regression. It fails to encode in JRuby because it always assumes OpenSSL::ASN1::ASN1Data objects if passed an array. That could perhaps be fixed, however not sure of the upside, considering that none would reliably decode such the generated bytes.

Given the above, should we perhaps stop here and comment the remainder of failing tests, or do you see anything worth still having a look at?

[kares]

Given the above, should we perhaps stop here and comment the remainder of failing tests, or do you see anything worth still having a look at?

well depends, I understand the direction you're heading but so far do not know why. as mentioned previously:

PR's concern is now only one ticket: #264, for that bug I do not understand the context and whether there's library code using similar code. Because, as far I checked what OpenSSL::ASN1::ASN1Data.new("bla", 0, :APPLICATION).to_der produces is not standard DER content, pretty much nothing can read that except for C OpenSSL itself. If you recall where code like that has been coming from please update the bug report.

we should understand where that is coming from, given the encoding ("@\x03bla") of such structure seems like non standard DER? as there a real need out there, where did you bump into the original bug, if you still recall.

obviously would be great to match C-OpenSSL behavior 100% but if it's turning out difficult we might need to consider the context, whether it's worth the effort...

[HoneyryderChuck]

tested the latest changes with the test suite of netsnmp, it still does not decode to the same structure. One example:

der = "0Q\x02\x01\x00\x04\x06public\xA2D\x02\x04Q\x19\x83\xFE\x02\x01\x00\x02\x01\x000604\x06\b+\x06\x01\x02\x01\x01\x05\x00\x04(zeus.snmplabs.com (you can change this!)"

OpenSSL::ASN1.decode(der)

# CRuby
#=> #<OpenSSL::ASN1::Sequence:0x00007f8d075a8ac0
 @indefinite_length=false,
 @tag=16,
 @tag_class=:UNIVERSAL,
 @tagging=nil,
 @value=
  [#<OpenSSL::ASN1::Integer:0x00007f8d075a8e80 @indefinite_length=false, @tag=2, @tag_class=:UNIVERSAL, @tagging=nil, @value=#<OpenSSL::BN 0>>,
   #<OpenSSL::ASN1::OctetString:0x00007f8d075a8e30 @indefinite_length=false, @tag=4, @tag_class=:UNIVERSAL, @tagging=nil, @value="public">,
   #<OpenSSL::ASN1::ASN1Data:0x00007f8d075a8b10
    @indefinite_length=false,
    @tag=2,
    @tag_class=:CONTEXT_SPECIFIC,
    @value=
     [#<OpenSSL::ASN1::Integer:0x00007f8d075a8de0 @indefinite_length=false, @tag=2, @tag_class=:UNIVERSAL, @tagging=nil, @value=#<OpenSSL::BN 322181180>>,
      #<OpenSSL::ASN1::Integer:0x00007f8d075a8d90 @indefinite_length=false, @tag=2, @tag_class=:UNIVERSAL, @tagging=nil, @value=#<OpenSSL::BN 0>>,
      #<OpenSSL::ASN1::Integer:0x00007f8d075a8d40 @indefinite_length=false, @tag=2, @tag_class=:UNIVERSAL, @tagging=nil, @value=#<OpenSSL::BN 0>>,
      #<OpenSSL::ASN1::Sequence:0x00007f8d075a8b60
       @indefinite_length=false,
       @tag=16,
       @tag_class=:UNIVERSAL,
       @tagging=nil,
       @value=
        [#<OpenSSL::ASN1::Sequence:0x00007f8d075a8bb0
          @indefinite_length=false,
          @tag=16,
          @tag_class=:UNIVERSAL,
          @tagging=nil,
          @value=
           [#<OpenSSL::ASN1::ObjectId:0x00007f8d075a8ca0 @indefinite_length=false, @tag=6, @tag_class=:UNIVERSAL, @tagging=nil, @value="1.3.6.1.2.1.1.5.0">,
            #<OpenSSL::ASN1::OctetString:0x00007f8d075a8c00 @indefinite_length=false, @tag=4, @tag_class=:UNIVERSAL, @tagging=nil, @value="zeus.snmplabs.com (you can change this!)">]>]>]>]>

# JRuby
#=>
#<OpenSSL::ASN1::Sequence:0x5c5a91b4
 @indefinite_length=false,
 @tag=16,
 @tag_class=:UNIVERSAL,
 @tagging=nil,
 @value=
  [#<OpenSSL::ASN1::Integer:0x5be51aa @indefinite_length=false, @tag=2, @tag_class=:UNIVERSAL, @tagging=nil, @value=#<OpenSSL::BN 0>>,
   #<OpenSSL::ASN1::OctetString:0x64c8fcfb @indefinite_length=false, @tag=4, @tag_class=:UNIVERSAL, @tagging=nil, @value="public">,
   #<OpenSSL::ASN1::ASN1Data:0x2de3b052
    @tag=2,
    @tag_class=:CONTEXT_SPECIFIC,
    @value=
     [#<OpenSSL::ASN1::Sequence:0x25a290ee
       @indefinite_length=false,
       @tag=16,
       @tag_class=:UNIVERSAL,
       @tagging=nil,
       @value=
        [#<OpenSSL::ASN1::Integer:0x57e4b86c @indefinite_length=false, @tag=2, @tag_class=:UNIVERSAL, @tagging=nil, @value=#<OpenSSL::BN 1360626686>>,
         #<OpenSSL::ASN1::Integer:0x729cd862 @indefinite_length=false, @tag=2, @tag_class=:UNIVERSAL, @tagging=nil, @value=#<OpenSSL::BN 0>>,
         #<OpenSSL::ASN1::Integer:0x20b829d5 @indefinite_length=false, @tag=2, @tag_class=:UNIVERSAL, @tagging=nil, @value=#<OpenSSL::BN 0>>,
         #<OpenSSL::ASN1::Sequence:0x61ab6521
          @indefinite_length=false,
          @tag=16,
          @tag_class=:UNIVERSAL,
          @tagging=nil,
          @value=
           [#<OpenSSL::ASN1::Sequence:0x20a1b3ae
             @indefinite_length=false,
             @tag=16,
             @tag_class=:UNIVERSAL,
             @tagging=nil,
             @value=
              [#<OpenSSL::ASN1::ObjectId:0x7103745 @indefinite_length=false, @tag=6, @tag_class=:UNIVERSAL, @tagging=nil, @value="1.3.6.1.2.1.1.5.0">,
               #<OpenSSL::ASN1::OctetString:0x6fef75c6 @indefinite_length=false, @tag=4, @tag_class=:UNIVERSAL, @tagging=nil, @value="zeus.snmplabs.com (you can change this!)">]>]>]>]>]>

[HoneyryderChuck]

@kares just pushed a commit that should address that (and hopefully doesn't break any tests beyond the ones under discussion for removal 😅 ).

I can also confirm that running the netsnmp test suite got much better. It now only fails on SNMPv3 with encryption, due to jruby-openssl not having implemented OpenSSL::ASN1#traverse yet (which is a warning message for some reason, should really be an exception). That's something that can be looked at in a separate pull request, I believe.