Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add digest to interface #159

Closed
wants to merge 2 commits into from
Closed

Add digest to interface #159

wants to merge 2 commits into from

Conversation

n0900
Copy link
Collaborator

@n0900 n0900 commented Oct 7, 2024

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

@n0900 n0900 requested a review from iaik-jheher October 7, 2024 16:06
@n0900 n0900 self-assigned this Oct 7, 2024
@iaik-jheher
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

n0900 commented Oct 9, 2024

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
Copy link
Collaborator

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

@n0900
Copy link
Collaborator Author

n0900 commented Oct 9, 2024

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
Copy link
Collaborator Author

n0900 commented Oct 14, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants