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

Add digest to interface #159

Closed n0900 closed 1 month ago

n0900 commented 1 month ago

I propose to add digest as a variable to the SignatureAlgorithm interface in order to prevent the following code snippet. @JesusMcCloud told me to ask you about it.

Screenshot from 2024-10-07 17-59-15

iaik-jheher commented 1 month ago

I had this discussion with @JesusMcCloud in the past; just because every kind of signature algorithm we currently support uses a Digest, that doesn't mean that every signature algorithm we might support in the future (PQC?) will use a Digest; so I was reluctant to add it to the interface.

We could maybe have a SignatureAlgorithm.WithDigest marker interface in between?

JesusMcCloud commented 1 month ago

This is really tricky, because even signature that could have a digest, might not always come with one, so nullable and not having it at all is semantically different.

We could maybe have a SignatureAlgorithm.WithDigest marker interface in between? This is probably the only sane way to go, because using it, as it is now, is just a pain and needs improvement. I don't think we can go wrong with this suggestion

iaik-jheher commented 1 month ago

I don't think we can go wrong with this suggestion

well, we've run into this before at various points... kotlin has no good way to express class traits, i.e., to say that "any instance of this class must have one or the other of this marker interface". This is not legal.

Also, there is no way to cleanly deal with multi-trait classes. This is also not legal.

So if you ever need two or more marker interfaces on the same type, it becomes a complete mess; you end up having to create new interfaces for each possible intersection you might need, and update every implementation's inheritance list.

I don't think this will be an issue for SignatureAlgorithm now (since we only have a single trait that we want to add), but we need to realize that it's the only trait we get to realistically add.

n0900 commented 1 month ago

I had this discussion with @JesusMcCloud in the past; just because every kind of signature algorithm we currently support uses a Digest, that doesn't mean that every signature algorithm we might support in the future (PQC?) will use a Digest; so I was reluctant to add it to the interface.

But do we not circumvent this problem by making digest nullable? This simply states that it might have one.

JesusMcCloud commented 1 month ago

see my previous comment. nullable and no digest at all are semantically different

n0900 commented 1 month ago

see my previous comment. nullable and no digest at all are semantically different

True, but so is X509SignatureAlgorithm(oid=..., isEc=False) and X509SignatureAlgorithm(oid) however functionally they are the same

n0900 commented 1 month ago

Any updates on this? Should this be closed? @JesusMcCloud @iaik-jheher