search

signalapp / libsignal

Home to the Signal Protocol as well as other cryptographic primitives which make Signal possible.
GNU Affero General Public License v3.0
3.08k stars 362 forks source link

remove Debug from SessionRecord #472

Closed cosmicexplorer closed 1 year ago

cosmicexplorer commented 2 years ago
  1. SessionRecord impls Debug, but it's not used anywhere. This removes that.
  2. PrivateKey is given a Debug impl which avoids leaking any useful data or making elliptic curve computations.
cosmicexplorer commented 1 year ago

I feel like logging them can leak more information than you might intend

I think this is a very reasonable concern. The best argument in favor I can think of is that if we define a canonical Debug here (and we take precautions like not printing private key contents), then it's perhaps less likely anyone will later feel the need to define an ad-hoc debug printing method later which might not take those precautions?

But to take a step back, this change in particular was motivated by me wanting to be able to more easily debug tests for my own code, a CLI tool which depends on libsignal-protocol and e.g. serializes those in-memory stores to files on disk. That use case is definitely not what Signal is concerned with, so if your gut is feeling weird about adding this, I would recommend we close this, since it's not very difficult for me to add a debug formatting method for my own purposes.

But I guess if that were really true then SessionRecord shouldn't be Debug in the first place, and it is, so okay.

It actually looks like removing Debug from SessionRecord causes no compile issues, so after convincing myself with the above argument I've actually modified this change to simply remove Debug from it! I've kept the PrivateKey debug impl though, with a comment that calculating the public key equivalent isn't free.

jrose-signal commented 1 year ago

Hm, if we're trending that way then I'd say PrivateKey doesn't need it either, since you can always ask for the key_type. (I'm thinking about a user looking at generated API docs and seeing that PrivateKey implements Debug, but then trying it and realizing it doesn't tell them anything useful.) What do you think?

cosmicexplorer commented 1 year ago

(I'm thinking about a user looking at generated API docs and seeing that PrivateKey implements Debug, but then trying it and realizing it doesn't tell them anything useful.)

This is a very useful framing to evaluate these docs changes in, and I agree that it would be more confusing than helpful. I have removed the Debug impl for PrivateKey.

cosmicexplorer commented 1 year ago

Hm, that's a strange little quirk. I have turned off "vigilant mode" now, as it is rarely needed. Please let me know if that lets you cherry-pick it without any smudges.

cosmicexplorer commented 1 year ago

Ping! I think this should be good to merge? Please feel free to edit the commit (including removing my authorship) if that is needed to safely merge.

jrose-signal commented 1 year ago

Sorry, it's in the private branch and will be in the next release! But we haven't had anything to drive a release just yet. I'll close this when the release happens and I can put the cherry-picked commit hash here.

cosmicexplorer commented 1 year ago

Great, thanks!

jrose-signal commented 1 year ago

Released in v0.22.0!