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

Don't pass on zmq identities #10

Closed
rgbkrk opened this issue Jan 13, 2016 · 14 comments
Closed

Don't pass on zmq identities #10

rgbkrk opened this issue Jan 13, 2016 · 14 comments

Comments

@rgbkrk
Copy link
Contributor

rgbkrk commented Jan 13, 2016

jmp.Message currently has zmq identities as part of its object (which is a Buffer as well). These should only get used as part of iopubSocket.subscribe(ident) and should not end up being something as part of the message since it's not part of the main JSON payload.

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Jan 13, 2016

Same goes for message.signatureOK.

@n-riesco
Copy link
Owner

In other words, the question is whether Message.idents, Message.signatureOK and Message.blobs could be removed. Here are my thoughts:

  • Message.idents: I really think this property belongs to a Message object. In fact, that's the way it is in ZMQ. I don't like to use authoritative arguments, so I'll try to explain further why:
    • A ZMQ identity "identifies" a connection between two sockets. In general, it can't be used to identify a socket, because unlike traditional sockets, which are limited to 1-to-1 connections, ZMQ sockets can handle 1-to-N connections.
    • Message.idents is defined as an array of ZMQ identities listing all the connections the message has travelled through. This array helps route any replies back to the message creator.
    • I think the argument about subscribe is actually a misunderstanding. It should be iopubSocket.subscribe(topic). See this example.
  • Message.signatureOK: What could be the alternative?
    • A private property Message._signature to store the signature and a method Message#isSignatureOK?
    • Or perhaps a visible Message.signature?
  • Message.blobs: It is defined in the standard. Message.blobs is handy to extend Jupyter messages, but Message.metadata can be used for that too. Do you know if there are any kernels using Message.blobs?

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Jan 14, 2016

Maybe this means I just copy the payload over or just delete these properties before sending on. I'm mostly aiming at compatibility of the raw messages with what the websocket payload ends up being. Blobs should stay, I think I just need to convert them to one standard format.

For each message that comes through currently, I'm trimming out what we don't end up having at the websockets layer (and for now, tearing out blobs):

.map(msg => {
  // Conform to same message format as notebook websockets
  // See https://github.com/n-riesco/jmp/issues/10
  delete msg.idents;
  delete msg.signatureOK;

  // Once we know what type blobs will be when using a websocket version of
  // enchannel, we'll make it consistent here. For now, delete the prop!
  delete msg.blobs;

  // Recursive Object.freeze
  deepFreeze(msg);
  return msg;
})

The IPython widgets use the binary blobs section. Anything else relying on comms can use this for efficient transport as well.

/cc @minrk

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Jan 14, 2016

Lo and behold, with node v4.x, a Buffer is now a subclass of Uint8Array. The ArrayBuffer underneath would be msg.blobs.buffer.

@n-riesco
Copy link
Owner

@rgbkrk Would it help if by default the jmp.Message constructor doesn't set idents, signatureOK and blobs?

I would like to remove Message#signatureOK from Message, but to do that probably means changing the signature of the on "message" handler. Something like this:

socket.on("message", function(msg, isSignatureOK) { ... });

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Jan 15, 2016

Likely so. It's only on the receiving end do I want them not to have the extras. Don't tear blobs out though, that was wrong on my part.

@n-riesco
Copy link
Owner

n-riesco commented Feb 1, 2016

15c250f closes this issue.

@n-riesco n-riesco closed this as completed Feb 1, 2016
@rgbkrk
Copy link
Contributor Author

rgbkrk commented Feb 1, 2016

When would someone use a message that has an invalid signature? For testing?

@n-riesco
Copy link
Owner

n-riesco commented Feb 1, 2016 via email

@n-riesco n-riesco reopened this Feb 1, 2016
@rgbkrk
Copy link
Contributor Author

rgbkrk commented Feb 1, 2016

Can always unpublish that version (though you'll have to use a different version number, even if just a patch).

@n-riesco
Copy link
Owner

n-riesco commented Feb 1, 2016

I've updated the README.md to advise that v0.3.0 is an alpha release. I will create a dev branch and commit the next changes there, so that you have a chance to review them before I publish a new release on npm.

@n-riesco
Copy link
Owner

n-riesco commented Feb 1, 2016

This commit should address this issue:

  • Message#signatureOK has been removed
  • "message" listeners only receive one argument, the JMP message; i.e.;
    socket.on("message", function(msg) {...});
  • and messages with an invalid signature are dropped

I've unpublished jmp@0.3.0 on npm and I will publish the next release as version 0.4.0 .

@n-riesco
Copy link
Owner

n-riesco commented Feb 9, 2016

I've just released version 0.4.0. In this version:

  • Message#signatureOK has been removed
  • I've reverted the change in v0.3.0 and "message" listeners simply receive JMP message.
  • And, from now on, all invalid JMP messages are dropped (including those with a bad signature).

@n-riesco n-riesco closed this as completed Feb 9, 2016
@rgbkrk
Copy link
Contributor Author

rgbkrk commented Feb 9, 2016

Great! One other reason dropping messages with a bad signature is a wise idea - this helps mitigate any possible denial of service in processing invalid messages (from a bad actor).

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

No branches or pull requests

2 participants