Skip to content
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

Support DingTalk robot, add sign security settings #1485

Merged
merged 7 commits into from
Jul 5, 2024

Conversation

innerpeacez
Copy link
Contributor

@innerpeacez innerpeacez commented Jul 3, 2024

Description

Checklist

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

Questions or Comments

@jertel
Copy link
Owner

jertel commented Jul 4, 2024

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
Copy link
Contributor Author

All done. Thx for your feedback.

@jertel
Copy link
Owner

jertel commented Jul 4, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

jertel commented Jul 5, 2024

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.

@jertel jertel merged commit c7b8ff9 into jertel:master Jul 5, 2024
1 check passed
@innerpeacez innerpeacez deleted the support-dingtalk-sign-send branch July 8, 2024 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants