Improving completeness of ASN1 encoding/decoding #335
+369
−281
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
excluding already the one that I know that I can't solve, as BC does not allow tag > 31 for UNIVERSAL tag class
…ion in the tag segment the tag segment also contains info about whether the payload is for a constructed DER, and whether it's indefinite length; this info was buried in the method, with no easy way to piggyback on, so it was easier to inline the logic (only used here anyway), and propagate the rest of the information, which allows setting the indefinite_length ivar for ASN1Data objects it also raises exceptions where it couldn't (or shouldn't?)
…ith EOC empty arrays on asn1data are encoded to BERSequences, which fixed some corner cases associated with asn1data EOC isn't supported OOTB by bouncycastle, so these have to be ignored in the ASN1 part, since there's no way to use DERTaggedObject some of the logic to add the EOC bytes are inlined based on the implemented from bouncycastle, which does not allow to compose on anything, as all entities are private and unextendable
… which is not an array this is the behaviour from upstream
this is what upstream does
the logic was unaligned with upstream (see ruby rewrite here: https://github.com/ruby/openssl/blob/master/lib/openssl/asn1.rb\#L107C40-L122)
ruby allows EOC objects to be built via ASN1Data initialization, so one has to use the info of tag and tag class instead
since ruby does not have abstract classes, instances of root/intermediate classes may be instantiated, and args will determine how those objects really have to be handled this follows the logic of upstream, which implements der-encode at the base class by outsourcing to specific impls based on ivar state
so overrides are correctly loaded
commenting out the one I could only half port, as no BC parser supports random tagged objects
kares
reviewed
Jul 17, 2025
There was a problem hiding this comment.
Hey Tiago, thanks a lot - these changes look great from a first look 💟.
Will need to circle back for deeper review, for some of the parts, or at least resolve CI failures on master to make sure there aren't any regressions. Unless there are these changes should IMHO get into the next release.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To atest some of the improvements of this PR, some encode/decode tests from the ruby-openssl repo were imported (which have been previously commented out).
In broad strokes, the main fixes/improvements from this changeset:
Constructiveand rawASN1Dataobjects (the checks around EndOfContent elements were ported from theruby-opensslrepo)Primitiveobjects was incorrect, it is now matching what upstream doesruby-openssl, whichjruby-opensslcould also benefit of, but I don't think it's worth atm to just copy code around, more on that later.isEOCcheck to take into account rawASN1Dataobjects which "quack" like anEndOfContentPrimitiveobjects with an OOTB-unsupported type will now raise an error if the value can't be encoded into a string (same as upstream).I have more detailed information in each commit message, if you prefer that method of review. There were two tests I wasn't able to port, as making them work would require to adapt even more code from BC than I already did here, so I'll leave those for some other time.
On a separate note, it'd be really cool if this could be taken into the finish line. It seems that it's hanging on licensing issues, and I don't know how hard it'd be to coalesce licenses, but besides the usual code copying churn and ability to track what's and what's not supported, I've been trying to lobby for more ruby in the source code (here and here), which would benefit both implementations, but it's hard to do it across multiple repos.
cc @kares @headius