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

Authenticate Mandrill Webhook Requests #3

Merged
merged 3 commits into from Sep 15, 2013

Conversation

Projects
None yet
2 participants
@zshannon
Contributor

zshannon commented Sep 15, 2013

I added support for authenticating Mandrill's webhook requests using the protocol described here: http://help.mandrill.com/entries/23704122-Authenticating-webhook-requests

I bumped the version number (separate commit) to account for this improvement, and described how to use it in the README.

I did not add tests because I could not readily determine a test that would be sufficiently orthogonal to the functionality.

tardate added a commit that referenced this pull request Sep 15, 2013

@tardate tardate merged commit 19e0972 into evendis:master Sep 15, 2013

1 check failed

default The Travis CI build could not complete due to an error
Details
@tardate

This comment has been minimized.

Member

tardate commented Sep 15, 2013

Thanks for this! I'd always been a bit concerned about the vulnerability of the webhook endpoints but hadn't caught up with the support that Mandrill now have for authenticating webhook requests.

I've merged in and added a very basic spec to verify the sig generation.

I think we'll probably need to change the configuration of the webhook keys, because we may not have the same key for different requests (in one app I have event callbacks using one key, and inbound mail processing using another). I'm going to have a quick look at that now before publishing a new gem version.

tardate added a commit that referenced this pull request Sep 15, 2013

\#3 refactor webhook authentication support
* NB: a new method for configuring the web hook api keys

tardate added a commit that referenced this pull request Sep 15, 2013

@tardate

This comment has been minimized.

Member

tardate commented Sep 15, 2013

@zshannon note my refactor to handle multiple API keys. I also changed the way to configure the keys. Does this work for you? Let me know if you have any comments before I publish a new gem version (I'll probably do that +24hrs unless you have any further input).

@zshannon

This comment has been minimized.

Contributor

zshannon commented Sep 15, 2013

@tardate Handling multiple keys is nice! I would advise changing the helper method to one that ends in '!' to better betray that normal execution will stop if the signature is not verifiable. Also, it would be better if, in the case of multiple keys, keys were tied to specific events. Would you like me to mock that up in a pull request?

@zshannon zshannon deleted the smileslaughs:authenticate-mandrill branch Sep 15, 2013

@tardate

This comment has been minimized.

Member

tardate commented Sep 16, 2013

@zshannon Sure, happy took look at any other ideas you have. Have you been able to test and prove that this is working for you already?

NB: I initially thought about configuring keys per message type, but then realised it's possible (though not sure how likely) to have more than one key for a message type. I'm also not sure what Mandrill does about batching messages that would normally have different API keys if sent individually. So I took the simple way out and said "just give us all your keys and we'll see which works";-) From a security perspective, I don't believe there is any significant diminution of protection, as it only changes the probability of compromise from 1 in some billions to 2 or 3 in some billions.

@zshannon

This comment has been minimized.

Contributor

zshannon commented Sep 16, 2013

@tardate Have been testing all along in development using Forward, and am running the branch I created this pull request from in production, both with no issues. Agreed on the security implications of leaving the keys without message type scopes. I'll create a new pull request in a moment with the modified syntax I proposed above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment