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

ZeroMQ publisher plugin #70

Merged
merged 1 commit into from
Dec 16, 2019
Merged

ZeroMQ publisher plugin #70

merged 1 commit into from
Dec 16, 2019

Conversation

jarret
Copy link
Contributor

@jarret jarret commented Dec 1, 2019

A plugin that subscribes to notification and passes them through to be published to ZeroMQ endpoints. It utilizes Twisted and txZMQ to facilitate the publication. An additional script is provided to serve as an example for how to subscribe to the events.

The presentation was chosen to follow how bitcoind sets up its ZMQ publishing functionality.

NOTE: The plugin source file is given the name cl-zmq.py as opposed to zmq.py because the package dependencies import the official ZeroMQ python packages via from zmq import ... which is a namespace conflict with a source file named zmq.py.

Copy link
Contributor

@cdecker cdecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice plugin, there is just the use of exec() which raises a couple of red flags to me. See comment for how you could work around that using native tools :-)

zmq/cl-zmq.py Outdated
for nt in NOTIFICATION_TYPES:
# subscribe to all notifications
subscribe = SUBSCRIBE_TEMPLATE.format(nt, nt, nt, nt)
exec(subscribe)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems rather dangerous. I'd suggest creating a generic handler, use functools.partial to generate variants with the notification type bound and then using plugin.add_subscription to register them.

Something like the following would do I think:

def on_notification(notification_type, plugin, *args, **kwargs):
    if len(args) != 0:
        plugin.log("got unexpected args: {}".format(args), level="warn")
    reactor.callFromThread(publisher.publish_notification, "{}",
                           *args, **kwargs)

for nt in NOTIFICATION_TYPES:
    f = functools.partial(on_notification, nt)
    plugin.add_subscription(nt, f)

This would not call the python interpreter with a variable payload. In this case it's likely ok with your mechanism, since it just fills in the notification types, but in other cases this could be a security vulnerability since you execute code that was provided from outside.

@jarret
Copy link
Contributor Author

jarret commented Dec 11, 2019

Great suggestion. Defining code in a string also is kind of ugly and functools.partial() appears to be suited.

One issue I bumped into in the revised implementation is that Plugin._coerce_arguments() expects the callable object to have the __annotations__ attribute set, for which the partial object does not. To fix, it had to be manually set to an empty dict:

on.__annotations__ = {} # needed to please Plugin._coerce_arguments()

@cdecker
Copy link
Contributor

cdecker commented Dec 11, 2019

Good catch, I'll add a check that should make that no longer necessary.

@cdecker
Copy link
Contributor

cdecker commented Dec 16, 2019

I have patched pyln-client to check for the existence of __annotations__ before accessing it here ElementsProject/lightning@dfa7f48. Sadly it didn't make the v0.8.0 release window, so it'll be available from 0.8.1 onwards :-)

@cdecker cdecker merged commit ec65c87 into lightningd:master Dec 16, 2019
@cdecker
Copy link
Contributor

cdecker commented Dec 16, 2019

It might be interesting to write a couple of tests by the way so that we don't accidentally break compatibility while working on c-lightning 😉

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