-
Notifications
You must be signed in to change notification settings - Fork 606
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.
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
Update Hashing to support HMACs, update names in fs2.hashing to be clearer, introduce Digest type #3465
Conversation
|
||
/** Creates a new MD-5 hash. */ | ||
def md5: Resource[F, Hash[F]] = create(HashAlgorithm.MD5) | ||
def md5: Resource[F, Hash[F]] = hash(HashAlgorithm.MD5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undecided if we should have all these aliases. Options:
- No aliases -- use
Hashing[F].hash(HashAlgorithm.SHA256)
instead - Aliases for most popular hashes only -- SHA1, SHA256, SHA3_256?
- Aliases for all built-in hashes (that's current state of this PR)
We don't have aliases for HMACs (though we could) -- e.g., def hmacSha256(key: Chunk[Byte]) = hmac(hash(HashAlgorithm.SHA256), key)
Thoughts @armanbilge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Aliases for all built-in hashes
It's not really possible to have this in general. By "built-in" we mean hashes that we have a case
for. Because what's actually built-in on the runtime system can be totally variable depending on the build of the JDK/Node.js/OpenSSL. In truth, any of these methods could raise an error. Maybe no aliases would be more honest to that, although I can see how that's annoying from a usability perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some docs to make the possibility of failure clear and removed all aliases.
object HashAlgorithm { | ||
case object MD5 extends HashAlgorithm | ||
case object SHA1 extends HashAlgorithm | ||
case object SHA224 extends HashAlgorithm | ||
case object SHA256 extends HashAlgorithm | ||
case object SHA384 extends HashAlgorithm | ||
case object SHA512 extends HashAlgorithm | ||
case object SHA512_224 extends HashAlgorithm | ||
case object SHA512_256 extends HashAlgorithm | ||
case object SHA3_224 extends HashAlgorithm | ||
case object SHA3_256 extends HashAlgorithm | ||
case object SHA3_384 extends HashAlgorithm | ||
case object SHA3_512 extends HashAlgorithm | ||
final case class Named(name: String) extends HashAlgorithm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If HashAlgorithm
is sealed
then we technically can't add new case
s in the future. There are various annoying workarounds, we could unseal it but make the ctor private for example ... or just ignore it and break compatibility in this relatively minor way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I made it unsealed with private constructor.
This PR adds a few features:
This PR makes a number of name changes including:
addChunk
withupdate
computeAndReset
withhash
hash
withdrain
Hash[F]
toHasher[F]
Hashing.create
toHashing.hasher
Additionally:
fs2.hashing.Hash
type was introduced, wrapping aChunk[Byte]
and providing a constant time equality checkHash
as the output type when appropriate