Support DingTalk robot, add sign security settings

jertel / elastalert2

ElastAlert 2 is a continuation of the original yelp/elastalert project. Pull requests are appreciated!

https://elastalert2.readthedocs.org

Apache License 2.0

859 stars 277 forks source link

Support DingTalk robot, add sign security settings #1485

Closed innerpeacez closed 2 weeks ago

innerpeacez commented 2 weeks ago

Description

Checklist

[x] I have reviewed the contributing guidelines.
[x] I have included unit tests for my changes or additions.
[x] I have successfully run make test-docker with my changes.
[x] I have manually tested all relevant modes of the change in this PR.
[x] I have updated the documentation.
[x] I have updated the changelog.

Questions or Comments

jertel commented 2 weeks ago

Thanks for your PR. I plan to release the next version in about 2 weeks. If you'd like this PR to be included please ensure the checklist items are completed before mid July.

innerpeacez commented 2 weeks ago

All done. Thx for your feedback.

jertel commented 2 weeks ago

The changelog entry needs to be in the correct format. It's mentioned in the CHANGELOG.md. Should be a < 1 minute change.

The schema.xml file needs updated with the new dingtalk_sign parameter. Should take just a few minutes to review the existing file structure and make the change.

The sign() method isn't fully unit tested. You could use mocks to add more coverage there. It's not critical but could avoid problems down the road if other contributors ever adjust that signing logic.

innerpeacez commented 2 weeks ago

The changelog entry needs to be in the correct format. It's mentioned in the CHANGELOG.md. Should be a < 1 minute change.

The schema.xml file needs updated with the new dingtalk_sign parameter. Should take just a few minutes to review the existing file structure and make the change.

The sign() method isn't fully unit tested. You could use mocks to add more coverage there. It's not critical but could avoid problems down the road if other contributors ever adjust that signing logic.

done

jertel commented 2 weeks ago

Thanks for your PR. I added some clarifications to the documentation on this new parameter. I find it curious that the algorithm expects only the HMAC secret and the timestamp to be used in the signing algorithm. This would seem to limit the value of this HMAC feature, as it doesn't allow the receiver to verify the integrity of the accompanying message, but rather only verifies that a sender knew the HMAC secret and that it was sent within the past hour. Therefore a man in the middle attack could change the content of the message itself but the receiver would still see a valid HMAC signature.