Add support for attached signatures (#201) #206
Conversation
There was a problem hiding this comment.
Thanks, this is a good start! I've left some notes in the diff with suggestions. I'd also suggest writing some tests, which will force you to also handle the verification path. Fortunately, the verification path is straightforward: accept a nil data object and grab the bytes to be verified from the CMS object instead in that case.
| @@ -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( | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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
testSimpleSigningVerifyingbut with non-detached signatures. - A test case like the above, but with signing time included. Roughly equivalent to
testSigningWithSigningTimeSignedAttr. - A version of (1) where the signature is made non-attached, but is verified as if it were attached. This should fail.
- A version of (2) where the signature is made non-attached, but is verified as if it were attached. This should fail.
-
- 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.
- 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..
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Merging over the API breakage, which is expected. |
Initial rough cut.