[refactor] replace methods removed in BC 1.75

[justinstoller]

Several methods and classes were deprecated in BC 1.70 and many of those in the org.bouncycastle.asn1.* were removed in 1.75.

This commit moves away from removed method and classes so BouncyCastle can be updated to 1.76.

[justinstoller]

I don't see any meaningful output from the PR tests. Should I?

[kares]

there seems to be this error:

Error: test_decode_application_specific(TestASN1)
  RuntimeError: object explicit - implicit expected.
/home/runner/work/jruby-openssl/jruby-openssl/src/test/ruby/test_asn1.rb:484:in `test_decode_application_specific'
     481: 
     482:   def test_decode_application_specific
     483:     raw = "0\x18\x02\x01\x01`\x13\x02\x01\x03\x04\to=Telstra\x80\x03ess"
  => 484:     asn1 = OpenSSL::ASN1.decode(raw)

which seems like a regression

[justinstoller]

Thanks @kares !

I believe I've resolved the failure. At least locally rake test succeeds but mvn test fails because my machine is missing some kind of state...

I would love for you to weigh in on whether ASN1TaggedObject.getBaseUniversal(false, SEQUENCE) (now called at ASN1.java#1000 ) is the correct method to call. It replaces a call to ASN1ApplicationSpecific.getObject(tagNo) which says it's a look up of an Implicitly tagged object. ASN1TaggedObject has a deprecated getObject() that says in its place one should use getBaseUniversal(bool, tagNo), that and because we're explicitly passing in a SEQUENCE is why I went with getBaseUniversal(bool, tagNo).

However, for ASN1TaggedObject there's also getImplicitBaseObject(baseTagClass, baseTagNumber) and getBaseObject() defined. I think getImplicitBaseObject(baseTagClass, baseTagNo) is for when we completely know the underlying object tag (which I don't think we do). And getBaseObject() seems disliked by the BC developers but "Needed for open types, until we have better type-guided parsing support.". I'm unsure if JRuby requires "open types".

[kares]

Thanks Justin, this does seem good as far as I checked.

Have been looking into some other related PRs, such as #265, which made me sure we need to bring in more ASN.1 tests to make sure things work ~ as before.

So it might take me a bit until I get those .rb tests in to merge this...

[kares]

believe we should simply switch to ASN1GeneralString.getInstance passing byte[], any reason not to?

[justinstoller]

I wasn't sure if moving from the more specific DER* instances to the more generic ASN1* instances would have knock on effects farther down the road. So went with a refactor that kept the same class but newer invocation. It does seem like ASN1* is what BC wants folks to be using. I'll move to using that instead.

[justinstoller]

Thanks for the review, @kares ! I've updated based on your comments. After initially putting this up I did see there was a fair bit of overlap between this and #265 . I'm not sure if I can/should help with that PR and/or testing. Let me know if you want me to try.

[kares]

After initially putting this up I did see there was a fair bit of overlap between this and #265 .

Been looking into that one and to me it seems risky in it's current state.
The ASN.1 behavior seems to regress in some cases, unfortunately it's not covered by the current test suite.

However, this PR, seems much smaller in scope and should not cause regressions or at least not that much as #265.

[jcharaoui]

Bouncycastle 1.77 entered Debian recently, so landing this would help a lot with packaging JRuby in Debian.

[kares]

Hey, this requires more work and testing (e.g. getting more ASN.1 test from upstream) as it seems to me there's potential for a lot of regressions. The current way ASN.1 works is not aligned with C OpenSSL and we at least need to make sure it does not diverge further.

I only have limited bandwidth and already tried looking into the ASN.1 PRs such as #283 which turned out a breaking change. I feel like the changes here might end up the same and need to allocate a few day to look into the upgrade.

[justinstoller]

Let me know if there's something you'd like me to do @kares

[chadlwilson]

I also wish I could do something to help here, as there are CVEs resolved in BC 1.78 which are about to hit the dependency scanner noise soon :( https://www.bouncycastle.org/releasenotes.html

[kares]

wanted to take a look and borrow some of MRI's ASN.1 tests (a lot of them are likely to fail but at least they should fail the same between BC version changes)

been also looking at ASN.1 since tagged objects are broken (#283 and #265 both of them not mergeable atm).

maybe we should take it just one step at a time, for starters could we rebase please (there's a lot of changes on master).

[justinstoller]

Rebased. I'll check out MRI's openssl tests later today.

[justinstoller]

I added a single test and it fails, though cherry picking the commit to master, it fails the same way there. It looks like you've been very busy lately updating those tests @kares .

I'm happy to add more of the MRI tests and open them in a separate PR against master, but I'm probably not going to be able to triage and update the failures in a reasonable amount of time, and I'm not sure if I'd just be interrupting the work you have in progress.

Let me know if my continued attempts at porting are worth the while.

[justinstoller]

fwiw, I imported some more tests and commented out the failing assertions with comments as to what is failing in master here: #296

[kares]

looks good thanks, please give me a week or two to circle back here...

indeed I have been busy but master should always be in a pretty much "stable" usable state.
there's currently a regression in 0.14.4 that is already fixed but will wait a bit if anything else pops up before pushing another 0.14.x. than we could ship this as 0.15.x maybe with some other ASN.1 fixes if time allows... 🤞

[justinstoller]

I've rebased onto master and cherry-picked the full MRI asn.1 tests from #296 . Note, I've commented out the tests that fail on the master PR (with notes as to what is failing about them) and likewise have the failing tests on master commented out in this. However, this should show that no new tests fail with this PR.

I've also ran the test_asn1.rb file using BC 1.78 and it passed.

I'm not sure what's up with the CA test failures. I assume it's from me rebasing onto master? I will look at them in the morning.
If time allows I may look into to seeing if any tests now pass with my refactor that fail in the master PR.

[chadlwilson]

If help can be offered I may have some time to dive into looking at some of these in the next couple of weeks. I have a bit of ASN.1 experience from the bouncy castle world (mainly x509, CRL, oscp) so might be able to dredge some of that knowledge back up if it's relevant.

[justinstoller]

The tests failing in this PR will fail if the tests on master are re-run. The issue is that the cert the cert_store is trying to verify expired April 12th (not_after=2024-04-12 12:41:04 UTC).

I attempted to follow the steps in src/test/ruby/x509/SETUP.txt but the instructions don't work on my machine. I do not have a /usr/lib/ssl/misc/CA.sh but instead /usr/lib/ssl/misc/CA.pl which appears to have different defaults and after half an hour I still hadn't figured out which additional parameters to supply to get the steps to work.

[justinstoller]

I tried uncommenting out tests in test_prim_explicit_tagging, test_prim_implicit_tagging, test_basic_primitive, and test_basic_asn1data as it seemed like those might be the most likely to be fixed by any of my refactors. However, at least when using BC 1.74 there didn't seem to be any change of behavior - all the tests failed in the same way as on master.

[justinstoller]

As far as this PR and my test import PR go. Once I hear back from kares that these are mostly correct (not sure if all the tests that are completely commented out are valuable at this point), I can rebase my test import PR and remove the test import commits from this PR.

Forward looking, I'm not sure if I'll be able to clear time to work on it in the next few weeks, but it seems like the tests point to several potentially low hanging fruit:

1. Cases where types aren't being coerced/transformed correctly, eg RubyString <-> String <-> ASN1Data.
2. Some minor serialization issues, eg <"\x01\x00"> was expected but was <"\x01\x00\xFF">
3. More deprecations in 1.78! This PR gets us working but it would be a shame to get this all working and then to have 1.79 remove more methods we're unprepared for.

[kares]

💟 thanks - its a good start.
as a follow-up, I wonder if there is a way to pend those commented tests while still running them. similar to how rspec does (do not want to use rspec just the same feature) on test-unit or moving to minitest if needed... anyone has ideas there? if not I would consider writing a test plugin, since I feel there's a lot of commented test code already in general and it would be helpful to be able to have them "un-commented" to see early on if some of the behavior changes due a change.

The tests failing in this PR will fail if the tests on master are re-run. The issue is that the cert the cert_store is trying to verify expired April 12th (not_after=2024-04-12 12:41:04 UTC).

oh man, was about to circle back here and got surprised by 🔴 CI, guess nothing can go smooth when you do not have dedicated maintainers on a project 😞. the SETUP.txt should have been up-to-date... will take a look.

Forward looking, I'm not sure if I'll be able to clear time to work on it in the next few weeks, but it seems like the tests point to several potentially low hanging fruit:

Cases where types aren't being coerced/transformed correctly, eg RubyString <-> String <-> ASN1Data.
Some minor serialization issues, eg <"\x01\x00"> was expected but was <"\x01\x00\xFF">
More deprecations in 1.78! This PR gets us working but it would be a shame to get this all working and then to have 1.79 remove more methods we're unprepared for.

awesome if anyone can take a look at those "low hanging fruits" esp. if it's ASN.1 related... feel free to open follow up PRs

[kares]

thanks for the MR, master is now on 1.76 ...
further updates (BC 1.77) are causing errors at runtime, thus something to look into, eventually