search

frasertweedale / hs-jose

Haskell JOSE and JWT library
http://hackage.haskell.org/package/jose
Apache License 2.0
122 stars 46 forks source link

Prevent expensive bytestring conversions #103

Closed infinisil closed 2 years ago

infinisil commented 2 years ago

Previously it was possible for JWS verification to take an incredibly long time to complete, see https://github.com/tweag/haskell-fido2/tree/a25308a07551ccd86af47774a74dbdf989454d51

This is seemingly caused by a conversion between different byte array representations, which https://hackage.haskell.org/package/concise seems to struggle with for some reason.

This commit fixes this by not doing any implicit conversion between byte array representations, making verifyJWS return a (strict) ByteString directly, which is how the payload is represented underneath.

frasertweedale commented 2 years ago

Thank you for the report. concise provides rewrite rules for common conversions (including same type) but it seems that they are not being used. Let me see if I can address this without monomorphising the types in jose.

frasertweedale commented 2 years ago

@Infinisil could you please test again with the following branch, and let me know if it solves your problem? https://github.com/frasertweedale/hs-jose/tree/perf/recons-rewrites

infinisil commented 2 years ago

@frasertweedale I can confirm that this works, but I'm not sure I agree with that being the best solution.

Something I haven't mentioned yet is how it took me hours to even figure out this problem, because for some reason, supposedly the combination of NOINLINE and rewrite rules in concise made it so that turning on profiling made the performance issues go away. Only after a while I happen to switch from a lazy ByteString to a strict ByteString (see https://github.com/tweag/haskell-fido2/commit/a25308a07551ccd86af47774a74dbdf989454d51#diff-894c9c531eb0d7c5f20f08d4008d2dfe4e48d3e542af618ed58a9b281cd28aa3), which was still extremely slow, but at least turning on profiling didn't make the problem go away. This is how I figured out that the problem lied in concise in the end.

So at this point I'm really wondering if making the return type parametric is really beneficial. If something can mess up GHC so much as to prevent profiling if you happen to use a different ByteString type, and an implicit expensive conversion might be done depending on the return type, all silently without any warning, then this feels like something that should be avoided, especially since arguing about Haskell performance is already hard as-is.

This is my motivation for the solution in this PR: It removes the abstraction of the implicit conversion, removing the possibility of either above things happening, and it means users will have to do potential expensive conversions knowingly themselves.

frasertweedale commented 2 years ago

@Infinisil point well made. That would break API compat, so I'll consider it for a future major release. (There is also a request to no longer depend on lens, which I am considering, and which would entail removing Cons from the API).

I will do a patch-level release with my change, in the next couple of days.

BTW, if you have any more issues with jose feel free to file issues straight away, even if you don't have a complete understanding or a patch ready. Had I known of JWS verification taking 4 minutes, I would have suspected concise rewrite rules not being applied immediately.

infinisil commented 2 years ago

Thanks, that sounds great! In the future I'll make sure to file issues sooner rather than later when I suspect hs-jose being related :)

infinisil commented 2 years ago

Another related note: While bisecting jose, I stumbled upon this specific change here: https://github.com/frasertweedale/hs-jose/commit/0de01cf6820ac95ecab83f73fe14e727dddf2d46#diff-358572c13fead02fd30a7e8d45dcfc21ee0b4b75f601cc271f1b8b5d138cb596L463-R465. This change happens to fix the same performance problem, but for decoding of compact JWTs instead of verification. Before this change decoding could take minutes, afterwards instantly. Notably if the l = _CompactDecodeError . _CompactInvalidText definition is inlined, it's slow again. This seems to also hint that the rewrite rules might not fire very reliably.

frasertweedale commented 2 years ago

@Infinisil thanks for the additional info. I'll definitely implement some performance regression tests, using the same big.jwt from FIDO.

frasertweedale commented 2 years ago

v0.8.5 released: https://hackage.haskell.org/package/jose-0.8.5

infinisil commented 2 years ago

Awesome, thanks! Do you still want to go for this PR for 0.9?

frasertweedale commented 2 years ago

@Infinisil I'm not sure yet. Let's just leave it open. If I do decide to remove the dependency on lens it will be inevitable.

infinisil commented 2 years ago

Closing this for now, as the original problem that motivated this PR is fixed