Account Name Restriction in System Contract

[greg7mdp]

Resolves #186

Note: the struct canon_name_t implements the checks of whether a name is a subset of another name. For performance reasons, this is implemented by shifting, masking and comparing the uint64_t value.

Probably the equivalent implementation below using strings is easier to understand (but less efficient), which is why I used the uint64_t version, but this could be changed if needed.

struct canon_name_t {
   std::string _pattern;

   canon_name_t(name n) : _pattern(n.to_string()) {}

   size_t size() const { return _pattern.size(); }

   bool is_suffix_of(const canon_name_t& target) const {
      assert(size() < target.size());
      size_t diff = target.size() - size();
      return strcmp(&_pattern[0], &target._pattern[diff]) == 0 && target._pattern[diff-1] == '.';
   }

   bool found_in(const canon_name_t& target) const {
      assert(size() < target.size());
      auto sz = size();
      size_t diff = target.size() - sz + 1;
      for (size_t i=0; i<diff; ++i)
         if (strncmp(&_pattern[0], &target._pattern[i], sz) == 0)
            return true;
      return false;
   }
};

From @arhag:

We want to add a restriction in the system contract when creating a new account.

These additional restrictions will not take affect is the action has the authority of eosio (which we can check with has_auth).

The additional restrictions are as follows:

We lookup restricted strings in some table that can be modified only by eosio permission. If one of those restricted strings can be found anywhere within the desired name of the account to create (with one exception), then the contract rejects creating the account. The one exception is if a restricted string forms a valid suffix of the desired account, then that restricted string is not an enforced restriction on that account creation.

So for example, consider if the BPs add the strings "esafe" and "e.safe" to the table:

The following account names cannot be created (no restriction on accounts that were already created with these names prior to adding the restricted strings to the contract):

• therealesafe
• esafe.xyz (cannot even be created by the xyz account)
• e.safe.abc (cannot even be created by the safe.abc account)
• 12e.safe.abc (cannot even be created by the safe.abc account)
• esafe.e.safe (cannot even be created by the e.safe account)

But the following account names can be created:

• thereal3safe (can be created by anyone)
• esave.xyz (can be created by the xyz account)
• esaf.e12 (can be created by the e12 account)
• esa.fe (can be created by the fe account)
• esafe.esafe (can be created by the esafe account)
• e.esafe (can be created by the esafe account)
• e.safe (can be created by the safe account)
• a.e.safe (can be created by the e.safe account)
• esafe (can only be created by someone who won the name auction bid for "esafe" or by eosio)

Note that esafe and e.safe restricted strings are meant only to be examples. They are not the actual restricted strings we intend the BPs to add in conjunction with this feature that we develop.

The requirements above are somewhat flexible in the sense that we can negotiate the actual rules for the restricted name (but then we would still need to make sure the allowed and disallowed tests cases are consistently reflecting the new rules). They are designed with a mind towards reasonable trade-offs between ease of implementation and the goal of restricting people from acquiring names that appear to be official while also still allowing the people who have control over the official accounts to create sub-accounts without require eosio permission each time.

But there may be some opportunity to change the requirements, especially if it reduces implementation complexity further without compromising the goal. There is more that can be done to better achieve the goal but at the cost of greater implementation complexity, so that is likely not in scope.

For example, we aren't interested in the code automatically replacing characters in the explicitly added restricted strings with other similar appearing characters (for example l and 1) when it is enforcing its restrictions on new account names. We would rather have the BPs add the additional restricted strings with replaced homoglyphs manually. There is in theory a combination explosion possible there, but in practice since we are dealing with Antelope names which are very restrictive, there are only a few homoglyphs and the total length of the restricted strings is bounded above by 12 anyway.

There is an additional cost on every newaccount action that scales with the number of entries in the restrticted strings table. So we want to limit the number of restricted strings in practice to a very small number. In practice, we may only have a single entry. But since we may have a handful, and every table row lookup adds extra cost, it makes sense for the implementation to use a singleton table and put all restricted strings into one vector in the singleton table row (that way only one table lookup is needed for each newaccount action).

[heifner]

Do we want to enforce a total max size? Seems some limit would be good.

[greg7mdp]

Because this is only allowed by a privileged action (multisig), I don't think we should worry about having a total limit.

[heifner]

Would be nice to have some unittests of just the cannon_name_t. It is difficult to reason that it is correct.

[greg7mdp]

I'd have to duplicate the code though, as canon_name_t is defined in eosio.system.cpp, and I don't think we want to move it to eosio.system.hpp.

[greg7mdp]

The tests would not pass if canon_name_t was wrong. Maybe we should add extra test cases though?
Here is a compiler explorer version https://godbolt.org/z/PhMz4rrbx

[heifner]

Could be moved to a cannon_name.hpp so available for testing. It does seem like additional test cases would be good regardless.

[linh2931]

I think I prefer to the string version of the implementation. It is cleaner and easier to understand. This is used in account creation, not a frequently as transactions. Performance is important but not critical.

[greg7mdp]

I like the uint64_t implementation better personally, so waiting for someone to be the tie breaker on this :-).

[arhag]

Still need to review actual implementation of canon_name_t. But of course it seems very likely to be correct given that it passes the test cases I provided. I would like to also see the canon_name_t code pulled out to its own header file and then tested directly in a unit test as Kevin suggested.

[linh2931]

Should we allow an account with some permissions to bypass the rules?

[greg7mdp]

Should we allow an account with some permissions to bypass the rules?

"eosio"_n can add any name without checks.

[greg7mdp]

Still need to review actual implementation of canon_name_t. But of course it seems very likely to be correct given that it passes the test cases I provided. I would like to also see the canon_name_t code pulled out to its own header file and then tested directly in a unit test as Kevin suggested.

canon_name_t is pulled in a separate file, but it is not easy to add unit tests for that class directly.
The unit tests use the spring implementation of name which has a private value accessed via to_uint64_t.
The contracts use the cdt implementation of name which has a public value and no to_uint64_t. api.

The canon_name.hpp header would have to jump through hoops to be able to support both nersions of name.hpp.