Add support for generating and verifying digital signatures

[paulw11]

At present the signing function throws an exception if you try and sign too much data. Ideally the function would generate an SHA1 digest of the data and sign that, but integration of CommonCrypto into Swift frameworks seems to be tricky. See - http://iosdeveloperzone.com/2014/10/03/using-commoncrypto-in-swift/

[quentinlesceller]

Hi Paulw11,

First of all, thank you for your contribution!
Right now, I cannot merge your pull request as it is not secure to use without a hash:

• We can only sign messages up to a certain size.
• Possible forgery attacks.
• Reordering attacks.

Sources : [1], [2], [3] and [4].

I think the best way for us to implement a digital signature in SwiftyRSA would be to implement a SHA1 function (like this one in CryptoSwift which is based on this Wiki pseudocode).

Moreover the hash should be padded (see example here).

For now, implementing a digital signature with the hash (without padding) should be a good start.
Again thank you for your contribution. Adding digital signatures would be very useful as I didn't find any framework which integrates it.

If you make those changes I will be more than happy to merge them into SwiftyRSA.

[paulw11]

Would it be more sensible to include CryptoSwift as a dependency than to implement SHA1 in SwiftyRSA?

[paulw11]

I have added an SHA1 digest function using the CommonCrypto library and added PKCSSHA1 padding. An integration test is needed to make sure it works once it is integrated with a project through the pod

[quentinlesceller]

Ok this looks promising, just tried it (by building it myself) and it worked perfectly.
However, Pod installation fails because of CommonCrypto.
Carthage installation also fails but not because of your code (working on it now).

I will review this and get back to you asap.

[paulw11]

This is the method I used for integrating CommonCrypto - http://stackoverflow.com/questions/25248598/importing-commoncrypto-in-a-swift-framework

[ldiqual]

@paulw11 Great work, thanks a lot for bringing this feature to SwiftyRSA!

In addition to @quentinlesceller 's comments, I'd like to make sure we have the following before we merge:

• Signing section the README that explains what are the available signing/verification methods in SwiftyRSA, and also documents the SHA1 part.
• Add a line in CHANGELOG that describes your changes. Please link to the current pull request.
• Swift + ObjC tests for class methods signString, signData, and both variants of verifySignatureString, verifySignatureData
• Swift + ObjC tests of the instance methods counterpart.

I'm still kind of torn about the need of duplicating Swift / ObjC tests, but I've been bitten in the past by the lack of @objc that prevented the library from being used in a ObjC environment. Let me know if you guys know a better alternative to make sure everything works fine.

I also see a couple style issues that I'd like you to address, I'll comment inline for those. I think it should be enforced with Swiftlint, but for now let's do it manually.

Again, great work, this is an awesome feature to add and I particularly appreciate the time you took to include CommonCrypto since it seems like a pain to implement.

[ldiqual]

Do you really need self here?

[paulw11]

It is up to you. My preference is to use self whenever referring to a property. This is a habit from Objective-C where there is a difference between accessing the property and accessing the iVar directly.

[paulw11]

signString and signData are both tested in the current Swift test suite. Since the class methods create an instance of SwiftyRSA and then invoke the corresponding instance method on that instance method, testing the class method tests the instance method. I don't believe that specific instance method tests are required.

In fact, as signString and verifySignatureString convert the string to data and then call signData and verifySignatureData explicit tests of the ...Data methods probably aren't needed either, but I have included them

[paulw11]

I was able to have the verify... methods return a Boolean with the minimum amount of disruption to the Swift usage while still being compatible with Objective C

[mwaterfall]

This is great guys, and perfect timing too as I'm just looking to work with digital signatures! Excellent stuff. Looking forward to seeing it merged in.

[quentinlesceller]

Okay, I have looked at your pull request. First, naive question, why do you return a VerificationResult instead of a Bool ?

Also, can you update your pull request for the latest version (0.2.1) ?

There is still an error with Cocoapod because CommonCrypto is missing. There should be a way by using s.module_map = 'CommonCrypto/iphoneos.modulemap' in SwiftyRSA.podspec but I did not find it yet (see here) or maybe Cocoapod can automatically generate a bridging header for CommonCrypto ?
Also, you should modify this line in SwiftyRSA.podspec : s.source_files = "SwiftyRSA/*.swift".

I've tried some integration tests (carthage and manual build) and everything works great.
After you make those changes, we should be ready to merge.

[ldiqual]

@quentinlesceller Please hold on this, I'd like to make sure we can't find a cleaner solution with booleans rather than having another object to document.

[ldiqual]

@paulw11 @quentinlesceller I looked into this issue a bit more, and as Paul said there's no way to return a Swift primitive from a throwing function if you want to bridge it to ObjC. The BooleanType trick is pretty neat as there's an implicit conversion to Bool in swift, so I'm good with the VerificationResult addition.

@paulw11 Thank you for documenting your methods, it'll be useful if/when we want auto-generation documentation. However I don't think it's quite the time yet to add a docs section, especially since html is not very useful without a website. Let's try to reduce the scope of the PR. Would you mind removing the generated doc? As long as it's documented in the README I think we're fine.

[ldiqual]

This should be in the master section, please copy the "style" of the other bullet points in there.

[paulw11]

As I was afraid, integrating Common Crypto into the pod process is quite tricky. There are a few options I can see. One is to use a different implementation of SHA1 by declaring CryptoSwift as a dependency, but I did like the idea of using Apple's crypto library as it is more likely to be patched. The second is to include the Apple headers into the SwiftyRSA bridging header like RNCryptor does - https://github.com/RNCryptor/RNCryptor but this requires the user to mess around with their bridging header when the integrate the Pod. The third approach that I am still experimenting with is creating a separate podspec within SwiftyRSA that creates the CommonCrypto target

[quentinlesceller]

Yeah that was I expected too.
I'm writing a SHA1 function in Swift for SwiftyRSA, if it works we would be able to use your code without any modifications.
But I also like the idea to uses Apple's library especially for things like crypto.
Don't know if it helps but there is a pod CommonCrypto and one especially made to use CommonCrypto's hash function: SwiftDataHash.

[paulw11]

I took an alternative approach; I moved the SHA1 function into an Objective-C category. This seems to work when I include the pod in a test Swift project

[quentinlesceller]

Okay, tried both integrating with Carthage and CocoaPod and everything seems to work.

However, it seems that your forked does not have the 0.2.1 tag (not really a problem since we will probably update the version once your work is merged).
@ldiqual

[paulw11]

I just fetched the upstream again and got the new tag. Not sure why it didn't come across when I did that the other day, but it doesn't want to push that tag to my repo, perhaps I need to make some other change in order to give it something to push. It wont be a problem anyway, because none of my changes should be tagged 0.2.1 anyway

[ldiqual]

Looking great! Thanks a lot @paulw11 for adding this feature! This is a great improvement. I'm gonna merge your changes now and tag this version as 0.3.0 after adding some linter rules. Also, @quentinlesceller thanks for taking time to review and test the changes. I'll go ahead and publish the missing tags on cocoapods.

@paulw11 Given the great work in this PR, I've sent you an invite to join the SwiftyRSA maintainers group -- no pressure to accept! Feel free to get back to me if you have any question.