Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Support eosio.ccode permission name in authorities which implicitly pins to code version active at the time the authority was updated #3050

Closed
arhag opened this issue May 14, 2018 · 9 comments
Labels
CONSENSUS Introduces a change that may modify consensus protocol rules on an existing blockchain.

Comments

@arhag
Copy link
Contributor

arhag commented May 14, 2018

We currently support the special eosio.code permission name in authorities which is automatically satisfied by a contract. So, for example, if Alice's active authority is set up such that it can be satisfied by the bob@eosio.code permission level, that means that the contract deployed on the bob account can send actions which are authorized by alice@active.

The limitation with the current approach is that the authority can grant permission to any code deployed on a particular account but currently cannot grant permission only to a particular code deployed on an account. Alice may have audited version 1 of the contract deployed on the bob account and added bob@eosio.code to her active authority based on that audit. But perhaps she does not fully trust Bob and is worried that he may change the contract to version 2 which may do something undesirable using Alice's active authority. (Bob can make the contract immutable, but it can be useful for Bob to reserve the right to update the code in the future. Or perhaps the only entity that can upgrade the code is the code itself, based on some decentralized governance process. Either way Alice may want the ability to explicitly renew her delegation of authorization to a smart contract each time it upgrades rather than have it automatically happen.)

Alice would like to be able to pin the eosio.code permission references in an authority to a particular version of the code. That way, if Bob were to then update the code, the new version of the code could no longer satisfy that permission level in Alice's authority.

We will not be supporting this feature by EOSIO 1.0, but we do want to be able to upgrade via hard fork with minimal disruption to support the new feature.

The simplest way to support this feature is to add a new virtual permission name with special semantics supported in authorities: eosio.ccode (short for current code). contract@eosio.code would continue to be satisfied by any code version deployed on the contract account; whereas contract@eosio.ccode would only be satisfied by the code version that was deployed on the contract account at the time of setting/updating the authority that included the contract@eosio.ccode permission level. This approach also means that the hard fork does not change the serialization of authorities or actions.

The authority_checker algorithm has to be updated to support this new feature. We are already tracking important time information for contracts and permissions: last_code_update in the account_object tracks the last time the code of an account was updated, and last_updated of the permission_object tracks the last time a permission was updated. This information needs to be made accessible to authority_checker. When authority_checker retrieves an authority from a permission_level, it should also retrieve the last_updated time of the permission. This time can be compared to time threshold passed into authority_checker which is set to the last_code_update time of the receiver account when the authorizations are being checked in the context of a sending an inline action or a deferred transaction. If the time the permission was last updated is not greater than the time threshold, then the eosio.ccode permission would be ignored by the authority_checker despite the fact that the receiver@eosio.ccode permission would be provided via check_authorization.

One corner case that needs to be addressed with this approach is when a contract changes around the same time a user is setting or updating a permission. Alice could be setting the bob@eosio.ccode permission level in her active authority at the same time that Bob updates the contract on his account to version 2. But Alice only audited version 1 of the contract and expected bob@eosio.ccode to be pinned to version 1, not later versions.

The solution to this is to take advantage of the TaPoS-referenced block. Alice's updateauth was created with the context of the database state as of the end of the block referenced by TaPoS in the header of the transaction including the updateauth action. Therefore, if any of the accounts referenced through the eosio.ccode permission of the new authority updated their code after the timestamp of the TaPoS-referenced block, the transaction containing that updateauth action should fail.

@arhag arhag added this to the Hard Fork 1.0 milestone May 14, 2018
@andresberrios
Copy link

@arhag I like this approach, but I was thinking that, even though it would require more changes to how the authorizations work, it would be great to be able to authorize specific contract versions using a hash of the contract's code itself. I know my friends at Protocol Labs and IPFS would definitely agree (content addressing). This would have a couple of nice benefits:

  • It would not suffer from the issue that you described, so there is no need for the TaPoS-referenced block solution. Specifying what contract code you are authorizing through a reference to the hash of the contract's code assures you that if the updateauth transaction came too close in time with the setcode transaction for changing the contract, it won't matter because that authorization was given to a different contract instead of the contract that happens to be there at that time.
  • It would allow the contract's developer to roll back to a previous version of the contract in case the new version has bugs or was updated by error without losing the authorizations that were given to that previous version by its users.

The difficulties I see with this approach are:

  • The system would need to start tracking the current contract's hash to not have to compute it each time it needs to check authorizations. I think in terms of storage requirements it shouldn't be much of a problem, since a short hash should allow more than enough safety against collisions, etc, since the authorization would be computed using a combination of the hash and the account name where the contract is deployed (user authorizes this account to do actions on their behalf through this specific contract code).
  • How to store the contract's hash in the authorization structure itself. On one hand it's great to have the abstraction of user accounts and contract accounts being treated the same way in many cases, but regarding contract code, as we've already seen, it's important to make a distinction of what you're authorizing: A user to do something on your behalf, or a contract. The introduction of the eosio.code method for authorizing a contract fits well into the already existing permissions scheme, but considering that contracts change, we are now in the need to allow users to specify that they authorize an account's specific code to act on their behalf. This can be done in the way you explained here but I think it's a bit forced into the current authorities structure and it depends on other things outside of that (such as the TaPoS solution), and things that are not easily visible to users for comparing (such as last_updated for permissions and last_code_update for contracts). Instead, modifying the permission_level struct to add a field for the contract hash that is being authorized would allow for a cleaner solution, but I am not sure about the implications regarding the data that is already stored (migrations?) and the full implications of this.

I would love to hear your comments!

@brianjohnson5972
Copy link
Contributor

@arhag @wanderingbort Are these requirements still valid with the upcoming permission changes?

@andresberrios
Copy link

@brianjohnson5972

Are these requirements still valid with the upcoming permission changes?

Where can I find info about these upcoming changes?

@arhag
Copy link
Contributor Author

arhag commented Nov 1, 2018

@arhag @wanderingbort Are these requirements still valid with the upcoming permission changes?

@brianjohnson5972:

I'm not sure what permission changes you are referring to, but this change will not be included in the first consensus protocol upgrade.

@andresberrios:

I can see the value in pinning a permission to a particular code hash. However, it is also valuable to pin to a particular code sequence number for reasons discussed in the background section of #6168 (specifically the second to last paragraph of the background section discussing how the audit was conditional on certain table invariants holding true).

Both the code sequence number and the code hash are already tracked in the native account objects (code hash as the field code_version in account_object and code sequence number as the field code_sequence in account_sequence_object). So that isn't the difficult part.

The tricky part is needing to upgrade the authority structure itself. It is still reasonable to do, it just requires more logic to handle that transition throughout the EOSIO codebase. Hence, this proposal attempts to reach the goal of pinning to specific contracts in a manner that is not at all straightforward just to avoid changing the authority structure.

I do now however think that if this feature is still considered important to have, it is worth doing it in a way that makes it straightforward to use, and that means putting in the effort to upgrade the authority structure. In that case I would not modify the permission_level (since that is used elsewhere like the authorization on actions where code hashes / sequence numbers would not be appropriate). Instead, I would simply add a new section called contracts (to accompany keys, accounts, and waits) which would be a vector of some other structure that requires specifying a uint16_t weight, an account_name for the contract account, and either a uint64_t code sequence number or a SHA256 code hash.

What I am questioning now, however, is whether the very concept of code pinning at the authority level is even worth having. Adding a contract code permission to a user's account permissions is rarely a good practice (this of course excludes adding the eosio.code permission to the account of the contract itself, which is almost a necessity for contracts). There are legitimate use cases in which a user account would elect to add a contract's eosio.code permission, but I suspect that after the activation of the feature in #6168, these legitimate uses could also be achieved while allowing things like code pinning through alternative means such as:

  1. Block producers deploy a privileged "inline action proxy" contract to an account like eosio.proxy.
  2. A user uses eosio.proxy actions to set up a whitelist of contract accounts (with optional code pinning) that are allowed to use some specified set of permissions of the user's account.
  3. The whitelisted contract sends a vector of actions to eosio.proxy that it wants it to send inline on its behalf (assuming it passes all the appropriate authorization checks).

@arhag arhag added the HARDFORK label Nov 1, 2018
@arhag arhag removed this from the Hard Fork 1.0 milestone Nov 1, 2018
@andresberrios
Copy link

andresberrios commented Nov 1, 2018

@arhag thanks for your answer. You make some very good points. I take back my support for pinning based on hash and instead I think it makes much more sense to pin based on sequence number of the deployment of code to the account, thus protecting you against attacks based on switching the code temporarily to make some unacceptable changes to the data in the tables and then reverting it to the approved hash. Users (and developers using inline actions) should have to approve each new deployment of the contract. I can imagine dapp developers prompting their users to approve the new version of their contract in order to use the dapp, along with a message that explains the changes and a link to the git diff for developers to inspect if they so wish.

What I am questioning now, however, is whether the very concept of code pinning at the authority level is even worth having.

Even though I really like the eosio.proxy idea that you are proposing, it is a BP-required chain-specific workaround to achieve what I believe is a very fundamental security feature that should be supported at the protocol level. I love the idea you proposed of adding it to the authority structure using a new contracts entry. It would be way cleaner and explicit, and would give security regarding contract interaction a first-class status.

Adding a contract code permission to a user's account permissions is rarely a good practice (this of course excludes adding the eosio.code permission to the account of the contract itself, which is almost a necessity for contracts).

I think this perception should change once there is more control over what exactly you are approving, like specifying the contract version as we're discussing. For instance, permissions could be set up so that your account is owned and mostly managed by you, but you give someone else permission to update the contract code (this would be a common scenario in dapps, as the management team could give the developers only permission to deploy new code). In this scenario, if you just specify the eosio.code permission, your account is basically compromised since the other party who has deployment permission can update the code to do whatever they want and everything will be pre-approved. You should be able to approve new versions of the code. I would argue that once that is possible, then using eosio.code as it stands today should either not be supported or heavily discouraged.

There are other important reasons for having better support for these authorizations and actually using them and it not being a bad practice:

  • The usually recommended way to avoid requiring users to give the contract permission is tied to deposits of tokens into contracts. For instance, in the case of a decentralized exchange, people are relying on doing a token transfer to the contract account and hoping that it will be considered a deposit and that they'll be able to withdraw eventually. They are not signing a "deposit" Ricardian contract with their transaction. They are, technically, just signing a transfer, and hoping that it will be considered a deposit by the contract developers. In legal terms, the developers now own those funds. There are no legal (intent of code) guarantees regarding what a deposit entails. Users should be able to sign a deposit transaction instead, with it including the inline transfer action produced by the contract, and it being authorized by this permission the users have given.

  • The aforementioned approach only works for token transfers or other actions that include a require_recipient call, which are not very common. If the contract developer would like their code to produce some other action, such as creating a new account on behalf of the user, buying or selling RAM for the user in some algorithmic fashion, or any other action of any other contract, it would be impossible unless the user granted this permission to the contract. It should be possible to do so, but in a granular and secure way.

I'd be happy to hear your thoughts! Thanks for all your great work.

@arhag
Copy link
Contributor Author

arhag commented Nov 2, 2018

Users should be able to sign a deposit transaction instead, with it including the inline transfer action produced by the contract, and it being authorized by this permission the users have given.

I think this is exactly the sort of practice with eosio.code that should be avoided. The permission system is a really blunt tool for something like this. Yes you can create a custom permission that is only used as a linkauth target for eosio.token::transfer. Then someone who can satisfy that permission can only use it for eosio.token::transfer (as well as billing the owner RAM usage as well as CPU and network bandwidth without restriction). But that is still way too permissive! The user typically wants to authorize a particular transfer of a specific amount of a specific token to a specific recipient. The permission system does not allow them to specify these restrictions.

A better pattern specifically for the token transfer example is to first open a zero-amount balance on the contract and then eosio.token::transfer tokens to the contract as a way of depositing into their balance stored within the contract. They could also use a withdraw to pull whatever amount of available tokens from their balance back out of the contract (which would do an inline eosio.token::transfer). With the funds fully in possession of the contract, it becomes trivial for it to move funds around internally as it tries to fulfill its various actions.

You bring up a good point about Ricardian contracts though. We don't have a good explicit solution in place for dealing with notified contracts and expressing the intent of those contracts' behavior when notified of another contract's actions. And with the model above, there wouldn't be an explicit action of the contract called deposit to which to attach a Ricardian contract. It would just be implied (perhaps made explicit in a Ricardian clause of the contract) as the behavior of the contract when the contract account is sent tokens via eosio.token::transfer. But at least the indirect process of first depositing funds before using them in actions allows those other actions to have proper Ricardian contracts (unlike the unfortunate pattern many contract developers seem to continue to practice of using functional memos of eosio.token::transfer to trigger their "actions").

The example above is maybe okay (notwithstanding the lack of the Ricardian) for depositing tokens. However, a better pattern in general is needed, not only to get an explicit deposit action with its own Ricardian contract, but also to enable contracts to send inline actions authorized on behalf of other user accounts in a way that does not violate the (potentially very fine-grained) security expectations of the users. The EOSIO permission system, as flexible as it is, does not allow specifying restrictions that depend on the contents of the action payload. But a proxy contract, as it is just a general contract that can apply any programmable rules on the validation of the action payload data, could in theory enforce such restrictions.

The tricky part is that the proxy contract would not allow the other contract to just send any inline action of a particular code and action name authorized by the user account. It would need to have proof that the user authorized that specific action (with particular action payload data). Even more than that, it would need proof that the user authorized that specific action (with specified payload data) for just one time and within the context of a particular transaction at least (better yet if it is restricted to the context of a particular action).

This is technically possible to do with the existing EOSIO functionality (with some caveats to consider), but it isn't a very nice way to do it. For example, an immediately preceding action within the transaction that is authorized by the user account may create a table record tagged with the computed transaction ID of the current transaction which authorizes the specified contract (contract name also stored in table record) to proxy send an inline action that hashes to a specified hash (hash of action also stored in the table record) but only within the current transaction. If the contract sends an inline action to the proxy (with the actual action to send nested in the action payload), the proxy contract could check that table row and validate all the necessary conditions along with deleting the table record and actually sending the nested action.

This approach however requires the transaction signer to know what the inline action payload data will be at the time of signing the transaction. But imagine the inline action was a transfer to pay for the cost of buying some asset from a DEX. The price of the asset (and therefore the calculated amount of tokens to transfer) would not be known until the moment of the trade. In this case the payload of the preceding action that authorized the contract to send the inline transfer would need to not include a simple hash, but a specification (effectively a contract) that restricts certain parameters of the inline action payload. For example it may restrict the token symbol and put an upper bound on the amount. But with this approach the proxy contract would need to be aware of these domain-specific rules. So perhaps instead it passes the inline action the contract wants it to send along with user-provided constraint specification as an inline action to another contract (specified by the user) with domain-specific knowledge about the particular constraints to allow it to validate the constraint.

So the action receipt hierarchy for this example might looking something like this:

  • 1 eosio.proxy::authorize -> eosio.proxy (authorized by alice@active via transaction signatures; payload data includes constraint_spec for the eosio.token::transfer action which constrains the token symbol to TOK and the amount to a maximum of 10.00)

  • 2 dex::limitbuy -> dex (authorized by alice@active via transaction signatures; payload data includes the symbol of the asset to buy, the symbol of the token to pay with which would be TOK, and the limit price)

    • 2.1 eosio.proxy::proxysend -> eosio.proxy (authorized by dex@active via dex@eosio.code; payload data includes nested eosio.token::transfer action which let's say transfers 5.23 TOK from alice to bob)

      • 2.1.1 eosio.proxy::validatesend -> eosio.proxy (authorized by eosio.proxy@active via eosio.proxy@eosio.code; payload data includes nested eosio.token::transfer action from above as well as the constraint_spec that the parent action found from the appropriate tables, which the parent action would have already deleted by now)

        • 2.1.1.1 eosio.proxy::validatesend -> eosio.token (notification of prior action sent to eosio.token so that it can do the appropriate validation)
      • 2.1.2 eosio.token::transfer -> eosio.token (authorized by alice@active via eosio.proxy@eosio.code; this would be the nested eosio.token::transfer action first provided to eosio.proxy with the proxysend action)

        • 2.1.2.1 eosio.token::transfer -> alice (notification of the transfer sent to from)

        • 2.1.2.2 eosio.token::transfer -> bob (notification of the transfer sent to to)

@andresberrios
Copy link

(unlike the unfortunate pattern many contract developers seem to continue to practice of using functional memos of eosio.token::transfer to trigger their "actions").

I wish people would stop doing that!

The design you've proposed is very powerful. It is also very intricate. Some help would be required from the wallets and end-user tools to make the UX more friendly. I believe it's a good opt-in system, in the same way as the opt-in "banking contract" proposal, but I don't see it becoming the general standard because of it complexity.

On the other side, pinning a permission to a to a specific contract deployment implicitly provides enough guarantees for the vast majority of cases, as the user would have been able to audit that version of the contract (code or Ricardian) and understand the logic that will produce the inline action, thus knowing what to expect. In the example you gave, the user would know well that the dex::limitbuy action will generate an eosio.token::transfer action with a payload composed of the TOK transfer in the amount necessary to buy the desired tokens, and since it is a limit order, the user will also know that there will already be an upper limit to how much will be spent in that transfer. The user can be sure of how the current deployment of the contract operates, and can trust that until a new version is deployed, he can know what to expect from the inline actions generated, in the same way that he can know what to expect from the contract action that is being directly called (limitbuy). By transitivity, this guarantee extends to the inline action and its payload. Adding more checks like the ones you've proposed is powerful, but once we have permission pinning to contract deployments, and contracts also pin the inline actions they send using #6168, it would be equivalent to adding those extra checks to the payload that you are directly sending to the dex::limitbuy yourself, which wouldn't make much sense.

Together with education about good permissions management practices, this would radically improve the security of the network in general.

Users should be able to sign a deposit transaction instead, with it including the inline transfer action produced by the contract, and it being authorized by this permission the users have given.

I think this is exactly the sort of practice with eosio.code that should be avoided. The permission system is a really blunt tool for something like this.

I agree, it is blunt for this right now, but with permission pinning to contract deployments it wouldn't be blunt anymore. It would be fine grained enough to ensure we know what's going on and there are no surprises. That's why we need it.

@arhag
Copy link
Contributor Author

arhag commented Nov 2, 2018

@andresberrios:

You have almost won me over with that argument. But I think there is a big difference in user expectations between the two solutions.

With the eosio.proxy contract solution, a user can read the Ricardian contracts of each transaction they want to do and understand, in hopefully clear natural language, what they are signing off on.

With the code pinning solution, a user is expected to first audit the smart contract that they wish to pin to (or more likely delegate that responsibility to an auditor that they trust), before broadcasting the signed transaction that changes one of their permissions to allow the contract to authorize certain actions on their behalf.

It is one thing to trust the audit for a handful of major critical contracts like eosio.proxy in order to understand how interacting with them can enable other contracts to do authorization on your behalf. But I think it is too much of a burden to require audits of all of the various contracts that a user may wish to interact with (particularly if they just want to use with only a small amount of funds put at risk).

By knowing that the permission system (perhaps with the augmentations to it provided by eosio.proxy) protects you from having more than some expected amount of your tokens taken from you, you don't need to worry as much about what the smart contract might actually do in various scenarios.

But with the code pinning solution used in this dex::limitbuy example, you would really need to be confident that no part of the contract (not just a dex::limitbuy action authorized by you) can lead to a code path that does an inline transfer pulling unexpected tokens from your account.

@arhag arhag added CONSENSUS Introduces a change that may modify consensus protocol rules on an existing blockchain. and removed HARDFORK labels Mar 22, 2019
@aclark-b1
Copy link

In order to focus our efforts on issues that are currently creating difficulty for the community we are closing tickets that were created prior to the EOSIO 2.0 release. If you believe this issue is still relevant please feel free to reopen it or create a new one. Thank you for your continued support of EOSIO!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CONSENSUS Introduces a change that may modify consensus protocol rules on an existing blockchain.
Projects
None yet
Development

No branches or pull requests

4 participants