Fix encoding problem #28

r-marques · 2016-09-28T15:09:59Z

resolves #22

This pull request replaces the ed25519 python library with pynacl.
pynacl is a python binding for libsodium and is under active development.

Besides replacing the libraries, also change the way encoding is handled. Now instead of the encode, to_ascii, and to_bytes methods we have only encode and we can pass the encoding that we want. Supported encoders are hex, bytes, base16, base32, base58, base64

TODO:

Fix the docstrings and documentation

Fixed tests

codecov-io · 2016-09-28T15:15:26Z

Current coverage is 85.94% (diff: 94.11%)

Merging #28 into master will decrease coverage by 0.48%

@@             master        #28   diff @@
==========================================
  Files            17         17          
  Lines           818        804    -14   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits            707        691    -16   
- Misses          111        113     +2   
  Partials          0          0

Powered by Codecov. Last update a2c8390...994f88e

sohkai · 2016-09-29T08:33:42Z

tests/conftest.py

@@ -3,13 +3,13 @@

 # ED25519
 VK_HEX_ILP = b'ec172b93ad5e563bf4932c70e1245034c35467ef2efd4d64ebf819683467e2bf'
-VK_B64_ILP = b'7Bcrk61eVjv0kyxw4SRQNMNUZ+8u/U1k6/gZaDRn4r8'
+VK_B64_ILP = b'7Bcrk61eVjv0kyxw4SRQNMNUZ+8u/U1k6/gZaDRn4r8='


Curious as to why the = was added?

There was some unnecessary adding and removing of padding

sohkai · 2016-09-29T08:52:20Z

cryptoconditions/crypto.py

        """
        Sign data with private key

        Args:
            data (str, bytes): data to sign
-            prefix:
            encoding (str): base64, hex


Probably should be encoding (str): {'base58'|'base64'|'base32'|'base16'|'hex'}?

sohkai · 2016-09-29T08:54:14Z

cryptoconditions/crypto.py

    """
    PrivateKey instance
    """

-    def __init__(self, key):
+    def __init__(self, key, encoding='base58'):
        """
        Instantiate the private key with the private_value encoded in base58


These docs should probably also be updated.

sohkai · 2016-09-29T08:56:54Z

cryptoconditions/crypto.py

-class Ed25519VerifyingKey(ed25519.VerifyingKey, VerifyingKey):
-
-    def __init__(self, key):
+    def __init__(self, key, encoding='base58'):
        """
        Instantiate the public key with the compressed public value encoded in base58


Should probably update docs.

sohkai · 2016-09-29T08:57:49Z

cryptoconditions/crypto.py

        """
        Verify if the signature signs the data with this verifying key

        Args:
-             data (bytes|str): data to be signed
+             data (bytes|str): data verify


Small typo: data to verify

sohkai · 2016-09-29T09:00:02Z

cryptoconditions/crypto.py

+            # The reason for using raw_signatures here is because the verify method of pynacl expects the message
+            # and the signature to have the same encoding. Basically pynacl does:
+            #   encoder.decode(signature + message)
+            raw_signature = _get_nacl_encoder(encoding).decode(signature)


I don't think the encoder throws; can we put this above the try?

sohkai · 2016-09-29T09:08:52Z

cryptoconditions/crypto.py


    # Public key
-    public_value_compressed_base58 = Ed25519VerifyingKey(base58.b58encode(vk.to_bytes())).to_ascii()
+    public_value_compressed_base58 = sk.verify_key.encode(encoder=Base58Encoder)


In regards to #23, we would want this to be .decode()ed before it gets returned. This seems to only be used outside of the library, and so decoding it into a string is easier for others to use. A named tuple would also be nice here 😉.

We can address #23 in a later PR though.

No sure if I want to do that inside this library.
If I do it for keys I also need to do it for signatures.
I feel like its more consistent if the this library uses only byte strings throughout and we can decode / encode what we need in the bigchaindb wrappers

The dict of fullfillments also decodes the key (although the signature isn't).

Perhaps this is something about external/internal handling; to me this method is only applicable to external users so it should be decoded. I agree that internally, we should just use byte strings.

sohkai · 2016-09-29T09:11:49Z

cryptoconditions/crypto.py

+        raise exceptions.UnknownEncodingError("Unknown or unsupported encoding")
+
+
+class Ed25519SigningKey(nacl.signing.SigningKey):


Given that we don't use the generate() classmethod (and it probably doesn't work?), perhaps composition here would be preferable to inheritance?

composition is a bit messy in python, as you need to explicitly implement each method

But... you realize we've already re-implemented each method anyway? And if we hadn't, the non-overridden functions would've likely been broken if accessed from an instance of this class (e.g. generate before being re-implemented)?

composition is a bit messy in python

This is simply not true 😄.

This is simply not true .

http://blog.thedigitalcatonline.com/blog/2014/08/20/python-3-oop-part-3-delegation-composition-and-inheritance/

you'll need to reimplement each method as you are wrapping the object in an attribute... messy/inefficient
Haven't encountered a good example of composition in python

BTW: in this case it seems not worth spending effort in discussing the most elegant pattern... bigger fish

sohkai · 2016-09-29T09:14:55Z

cryptoconditions/crypto.py



 def ed25519_generate_key_pair():
    """
    Generate a new key pair and return the pair encoded in base58
    """
-    sk, vk = ed25519.create_keypair()
+    sk = nacl.signing.SigningKey.generate()


Does seems slightly awkward that we just defined our own version of SigningKey and then use the pynacl one here. Maybe we should expose generate in our class and use it here?

sohkai · 2016-09-29T09:16:38Z

Really like how much nicer the crypto module is now :)!

So glad we don't have things like this base58.b58encode(self.to_seed()).encode('ascii').decode('ascii').rstrip("=").encode('ascii') running around 😉.

sohkai · 2016-09-29T09:30:52Z

cryptoconditions/types/ed25519.py

@@ -154,4 +154,4 @@ def validate(self, message=None, **kwargs):
        if not (message and self.signature):
            return False

-        return self.public_key.verify(message, self.signature, encoding=None)
+        return self.public_key.verify(message, self.signature, encoding='bytes')


Should probably update the docstring to specify that message has to be a bytestring.

sohkai · 2016-09-29T09:32:54Z

cryptoconditions/types/ed25519.py

@@ -66,7 +66,7 @@ def sign(self, message, private_key):
        #   .update(Buffer.concat([this.messagePrefix, this.message]))
        #   .digest()

-        self.signature = sk.sign(message, encoding=None)
+        self.signature = sk.sign(message, encoding='bytes')


Should probably update the docstring to specify that message has to be a bytestring

make sure it is in line with the cc spec. Buffer means bytes

This didn't change anything. encoding='bytes' is that same as the old to_bytes()

from the docstring: message (string): message to be signed
needs to be:

message (bytes): message to be signed

diminator

Great stuff - minor changes

diminator · 2016-09-29T14:59:15Z

cryptoconditions/crypto.py

+        raise exceptions.UnknownEncodingError("Unknown or unsupported encoding")
+
+
+class Ed25519SigningKey(nacl.signing.SigningKey):


composition is a bit messy in python, as you need to explicitly implement each method

diminator · 2016-09-29T14:59:46Z

cryptoconditions/crypto.py


        Args:
-            key (base58): base58 encoded private key
+            key (str): encoded private value.


str or bytes?

maybe also typecheck on either

It works well we both. I guess the encoders handle both cases.
But to be consistent I will put bytes

diminator · 2016-09-29T15:00:53Z

cryptoconditions/crypto.py

+        Instantiate the public key with the public value.
+
+        Args:
+            key (str): encoded compressed value.


str or bytes? typecheck?

diminator · 2016-09-29T15:02:34Z

cryptoconditions/crypto.py

    # Private key
-    private_value_base58 = Ed25519SigningKey(base58.b58encode(sk.to_bytes())).to_ascii()
+    private_value_base58 = sk.encode(encoding='base58')


rename to signing_key_base58 or sk_base58
or better: add encoding as a kwarg: ed25519_generate_key_pair(encoding='base58')

diminator · 2016-09-29T15:03:11Z

cryptoconditions/crypto.py


    # Public key
-    public_value_compressed_base58 = Ed25519VerifyingKey(base58.b58encode(vk.to_bytes())).to_ascii()
+    public_value_compressed_base58 = sk.get_verifying_key().encode(encoding='base58')


rename to vk_base58 or verifying_key_base58
consider encoding as kwarg

diminator · 2016-09-29T15:04:15Z

cryptoconditions/types/ed25519.py

@@ -57,7 +57,7 @@ def sign(self, message, private_key):
            private_key (string) Ed25519 private key
        """
        sk = private_key
-        vk = VerifyingKey(base58.b58encode(sk.get_verifying_key().to_bytes()))
+        vk = VerifyingKey(base58.b58encode(sk.get_verifying_key().encode(encoding='bytes')))


vk = sk.get_verifying_key().encode(encoding='base58') ?

Does get_verifying_key come from nacl inheritance and runs a nacl.signing.VerifyKey?
maybe we can override that method to return a Ed25519VerifyingKey?

get_verifying_key was already part of the Ed25519SigningKey api. Didn't want to change it since its also used in the rest of the cc code.

ok, but you can get rid of the base58.b58encode

diminator · 2016-09-29T15:08:26Z

cryptoconditions/types/ed25519.py

@@ -66,7 +66,7 @@ def sign(self, message, private_key):
        #   .update(Buffer.concat([this.messagePrefix, this.message]))
        #   .digest()

-        self.signature = sk.sign(message, encoding=None)
+        self.signature = sk.sign(message, encoding='bytes')


make sure it is in line with the cc spec. Buffer means bytes

diminator · 2016-09-29T15:51:41Z

But... you realize we've already re-implemented each method anyway?

agreed

diminator · 2016-09-30T13:59:18Z

👍 mergeable!

Rodolphe Marques added 2 commits September 14, 2016 14:00

initial implentation of the switch to nacl

3950dde

Better handling of encoding

0d72c99

Fixed tests

r-marques added the z: dummy-label-for-waffle label Sep 28, 2016

sohkai reviewed Sep 29, 2016

View reviewed changes

Rodolphe Marques added 2 commits September 29, 2016 13:43

Fixed docstrings

fbc298f

Expose generate classmethod

a5b7cee

diminator reviewed Sep 29, 2016

View reviewed changes

diminator mentioned this pull request Sep 30, 2016

ed25519_generate_key_pair should return decoded strings #23

Closed

fixed docstring

994f88e

r-marques merged commit 1b0fc30 into master Oct 4, 2016

r-marques removed the z: dummy-label-for-waffle label Oct 4, 2016

This was referenced Oct 12, 2016

VerifyingKey.verify throws ValueError or AttributeError #27

Open

Add support for utf-8 #19

Open

Ed25519VerifyingKey bug bigchaindb/bigchaindb#617

Closed

r-marques mentioned this pull request Oct 13, 2016

Test new cryptoconditions changes bigchaindb/bigchaindb-common#48

Closed

		raise exceptions.UnknownEncodingError("Unknown or unsupported encoding")


		class Ed25519SigningKey(nacl.signing.SigningKey):

Fix encoding problem #28

Fix encoding problem #28

Conversation

r-marques commented Sep 28, 2016 • edited Loading

codecov-io commented Sep 28, 2016 • edited Loading

Current coverage is 85.94% (diff: 94.11%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai commented Sep 29, 2016

sohkai Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diminator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diminator Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diminator Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diminator commented Sep 29, 2016

diminator commented Sep 30, 2016

r-marques commented Sep 28, 2016 •

edited

Loading

codecov-io commented Sep 28, 2016 •

edited

Loading

sohkai Sep 29, 2016 •

edited

Loading

sohkai Sep 29, 2016 •

edited

Loading

diminator Sep 29, 2016 •

edited

Loading

diminator Sep 30, 2016 •

edited

Loading