Decode both smessage and signature in VerifyKey.verify()

[blag]

This PR makes it easier to verify messages by properly combining encoded messages and signatures.

Basically it fixes VerifyKey.verify() to decode both the smessage and signature before appending them together. This PR makes it possible to do this:

smsg = sender_signing_key.sign(msg, encoder=Base64Encoder)

verified_msg_1 = sender_verify_key.verify(smsg, encoder=Base64Encoder)  # The sig+msg as smsg

msg = smsg.message  # b64-encoded
sig = smsg.signature  # b64-encoded

verified_msg_2 = sender_verify_key.verify(msg, sig, encoder=Base64Encoder)  # Split msg and sig

assert verified_msg_1 == verified_msg_2

It also adds two tests that make sure both ways work and return the same result.

[codecov-io]

Codecov Report

Merging #256 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #256      +/-   ##
==========================================
+ Coverage   99.85%   99.85%   +<.01%     
==========================================
  Files          35       35              
  Lines        1408     1422      +14     
  Branches       69       69              
==========================================
+ Hits         1406     1420      +14     
  Misses          1        1              
  Partials        1        1

Impacted Files	Coverage Δ
tests/test_signing.py	100% <100%> (ø)	⬆️
src/nacl/signing.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c19731b...a349f02. Read the comment docs.

[blag]

Can I get some feedback on this?

[reaperhulk]

cc @dstufft since he did the original encoder/decoder design, but based on the docstring it does appear the encoder should do this.

[dstufft]

I feel like I had a reason for this at one point, but I don't actually remember what it was or if it was really important. I don't have a problem with it.

[blag]

@dstufft Thanks for the review!

@reaperhulk Let me know if there's anything else you need me to do to get this merged.

[lmctv]

@blag I'm unsure we really want to decode a detached signature on input, since it's very simple to think about use cases where a detached signature is encoded in a different way than the input message e.g. the case of a hex-encoded signature for a binary stream. IMHO, it's better to keep the advanced use case of detached signatures as it stands, and in line with what we do with detached nonces in SecretBox and Box encrypt and decrypt methods

[blag]

@lmctv I don't understand your argument. If you pass in a raw (binary) smessage and a hex-encoded signature, the current function will still not work.

Here's the current function (without comments):

def verify(self, smessage, signature=None, encoder=encoding.RawEncoder):
    if signature is not None:
        smessage = signature + smessage

    # Decode the signed message
    smessage = encoder.decode(smessage)

    return nacl.bindings.crypto_sign_open(smessage, self._key)

what happens is that the hex-encoded signature is concatenated with the raw binary smessage, resulting in an smessage that is not correct hex-encoded data or binary data!

The current code currently assumes that both the encoded smessage and signature data can be correctly combined by concatenating them. That assumption only holds true for raw smessage and signatures, and for hex-encoded smessage and signatures, but it does not hold for base64- or base32-encoded smessages or signatures.

The current code is absolutely broken for encoded smessages and/or encoded signatures (except for hex encodings).

Furthermore, the functions you point out aren't even consistent among themselves.

The SecretBox.encrypt function absolutely does encode the nonce and the ciphertext with the same encoder (source). This PR is consistent with that.

The SecretBox.decrypt function assumes that signatures are always raw binary data (source).

Even if you insist on keeping this function consistent with that, this verify function is still broken!

In that case, this function should be:

def verify(self, smessage, signature=None, encoder=encoding.RawEncoder):
    if signature is not None:
        smessage = signature + smessage_encoder.decode(smessage)
    else:
        # Decode the signed message
        smessage = smessage_encoder.decode(smessage)

    return nacl.bindings.crypto_sign_open(smessage, self._key)

@reaperhulk Please let me know what you would like me to do to fix this function.

[lmctv]

@blag please accept my apologies for having been too terse in my comment! IMHO, the suggested change at the end of #256 (comment) is a correct bug fix, and stays in line with my suggestion about keeping in line with what happens for Box and SecretBox. As such, I'm inclined to promptly accept a PR fixing the missing decode of the input message in case of detached signatures.

[lmctv]

Replaced by #504. Closing.