apple / swift-certificates

An implementation of X.509 for Swift
https://swiftpackageindex.com/apple/swift-certificates/main/documentation/x509
Apache License 2.0
223 stars 53 forks source link

Adds support for including signing time in signedAttrs for CMS signing. #176

Closed R4N closed 3 months ago

R4N commented 4 months ago

Motivation:

Many CMS signing validations require the inclusion of signing time in signedAttrs (for example Apple Wallet's Pass CMS signing validation)

Modifications:

Result:

Providing the includeSigningTime = true flag to CMS.sign will cause signing-time, content-type, and message-digest signedAttrs to be included in the CMS signature and the signature bytes will represent signing those signedAttrs. Using CMS.sign without the flag (defaulting to false) should leave the previous CMS.sign behavior unchanged.

Resolves: https://github.com/apple/swift-certificates/issues/147

Submitted on behalf of Zetetic, LLC

R4N commented 4 months ago

Thanks for the review/feedback. Definitely good suggestions. Using Date.utcDate to construct the UTCTime from date components allows me to remove the previously defined CMSSigningErrors as both of the defined errors were related to DateComponents construction.

R4N commented 4 months ago

Added in the explicit self's for the internal methods calls within CMSOperations.

Lukasa commented 3 months ago

@swift-server-bot test this please

Lukasa commented 3 months ago

These failures aren't your fault, our allocation baselines have regressed. I'll update them.

Lukasa commented 3 months ago

Feel free to rebase on to the new main branch.

R4N commented 3 months ago

Apologies, I thought it was going to give me the option to "Update with rebase" in the GH UI on my fork, but it just merged instead. What's the best way to proceed at this point?

R4N commented 3 months ago

I believe that should fix it up with the rebase now.

Lukasa commented 3 months ago

@swift-server-bot test this please

R4N commented 3 months ago

Just rebased off of main for the latest commit that came in. The last swift-server-bot check had a failure because of the CMS.sign API method signature changing. I believe this is a false positive because of the optional signingTime parameter addition. Let me know if anything else needs to be done on my end.

Lukasa commented 3 months ago

@swift-server-bot test this please

Lukasa commented 3 months ago

Merging over the API breakage.

dnadoba commented 3 months ago

Note that this is in theory an API breaking change as unapplied methods do not take default values into account:

// this fails to compile now but previously compiled successfully
let ref = CMS.sign(_:signatureAlgorithm:additionalIntermediateCertificates:certificate:privateKey:)

We can partially workaround this limitation by keeping the old signature and introducing an overload with the new parameter. The func with the old signature would just call into the new method. However, this only partially solves the source break as

// error: Ambiguous use of 'CMS.sign'
let ref = CMS.sign

now becomes ambiguous and fails to compile as well.

I personally think this is fine though. @Lukasa wdyt?

Lukasa commented 3 months ago

Yeah I don't mind at all. These are also CMS functions so they're not guaranteed to be API stable, so this isn't very egregious.