user can now specify signature auth header name

[adlaika]

Hey! Great library, does everything I need it to...except that the authHeaderName is hardcoded to "Authentication". Some webhooks, such as Kard's, use Notify-Signature or others. So I added the ability to pass an arbitrary header name around. Lemme know if there's anything you'd like done differently :)

[jhrcek]

This change seems reasonable to me. I'd just keep defaultAuthHeaderName to make it clearer that to people that auth header name is not completely arbitrary choice.

[jhrcek]

Thanks, looks good to me.
@michivi and @kahlil29 please take a look too (and merge if ok) as you might have more experience with this package.

[kahlil29]

Thanks @jhrcek for tagging me.
Even though I have Watching enabled with All Activity, I still do not get notified when PRs are raised in this repo 😞

I just triggered CI.

Change looks reasonable to me, although at first glance I cannot tell if it would disrupt or affect existing usage/users of the library 🤔

[adlaika]

@kahlil29 it does add an argument to several functions that folks might be using, so it's definitely a breaking change.

[kahlil29]

I see, thanks for explaining @adlaika
In that case, I'll wait for @michivi & @thongpv87 to also look at this PR.

@ocharles As the (known to me) other external party who uses this library [CircuitHub], would this affect you in any way?

[adlaika]

@kahlil29 I went ahead and changed the API such that it's non-breaking; all the changes are now in exported functions named like hmacAuthServerContext' (that is, suffixed with a backtick), and those are used to export the original functions with their original types.

Also, on further reflection, I've discovered a few other issues that are keeping us from using this as-is:

1. The assumption that the signature header with be prefixed with HMAC , and
2. The assumption that the signature will have been generated using a specific combination of headers + body (Kard, in their idiosyncrasy, uses only the request body).

Currently working to add functionality on those fronts; I will again try to keep it non-breaking. Thanks for the lib, and the help!

[adlaika]

So I've come across a pretty major issue: as far as I can tell, this lib doesn't actually use the request payload's content to generate the request signature. You have tests in Servant.Auth.Hmac.CryptoSpec that test requestSignature, but all the actual use points of requestSignature come after setting the request payload's content to empty string.

Am I reading that correctly? If so, isn't this is a pretty serious vector for MITM attacks, since changes to request body won't affect the signature?

I've been trying to fix it, but it's a challenge because--as far as I can tell--there's no way to consume the http body in Servant's auth context while leaving it un-consumed for the "normal" handlers. I suspect this is why there's a bunch of commented-out code where the payload's content might have been used to generate a signature.

Anyway, please advise. I hate to give up on this, since I think in general it's The Right Approach, but at this point I might just have to give up and do all the auth in standard handler-land :(

[adlaika]

Ah, I've just come across @chshersh's comment in haskell-servant/servant#1120, which seems to confirm my suspicions.

[kahlil29]

@adlaika Thanks for making an attempt at making the change non-breaking, appreciate it.
Regarding the issue of Request Body, I see you've already got your answer.

However, I can confirm that we recently hit this issue while trying to implement Webhook handlers for incoming webhooks from Sendbird and/or Stripe to our Haskell backend server for one of our projects.

I believe we've used the same approach outlined by Dmitrii (or someone else) that involves freezing the request body if the request matches a certain condition (or depending on the endpoint being called) and we have a working solution in place. I'm not sure if it's the most optimal though. @thongpv87 has implemented it so he could confirm and give some inputs here 👍🏻

[michivi]

@adlaika There is a current attempt at signing the content as well in #57. It is a functional implementation but as noted, we still have the issue of content consumption. If you do not use streaming and if you know the size of the payloads in advance and if it can fit in memory, then the solution should be working for you. But the authentication header is still hard coded in this variant though.

[adlaika]

Due to time and business constraints, I ended up implementing our Kard web hook auth logic in a non-Context handler (i.e. business logic)--their HMAC implementation simply uses the minified JSON of the request, making it easy to reconstitute from the parsed data Servant provides. At some point in the future, I'd like to take a stab at reconciling this with #57, but I don't think I'm going to have time for awhile.

In the meantime, the code in this PR is non-breaking and adds value, so you're welcome to merge if it pleases you :)

[thongpv87]

@kahlil29 @adlaika We have implemented this solution to work around the issue