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.

Sign up for GitHub

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 attached signatures (#201) #206

Merged
merged 11 commits into from
Nov 22, 2024
Merged

Add support for attached signatures (#201) #206

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6646bdc
Add support for attached signatures (#201)
jonct Nov 4, 2024
92266b4
Relay detachment through with signing time
jonct Nov 4, 2024
40dc8ee
Factor attachment into generateSignedData
jonct Nov 4, 2024
aa05e47
Adjust test coverage
jonct Nov 4, 2024
8ece446
Merge branch 'apple:main' into attachment
jonct Nov 22, 2024
fd0ecce
Test verifying an attached signature
jonct Nov 22, 2024
7f32522
Test verifying an attached signature and timestamp
jonct Nov 22, 2024
a61dba3
Test failing detached signature as attached
jonct Nov 22, 2024
63ab1ef
Test failing detached timed signature as attached
jonct Nov 22, 2024
5c3e4f5
Test accepting attached signature as detached
jonct Nov 22, 2024
2c6754f
Test accepting attached timed signature as detached
jonct Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions Sources/X509/CryptographicMessageSyntax/CMSOperations.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ public enum CMS {
additionalIntermediateCertificates: [Certificate] = [],
certificate: Certificate,
privateKey: Certificate.PrivateKey,
signingTime: Date? = nil
signingTime: Date? = nil,
detached: Bool = true
) throws -> [UInt8] {
if let signingTime = signingTime {
return try self.signWithSigningTime(
Lukasa marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -38,12 +39,17 @@ public enum CMS {

// no signing time provided, sign regularly (without signedAttrs)
let signature = try privateKey.sign(bytes: bytes, signatureAlgorithm: signatureAlgorithm)
let signedData = try self.generateSignedData(
var signedData = try self.generateSignedData(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should sink the generation of the signed data down into generateSignedData. This has two advantages.

First, it means the extension to the "with signing time" case is straightforward. That case also ends up calling into generateSignedData`, which makes our lives a lot easier.

Secondly, it means we don't have to do the transformation back and forth through CMSContentInfo, which makes the operation much cheaper. Transforming through ASN1Any is fairly expensive and it'd be better if we set this up correctly the first time, instead of ping-ponging through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This requires passing the bytes into generateSignedData. I hope I've picked suitable function signatures for that.

I feel uncomfortable tweaking the test cases without domain-specific expertise. That skirts a QA principle, so I'm counting on your thorough scrutiny.

Copy link
Contributor

Choose a reason for hiding this comment

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

So this is broadly looking pretty good.

For test cases, you want to add a few new ones, rather than tweak the existing ones. You probably want the following:

  1. A test case that tests that the non-signing-time path can both produce a signature, and have that signature verified, with the signature in non-detached mode. Roughly equivalent to testSimpleSigningVerifying but with non-detached signatures.
  2. A test case like the above, but with signing time included. Roughly equivalent to testSigningWithSigningTimeSignedAttr.
  3. A version of (1) where the signature is made non-attached, but is verified as if it were attached. This should fail.
  4. A version of (2) where the signature is made non-attached, but is verified as if it were attached. This should fail.
    1. A version of (1) where the signature is made attached, but is verified as if it were non-attached. This should pass: all th eneeded data is present.
  6. A version of (2) where the signature is made attached, but is verified as if it were nonattached. This should pass: all the needed data is present..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Humble apologies for the delay; it was not my intention to hit and run.

FWIW, in the meantime my intended target project has been building from this outside branch. And I'm happy to say it's been generating correctly signed .mobileconfig files, recognized by iOS and macOS. Thank you!

I believe that I've addressed each of these six tests, in order. Please forgive the clumsy naming.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's absolutely nothing to apologise for, it's not always easy to get the time management lined up. This is the nice thing about OSS, I can be event-driven, so waiting for you to come back isn't a burden at all! I'll re-review now.

signatureBytes: ASN1OctetString(signature),
signatureAlgorithm: signatureAlgorithm,
additionalIntermediateCertificates: additionalIntermediateCertificates,
certificate: certificate
)
if !detached {
var signed = try CMSSignedData(asn1Any: signedData.content)
signed.encapContentInfo.eContent = ASN1OctetString(contentBytes: Array(bytes)[...])
signedData.content = try ASN1Any(erasing: signed)
}

return try self.serializeSignedData(signedData)
}
Expand All @@ -55,7 +61,8 @@ public enum CMS {
additionalIntermediateCertificates: [Certificate] = [],
certificate: Certificate,
privateKey: Certificate.PrivateKey,
signingTime: Date
signingTime: Date,
detached: Bool = true
) throws -> [UInt8] {
var signedAttrs: [CMSAttribute] = []
// As specified in RFC 5652 section 11 when including signedAttrs we need to include a minimum of:
Expand Down Expand Up @@ -86,13 +93,18 @@ public enum CMS {
try coder.serializeSetOf(signedAttrs)
let signedAttrBytes = coder.serializedBytes[...]
let signature = try privateKey.sign(bytes: signedAttrBytes, signatureAlgorithm: signatureAlgorithm)
let signedData = try self.generateSignedData(
var signedData = try self.generateSignedData(
signatureBytes: ASN1OctetString(signature),
signatureAlgorithm: signatureAlgorithm,
additionalIntermediateCertificates: additionalIntermediateCertificates,
certificate: certificate,
signedAttrs: signedAttrs
)
if !detached {
var signed = try CMSSignedData(asn1Any: signedData.content)
signed.encapContentInfo.eContent = ASN1OctetString(contentBytes: Array(bytes)[...])
signedData.content = try ASN1Any(erasing: signed)
}
return try self.serializeSignedData(signedData)
}

Expand Down
Loading