Improving completeness of ASN1 encoding/decoding #335
Conversation
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
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.
|
@kares thx for taking a look 🙏 I believe it's in a better state compatibility-wise (even if some of the java code is questionable, apologies for that 😂 ). FYI I'm also working on the side to add the remainder of upstream asn1 tests here. It'll take way more time though, as openssl allows to build asn1 objects in too flexible a way when compared to BC (which just prevents certain combinations OTTB, and that's when one needs to import some of that code into here). But first things first, let's get this merged in once you give the sign-off away! 🙏 |
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