Implement webhook validation #279

mickmister · 2022-08-11T17:53:18Z

Summary

Viewing the second commit in isolation will make reviewing this PR much more straightforward, since I moved around some code in the first commit.

Description copied from #278:

Zoom has changed how their webhook setup works, and requires some logic on our end to validate the webhook is setup properly. This involves:

Validating the webhook during setup
Verifying the webhook's signature during webhook events

The webhook must be validated before Zoom's interface will allow the admin to save the webhook configuration in Zoom's app setup UI. This does not affect existing setups of the plugin, but it will affect any new setup, and any instance of an admin wanting to update the webhook secret or change the events associated with the webhook configuration.

Setting up the webhook for validation requires us to store a new Secret Token from Zoom's webhook configuration UI. If this value is not set, we simply skip the "signature verification" step when processing webhook events. We technically don't need to do this step, because Zoom has no way of verifying this is occurring on subsequent webhook events, and because we are already using our own secret for the webhook. Either way, it's best to comply with what Zoom is asking for, and it adds another layer of security provided by Zoom.

Ticket Link

Fixes #278

server/webhook.go

+	if !strings.Contains(r.Header.Get("Content-Type"), "application/json") {
+		res := fmt.Sprintf("Expected Content-Type 'application/json' for webhook request, received '%s'.", r.Header.Get("Content-Type"))
+		p.API.LogWarn(res)
+		http.Error(w, res, http.StatusBadRequest)


codecov · 2022-08-15T14:02:21Z

Codecov Report

Patch coverage: 58.38% and project coverage change: +23.51 🎉

Comparison is base (cb15d74) 0.00% compared to head (47712f6) 23.51%.

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #279       +/-   ##
===========================================
+ Coverage    0.00%   23.51%   +23.51%     
===========================================
  Files           8        9        +1     
  Lines        1143     1212       +69     
===========================================
+ Hits            0      285      +285     
+ Misses       1143      874      -269     
- Partials        0       53       +53

Impacted Files	Coverage Δ
server/configuration.go	`25.24% <ø> (+25.24%)`	⬆️
server/http.go	`20.00% <0.00%> (+20.00%)`	⬆️
server/webhook.go	`59.11% <59.11%> (ø)`

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

DHaussermann · 2022-08-15T20:12:31Z

This seems functional I can see the hook working when It's deployed. However the validation functionality seems to now be removed from the Zoom so I can't test the API change.

Also it's a bit odd that we're keeping the webhook secret in the query string. We would need to be very clear with the documentation about this but it is working 👍

DHaussermann · 2022-08-15T20:12:53Z

cc @mickmister ☝️

mickmister · 2022-08-15T20:57:46Z

@DHaussermann I kept the webhook secret in the query string because it is something we have control over and is battle tested in our system. We can treat it as the primary secret check, while adhering to Zoom's new requirements when they take effect. This also was the simplest route for backwards compatibility.

mickmister · 2022-11-08T18:43:51Z

@DHaussermann Can you please test this for backwards compatibility? Thank you!

catalintomai · 2023-01-26T13:32:43Z

cc: @DHaussermann , when you have the bandwidth (or have Malik looking into it).

DHaussermann · 2023-01-27T00:34:16Z

/update-branch

trilopin

There's some work here :)

I left some comments, the only ones I consider blocking are the double-deserialization ones.

~~On the other hand, I had some problems doing manual QA here but it's probably my fault.~~ Also manually tested 🎉 .

trilopin · 2023-03-06T10:05:54Z

plugin.json

+                "key": "ZoomWebhookSecret",
+                "display_name": "Zoom Webhook Secret:",
+                "type": "text",
+                "help_text": "`Secret Token` taken from Zoom's webhook configuration page",


is there a followup jira task/PR to update the documentation with this step?

trilopin · 2023-03-06T10:09:52Z

server/configuration.go

 	EncryptionKey     string
 	WebhookSecret     string
+	ZoomWebhookSecret string


Suggested change

ZoomWebhookSecret string

// ZoomWebhookSecret is the secret taken from Zoom's webhook configuration page

ZoomWebhookSecret string

2/5 I know that this is a bit repetitive but it helps a lot to disambiguate when seeing at WebhookSecret vs ZoomWebhookSecret as conf keys.

I'd add the docstring at least in both secret fields.

trilopin · 2023-03-06T10:14:43Z

server/webhook.go

+
+func (p *Plugin) handleWebhook(w http.ResponseWriter, r *http.Request) {
+	if !p.verifyWebhookSecret(r) {
+		p.API.LogWarn("Could not verify webhook secret")


0/5 if it's easy I'd return a different log/string-error for secrets that are empty or exactly "WEBHOOKSECRET" (people copying exactly the doc).

We actually enforce the value to be defined on plugin startup, so no need to have that condition handled here

server/webhook.go

trilopin · 2023-03-06T10:24:51Z

server/webhook.go

+		p.API.LogError("Error unmarshaling webhook", "err", err.Error())
+		http.Error(w, err.Error(), http.StatusBadRequest)
+		return
+	}


thoughts on merging the read block and unmarshal block?

if err := json.NewDecoder(r.Body).Decode(&webhook); err != nil { // log http.Error(w, err.Error(), http.StatusBadRequest) return }

I think this is written the way that it is because we would essentially be "discarding" the HTTP body that would need to be re-read if json.NewDecoder was used.

server/webhook.go

trilopin

Thank you!

mickmister · 2023-03-10T20:00:45Z

server/http.go

 func (p *Plugin) deauthorizeUser(w http.ResponseWriter, r *http.Request) {
-	if !p.verifyWebhookSecret(r) {
+	if !p.verifyMattermostWebhookSecret(r) {
 		http.Error(w, "Not authorized", http.StatusUnauthorized)
 		return
 	}


After renaming this called method and durfaced this change, it seems odd to me that we are using a webhook secret for a deauthorizeUser operation. I'm not sure if this route gets called

It's a feature of the plugin that uses the webhooks secret:

mattermost-plugin-zoom/docs/installation/zoom-configuration/zoom-setup-oauth.md

Lines 44 to 53 in 229517f

## Deauthorization

This plugin allows users to be deauthorized directly from Zoom, in order to comply with Zoom’s commitment to security and the protection of user data. If you **would like to publish on Zoom Marketplace**, you can set up a deauthorization URL.

1. Select **Information**.

2. Near the end of the page, is a section called **Deauthorization Notification**.

3. Enter a valid **Endpoint URL** \(`https://SITEURL/plugins/zoom/deauthorization?secret=WEBHOOKSECRET`\).

* `SITEURL` should be your Mattermost server URL.

* `WEBHOOKSECRET` is generated during [Mattermost Setup](../mattermost-setup.md).

We aren't checking the verification of Zoom's signature in this case. I don't think this is worth holding up this PR though.

agree on doing that in a separate effort, pls create a Jira ticket for that and add a comment with the jira link close to the related code

Created ticket here #291

Thanks! Could you please add a comment to deauthorizeUser with the link to the ticket?

DHaussermann · 2023-03-22T21:43:46Z

@mickmister I do not see a change in behavior when running this build.
Webhook end point validation is still failing in the Zoom Setup wizard.

I double checked the URL and secret. I'm a bit confused....

I assume the path where webhook is exposed is not changing?
So what path is the Zoom wizard checking and is there a way I can check it manually with the plugin running? If I could confirm the change with CURL or Postman it will help isolate what's happening here.
Probably not important but I get an older version stamp when I build this ☝️

mickmister · 2023-03-23T20:49:32Z

@DHaussermann I tested this out again and it's working for me. Can we meet to discuss this?

DHaussermann

Tested and passed

Webhooks are working
Webhook validation wizard is working
Dev provided doc changes for new secret value
Tested on Zoom upgrade and Fresh install
Tested on new Zoom app and editing features of an existing Zoom app

LGTM!

* move webhook handlers to its own file * implement webhook validation * lint * return useful error message * document usage of zoom's webhook secret * PR feedback * rename methods for readability * add todo comment --------- Co-authored-by: Mattermost Build <[email protected]>

* bump version to 1.6.2 * update github actions * .nvmrc * golangci * Implement webhook validation (#279) * move webhook handlers to its own file * implement webhook validation * lint * return useful error message * document usage of zoom's webhook secret * PR feedback * rename methods for readability * add todo comment --------- Co-authored-by: Mattermost Build <[email protected]> * fix merge issues * go mod tidy * [MM-51751] Fix regex for tags trigger in CI/CD (#294) --------- Co-authored-by: Mattermost Build <[email protected]> Co-authored-by: Ben Schumacher <[email protected]>

mickmister added 2 commits August 10, 2022 20:27

move webhook handlers to its own file

7fb68d3

implement webhook validation

58ad0fa

mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Aug 11, 2022

mickmister requested review from DHaussermann and catalintomai August 11, 2022 17:53

mickmister requested a review from larkox as a code owner August 11, 2022 17:53

github-advanced-security bot found potential problems Aug 11, 2022

View reviewed changes

larkox approved these changes Aug 12, 2022

View reviewed changes

lint

c47f8d7

return useful error message

895bacb

DHaussermann mentioned this pull request Aug 15, 2022

Release 1.6.1 #275

Merged

Merge branch 'master' into webhook-validation

0118b52

mattermost-build and others added 2 commits January 27, 2023 02:34

Merge branch 'master' into webhook-validation

4751e59

Merge branch 'master' into webhook-validation

b602ac0

hanzei requested review from trilopin and srkgupta and removed request for catalintomai, trilopin and srkgupta February 21, 2023 09:58

trilopin reviewed Mar 6, 2023

View reviewed changes

trilopin approved these changes Mar 7, 2023

View reviewed changes

mickmister added 3 commits March 8, 2023 15:31

document usage of zoom's webhook secret

b923042

Merge branch 'master' into webhook-validation

bc5a37a

PR feedback

58d80a2

mickmister requested a review from trilopin March 8, 2023 20:37

rename methods for readability

229517f

mickmister commented Mar 10, 2023

View reviewed changes

trilopin approved these changes Mar 20, 2023

View reviewed changes

mickmister mentioned this pull request Mar 20, 2023

Investigate whether webhook verification should be used on deauthorize route #291

Closed

mickmister added 2 commits March 22, 2023 10:49

add todo comment

49161f5

Merge branch 'master' into webhook-validation

47712f6

mickmister removed the 2: Dev Review Requires review by a core committer label Mar 22, 2023

DHaussermann approved these changes Mar 27, 2023

View reviewed changes

DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Mar 27, 2023

mickmister merged commit 6331608 into master Mar 27, 2023

mickmister deleted the webhook-validation branch March 27, 2023 21:49

mickmister mentioned this pull request Mar 27, 2023

Release 1.6.2 #292

Merged

This was referenced Nov 14, 2023

Bump version v2.0.0 (closed, renamed to v1.7.0) #321

Closed

Bump version v1.7.0. #322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement webhook validation #279

Implement webhook validation #279

mickmister commented Aug 11, 2022

codecov bot commented Aug 15, 2022 •

edited

Loading

DHaussermann commented Aug 15, 2022

DHaussermann commented Aug 15, 2022

mickmister commented Aug 15, 2022

mickmister commented Nov 8, 2022

catalintomai commented Jan 26, 2023

DHaussermann commented Jan 27, 2023

trilopin left a comment •

edited

Loading

trilopin Mar 6, 2023

trilopin Mar 6, 2023

trilopin Mar 6, 2023

mickmister Mar 8, 2023

trilopin Mar 6, 2023

mickmister Mar 8, 2023

trilopin left a comment

mickmister Mar 10, 2023

mickmister Mar 10, 2023 •

edited

Loading

trilopin Mar 20, 2023 •

edited

Loading

mickmister Mar 20, 2023

trilopin Mar 20, 2023

DHaussermann commented Mar 22, 2023

mickmister commented Mar 23, 2023

DHaussermann left a comment

	ZoomWebhookSecret string
	// ZoomWebhookSecret is the secret taken from Zoom's webhook configuration page
	ZoomWebhookSecret string

	## Deauthorization

	This plugin allows users to be deauthorized directly from Zoom, in order to comply with Zoom’s commitment to security and the protection of user data. If you would like to publish on Zoom Marketplace, you can set up a deauthorization URL.

	1. Select Information.
	2. Near the end of the page, is a section called Deauthorization Notification.
	3. Enter a valid Endpoint URL \(`https://SITEURL/plugins/zoom/deauthorization?secret=WEBHOOKSECRET`\).
	* `SITEURL` should be your Mattermost server URL.
	* `WEBHOOKSECRET` is generated during [Mattermost Setup](../mattermost-setup.md).

Implement webhook validation #279

Implement webhook validation #279

Conversation

mickmister commented Aug 11, 2022

Summary

Ticket Link

codecov bot commented Aug 15, 2022 • edited Loading

Codecov Report

DHaussermann commented Aug 15, 2022

DHaussermann commented Aug 15, 2022

mickmister commented Aug 15, 2022

mickmister commented Nov 8, 2022

catalintomai commented Jan 26, 2023

DHaussermann commented Jan 27, 2023

trilopin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trilopin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

trilopin Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHaussermann commented Mar 22, 2023

mickmister commented Mar 23, 2023

DHaussermann left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 15, 2022 •

edited

Loading

trilopin left a comment •

edited

Loading

mickmister Mar 10, 2023 •

edited

Loading

trilopin Mar 20, 2023 •

edited

Loading