Skip to content
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

have verify_account_authority check passed-in keys #1384

Merged
merged 10 commits into from Oct 25, 2018

Conversation

Projects
5 participants
@jmjatlanta
Copy link
Contributor

commented Oct 16, 2018

Potential fix for #1366

The original code always returned false, due to not using the passed in keys to do anything.

This PR uses a different approach. Compare the public keys of what was passed in versus what is known.

@@ -167,7 +167,7 @@ class database_api_impl : public std::enable_shared_from_this<database_api_impl>
vector<withdraw_permission_object> get_withdraw_permissions_by_giver(const std::string account_id_or_name, withdraw_permission_id_type start, uint32_t limit)const;
vector<withdraw_permission_object> get_withdraw_permissions_by_recipient(const std::string account_id_or_name, withdraw_permission_id_type start, uint32_t limit)const;

//private:
// private:

This comment has been minimized.

Copy link
@oxarbitrage

oxarbitrage Oct 16, 2018

Member

we maybe just remove this comment ?

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Oct 16, 2018

Author Contributor

Yes, I tried (hence the space I accidentally added). It did not compile due to _db being used outside, plus some others. I was thinking of fixing all of it, but resisted the urge because I was unsure why it was originally commented out.

@abitmore

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

IMHO this is not the correct approach, since you ignored multi-auth accounts at all.

Correct approach would be to call verify_authority function somehow with available public keys as "approved".

@abitmore
Copy link
Member

left a comment

As commented.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Thank you @abitmore for partially answering a question I had.

Is the purpose of this function to prove that the public keys are valid? Or is this function saying "If you send a transaction signed by the private keys related to these public keys, it will go through"?

If you want to prove that those public keys are related to this account, well, I think I am done.

If you want to take in to account multi-auth accounts, then more will need to be added. And now with projects being worked on that make keys more granular, I believe it would depend on what type of transaction you are doing as to if such a signature would pass or fail.

If my thinking and assumptions on the last paragraph are correct, perhaps it would be best to pause working on this issue until those features are added.

Thoughts?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

Doc comment says

@return true if the signers have enough authority to authorize an account

IMO it is clear that multi-level signatures must be taken into account.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

In the latest approach, I am using graphene::chain::verify_authority with a transfer operation and the passed in account and keys. I have not tested with a multi-level signature account. I will take that on next, unless someone raises their hand and says this approach is also invalid.

std::vector<operation> ops;
ops.emplace_back(op);

try

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Oct 20, 2018

Member

why not just to create signed_transaction and call it's verify_authority method ?
So the full implementation of verify_account_authority might be like next:

bool database_api_impl::verify_account_authority( const string& account_name_or_id, 
      const flat_set<public_key_type>& keys )const
{
   // create a dummy transfer
   transfer_operation op;
   op.from = get_account_from_string(account_name_or_id)->id;
   std::vector<operation> ops;
   ops.emplace_back(op);
   signed_transaction trx;
   trx.signees = keys;
   trx.operations.push_back(op);

   return verify_authority(trx);
}

It's less code and more simple IMHO.

This comment has been minimized.

Copy link
@pmconrad

pmconrad Oct 20, 2018

Contributor

Nice idea, but IMO the signees field should be used as an internal cache only, not part of the tx struct's interface.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Oct 20, 2018

Member

graphene::chain::verify_authority will use signees returned by signed_transaction::get_signature_keys method, so it should work

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

I have added a muti-signature test (2 of 3) to wallet_test. Please let me know what you think.

@cogutvalera
Copy link
Member

left a comment

described in comments

try {
account_update_operation op;
op.account = nathan.id;
op.active = authority(2, public_key_type(nathan_key1.get_public_key()), 1, public_key_type(nathan_key2.get_public_key()), 1, public_key_type(nathan_key3.get_public_key()), 1);

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Oct 23, 2018

Member

this line is too long, please reduce it something like next example of code:

         op.active = authority(
            2, 
            public_key_type(nathan_key1.get_public_key()), 1, 
            public_key_type(nathan_key2.get_public_key()), 1, 
            public_key_type(nathan_key3.get_public_key()), 1
         );
@cogutvalera

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

All tests (verify_account_authority & any_two_of_three) have passed successfully with no errors.
Nice work ! Thanks !

@cogutvalera

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

IMHO just required little improvements with that line as commented above ...

@cogutvalera
Copy link
Member

left a comment

Thanks !

edump((e.to_detail_string()));
throw;
}
}

This comment has been minimized.

Copy link
@pmconrad

pmconrad Oct 23, 2018

Contributor

Please add a test where nathan's account has a multisig from one key and two other accounts.

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Oct 23, 2018

Author Contributor

Added what I think you are asking. It is not much different than the last, so I may be misunderstanding your request.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Oct 23, 2018

Member

IMHO we should check different combinations as in any_two_of_three test, perhaps the same combinations, but with 3 accounts, 1 key per 1 account, just my thoughts ...

account_update_operation op;
op.account = nathan.id;
op.active = authority(3, public_key_type(nathan_key.get_public_key()), 1,
public_key_type(alice_key.get_public_key()), 1, public_key_type(bob_key.get_public_key()), 1);

This comment has been minimized.

Copy link
@pmconrad

pmconrad Oct 23, 2018

Contributor

Here you should use alice_id and bob_id, not their keys

try {
fc::ecc::private_key nathan_key = fc::ecc::private_key::regenerate(fc::digest("key1"));
fc::ecc::private_key alice_key = fc::ecc::private_key::regenerate(fc::digest("key2"));
fc::ecc::private_key bob_key = fc::ecc::private_key::regenerate(fc::digest("key3"));

This comment has been minimized.

Copy link
@pmconrad

pmconrad Oct 23, 2018

Contributor

Please use ACTORS macro

public_keys.emplace(bob_key.get_public_key());
BOOST_CHECK(db_api.verify_account_authority("nathan", public_keys));
}

This comment has been minimized.

Copy link
@pmconrad

pmconrad Oct 23, 2018

Contributor

Also need a negative test, e. g. alice_key missing.

op.active = authority(3, public_key_type(nathan_key.get_public_key()), 1,
public_key_type(alice_key.get_public_key()), 1, public_key_type(bob_key.get_public_key()), 1);
op.active = authority(3, nathan_public_key, 1,
alice.id, 1, bob.id, 1);

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Oct 23, 2018

Member

IMHO we don't need to split this line, it isn't long now

@cogutvalera
Copy link
Member

left a comment

Thanks !

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

Good, thanks!

@jmjatlanta jmjatlanta dismissed stale reviews from pmconrad and cogutvalera via 673ac2b Oct 23, 2018

@cogutvalera

This comment has been minimized.

Copy link
Member

commented on 673ac2b Oct 23, 2018

Thanks !

Stale

@@ -663,9 +663,12 @@ class database_api
bool verify_authority( const signed_transaction& trx )const;

/**
* @return true if the signers have enough authority to authorize an account
* @brief Verify that the public keys have enough authority to authorize a transaction

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 23, 2018

Member

Why this change? There is no transaction in the API.

* @brief Verify that the public keys have enough authority to authorize a transaction
* @param account_name_or_id the account to check
* @param signers the public keys
* @return true if the passed in keys have enough authority to authorize a transaction

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 23, 2018

Member

Why this? There is no transaction in the API.

try {

ACTORS( (nathan) );
graphene::app::database_api db_api(db);

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 23, 2018

Member

I think it's better to move these test cases to database_api_tests.cpp.

@abitmore

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

Code looks fine to me.

@@ -663,9 +663,12 @@ class database_api
bool verify_authority( const signed_transaction& trx )const;

/**
* @return true if the signers have enough authority to authorize an account
* @brief Verify that the public keys have enough authority to approve an operation

This comment has been minimized.

Copy link
@abitmore

abitmore Oct 23, 2018

Member

I think we should mention the account in this sentence and the return

This comment has been minimized.

Copy link
@jmjatlanta

jmjatlanta Oct 24, 2018

Author Contributor

I think the latest commit will be more to your liking. Please take a look.

@abitmore abitmore added this to the 201812 - Feature Release milestone Oct 25, 2018

@abitmore abitmore added this to In progress in Feature Release (201902) via automation Oct 25, 2018

@pmconrad pmconrad merged commit 2e15e77 into develop Oct 25, 2018

3 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Feature Release (201902) automation moved this from In progress to Done Oct 25, 2018

@pmconrad pmconrad deleted the jmj_1366 branch Oct 25, 2018

@pmconrad pmconrad referenced this pull request Oct 25, 2018

Closed

database_api::verify_account_authority is broken #1366

1 of 17 tasks complete

pmconrad added a commit that referenced this pull request Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.