-
Notifications
You must be signed in to change notification settings - Fork 232
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
Decode smessage #504
Decode smessage #504
Conversation
It seems the doctest failure in signing.rst you can see at https://travis-ci.org/pyca/pynacl/jobs/478988632 is the only remaining problem. |
I did not realize that the code in the documentation was actually tested - that's hella cool! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I think the documentation changes are putting a bit too much emphasis on the detached signatures case, we can get a better balance later. The PR as it stands corrects a real bug. Approved.
@blag Thank you for your persistence. |
This looks backwards incompatible for anyone relying on the current behavior. Was this taken into account before merging this? |
This does not change any of the previous behavior (when it worked). This PR does fix some previous broken behavior. Here's the previous behavior:
Here's the new behavior:
@alex Do you have any specific use case that is now broken? |
As far as I can tell, if you were previously passing both |
Yes, so whether it worked was dependent on what encoding you were using. This doesn't change the fact that there were sets of arguments that used to work, which no longer do. Frankly, I'd like to just remove encoder/decoder support entirely, as it's full of edge cases like this, and it adds basically no value. |
That's a fair point. Take it up with @lmctv, it's not my decision.
So, if I understand you correctly, you don't want this PR to break anything, but you would be happy to break everybody else's code that uses encoders? If we support encoders, and we want to support signatures that have different encodings than the message, then we really need to be able to specify two different encoders: def verify(self, smessage, signature=None, encoder=encoding.RawEncoder, sig_encoder=encoding.RawEncoder): Then, decoding and combining if signature is not None:
smessage = sig_encoder.decode(signature) + encoder.decode(smessage) If that's amenable to you, I'll whip up a PR for it. |
My proposal would be to deprecate them, and emit a warning, with a hope of someday in the future removing them, not simply break everything right away. |
@lmctv yeah okay while the previous behavior is weird, this breaks BC. Separately encoding/decoding both for all encoders seems to be the backwards compatible compromise here. I agree with Alex that we should also strongly consider deprecating encoders entirely with an eye to removing them ASAP (which is probably years, but might as well start now) |
|
@reaperhulk I prepared a revert PR to save some time. |
@lmctv I have some questions around points of clarification:
I don't know what you are referring to here. What are "all in one" cases?
That's fine, the entire
But users of this class still have to keep track of what encodings were used for each part, and they can differ (the "detached signature" case). It would make more sense - to me, if you are going to have a import nacl.bindings
import nacl.encoding
class SignedMessage(bytes):
# Check Python 2 & 3 compatibility with this function signature
def __init__(self, bytestream, encoder=None):
# Guard against double encoding, since some users might pass
# in hex-encoded or base64-encoded bytestreams, that we would
# then double-encode when read out via .signature or .message
if encoder is None:
raise NotImplementedError("You must specify the encoding of "
"SignedMessage objects with 'encoder'.")
super(SignedMessge, self).__init__(encoder.decode(bytestream), **kwargs)
@property
def signature(self, encoder=nacl.encoding.RawEncoder):
return encoder.encode(self[:nacl.bindings.crypto_sign_BYTES])
@property
def message(self, encoder=nacl.encoding.RawEncoder):
return encoder.encode(self[nacl.bindings.crypto_sign_BYTES:]) That way there's no need for That is a rewrite of the API, however, so it would necessitate probably a major version bump.
The prefix Edit: clarified a few things. |
@lmctv Can I get your feedback on ^ please. |
This PR fixes
VerifyKey.verify()
whensmessage
is not raw bytes.I also add two tests, one for a hex-encoded
smessage
and another with a base64-encodedsmessage
. In both tests,signature
is decoded to raw bytes before being passed toVerifyKey.verify()
.I also make it more clear in the documentation that
signature
MUST always be raw bytes, even ifsmessage
can be encoded.This PR is a different fix then #256, as pointed out in this comment.
Special thanks to @lmctv for guiding me towards this solution.