Enhancement/Decoding X509Certificates with unsupported X509SignatureAlgorithm #312

StjepanovicSrdjan · 2025-07-02T10:23:54Z

Besides storing rawSignature in X509Certificate, a similar approach is used in tbsCertificate for storing rawPublicKey. This helps avoid decoding failures in certificates that use, for example, DSA keys.
Added a new function in Asn1Structure to consume all remaining children. When parsing an unknown X509SignatureAlgorithm, since it’s unsupported, we don’t need to process its internal structure. For example, a DSA Asn1Sequence may have a child for parameters (or null), which would otherwise cause a trailing data exception.

…g algo

JesusMcCloud · 2025-07-02T12:21:04Z

please amend the changelog. I'll review later

JesusMcCloud · 2025-07-02T12:37:07Z

indispensable-asn1/src/commonMain/kotlin/at/asitplus/signum/indispensable/asn1/Asn1Elements.kt

+    /**
+     * Consumes all remaining children
+     * */
+    fun consumeRemainingChildren() { index = children.size }


but that just advances the index and discards everythin?!

When decoding an unsupported X509SignatureAlgorithm, we don’t need to consume the ASN.1 structure; instead, we just store it as a value. This causes the ASN.1 trailing exception to occur. ConsumeRemainingChildren() serves as a workaround for calling nextChild() repeatedly until the structure is fully parsed. Therefore, this function is called only for algorithms that we don’t know how to parse.

I don't get it. we're not doing any decoding operation on the ASN.1 element in question, so we don't need to touch the element or its child nodes at all, so there shouldn't be any checks about training elements anyhow?

I also don't feel this function should even be a thing. it is just ignoring content and thus misleading. If (at all) it should be discardRemainignChildren, but I'd need convincing of a use case for that

If you check the doDecode function in X509SignatureAlgorithm:

override fun doDecode(src: Asn1Sequence): X509SignatureAlgorithm = runRethrowing { when (val oid = (src.nextChild() as Asn1Primitive).readOid()) { KnownOIDs.rsaPSS -> parsePssParams(src) else -> { val alg = fromOid(oid) if (alg is RSA) { val tag = src.nextChild().tag if (tag != Asn1Element.Tag.NULL) throw Asn1TagMismatchException(Asn1Element.Tag.NULL, tag, "RSA Params not allowed.") } alg ?: Other(oid, src).also { src.consumeRemainingChildren() } } } }

This is where I use that function. For DSA alg, the same handling as for RSA is required—parsing the next child, which should have the NULL tag to indicate that there are no parameters. If I don't do that, or if I don't call consumeRemainingChildren(), there will be trailing children left, which will cause an error.

The reason for me not to do the same thing as for RSA is that this is the case for specific algorithm and since we don't support it, I assume that we shouldn't implement parsing specifics for that sole alg.

is the NULL-tag-to-indicate-last-child something that is specified generically for any algorithm in the RFC?

AlgorithmIdentifier ::= SEQUENCE { algorithm OBJECT IDENTIFIER, parameters ANY DEFINED BY algorithm OPTIONAL }

"The contents of the optional parameters
field will vary according to the algorithm identified." RFC-4.1.1.2

In the RSA signature algorithm, there should always be a NULL tag, whereas for EC algorithms, only the OID should be present without any additional parameters. Also, if I understood the RFC correctly, DSA shouldn't have any additional parameters either. In my opinion, the consumeRemainingChildren() function makes sense because the "parameters" field in the ASN.1 structure is OPTIONAL.

In this case, I then agree that for unknown algorithms, any parameters are permitted. They should still be consumed and represented as a list/sequence/whatever, though.

Updated X509SignatureAlgorithm to store list of parameters as Asn1Elements, allowing me to remove discussed consumeRemainingChildren()" function and just parse remaining children as parameters

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/X509SignatureAlgorithm.kt

…with the base class; add runtime registration mechanism for signature algorithms

…eters

JesusMcCloud

Looking good, but I think we can push this further and encapsulate the newly introduced pattern into an interface. Let's wait for comments form @iaik-jheher, before actually doing it.

JesusMcCloud · 2025-07-07T12:23:11Z

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/X509SignatureAlgorithm.kt

@@ -204,6 +257,6 @@ object X509SignatureAlgorithmSerializer : KSerializer<X509SignatureAlgorithm> {

    override fun deserialize(decoder: Decoder): X509SignatureAlgorithm {
        val decoded = decoder.decodeString()
-        return X509SignatureAlgorithm.entries.first { it.name == decoded }
+        return X509SignatureAlgorithm.registeredAlgorithms.first { it.name == decoded }


good thing I have a PR going that gets rid of this ugly hack ;-). fine here, of course

JesusMcCloud · 2025-07-07T12:23:53Z

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/pki/X509Certificate.kt

@@ -171,7 +199,7 @@ constructor(
                RelativeDistinguishedName.decodeFromTlv(it as Asn1Set)
            }

-            val cryptoPublicKey = CryptoPublicKey.decodeFromTlv(src.nextChild() as Asn1Sequence)
+            val rawKey = src.nextChild() as Asn1Sequence


there's a shorthand .asSequence but this is just cosmetic.

I usually use that shorthand, but since the rest of the function use as Asn1Something I followed the pattern to keep it consistent.

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/pki/X509Certificate.kt

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/X509SignatureAlgorithm.kt

JesusMcCloud · 2025-07-07T12:26:56Z

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/X509SignatureAlgorithm.kt

+        private val _registeredAlgorithms = MutableStateFlow(
+            setOf(
+                ES256, ES384, ES512,
+                PS256, PS384, PS512,
+                RS1, RS256, RS384, RS512
+            )
+        )
+        val registeredAlgorithms: Set<X509SignatureAlgorithm>
+            get() = _registeredAlgorithms.value

-        @Throws(Asn1OidException::class)
-        private fun fromOid(oid: ObjectIdentifier) = catching { entries.first { it.oid == oid } }.getOrElse {
-            throw Asn1OidException("Unsupported OID: $oid", oid)
+        fun register(algorithm: X509SignatureAlgorithm) {
+            _registeredAlgorithms.update { it + algorithm}


We will need this exact pattern in many other places in the near future. I think we should seize the opportunity and introduce an interface (with default implementations of methods, where possible) that encapsulates this functionality. @iaik-jheher do you agree?

Here, I use a set to store registered algorithms, but in the interface, it would make more sense to use a map. For example, in certificate validation, I use a Map with the OID as the key to register new X509Extensions. It would also be easier here to use a map with the OID as the key and access values directly by OID instead of using the fromOid method. However, that wasn’t possible because RSA-PSS algorithms use the same OID regardless of the number of bits.

Introduced Registry interface in ac90ceb. Tried to make it as generic as possible by storing a Map of registered items, with the ability for subclasses to define how the key is extracted from each item. Usually that can be OID (for example X509CertificateExtension in #297 ) but in the X509SignatureAlgorithm case that wasn't possible since RSA-PSS use the same OID regardless of the number of bits.

…to core data classes

JesusMcCloud

except for the open discussion point, this looks exactly as it is supposed to. Let's try to finish this on Monday

JesusMcCloud · 2025-07-10T10:50:22Z

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/Registry.kt

+ * key extraction from the item using the [extractKey] function.
+ */
+abstract class MutableRegistry<K, T>(
+    private val extractKey: ((T) -> K)? = null


not limited to a key, but a generic transformation, so I'd rename this.

Is something like transform good enough or is it too generic, or should I use something that still indicates it is used to transform an item into a key (keySelector, itemToKey)?

JesusMcCloud · 2025-07-10T10:52:23Z

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/X509SignatureAlgorithm.kt

@@ -15,28 +28,18 @@ import kotlinx.serialization.encoding.Decoder
 import kotlinx.serialization.encoding.Encoder

 @Serializable(with = X509SignatureAlgorithmSerializer::class)
-enum class X509SignatureAlgorithm(
+open class X509SignatureAlgorithm private constructor(


the constructor should be public to allow for constructing some "unknown" signature algorithms, if needed for passing them along without accessing them

JesusMcCloud · 2025-07-10T11:00:32Z

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/Registry.kt

+ * This abstract class allows runtime extension of the registry and optionally supports automatic
+ * key extraction from the item using the [extractKey] function.
+ */
+abstract class MutableRegistry<K, T>(


ideally, this should be an interface with default implementations of functions. Of course, then the encapsulation with registeredItems wont work. I think we need to discuss this offline with @iaik-jheher, as he will be implementing more advanced registration and mapping, so for now, something simple and functional is fine, but we should probably make those interfaces internal anyway until we settle on something.

JesusMcCloud · 2025-07-10T11:01:40Z

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/X509SignatureAlgorithm.kt

+        val RS384 = RSA(KnownOIDs.sha384WithRSAEncryption, "RS384")
+        val RS512 = RSA(KnownOIDs.sha512WithRSAEncryption, "RS512")
+
+        override val _registeredItems = MutableStateFlow(


I really like how readable this is by overrding theregisterdItems, but this should probably somehow be encapsulated, so that we don't need to access a member that starts with an underscore. but also here: part of an offline discussion we need to have

…rithm registry

JesusMcCloud

nitpicks

JesusMcCloud · 2025-07-14T14:53:36Z

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/X509SignatureAlgorithm.kt

+        val RS384 = RSA(KnownOIDs.sha384WithRSAEncryption, "RS384")
+        val RS512 = RSA(KnownOIDs.sha512WithRSAEncryption, "RS512")
+
+        val registeredAlgorithms = setOf(


We called such constructs entries in other locations

Suggested change

val registeredAlgorithms = setOf(

val entries = setOf(

JesusMcCloud · 2025-07-14T14:53:57Z

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/X509SignatureAlgorithm.kt

+        )
+
+        private fun fromOid(oid: ObjectIdentifier): X509SignatureAlgorithm? =
+            registeredAlgorithms.firstOrNull { it.oid == oid }


Suggested change

registeredAlgorithms.firstOrNull { it.oid == oid }

entries.firstOrNull { it.oid == oid }

JesusMcCloud · 2025-07-14T14:54:59Z

indispensable/src/commonMain/kotlin/at/asitplus/signum/indispensable/X509SignatureAlgorithm.kt

@@ -204,6 +250,6 @@ object X509SignatureAlgorithmSerializer : KSerializer<X509SignatureAlgorithm> {

    override fun deserialize(decoder: Decoder): X509SignatureAlgorithm {
        val decoded = decoder.decodeString()
-        return X509SignatureAlgorithm.entries.first { it.name == decoded }
+        return X509SignatureAlgorithm.registeredAlgorithms.first { it.name == decoded }


Suggested change

return X509SignatureAlgorithm.registeredAlgorithms.first { it.name == decoded }

return X509SignatureAlgorithm.entries.first { it.name == decoded }

JesusMcCloud · 2025-07-14T14:55:24Z

indispensable/src/jvmTest/kotlin/at/asitplus/signum/indispensable/pki/X509ConversionTests.kt

@@ -11,7 +11,7 @@ infix fun <T> KmmResult<T>.shouldSucceedWith(b: T) : T =
    (this.getOrThrow() shouldBe b)
 class X509ConversionTests : FreeSpec({
    "X509 -> Alg -> X509 is stable" - {
-        withData(X509SignatureAlgorithm.entries) {
+        withData(X509SignatureAlgorithm.registeredAlgorithms) {


Suggested change

withData(X509SignatureAlgorithm.registeredAlgorithms) {

withData(X509SignatureAlgorithm.entries) {

StjepanovicSrdjan added 3 commits July 2, 2025 12:12

refactor X509Certificate to have rawSignature so it can decode any si…

8e58f48

…g algo

replace enum with sealed class hierarchy for X509SignatureAlgorithm

094bb16

refactor tbsCertificate to store rawPublicKey

abda23d

StjepanovicSrdjan requested a review from JesusMcCloud July 2, 2025 10:24

StjepanovicSrdjan marked this pull request as ready for review July 2, 2025 12:16

update CHANGELOG

438918a

JesusMcCloud requested changes Jul 2, 2025

View reviewed changes

StjepanovicSrdjan added 3 commits July 3, 2025 10:48

make X509SignatureAlgorithm open to represent unsupported algorithms …

5f88058

…with the base class; add runtime registration mechanism for signature algorithms

renamed function for discarding all remaining children in Asn1Structure

7378ac1

replace raw value in X509SignatureAlgorithm with oid and parsed param…

79905e8

…eters

JesusMcCloud requested changes Jul 7, 2025

View reviewed changes

add todo comments on functions that transform X509SignatureAlgorithm …

75e639e

…to core data classes

JesusMcCloud reviewed Jul 10, 2025

View reviewed changes

make X509SignatureAlgorithm public and remove external signature algo…

708b79e

…rithm registry

StjepanovicSrdjan force-pushed the enhancement/x509SignatureAlgorithm-hierarchy branch from ac90ceb to 708b79e Compare July 14, 2025 14:40

remove coroutines lib from indispensable

b19a7af

StjepanovicSrdjan requested a review from JesusMcCloud July 14, 2025 14:50

JesusMcCloud requested changes Jul 14, 2025

View reviewed changes

	registeredAlgorithms.firstOrNull { it.oid == oid }
	entries.firstOrNull { it.oid == oid }

	return X509SignatureAlgorithm.registeredAlgorithms.first { it.name == decoded }
	return X509SignatureAlgorithm.entries.first { it.name == decoded }

	withData(X509SignatureAlgorithm.registeredAlgorithms) {
	withData(X509SignatureAlgorithm.entries) {

Enhancement/Decoding X509Certificates with unsupported X509SignatureAlgorithm #312

Are you sure you want to change the base?

Enhancement/Decoding X509Certificates with unsupported X509SignatureAlgorithm #312

Uh oh!

Conversation

StjepanovicSrdjan commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JesusMcCloud commented Jul 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StjepanovicSrdjan Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StjepanovicSrdjan Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JesusMcCloud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JesusMcCloud Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JesusMcCloud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JesusMcCloud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

StjepanovicSrdjan commented Jul 2, 2025 •

edited

Loading

StjepanovicSrdjan Jul 2, 2025 •

edited

Loading

StjepanovicSrdjan Jul 7, 2025 •

edited

Loading

JesusMcCloud Jul 7, 2025 •

edited

Loading