RFC Deprecate encoding of detached parts

[lmctv]

@blag @pyca/pynacl-core This is a follow-up to the revert of #504, to show a real proposal instead of some hand-waving... I think this way we gain a clear path to deprecate the encoded parts, while keeping the capability to encode the full signed message, which someone seems to find useful in simple cases.

[blag]

This is an absolutely terrible warning message. It doesn't tell the user (or developer) which attribute is deprecated (is it signature? message? raw_signature? raw_message? __get__? __mro__? __getattr__?), and it doesn't hint at what the correct, non-deprecated replacements are.

A much better warning message would be "Use of the signature property is deprecated. Please update your code to use the raw_signature attribute instead, and decode it properly."

Same for the message property below.

[lmctv]

@blag while I appreciate your interest in pynacl, before investing more time in this change, we must agree it goes in the right direction for our users.

The intent is to just deprecate the encoded parts, while keeping the raw parts for advanced usage, therefore I only added warnings on access to .message and .signature.

If @pyca/pynacl-core find some merit in this proposal, I'll be grateful for better message suggestion.

[blag]

Thanks for fixing the warning message - looks much better now! 😄

[blag]

This is unnecessary. While warnings in Python appear like they are exceptions, the two are handled very differently.

The reason to have a base exception class for library exceptions is to allow users to catch only those exceptions.

But there already exists the warnings.filterwarnings mechanism to filter out warnings, and you can filter out warnings by their module.

So this simply (poorly) duplicates a built-in Python feature.

[lmctv]

Right, will remove if we all agree this should go on.

[lmctv]

Just for completeness, I'm adding a transcript of a simple sign/verify session showing that SigningKey.verify was already broken in the cases of encoder=Base64Encoder and encoder=Base32Encoder;

Python 2.7.15+ (default, Nov 28 2018, 16:27:22) 
[GCC 8.2.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from nacl.encoding import *
>>> from nacl.signing import SigningKey
>>> msg = 256*b'A'
>>> sk = SigningKey.generate()
>>> signed_b64 = sk.sign(msg, encoder=Base64Encoder)
>>> verified = sk.verify_key.verify(signed_b64, encoder=Base64Encoder)                   
>>> verified = sk.verify_key.verify(signed_b64.message,
                                    signature=signed_b64.signature,
                                    encoder=Base64Encoder)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/dist-packages/nacl/signing.py", line 112, in verify
    return nacl.bindings.crypto_sign_open(smessage, self._key)
  File "/usr/lib/python2.7/dist-packages/nacl/bindings/crypto_sign.py", line 111, in crypto_sign_open
    raise exc.BadSignatureError("Signature was forged or corrupt")
nacl.exceptions.BadSignatureError: Signature was forged or corrupt
>>> signed_b32 = sk.sign(msg, encoder=Base32Encoder)
>>> verified = sk.verify_key.verify(signed_b32, encoder=Base32Encoder)                    
>>> verified = sk.verify_key.verify(signed_b32.message,
                                    signature=signed_b32.signature,
                                    encoder=Base32Encoder)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/dist-packages/nacl/signing.py", line 110, in verify
    smessage = encoder.decode(smessage)
  File "/usr/lib/python2.7/dist-packages/nacl/encoding.py", line 62, in decode
    return base64.b32decode(data)
  File "/usr/lib/python2.7/base64.py", line 229, in b32decode
    raise TypeError('Non-base32 digit found')
TypeError: Non-base32 digit found
>>> signed_b32.signature + signed_b32.message
'2YVHBYCE3HTGYPI5F73VNTUY7LYH4RO55KCB2TTSZW2OWHRFNAMJT4TLUEQ2JVKZ6RKJP5T2B2O5ZEGJF54DUZOGSWHT2L3H7K7QSBI=IFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIFAUCQKBIE======'
>>> signed_b64.signature + signed_b64.message
'1ipw4ETZ5mw9HS/3Vs6Y+vB+Rd3qhB1Ocs206x4laBiZ8muhIaTVWfRUl/Z6Dp3ckMkveDplxpWPPS9n+r8JBQ==QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQQ=='
>>> 

[lmctv]

OK, with the last couple of commits I think the proposal is complete. I'd like to know if there is some agreement in @pyca/pynacl-core to follow this route, or someone thinks it would be better to keep the encoded attributes instead.
In that case, we'll need to restructure the decoding to avoid the padding = signs getting inserted between the signature and the message, or just reconsider merging #256.

[lmctv]

@dstufft, would you mind taking a look at these proposed changes and give your opinion?
@reaperhulk I've just seen you don't receive messages directed to pyca/pynacl-core; could you spare some time on this?

[lmctv]

@reaperhulk could you please take a look at this too?

[lmctv]

@reaperhulk @dstufft @blag @pyca/pynacl-core
For completeness, a possible alternative to these changes would be a direct deprecation of the one-shot sign and encode API like in:

class SigningKey(encoding.Encodable, StringFixer, object):
    def sign(self, message, encoder=None):
        if encoder is None:
            encoder = encoding.RawEncoder
        else:
            warn("Direct result encoding is deprecated "
                 "in favor of returning a raw result.",
                 PyNaclDeprecated)

and a similar change in VerifyKey, but I fear such a change would cause more disruptions for our downstream users.

[reaperhulk]

Okay, I've finally thought about this a bit (sorry about the delay). Our ultimate goal is to get to a world where we only deal in bytes so I think marking all encoder inputs other than RawEncoder as deprecated is our first step. We then follow that up (in a year or so, sigh) with a release where every default switches to RawEncoder (that will be a breaking change for nacl.hash), and then a release where we make the encoder arg a no-op across the project. Then hopefully a year or two after that we can remove accepting the arg entirely.

The raw_* getters in this PR are unnecessary if we choose to do it that way. What do you think?

[lmctv]

Closing since #523 seems more useful.