The stuff to get us off coffee. #351

jMyles · 2018-07-05T01:50:44Z

Ursula now signs her verifying key with her ethereum key.
Nodes in decentralized mode verify Ursula's wallet signature.
Ursula includes her public address in her signed interface info.
All learning nodes now track other nodes by address.
Several new tests again plausible attacks, such as Vladimir copies an Ursula, but substitues his own Ethereum address #353

Fixes #294
Fixes #295

KPrasch · 2018-07-05T02:25:43Z

Build failure due to a version of bytestring splitter?

tuxxy

We need to talk about returning Signature in blockchain power. This isn't a great idea, imho.

tuxxy · 2018-07-05T18:06:36Z

nucypher/network/protocols.py

@@ -58,7 +58,7 @@ def rpc_store(self, sender, nodeid, key, value):
        if header == constants.BYTESTRING_IS_URSULA_IFACE_INFO:
            from nucypher.characters import Ursula
            stranger_ursula = Ursula.from_bytes(payload,
-                                                federated_only=True)  # TODO: Is federated_only the right thing here?
+                                                federated_only=self.sourceNode.federated_only)  # TODO: Is federated_only the right thing here?


👍 On having this discussion at some point. Good TODO.

tuxxy · 2018-07-05T18:08:42Z

tests/utilities.py

@@ -196,8 +190,7 @@ def get_nodes_via_rest(self, address, port, node_ids):
        return response

    def put_treasure_map_on_node(self, node, map_id, map_payload):
-        running_node = _ALL_URSULAS[node.rest_interface.port]
-        mock_client = TestClient(running_node.rest_app)
+        mock_client = self.__get_mock_client_by_port(node.rest_interface.port)


Thoughts on making this a fixture?

tuxxy · 2018-07-05T18:09:48Z

.circleci/config.yml

-      - mypy_type_check:
-          requires:
-            - bundle_dependencies-36
+#      - mypy_type_check:


Perhaps we just allow failure here

tuxxy · 2018-07-05T18:15:24Z

nucypher/crypto/powers.py

@@ -99,7 +104,7 @@ def sign_message(self, message: bytes):
        signature = self.blockchain.interface.call_backend_sign(self.account, message)
        return signature

-    def verify_message(self, address: str, pubkey: bytes, message: bytes, signature: str):
+    def verify_message(self, address: str, pubkey: bytes, message: bytes, signature: Signature):


This is a step in the wrong direction.

This is meant to be an interface to what the blockchain sees. Providing the eth_keys Signature object is only going to allow users to make mistakes with that.

This is meant to be an interface for exactly what a smart contract would most likely interact with. I'm not very keen on providing an object that we don't have control over.

Providing the eth_keys Signature object is only going to allow users to make mistakes with that.

...why? Why doesn't it also allow them to do the right thing with that?

I'm not very keen on providing an object that we don't have control over.

Wait, what? Are you saying you are just wholesale against returning objects from dependency libraries?

I don't see how it makes any sense to return the hex. We never use it that way.

For my part, I don't even really see the point of verify_message on BlockchainPower - I prefer to deprecate it entirely rather than make design decisions based on it.

At the end of the day, all we care about it that we have the correct public address, and we can get that by deriving it from the public key, which we can get from the signature. This is really not the same use case as general-purpose signing. The Signature class serves us very well for our singular need.

Providing the raw signature allows them to use eth_keys very not misuse resistant API. Potentially they could use that to verify the signature and not the BlockchainPower and by very simple accident, would allow them to verify a signature improperly. This should not be that simple.

Wait, what? Are you saying you are just wholesale against returning objects from dependency libraries?

That's not what I said. I'm not very keen on providing an object we don't have control over in such a critical interface as this. If we wanted to make our own Signature class that is misuse resistant, then I'm all for it.

Essentially, I'm saying that eth_keys Signature class is too low-level to be exposed here. If we can restrict what can be done on this Signature class, then we could return that. Like we discussed in PDX, I would prefer this, but the ecosystem has yet to catch up here.

I see.

Well, I think that the proper pattern here is:

Have address to verify

Have message purporting to come from address

Have signature against this message

Validate signature

Get public key from signature

Derive address from signature

Confirm that addresses match

That appears to be the intention of this object, and it performs this task very well. So, do you want to cast to bytes and then cast back to Signature? I guess that's fine.

Yes, that would be ideal (for now). In the verify_message method, at least.

tuxxy · 2018-07-05T18:18:55Z

nucypher/blockchain/eth/interfaces.py

@@ -418,7 +418,7 @@ def call_backend_sign(self, account: str, message: bytes) -> str:
            address = to_canonical_address(account)
            sig_key = provider.ethereum_tester.backend._key_lookup[address]
            signed_message = sig_key.sign_msg(message)
-            return signed_message.to_hex()
+            return signed_message


This breaks parity with the rest of the web3 API.

So, if we're running a test, this API is called, which returns a string. In most cases, this is what we should expect when calling some node to sign something for us, not the object.

This is why I hate this code and want a better signing solution from web3py here.

I... don't see the problem.

In one case (running tests), we return a Signature. In another, we return a string.

This is not good API design for signature, imho. They should both return an object (which is problematic), or they should both return a string.

Ahh, I see. So w3.eth.sign returns a string?

In any case, remember this is a [WIP] PR - I haven't even looked at the else clause in this function yet.

jMyles · 2018-07-10T01:22:19Z

I propose that, upon merging this, we freeze #286 for review.

tuxxy

🐧 Looks good, nice changes.

michwill · 2018-07-10T05:18:43Z

nucypher/crypto/powers.py


-    def verify_message(self, address: str, pubkey: bytes, message: bytes, signature: str):
+    def verify_message(self, address: str, pubkey: bytes, message: bytes, signature_bytes: bytes):


Not that it matters much, but is it necessary to have the type indicated in the argument name as signature_bytes rather than just naming signature?

Yeah, this is a perennial wonder of mine. Since we make a Signature object in the next line, doesn't it make sense to clarify that this object is a bytes representation? I dunno.

I'm not too strong on one or the other way though. It's not super-important here. So, I'm ok with _bytes

cygnusv · 2018-07-10T07:44:16Z

nucypher/blockchain/eth/actors.py

@@ -27,7 +25,7 @@ class NucypherTokenActor:
    class ActorError(Exception):
        pass

-    def __init__(self, checksum_address: str=None, token_agent: NucypherTokenAgent=None):
+    def __init__(self, checksum_address: str = None, token_agent: NucypherTokenAgent = None):
        """
        :param checksum_address:  If not passed, we assume this is an unknown actor



I don't understand what's the deal with self.checksum_public_address attribute below (L37). Isn't it always going to raise an AttributeError since it doesn't exist yet?

It's a compromise for the case (which occurs in practice, almost without exception) where this class is mixed in with Character.

…t testerchain accounts are always unlocked.

…r#294.

…th other classes.

…n't need to learn at all.

jMyles changed the base branch from master to coffee July 5, 2018 01:50

jMyles force-pushed the matcha branch from 7f86d07 to 7b6d3b6 Compare July 5, 2018 02:03

tuxxy requested changes Jul 5, 2018

View reviewed changes

tuxxy reviewed Jul 5, 2018

View reviewed changes

jMyles force-pushed the matcha branch from c1bbd00 to aa6eb62 Compare July 5, 2018 19:31

jMyles changed the title ~~[WIP] The stuff to get us off coffee.~~ The stuff to get us off coffee. Jul 10, 2018

tuxxy approved these changes Jul 10, 2018

View reviewed changes

jMyles mentioned this pull request Jul 10, 2018

Coffee #286

Merged

jMyles requested review from KPrasch, michwill and cygnusv July 10, 2018 03:33

michwill reviewed Jul 10, 2018

View reviewed changes

michwill approved these changes Jul 10, 2018

View reviewed changes

cygnusv approved these changes Jul 10, 2018

View reviewed changes

jMyles added 14 commits July 10, 2018 10:14

Adding pytest-twisted to dev reqs.

8ef6cb8

Character tests generate coverage for entire package.

52721c7

Commenting out MyPy check for the moment. See nucypher#350.

72d0c8e

Quieter mypy options. See nucypher#350.

3d412cd

unlock_account on blockchain, to account for the apparent reality tha…

695e6dc

…t testerchain accounts are always unlocked.

Using plain signature (instead of hex-casted) in BlockchainPower.

a1ca7d4

Ursula now substantiates her stamp. This is the first half of nucyphe…

d7c7e06

…r#294.

Test for stamp evidence.

eddcf0f

Making build_dependencies use verbose output for pipenv.

9044ad8

Ticking cache version for CircleCI.

7c46c69

Smoothing out init args for Actors. Style fixes.

b479103

Moving SuspiciousActivity up to protocols so that it can be shared wi…

87a14b8

…th other classes.

Going back to simply returning sampled miners.

6625731

Adding federated flag to both Arrangements.

f59d2ff

jMyles added 5 commits July 10, 2018 10:14

Moved node verification logic to a separate class.

13e8c45

Verification logic by public address. Fixes nucypher#353.

d8a5a4a

Separating blockchain Alice from regular ole' Alice. Style touch-ups.

a88052c

No reason to always_be_learning any more as a default. Some actors do…

8045bc6

…n't need to learn at all.

Passing bytes-in / bytes-out instead of eth_keys Signature object.

39e6433

jMyles force-pushed the matcha branch from 2b9acfa to 39e6433 Compare July 10, 2018 17:14

KPrasch approved these changes Jul 10, 2018

View reviewed changes

KPrasch merged commit 53d2104 into nucypher:coffee Jul 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The stuff to get us off coffee. #351

The stuff to get us off coffee. #351

jMyles commented Jul 5, 2018 •

edited

KPrasch commented Jul 5, 2018

tuxxy left a comment

tuxxy Jul 5, 2018

tuxxy Jul 5, 2018

tuxxy Jul 5, 2018

KPrasch Jul 10, 2018

tuxxy Jul 5, 2018

jMyles Jul 5, 2018 •

edited

tuxxy Jul 5, 2018

tuxxy Jul 5, 2018

jMyles Jul 5, 2018

tuxxy Jul 5, 2018

tuxxy Jul 5, 2018

tuxxy Jul 5, 2018

jMyles Jul 5, 2018

tuxxy Jul 5, 2018

jMyles Jul 5, 2018

jMyles commented Jul 10, 2018

tuxxy left a comment

michwill Jul 10, 2018

jMyles Jul 10, 2018

michwill Jul 10, 2018

cygnusv Jul 10, 2018

jMyles Jul 10, 2018


		def verify_message(self, address: str, pubkey: bytes, message: bytes, signature: str):
		def verify_message(self, address: str, pubkey: bytes, message: bytes, signature_bytes: bytes):

The stuff to get us off coffee. #351

The stuff to get us off coffee. #351

Conversation

jMyles commented Jul 5, 2018 • edited

KPrasch commented Jul 5, 2018

tuxxy 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

jMyles Jul 5, 2018 • edited

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

Choose a reason for hiding this comment

jMyles commented Jul 10, 2018

tuxxy 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

jMyles commented Jul 5, 2018 •

edited

jMyles Jul 5, 2018 •

edited