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 support for Ed25519 to CMS #220

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

baarde
Copy link
Contributor

@baarde baarde commented Jan 15, 2025

Contrary to RSA and ECDSA that take a SHA digest of the message as their input, Ed25519 uses the original message when computing the signature. For this reason, no digest algorithm was previously defined for the Ed25519 signature algorithm. However CMS requires a digest algorithm to be present in the SignerInfo. Therefore creating a CMS signature using a Ed25519 private key would previously fail.

Per RFC 8419 section 2.3, "When signing with Ed25519, the message digest algorithm MUST be SHA-512".

AlgorithmIdentifier.init(digestAlgorithmFor:) has been updated accordingly.

Signature operations have been updated to delegate the validation of the signature algorithm and the computation of the message digest (when relevant) to the underlying key.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this change @baarde! I think this is worth doing, but I wonder if we should tackle it a different way.

The constraint around SHA512 is a slightly weird one for CMS, and differs meaningfully from the X.509 use-case. I think it might be nicer to take this opportunity to give AlgorithmIdentifier(digestAlgorithmFor:) a purpose argument, which can be CMS or X509. Depending on that argument, Ed25519 could either throw or return SHA512. That'll help us avoid a confusion problem where we inappropriately apply SHA512 to the X509 use-case.

What do you think?

@baarde
Copy link
Contributor Author

baarde commented Jan 17, 2025

@Lukasa I agree that the current situation is slightly weird. Rather than distinguishing between two purposes, my suggestion would be instead to restrict the use of AlgorithmIdentifier.init(digestAlgorithmFor:) to a single purpose: CMS.

The reason for this is the following: Digests and digests algorithms are specific to CMS and don't exist in X509. The word "digest" doesn't appear at all in RFC 5280 and the only mentions of digests can be summarized as "SHA-1 is used to compute the subject key identifier" and "RSA with SHA-1 and DSA with SHA-1 are signature algorithms". X509 only care about signature sand signature algorithms: digests are an implementation details of those signature algorithms.

The current repurposing of digests and digests algorithms as helpers to implement signature operations make sense as long as the assumption "signature of message = signature of digests of message" is correct. I made the same assumption in my own implementation of CMS (which I'm in the process of getting rid of) which only supports RSA and ECDSA. However, once we introduce Ed25519, that assumption is no longer correct and leads to complications.

I think the Certificate.PrivateKey and Certificate.PublicKey currently know too much about their underlying keys and signature operations. I think it would be better to treat the underlying keys as opaque values and completely delegate the signature operations to them (we could even consider adding PrivateKeyProtocol/PublicKeyProtocol protocols in the future and move some implementations to distinct modules).

I propose to refactor the signature operations like this:

  • Use a common method signature for all the underlying keys that match the method signature of PrivateKey/PublicKey (with a signatureAlgorithm parameter).
  • Validate the signature algorithm in the underlying key.
  • Compute the digest (when relevant) in the underlying key.
  • Use SHA256/SHA384/SHA512/Insecure.SHA1 instead of Digest to compute the digest.
  • Use Digest and digest algorithms only within CMS.
  • Move signature operations from Digest.swift to Signature.swift.

I've updated my PR with those changes.

Contrary to RSA and ECDSA that take a SHA digest of the message as their input, Ed25519 uses the original message when computing the signature. For this reason, no digest algorithm was previously defined for the Ed25519 signature algorithm. However CMS requires a digest algorithm to be present in the SignerInfo. Therefore creating a CMS signature using a Ed25519 private key would previously fail.

Per RFC 8419 section 2.3, "When signing with Ed25519, the message digest algorithm MUST be SHA-512".

AlgorithmIdentifier.init(digestAlgorithmFor:) has been updated accordingly.

Signature operations have been updated to delegate the validation of the signature algorithm and the computation of the message digest (when relevant) to the underlying key.
@Lukasa
Copy link
Contributor

Lukasa commented Jan 20, 2025

So this is a very lovely change @baarde, mind fixing up the formatting check for me?

@baarde
Copy link
Contributor Author

baarde commented Jan 21, 2025

@Lukasa Sure, no problem.

@Lukasa Lukasa enabled auto-merge (squash) January 21, 2025 16:28
@Lukasa
Copy link
Contributor

Lukasa commented Jan 21, 2025

Really nice, thanks @baarde!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants