-
Notifications
You must be signed in to change notification settings - Fork 42
Add HMAC-SHA256 signature verification for HTTP webhook shovels #1423
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
base: main
Are you sure you want to change the base?
Conversation
|
I have read the CLA Document and I hereby sign the CLA 0 out of 2 committers have signed the CLA. |
src/lavinmq/shovel/shovel.cr
Outdated
| # Note: This requires reading the entire body into memory to compute HMAC. | ||
| # For webhook use cases, messages are typically small event notifications. |
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.
Because of this there should probably be a (configurable?) max size.
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.
💯
src/lavinmq/shovel/shovel.cr
Outdated
| if secret = @signature_secret | ||
| body = msg.body_io.gets_to_end | ||
| signature = OpenSSL::HMAC.hexdigest(OpenSSL::Algorithm::SHA256, secret, body) | ||
| headers["X-LavinMQ-Signature-256"] = "sha256=#{signature}" |
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.
Why is it called "LavinMQ-Signature-256"? it's nothing "lavinmq" about it, and sha256 is already in the value, why not just X-HMAC-Signature?
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.
Ok, looking on other implementations of this I see that there's no real standard, hence the product name in the header, and then you have to look up the docs for that header. Well, then either 256 in the key or the sha256= in the value should at least go away.
I also liked that slack included the date/time in the signature to prevent reply attacks. Have we considered that? (But instead of adding a custom time/date header, why not just use the standard Date header?)
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.
Key rotation is also not supported, as PagerDuty does
src/lavinmq/shovel/shovel.cr
Outdated
| # Note: This requires reading the entire body into memory to compute HMAC. | ||
| # For webhook use cases, messages are typically small event notifications. |
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.
💯
WEBHOOK_SIGNATURE_VERIFICATION.md
Outdated
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.
all caps in the filename doesn't align with how we normally name files, and should it be in the root dir?
Could it be shorter? It's very verbose right now, maybe works better as a blog post. And just short summary/note in the README instead?
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.
+1 short note in README + blog post
Co-authored-by: baelter <[email protected]>
Co-authored-by: baelter <[email protected]>
Co-authored-by: baelter <[email protected]>
Co-authored-by: baelter <[email protected]>
Co-authored-by: baelter <[email protected]>
Switch from custom X-LavinMQ-Signature-256 header to Standard Webhooks
specification (standardwebhooks.com) format with three headers:
- webhook-id: Unique message ID (msg_<uuid>)
- webhook-timestamp: Unix timestamp in seconds
- webhook-signature: HMAC-SHA256 as v1,<base64>
Signature now covers "{id}.{timestamp}.{body}" which enables replay
attack prevention through timestamp validation.
49a3c63 to
bcb1d82
Compare
|
Claude finished @copilot's task —— View job Code Review AnalysisReviewing PR for problems that need fixing:
Issues Found1. Timing Attack Vulnerability -
|
Remove standalone WEBHOOK_SIGNATURE_VERIFICATION.md and add a short mention of Standard Webhooks signature support in the README features list. Detailed documentation will be covered in a blog post.
- Add DEFAULT_MAX_SIGNED_WEBHOOK_PAYLOAD constant (16MB) in shovel/constants.cr - Add max_signed_webhook_payload config option with INI parsing support - Add AMQPSource#reject method for properly rejecting messages without requeue - Add payload size validation in HTTPDestination before reading into memory - Messages exceeding limit are logged and rejected without requeue - Add comprehensive test coverage for payload size validation - Fix default_user_only_loopback property definition (remove ?) - Fix generate_timestamp to use integer division (//)
940c20d to
376a484
Compare
376a484 to
d278508
Compare
Add webhook_server helper method to ShovelSpecHelpers that: - Creates and starts an HTTP server for testing webhooks - Captures headers, body, and path from requests - Supports both String and Channel for body (for async tests) - Returns server, address, and captured values as tuple Replace 5 duplicated HTTP server setup blocks with helper calls.
HTTP shovels can now sign webhook requests with HMAC-SHA256, enabling endpoints to verify request authenticity. Compatible with GitHub/PagerDuty signature formats.
Implementation
signature_secretparameter, computes HMAC-SHA256 of body and addsX-LavinMQ-Signature-256: sha256=<hex>headerdest-signature-secretfrom configurationConfiguration
{ "src-uri": "amqp://localhost", "src-queue": "events", "dest-uri": "https://example.com/webhook", "dest-signature-secret": "my-secret-key" }Verification Example (Python)
Additional verification examples for Node.js and Go included in
WEBHOOK_SIGNATURE_VERIFICATION.md.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
moment.github.iocurl --fail --retry 5 -sLo static/js/lib/luxon.js REDACTED(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.