a-sit-plus / signum

Kotlin Multiplatform Crypto/PKI Library and ASN1 Parser + Encoder
https://a-sit-plus.github.io/signum/
Apache License 2.0
76 stars 6 forks source link

Fix/oid #158

Closed JesusMcCloud closed 1 month ago

JesusMcCloud commented 1 month ago

Revamps OIDs to be based on BigIntegers and directly supports 2.25. OIDs.

Yes, this halves OID performance, but the thing is: we need all validations while parsing, so basing everything on Bytes is nice for creation, but we'd need to move all parsing logic into the primary constructor.
Hence, using BigInts as the foundation may be a bit slower, but creating and parsing remains symmetric. The performance difference would be 1/3 AT BEST, realistically, it would probably more like 15-20% at the cost of harder-to-comprehend code

iaik-jheher commented 1 month ago

have you benchmarked this? BigInteger is heavyweight enough that you're probably knocking out a bunch of locality optimizations on JVM with this

iaik-jheher commented 1 month ago

basing everything on Bytes is nice for creation, but we'd need to move all parsing logic into the primary constructor

I'm not sure I understand this option, can you elaborate?

JesusMcCloud commented 1 month ago

I benchmarked only the JVM, not on native and the performance loss for encoding and decoding combined was 50%.

An alternative would be to only work on bytes, i.e. have neither UInt nor BigInteger nodes, but just the encoded bytes as the foundation of the OID class. This would remove all the BigInteger overhead from creation, if, for example, you pass only UInts to the constructor (which would be 99.999% of the time). When decoding from DER, or encoding to String, most of the BigInteger overhead would come back though, because we'd need to verify that the bytes are indeed a valid representation of an OID. You could, of course optimize the code when parsing to not ever create a BigInteger instance. Then only toString would be slow but we'd have more almost code duplicates. Probably still a viable approach, but it would increase the complexity of the code.

JesusMcCloud commented 1 month ago

Also nevermind the failing tests. Seems like it just so happens that the random bytes accidentally made up an ASN.1 structure.

iaik-jheher commented 1 month ago

that approach sounds a lot more desirable, since toString is mostly needed in debug contexts...

JesusMcCloud commented 1 month ago

I've just revamped the validations. at the expense of some performance it now happens in one place and is exactly aligned with Bouncy castle behaviour. Also: Everything is not thoroughly tested and there should not be any loopholes any more

iaik-jheher commented 1 month ago

Now I am confused, I thought you concluded the validation was always passing?

JesusMcCloud commented 1 month ago

Now I am confused, I thought you concluded the validation was always passing?

The first two nodes require validation and when passing bigints we need to make sure that every component is positive