Sign and verify hashed data

[davidscheutz]

Enables the usage of other hash algorithms that aren't supported by the library like SHA3 for example.

[davidscheutz]

@finestructure @csjones I would appreciate if one of you could take the time to review this little change so more people can benefit from it. Thanks! 😃

[finestructure]

I'm not a maintainer 🙂

[csjones]

Hey @davids11 👋

First off, thank you for contribution and suggesting these changes. Your pull request touches on a previous topic which mentioned that supporting other hash algorithms is already supported by this package via the Digest protocol. Given that the ECDSA APIs wouldn't need modification to support your use case, what are your thoughts on instead introducing a more generic 32BytesDigest struct? All thats required is to be similar to SHA256Digest except the initializer takes an [UInt8] argument and then simply loads the tuple of bytes.

Let me know if your willing to make these changes for this PR 🙂

[davidscheutz]

Hi @csjones 👋

Thank you for the answer and the feedback!
I saw the Digest protocol and implemented a digest wrapper struct in my project, as you proposed. I wasn't happy with the complexity compared to simply passing in an [UInt8]... Mainly because the generic Digest constraint forces us to implement more than needed, such as static var byteCount: Int, var description: String and func hash(into hasher: inout Hasher), which is violating the Interface Segregation Principle.

Happy to discuss this further :)

[csjones]

Mainly because the generic Digest constraint forces us to implement more than needed

Sorry for the confusion, my request was to implement 32BytesDigest in this repository (not yours) thus eliminating all of the overhead and complexity you mentioned. This would be ideal because it would start to enable 32BytesDigest replacing the existing SHA256Digest.

In terms of "simply passing in an [UInt8]", this deviation from how the CryptoKit APIs are structured not only introduces redundant logic previously mentioned but would also make the move to more strongly typed protocol constraints difficult. Using [UInt8] everywhere makes error checking more ambiguous because of all different meanings an array of bytes can have.

Evolving this codebase to make bytes explicitly typed will also catch errors at build time where the existing APIs using [UInt8] will accept pretty much anything without much concern.

Would you mind telling me more about how / what you're using this change for?

[davidscheutz]

Sorry for the confusion, my request was to implement 32BytesDigest

No problem! :) Let's see if we have a shared understanding:

So instead of func signature<D: Digest> we would use func signature<D: 32BytesDigest>?

If that's what you mean, where do I find the 32BytesDigest protocol? It's not part of the swift-crypto framework

If you mean instead to add another struct called 32BytesDigest similar to SHA256Digest that implements the Digest protocol but can be constructed with an [UInt8], that would still mean we have to implement the overhead that comes with the Digest protocol...

Would you mind telling me more about how / what you're using this change for?

We use SHA3-256 to hash our data and key derivation.

[csjones]

If you mean instead to add another struct called 32BytesDigest similar to SHA256Digest that implements the Digest protocol but can be constructed with an [UInt8], that would still mean we have to implement the overhead that comes with the Digest protocol...

This is closest to what I mean but I'm confused by your reasoning of needing to implement Digest protocol because that protocol is already implemented by this package via the SHA256Digest.

A better detailed breakdown of the requested changes are:

• Rename existing SHA256Digest to 32BytesDigest (Open to suggestions for a better name than 32BytesDigest)
• Add a typealias SHA256Digest = 32BytesDigest for existing usage of SHA256Digest
• Add either a public initializer or static function to 32BytesDigest (now formerly known as SHA256Digest) similar to the code below
• EDIT: Remove this function and replace its usage with the previous step's implementation.
• Create a simple unit test to verify the change works as expected.

struct 32BytesDigest: Digest {
...
    public static func create(from bytes: [UInt8]) -> 32BytesDigest {
        let first = output[0..<8].withUnsafeBytes { $0.load(as: UInt64.self) }
        let second = output[8..<16].withUnsafeBytes { $0.load(as: UInt64.self) }
        let third = output[16..<24].withUnsafeBytes { $0.load(as: UInt64.self) }
        let forth = output[24..<32].withUnsafeBytes { $0.load(as: UInt64.self) }

        return 32BytesDigest(bytes: (first, second, third, forth))
    }
...
}

Hope that helps the shared understanding 🙂

[davidscheutz]

Thanks for the detailed answer, that definitely helped! 😃

Changes are implemented and ready to review.

Instead of 32BytesDigest I used SHA32BytesDigest because struct/class names can only start with a letter or underscore, not a number.

I'm confused by your reasoning of needing to implement Digest protocol because that protocol is already implemented by this package via the SHA256Digest

My question is: Why does SHA32BytesDigest (now formerly known as SHA256Digest) needs to implement Hashable, ContiguousBytes and CustomStringConvertible (protocols included in the Digest protocol)?

[csjones]

Glad it helped and thank you for the changes! Pretty good to go, only minor comments. Up to you if you want to address the comment with small changes.

My question is: Why does SHA32BytesDigest (now formerly known as SHA256Digest) needs to implement Hashable, ContiguousBytes and CustomStringConvertible (protocols included in the Digest protocol)?

Ahh I see what you mean now, good question. I do agree that Hashable and CustomStringConvertible in particular are not used in this package but I decided to include Apple's CryptoKit source files instead of re-implementing them.

[csjones]

Typo for the struct name.

[motobrowning]

• [ ]