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

R4N · 2024-05-29T20:05:03Z

Motivation:

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

Modifications:

Adds optional includeSigningTime flag to CMS.sign method (defaults to false)
When includeSigningTime flag is true: includes signing-time (UTCTime ASN1Type), content-type, and message-digest signedAttrs and signs those instead of content bytes (as specified in RFC 5652 section 5.4)
Adds contentType ASN1ObjectIdentifier
Adds CMSSigningError (noTimeZone, invalidSigningDateComponent) for when includeSigningTime is true and unable to get UTC time zone or date components
Adds tests for valid and invalid CMS signature when including signedAttrs

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: #147

Submitted on behalf of Zetetic, LLC

Lukasa

Thanks for this! I've left a few notes in the diff.

Lukasa · 2024-05-30T15:48:03Z

Sources/X509/CryptographicMessageSyntax/CMSOperations.swift

@@ -15,6 +15,11 @@ import Foundation
 import SwiftASN1
 import Crypto

+public enum CMSSigningError: Error {


Let's avoid using an enum error here. We've got a pattern of struct-based errors elsewhere in the project (see CertificateError), can we reproduce it or use it here?

Lukasa · 2024-05-30T15:48:53Z

Sources/X509/CryptographicMessageSyntax/CMSOperations.swift

-        return try sign(
+        var signature: Certificate.Signature
+        var completeSignedAttrs: [CMSAttribute]? = nil
+        if includeSigningTime {


Can we factor this brach out to a separate internal method? It makes it harder to audit if we don't.

Lukasa · 2024-05-30T15:49:16Z

Sources/X509/CryptographicMessageSyntax/CMSOperations.swift

@@ -400,3 +481,27 @@ extension Certificate.Signature {
        )
    }
 }
+
+extension CMSSigningError: LocalizedError {


Let's not conform to LocalizedError, it's not worth it.

Lukasa · 2024-05-30T15:59:03Z

Sources/X509/CryptographicMessageSyntax/CMSOperations.swift

@@ -23,15 +28,82 @@ public enum CMS {
        signatureAlgorithm: Certificate.SignatureAlgorithm,
        additionalIntermediateCertificates: [Certificate] = [],
        certificate: Certificate,
-        privateKey: Certificate.PrivateKey
+        privateKey: Certificate.PrivateKey,
+        includeSigningTime: Bool = false


I don't think this is quite the right spelling: we should have the signing time provided as a Date?, and if we want to omit it then it can be nil.

Lukasa · 2024-05-30T15:59:31Z

Sources/X509/CryptographicMessageSyntax/CMSOperations.swift

+            signedAttrs.append(contentTypeAttribute)
+
+            // add message-digest sha256 of provided content bytes
+            try SHA256.hash(data: bytes).withUnsafeBytes { bufferPointer in


Can we factor this out to a small helper that is an init on ASN1OctetString? It's nice to reduce the scope of the unsafe pointer.

We should not need any unsafe Swift here. The returned digest conforms to Sequence<UInt8>. Just convert the returned bytes to an ArraySlice.

Lukasa · 2024-05-30T16:01:13Z

Sources/X509/CryptographicMessageSyntax/CMSOperations.swift

+            }
+            var calendar = Calendar(identifier: .gregorian)
+            calendar.timeZone = timeZone
+            let nowComponents = calendar.dateComponents(desiredComponents, from: Date())


We should avoid this. We have a Date.utcTime transformation that already exists.

dnadoba · 2024-05-30T16:11:39Z

Sources/X509/CryptographicMessageSyntax/CMSOperations.swift

+            signedAttrs.append(contentTypeAttribute)
+
+            // add message-digest sha256 of provided content bytes
+            try SHA256.hash(data: bytes).withUnsafeBytes { bufferPointer in


We should not need any unsafe Swift here. The returned digest conforms to Sequence<UInt8>. Just convert the returned bytes to an ArraySlice.

dnadoba · 2024-05-30T16:12:40Z

Tests/X509Tests/CMSTests.swift

+        signedAttrs.append(contentTypeAttribute)
+
+        // add message-digest sha256 of provided content bytes
+        try SHA256.hash(data: bytes).withUnsafeBytes { bufferPointer in


Also, here. No need for unsafe Swift.

R4N · 2024-05-31T15:09:36Z

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.

Lukasa · 2024-06-04T12:40:43Z

Sources/X509/CryptographicMessageSyntax/CMSOperations.swift

    ) throws -> [UInt8] {
+        if let signingTime = signingTime {
+            return try signWithSigningTime(


Nit: we prefer to use explicit self in all places.

Lukasa · 2024-06-04T12:41:28Z

Sources/X509/CryptographicMessageSyntax/CMSOperations.swift

            signatureBytes: ASN1OctetString(signature),
            signatureAlgorithm: signatureAlgorithm,
            additionalIntermediateCertificates: additionalIntermediateCertificates,
            certificate: certificate
        )
+
+        return try serializeSignedData(signedData)


Same nit here, prefer self.

Lukasa · 2024-06-04T12:45:59Z

Sources/X509/CryptographicMessageSyntax/CMSOperations.swift

+            certificate: certificate,
+            signedAttrs: signedAttrs
+        )
+        return try serializeSignedData(signedData)


Same note on self here.

R4N · 2024-06-04T14:13:00Z

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

Lukasa · 2024-06-07T13:16:02Z

@swift-server-bot test this please

Lukasa · 2024-06-07T13:48:48Z

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

Lukasa · 2024-06-07T15:27:52Z

Feel free to rebase on to the new main branch.

R4N · 2024-06-07T15:53:16Z

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 · 2024-06-07T16:05:34Z

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

Lukasa · 2024-06-07T16:06:01Z

@swift-server-bot test this please

Motivation: Many CMS signing validations require the inclusion of signing time in signedAttrs (for example Apple Wallet's Pass CMS signing validation) Modifications: * Adds optional includeSigningTime flag to CMS.sign method (defaults to false) * When includeSigningTime flag is true: includes signing-time (UTCTime ASN1Type), content-type, and message-digest signedAttrs and signs those instead of content bytes (as specified in RFC 5652 section 5.4) * Adds contentType ASN1ObjectIdentifier * Adds CMSSigningError (noTimeZone, invalidSigningDateComponent) for when includeSigningTime is true and unable to get UTC time zone or date components * Adds tests for valid and invalid CMS signature when including signedAttrs 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.

Motivation: Callers may want to provide their own signingTime (Date). Code improvements for inclusion of singing time in signedAttrs. Modifications: * Changes CMS.sign method to take Date? instead of Bool for signingTime so callers can provide their own signingTime Date * Adds internal method CMS.signWithSigningTime to separate logic for CMS signing with signingTime in signedAttrs * Uses Date.utcDate transformation to get DateComponents instead of manually constructing them within CMS.signWithSigningTime * Removes CMSSigningError from CMSOperations as its no longer needed after removal of manual DateComponents construction * Switches message digest SHA256 from using unsafe bytes to converting Sequence<UInt8> to ArraySlice directly Result: CMS.sign callers can now provide signingTime (Date). CMS.sign with signingTime signedAttrs code is improved.

…ons. Motivation: Conform to standards regaring usage of explicit self for internal methods calls. Modifications: * Adds explicit self to signWithSigningTime and serializeSignedData internal methods calls with CMSOperations Result: Internal CMSOperations method calls are now compliant to standards.

R4N · 2024-06-20T14:47:53Z

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 · 2024-06-21T09:41:59Z

@swift-server-bot test this please

dnadoba

LGTM. Thanks!

Lukasa · 2024-06-21T10:38:35Z

Merging over the API breakage.

dnadoba · 2024-06-21T10:39:19Z

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 · 2024-06-21T10:42:00Z

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.

Lukasa requested changes May 30, 2024

View reviewed changes

dnadoba requested changes May 30, 2024

View reviewed changes

dnadoba reviewed May 30, 2024

View reviewed changes

R4N requested review from dnadoba and Lukasa May 31, 2024 18:28

Lukasa reviewed Jun 4, 2024

View reviewed changes

R4N requested a review from Lukasa June 4, 2024 13:58

Lukasa added the 🔨 semver/patch No public API change. label Jun 7, 2024

R4N force-pushed the signing-time branch from b3167bb to add6450 Compare June 7, 2024 16:01

R4N added 3 commits June 20, 2024 10:41

R4N force-pushed the signing-time branch from add6450 to d40a215 Compare June 20, 2024 14:41

Merge branch 'main' into signing-time

28f7a71

Lukasa approved these changes Jun 21, 2024

View reviewed changes

dnadoba approved these changes Jun 21, 2024

View reviewed changes

Lukasa merged commit 44f2a33 into apple:main Jun 21, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

R4N commented May 29, 2024

Lukasa left a comment

Lukasa May 30, 2024

Lukasa May 30, 2024

Lukasa May 30, 2024

Lukasa May 30, 2024

Lukasa May 30, 2024

dnadoba May 30, 2024

Lukasa May 30, 2024

dnadoba May 30, 2024

dnadoba May 30, 2024

R4N commented May 31, 2024

Lukasa Jun 4, 2024

Lukasa Jun 4, 2024

Lukasa Jun 4, 2024

R4N commented Jun 4, 2024

Lukasa commented Jun 7, 2024

Lukasa commented Jun 7, 2024

Lukasa commented Jun 7, 2024

R4N commented Jun 7, 2024

R4N commented Jun 7, 2024

Lukasa commented Jun 7, 2024

R4N commented Jun 20, 2024

Lukasa commented Jun 21, 2024

dnadoba left a comment

Lukasa commented Jun 21, 2024

dnadoba commented Jun 21, 2024

Lukasa commented Jun 21, 2024

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

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

Conversation

R4N commented May 29, 2024

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R4N commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R4N commented Jun 4, 2024

Lukasa commented Jun 7, 2024

Lukasa commented Jun 7, 2024

Lukasa commented Jun 7, 2024

R4N commented Jun 7, 2024

R4N commented Jun 7, 2024

Lukasa commented Jun 7, 2024

R4N commented Jun 20, 2024

Lukasa commented Jun 21, 2024

dnadoba left a comment

Choose a reason for hiding this comment

Lukasa commented Jun 21, 2024

dnadoba commented Jun 21, 2024

Lukasa commented Jun 21, 2024