Unintuitive API for trust/signing a notebook

[Madhu94]

I was working on this issue in the notebook repo and was confused by the NotebookNotary class's API.

The check_cells method returns a boolean, but it also pops off the trusted key from cells' metadata. IMO, you can never tell, looking at the API, that it mutates the notebook contents model passed to it. Also, if you do not use check_cells and call sign or compute_signature or check_signature directly, the trusted key is not popped off.

Could we not mutate the notebook model here ? And, it should be conveyed to the user that sign and other like methods would require you to remove the trusted key.

If you agree something should be done here, I'd be willing to work on it.

Thanks!

[westurner]

At some point, a transition to ld-signatures (signature: { signatureValue, type (signature suite) }) would be justified. Here's some background information on ld-signatures (and blockcerts (MerkleProof2017 signature suite)). blockchain-certificates/cert-schema#25 (comment) Spec: https://w3c-dvcg.github.io/ld-signatures/ Multiple Implementations: - https://github.com/digitalbazaar/jsonld-signatures - https://github.com/WebOfTrustInfo/ld-signatures-js - https://github.com/WebOfTrustInfo/ld-signatures-python/blob/master/jld_signatures.py To be clear, - I haven't even read this code (I'm on my phone) - ld-signatures is a standard with multiple implementations. - We could start moving toward ld-signatures in the near future.

…

On Tuesday, August 1, 2017, Madhu94 ***@***.***> wrote: I was working on this issue <jupyter/notebook#195> in the notebook repo and was confused by the NotebookNotary class's API. The check_cells method returns a boolean, but it also pops off the trusted key from cells' metadata. IMO, you can never tell, looking at the API, that it mutates the notebook contents model passed to it. Also, if you do not use check_cells and call sign or compute_signature or check_signature directly, the trusted key is not popped off. Could we not mutate the notebook model here <https://github.com/jupyter/nbformat/blob/master/nbformat/sign.py#L484> ? And, it should be conveyed to the user that sign and other like methods would require you to remove the trusted key. ( If you agree something should be done here, I'd be willing to work on it :) ) Thanks! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#98>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADGyzFIZk6N7pj4acZtnbqQDdu_vgPOks5sT214gaJpZM4OqHLG> .

[Madhu94]

@westurner Thanks a lot for getting back so soon. Not to be rude, but this doesn't really answer my question - i.e. (1) let's remove the trust info from the cells' metadata before signing? and (2) check_cells should not be doing anything other than checking the cells.

Thank you!

[westurner]

On Tuesday, August 1, 2017, Madhu94 ***@***.***> wrote: @westurner <https://github.com/westurner> Thanks a lot for getting back so soon. Not to be rude, but this doesn't really answer my question - i.e. (1) let's remove the trust info from the cells' metadata before signing? and (2) check_cells should not be doing anything other than checking the cells.

1. That sounds like a good idea. With ld-signatures you pop() the ``signature`` key and {object} before signing. 2. That also makes sense. With ld-signatures there's a ``verify()`` function which also should not mutate the data under review.

…

Thank you! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#98 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADGy0BlCMO-HW84jXX_KXZ18qd0aNjNks5sT5YIgaJpZM4OqHLG> .

[minrk]

I would say that check_cells and mark_cells probably should not be in nbformat at all (they were defined here before the web application and document format were moved to separate packages). These are really implementation details of the notebook web application and not part of the document format. sign and check_signature should not mutate the document, but the whole point of check_cells and mark_cells is to consume and define the trust metadata when communicating with the frontend.

If there's anything to do here, I might say that sign and check_signature could raise an error if trust metadata is seen, since that indicates that it's not being called on a correctly processed notebook document.

@westurner we specifically don't want to store signatures in the document. We used to do that and discovered that it caused a nightmare for version control, which was solved by computing signatures and storing them elsewhere. So any solution that includes signatures in the document would not be appropriate.

[westurner]

On Wednesday, August 2, 2017, Min RK ***@***.***> wrote: @westurner <https://github.com/westurner> we specifically don't want to store signatures in the document. We used to do that and discovered that it caused a nightmare for version control, which was solved by computing signatures and storing them elsewhere. So any solution that includes signatures in the document would not be appropriate.

The update() and pop() calls here could just as easily be updating and reading an auxiliary manifest with a ``signature`` dict/object: https://github.com/WebOfTrustInfo/ld-signatures-python/blob/master/jld_signatures.py I'll read more source here about why the checksums/signatures are not stable enough for whichever VCS.

…

[rgbkrk]

Is the next step here to move check_cells and mark_cells to the notebook package?

[Madhu94]

Thanks for reviving the stalled issue :)

Yes, I think doing that, and also @minrk's suggestion -

... sign and check_signature could raise an error if trust metadata is seen...

I'll proceed with doing this unless someone has any more suggestions?

[rgbkrk]

Sounds good @Madhu94. Feel free to @ mention me on the PR to review it.