Introduces incremental hash. #188

yilinwei · 2023-08-07T18:02:09Z

Fixes #181. Each API is subtly different, but in general we have a new/update/finalize/reset.

The WebCrypto API for browsers doesn't support incremental hashes yet; we could either shim it (i.e. build up the ByteVector in memory) or just throw. I feel that it is better to throw so that the user does not have an expectation of memory usage, but it might be a PITA for anyone cross-compling.

Fixes typelevel#181. Each API is subtly different, but in general we have a new/update/finalize/reset. The WebCrypto API for browsers doesn't support incremental hashes yet; we could either shim it (i.e. build up the `ByteVector` in memory) or just throw. I feel that it is better to throw so that the user does not have an expectation of memory usage, but it might be a PITA for anyone cross-compling.

core/js/src/main/scala/bobcats/HashPlatform.scala

core/native/src/main/scala/bobcats/HashPlatform.scala

core/jvm/src/main/scala/bobcats/HashPlatform.scala

armanbilge · 2023-08-07T21:48:20Z

core/shared/src/main/scala/bobcats/Hash.scala

+/**
+ * Used for incremental hashing.
+ */
+sealed trait Digest[F[_]] {


I'm not against this, but I do want to point out some trade-offs ...

An incremental hashing API in terms of update/get/reset is "lower-level" vs an API expressed as a Stream.

This is relevant because there is a proposal for browsers to support incremental hashing, but this will likely be in the form of streams. I think it might still be possible to implement the API you've declared here with a stream, but we should be thoughtful about that.

https://github.com/wintercg/proposal-webcrypto-streams

I agree; I wasn't sure which way we wanted the dependency graph (as in bobcats <- fs2 or fs2 -> bobcats where -> is dependent on). If the former then more flexibility is always nicer.

EDIT. I'll take a look at the webcrypto-streams proposal and see if I can stub an implementation with this API.

bobcats <- fs2

Definitely not this. There is a high bar for FS2 taking on new dependencies and although it currently has crypto code I believe moving it out to a library like this makes much more sense.

👍 Then I'll switch over to a Stream based API - we can always split into multiple modules if we don't want consumers to pull in fs2.

armanbilge · 2023-08-08T02:53:33Z

core/native/src/main/scala/bobcats/HashPlatform.scala

-  implicit def forAsync[F[_]](implicit F: Async[F]): Hash[F] =
+  implicit def forAsync[F[_]](implicit F: Async[F]): Hash[F] = forSync


Btw: I now believe offering implicits in exchange for Sync/Async is an anti-pattern. See:

Implicit instances for IO only fs2#3179

yilinwei · 2023-08-08T17:57:43Z

core/jvm/src/main/scala/bobcats/Hash1Platform.scala

+    implicit F: Applicative[F])
+    extends UnsealedHash1[F] {
+
+  override def digest(data: ByteVector): F[ByteVector] = F.pure {


So I think by convention this shouldn't throw if the MessageDigest is well behaved but we can err on the side of caution and just catchNonFatal.

armanbilge · 2023-08-08T18:01:49Z

core/jvm/src/main/scala/bobcats/Hash1Platform.scala

+    extends UnsealedHash1[F] {
+
+  override def digest(data: ByteVector): F[ByteVector] = F.pure {
+    val h = hash.clone().asInstanceOf[MessageDigest]


Doesn't look like cloning is always supported.

Implementations are free to implement the Cloneable interface. Client applications can test cloneability by attempting cloning and catching the CloneNotSupportedException:

https://docs.oracle.com/javase/8/docs/api/java/security/MessageDigest.html

Argh! This is very annoying.

I think I'll double check bouncy castle (since that's probably going to be the biggest implementation) and if they support cloning then I'll do a .clone when creating the Hash1 and switch out to a different implementation if it can't.

core/jvm/src/main/scala/bobcats/Hash1Platform.scala

yilinwei · 2023-08-08T18:04:58Z

core/shared/src/main/scala/bobcats/Hash1.scala

+ * Use this class if you have a specific `HashAlgorithm` known in advance or you're using a
+ * customized algorithm not covered by the `HashAlgorithm` class.
+ */
+sealed trait Hash1[F[_]] {


The idea is that this should wrap the MessageDigest/EVP_MD/Hash equivalent on each platform so folks can construct them when the HashAlgorithm isn't complete.

It's also means that you can fail-fast if you have an application which is dependent on certain algorithm's, which imo is better than waiting until later.

If I feel like this mechanism is good, we can reuse the same pattern for most of the other algorithm selection. Hash is the simplest so doing it here first is good.

armanbilge · 2023-08-08T18:16:24Z

Another high-level idea: have you seen this issue?

High Overhead - MessageDigest fs2#3157

You could set up some sort of keypool of MessageDigest objects and re-use instances from that.

yilinwei · 2023-08-08T18:43:39Z

Yes I have; I think that apart from the performance gains, it doesn't help in this case (apologies if I've misinterpreted your comment).

I think the underlying issue that MessageDigest#getInstance is side-effecting is the more annoying problem (hence trying to side-step it via .clone()).

Consider you set up a pool and there aren't any more MessageDigest's and getInstance fails when trying to construct a new digest. You either fail or need to wait until an instance is free; you've just changed your function from something that definitely will fail to something that might fail.

Maybe this is overthinking it though, since 99% of users won't be doing addSecurityProvider and even less doing removeSecurityProvider.

armanbilge · 2023-08-08T19:24:25Z

I think the underlying issue that MessageDigest#getInstance is side-effecting is the more annoying problem (hence trying to side-step it via .clone()).

Ahh, I'm sorry, I just understood what you are trying to do with that. Hmm.

Ok, how about this: if you have a known list of Providers, then there should be no side-effect right? e.g. you can use catchNonFatal. So you can use the getInstance method that explicitly takes a Provider.

https://docs.oracle.com/javase/8/docs/api/java/security/MessageDigest.html#getInstance-java.lang.String-java.security.Provider-

Then to construct the Hash you will need that list of Providers. The user can either provide this or it can be obtained side-effectually via Security.getProviders.

https://docs.oracle.com/javase/8/docs/api/java/security/Security.html#getProviders--

Edit: hmmmmmmm, maybe not 😭 even Providers look to be mutable.

Edit 2: perhaps if you clone the providers beforehand?

core/jvm/src/main/scala/bobcats/Hash1Platform.scala

core/jvm/src/main/scala/bobcats/HashPlatform.scala

core/jvm/src/main/scala/bobcats/CryptoPlatform.scala

core/jvm/src/main/scala/bobcats/Providers.scala

core/jvm/src/main/scala/bobcats/Hash1Platform.scala

core/native/src/main/scala/bobcats/Hash1Platform.scala

core/shared/src/test/scala/bobcats/HotpSuite.scala

yilinwei · 2023-08-10T17:50:37Z

A lot of code for very little functionality; but it opens up it up for #189.

I'm generally happy with the pattern as well, exposing a variant of the capabilities for #58. I think the names are a little meh; the reason I'm not using forPlatform is that NodeJS for example offers both SubtleCrypto and Crypto; I'm not actually sure if the names are guaranteed to be consistent between the both of them...

Some love needs to be put into ignoring the test cases and fixtures (maybe a switch to weaver would be good).

yilinwei force-pushed the incremental-hash branch from 3a9501e to 502c96d Compare August 7, 2023 18:04

armanbilge reviewed Aug 7, 2023

View reviewed changes

core/js/src/main/scala/bobcats/HashPlatform.scala Outdated Show resolved Hide resolved

Wrap definitely side-effecting code.

bf9832e

yilinwei commented Aug 7, 2023

View reviewed changes

core/native/src/main/scala/bobcats/HashPlatform.scala Outdated Show resolved Hide resolved

yilinwei commented Aug 7, 2023

View reviewed changes

core/jvm/src/main/scala/bobcats/HashPlatform.scala Outdated Show resolved Hide resolved

wrap possibly side-effecting code

13d57f4

armanbilge reviewed Aug 7, 2023

View reviewed changes

armanbilge reviewed Aug 8, 2023

View reviewed changes

Sketch of allowing extensibility for HashAlgorithm.

c429339

yilinwei changed the title ~~Introduces incremental hash.~~ WIP: Introduces incremental hash. Aug 8, 2023

yilinwei commented Aug 8, 2023

View reviewed changes

armanbilge reviewed Aug 8, 2023

View reviewed changes

core/jvm/src/main/scala/bobcats/Hash1Platform.scala Outdated Show resolved Hide resolved

yilinwei commented Aug 8, 2023

View reviewed changes

switch to pipe

8f5e144

switch to caching the Provider

4062517

yilinwei commented Aug 8, 2023

View reviewed changes

core/jvm/src/main/scala/bobcats/Hash1Platform.scala Outdated Show resolved Hide resolved

yilinwei commented Aug 8, 2023

View reviewed changes

core/jvm/src/main/scala/bobcats/HashPlatform.scala Outdated Show resolved Hide resolved

Wrap it in a F.

dfad621

yilinwei commented Aug 9, 2023

View reviewed changes

core/jvm/src/main/scala/bobcats/CryptoPlatform.scala Outdated Show resolved Hide resolved

yilinwei commented Aug 9, 2023

View reviewed changes

core/jvm/src/main/scala/bobcats/Providers.scala Outdated Show resolved Hide resolved

yilinwei added 2 commits August 9, 2023 22:20

Add native platform

3e0a99e

remove uneeded function

8220dc0

yilinwei commented Aug 9, 2023

View reviewed changes

core/jvm/src/main/scala/bobcats/Hash1Platform.scala Outdated Show resolved Hide resolved

run prePR

bf4ac50

yilinwei commented Aug 9, 2023

View reviewed changes

core/native/src/main/scala/bobcats/Hash1Platform.scala Show resolved Hide resolved

Add Hmac back in.

8e3be19

yilinwei commented Aug 10, 2023

View reviewed changes

core/shared/src/test/scala/bobcats/HotpSuite.scala Show resolved Hide resolved

Add JS

0c38396

yilinwei force-pushed the incremental-hash branch from dc23f27 to 0c38396 Compare August 10, 2023 16:53

yilinwei added 4 commits August 10, 2023 18:09

Ugly hack to skip browser tests

2fe0529

log the info

a0f1ede

Ignore for MD5.

5796fba

Stop being such an idiot.

6db3a56

yilinwei changed the title ~~WIP: Introduces incremental hash.~~ Introduces incremental hash. Aug 10, 2023

get rid of legacy functions for evp

bf68912

yilinwei merged commit a9130e3 into typelevel:main Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduces incremental hash. #188

Introduces incremental hash. #188

yilinwei commented Aug 7, 2023

armanbilge Aug 7, 2023 •

edited

Loading

yilinwei Aug 7, 2023 •

edited

Loading

armanbilge Aug 7, 2023

yilinwei Aug 7, 2023

armanbilge Aug 8, 2023

yilinwei Aug 8, 2023

armanbilge Aug 8, 2023

yilinwei Aug 8, 2023

yilinwei Aug 8, 2023 •

edited

Loading

armanbilge commented Aug 8, 2023

yilinwei commented Aug 8, 2023

armanbilge commented Aug 8, 2023 •

edited

Loading

yilinwei commented Aug 10, 2023

		implicit def forAsync[F[_]](implicit F: Async[F]): Hash[F] =
		implicit def forAsync[F[_]](implicit F: Async[F]): Hash[F] = forSync

Introduces incremental hash. #188

Introduces incremental hash. #188

Conversation

yilinwei commented Aug 7, 2023

armanbilge Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

yilinwei Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

armanbilge Aug 7, 2023

Choose a reason for hiding this comment

yilinwei Aug 7, 2023

Choose a reason for hiding this comment

armanbilge Aug 8, 2023

Choose a reason for hiding this comment

yilinwei Aug 8, 2023

Choose a reason for hiding this comment

armanbilge Aug 8, 2023

Choose a reason for hiding this comment

yilinwei Aug 8, 2023

Choose a reason for hiding this comment

yilinwei Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

armanbilge commented Aug 8, 2023

yilinwei commented Aug 8, 2023

armanbilge commented Aug 8, 2023 • edited Loading

yilinwei commented Aug 10, 2023

armanbilge Aug 7, 2023 •

edited

Loading

yilinwei Aug 7, 2023 •

edited

Loading

yilinwei Aug 8, 2023 •

edited

Loading

armanbilge commented Aug 8, 2023 •

edited

Loading