Implement webhook plugin #104

sudowork · 2021-03-31T04:59:30Z

This plugin provides functionality to trigger a webhook after a shot has completed.

Provides the following settings:
webhook_domain
webhook_endpoint
webhook_secret_key
webhook_trigger_after

Sends an HTTP webhook to:

POST https://$webhook_domain$webhook_endpoint
Authorization: $clock!$hmac_signature

$body

The HMAC signature is used so your server can verify the authenticity of a
message. The signature is created from:
HMAC($webhook_secret_key, "$path\n$clock\n$body")

Mimoja · 2021-03-31T11:16:53Z

Maybe it should be intergrated into the web_api plugin (or the other way round?) and the callback paths can be set from the outside that way?

jeffsf · 2021-04-01T02:46:44Z

As a general comment, spurred by this code, not targeting this code specifically, and remembering that I'm just a customer, not even a committer here, I am hoping to get some clarity from @decentjohn around a policy for third-party plugins as to how they are hosted and delivered.

On a more specific note, I agree with @Mimoja that unification of various, related services is a good idea. I think the existing plugin would benefit from a going over around security. One open port is, for some, already one port too many.

sudowork · 2021-04-01T03:05:47Z

Yes good question. I wasn’t really sure what the contribution policy was for third party plugins. Happy to take a look at the web_api plugin and see if it makes sense to merge in there, though it may actually be spiritually closer to the visualizer upload.

Also I don’t really mind if this never gets merged in. It’s probably somewhat bespoke.

decentjohn · 2021-04-01T03:12:22Z

I'm ok with any open port that defaults to localhost. To change it to an external IP, the user has to do something, and thus they take on that liability. That's a fairly common scheme I've seen. So.. bind() needs to be set to 127.0.0.1 as a default for anything that takes communication from the outside.

jeffsf · 2021-04-01T02:48:44Z

de1plus/plugins/webhook/plugin.tcl

+            array set ::plugins::webhook::settings {
+                webhook_domain "example.com"
+                webhook_endpoint "/decent/webhook"
+                webhook_secret_key "SecretKeyForSigning"


You can't have a default password on a device that is sold in California unless it is device specific.

ref: https://leginfo.legislature.ca.gov/faces/billTextClient.xhtml?bill_id=201720180SB327

Edit: This may not be an incoming password, but a signing secret.

Yes, this is just a signing secret for an outbound request. The signature is then attached as a header to the outbound request.

why not make the password blank, and required to be created by the user before the plugin will work?

jeffsf · 2021-04-01T02:50:56Z

de1plus/plugins/webhook/plugin.tcl

+        msg "triggering webhook"
+        borg toast [translate "Triggering Webhook"]
+
+        http::register https 443 [list ::tls::socket -servername $settings(webhook_domain)]


The docs might want to mention how to install a server certificate

Getting into how to generate one can probably be skipped, with "search for 'self-signed certificate'" or something appropriate.

If this is just sending POSTs, why is it opening a socket?

This may be my misunderstanding, but I don’t believe this is binding anything/ opening a socket for listening. Based on my read of the docs, it’s used to establish a TLS transport for making the HTTPS request to the server: https://www.tcl-lang.org/man/tcl/TclCmd/http.htm#M49
https://wiki.tcl-lang.org/page/HTTPS

yes that is correct, there is no listen there, that code is registering essentially an 'extension' to a socket. This is how https GET is implemented on tcl.

jeffsf · 2021-04-01T02:53:52Z

de1plus/plugins/webhook/plugin.tcl

+        http::register https 443 [list ::tls::socket -servername $settings(webhook_domain)]
+
+        # Craft HTTP Body
+        set espresso_data [format_espresso_for_history]


This can take over a second to generate on a v1.3 stock tablet with a long shot. I've seen scale timeouts at one second with just the core app and things like the flow-cal profile. Though you've got this on the idle queue and after a delay, it can cause the BLE stack to back up and potentially lose packets.

This would have it running potentially three times; core app, visualiser upload, and web hook

Good call. For my particular use case, I don’t actually require the history, so I’m fine with actually removing it unless someone finds it useful.

jeffsf · 2021-04-01T02:56:03Z

de1plus/plugins/webhook/README.md

+               ^ clock    ^ HMAC signature
+```
+
+Where `1617092331` represents the clock at the time of the shot, and `f728a8092b6882749efa61a6609414d14c6f25286bdb1fd76e0dda4bab0f0cdf` is the calculated HMAC signature. Note that they are delimited by the character `!`.


...clock at the time the payload generation began...

Why something predictable here? If this is just confirming that the sender and the receiver share the same secret, why not just a random number?

Clock is used as a way to have a time-based component to the signature to mitigate replay attacks (really probably overkill for this). On the server receiving the request, it can verify that the timestamp is within some interval of the server time.

OK, reasoning makes sense to me now

jeffsf · 2021-04-01T02:58:43Z

de1plus/plugins/webhook/plugin.tcl

+    proc post_shot_data {} {
+        variable settings
+
+        msg "triggering webhook"


On msg, please set severity codes. We're working on getting the logs down to the point where they can be meaningfully run by everyone, all the time, especially to help out the support team.

See https://github.com/decentespresso/de1app/blob/main/de1plus/logging.tcl#L28

jeffsf · 2021-04-01T03:02:57Z

de1plus/plugins/webhook/plugin.tcl

+        after [expr 1000 * $::plugins::webhook::settings(webhook_trigger_after)] ::plugins::webhook::post_shot_data
+    }
+
+    proc msg { msg } {


You can go straight to ::logging::default_logger

https://github.com/decentespresso/de1app/blob/main/de1plus/logging.tcl#L6

jeffsf · 2021-04-01T03:04:31Z

de1plus/plugins/webhook/plugin.tcl

+        borg toast "Webhook succeeded!"
+    }
+
+    proc async_dispatch {old new} {


The {old new} arg list was from the previous system that only triggered on major state changes.

As you're not doing anything with the states (yet), {event_dict} is what the event will be called with.

jeffsf · 2021-04-01T03:06:24Z

de1plus/plugins/webhook/plugin.tcl

+
+    proc main {} {
+        ::de1::event::listener::after_flow_complete_add \
+            [lambda {event_dict} {


See note above at Line 113

If your callback accepts {event_dict} the wrapper for old-style callbacks is not needed.

jeffsf · 2021-04-01T03:12:33Z

de1plus/plugins/webhook/plugin.tcl

+    }
+
+    proc main {} {
+        ::de1::event::listener::after_flow_complete_add \


package require wibble ??

Definitely should not be in the outer body or file, as it causes loading of the library even if the plugin is disabled.

There's some complexity here, looking back at #73

Is this comment meant for another PR?

I had thought you were opening a server, which I think people do with the wibble library. Though installed with AndroWish, it isn't part of the stock distro. ::http is shown as part of the standard distro, so you don't need wibble.

jeffsf · 2021-04-01T03:19:10Z

Please don't take the comments as a lack of appreciation for the work!

I'm picky. If I didn't think the concept was worth pursuing, I wouldn't have taken the time.

decentjohn · 2021-04-01T05:43:35Z

Definitely, if you're doing a API, use ::http not wibble. Simpler is better.

Provides the following settings: webhook_domain webhook_endpoint webhook_secret_key webhook_trigger_after Sends an HTTP webhook to: ``` POST https://$webhook_domain$webhook_endpoint Authorization: $clock!$hmac_signature $body ``` The HMAC signature is used so your server can verify the authenticity of a message. The signature is created from: `HMAC($webhook_secret_key, "$path\n$clock\n$body")`

jeffsf reviewed Apr 1, 2021

View reviewed changes

Mimoja force-pushed the main branch from 3ee38b2 to 29a690e Compare April 9, 2021 20:58

sudowork force-pushed the webhook-plugin branch from d3bfc76 to 382be38 Compare April 29, 2021 16:02

Use ::shot::create API

7b81514

sudowork force-pushed the webhook-plugin branch from 382be38 to 7b81514 Compare April 29, 2021 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement webhook plugin #104

Implement webhook plugin #104

sudowork commented Mar 31, 2021 •

edited

Loading

Mimoja commented Mar 31, 2021

jeffsf commented Apr 1, 2021

sudowork commented Apr 1, 2021

decentjohn commented Apr 1, 2021

jeffsf Apr 1, 2021

sudowork Apr 1, 2021

decentjohn Apr 1, 2021

jeffsf Apr 1, 2021

sudowork Apr 1, 2021 •

edited

Loading

decentjohn Apr 1, 2021

jeffsf Apr 1, 2021

sudowork Apr 1, 2021

jeffsf Apr 1, 2021

sudowork Apr 1, 2021

jeffsf Apr 1, 2021

jeffsf Apr 1, 2021

sudowork Apr 1, 2021

jeffsf Apr 1, 2021

jeffsf Apr 1, 2021 •

edited

Loading

jeffsf Apr 1, 2021

jeffsf Apr 1, 2021

sudowork Apr 1, 2021

jeffsf Apr 1, 2021

jeffsf commented Apr 1, 2021

decentjohn commented Apr 1, 2021

Implement webhook plugin #104

Are you sure you want to change the base?

Implement webhook plugin #104

Conversation

sudowork commented Mar 31, 2021 • edited Loading

Mimoja commented Mar 31, 2021

jeffsf commented Apr 1, 2021

sudowork commented Apr 1, 2021

decentjohn commented Apr 1, 2021

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

sudowork Apr 1, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffsf Apr 1, 2021 • 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

jeffsf commented Apr 1, 2021

decentjohn commented Apr 1, 2021

sudowork commented Mar 31, 2021 •

edited

Loading

sudowork Apr 1, 2021 •

edited

Loading

jeffsf Apr 1, 2021 •

edited

Loading