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

Proposal: Simplify World v2 Access Control #462

Closed
alvrs opened this issue Mar 6, 2023 · 6 comments · Fixed by #467
Closed

Proposal: Simplify World v2 Access Control #462

alvrs opened this issue Mar 6, 2023 · 6 comments · Fixed by #467

Comments

@alvrs
Copy link
Member

alvrs commented Mar 6, 2023

Current Situation / Recap

  • In Proposal: World framework v2 #393 / feat(world): add World prototype #405 we introduced a new access control mechanism for resources in the World (tables, systems, maybe more, see Proposal: Extending the Filesystem of World #456)
  • The access control mechanism is based on routes
    • A single route fragment starts with / and doesn’t include further /.
    • A route can have arbitrary depth (as many route fragments chained together as you want, eg /skystrife/combat/AttackSystem).
    • Access rights are inherited to subroutes: if an address has access to /skystrife, it automatically has access to /skystrife/combat/AttackSystem.
    • This is achieved by passing two separate parts of the route to any method that requires access control: an accessRoute, and a subRoute. Access control checks are done on the accessRoute, and if the access control passes the subRoute is appended to the accessRoute to form the full route of the resource to access. (Eg. accessRoute = /skystrife, subRoute = /combat/AttackSystem)
  • This method is relatively gas efficient, because only a single storage access is required to verify the permission of a caller (only checking accessRoute) for a nested route of arbitrary depth.
  • However, the method is not very developer friendly - a developer has to know which parent route their access to a specific resource is based on to be able to specify this part of the route as accessRoute.
  • Further downsides of this approach:
    • Passing two dynamic length strings as part of every resource access adds the gas overhead of two bytes32 fields that Solidity uses to store the length of the strings.
    • Internally the full route has to be hashed to normalise it to a 32 byte word (eg to make it compatible with Store, among other gas efficiency reasons). This is bad for developer experience, because it is not immediately obvious what a route refers to without checking the hash's pre-image from the table that we have to create for this purpose.
    • The whole approach of deeply nested routes and splitting them into accessRoute and subRoute and thereby inheriting access is relatively hard to onboard / wrap your head around.

Alternative approach 1: Simple Namespaces

  • What if instead of allowing arbitrary depth of routes, we limit the depth of routes to 2: /namespace/file.
  • Access control would be based on the namespace by default (= an address with access to the namespace has access to every resource in the namespace), but could also be granted for individual files.
  • We would lose support for deeply nested routes at launch, but could always have a separate entry point in the World contract to add support for deeply nested routes using the previous approach, if we realize we need them. So far it doesn’t seem like there are strong examples for situations where deeply nested routes are needed.
  • In turn we would get a bunch of advantages:
    • We can limit the size of a namespaces selector to bytes16, and the size of a file selector to bytes15, so they can fit into a single bytes32 word when concatenated with a separator character (eg /).
      • More gas efficient: This means we save on gas for calldata, because instead of 2x bytes32 for dynamic length strings plus ~1 byte per character in each string, we only require a single 32 byte word for the full concatenated namespace/file selector.
      • Better developer experience for understanding routes: This also means we can keep the full route to any resource in clear text, without hashing it, as long as the namespace and file are strings with max length ~16/15 characters respectively. If longer names are required, we could still fall back to hashing them to the required length.
    • Better developer experience for accessing resources: When accessing a resource, the caller would provide both the namespace and file selector of the resource they want to access. The access control check could first check access on the namespace (the more common use case), and if it fails check access on the individual namespace/file selector. This means the caller doesn’t need to know if their access is based on the namespace of the individual file - the contract would figure it out. In the worst case it would mean two storage accesses (if access is based on the file). As an optimization, we could add a second entry point that can be called with a single bytes32 namespace_file selector, if the caller already knows their access is based on the full file selector, to skip the separate namespace check.

Code exploration

  • Here is a simple code exploration of what this change would look like in the World contract: 4c4c6e2

    • Very little actually has to change, the required changes make the code simpler (because we don’t need to juggle between “id” and “route pre-image”)
  • Here is a little exploration of what calling systems via the World would look like (without wrapper libraries / types)

    // Default - check access first on namespace, then on file if no namespace access:
    world.call("mud", "increment", abi.encodeWithSelector(IncrementSystem.increment.selector, 1)); 
    
    // Optimized - only check access on the full selector if the caller knows they don't have namespace access, or if the system is public
    world.call("mud/increment", abi.encodeWithSelector(IncrementSystem.increment.selector, 2));
    
    // Even simpler if the system only has a single fallback function
    world.call("mud/increment", abi.encode(3));
  • Not too relevant for this proposal, but still mentioning here for context:

    • For the first two cases we’d add autogenerated wrapper libraries to allow calling the systems with native types.
    • For the last case we could even go as far as registering a world function selector to be called via the World’s fallback function - this would allow us to easily generate an interface (instead of a library) for the World via which systems can be called with native Solidity types instead of manual abi.encode.

Alternative Approach 2: Roles

This came up when discussing the idea with @holic

  • What if instead of namespace, we implement access control based on roles?
  • The general approach would be very similar to the namespace approach (see above), but instead of files being part of exactly one namespace, they could be accessible by different roles. By default all resources by single developer would have access to all the other resources (equivalent to a namespace). But in addition, access to different subsets of the resources could be shared with other roles.
  • To have the same gas efficiency as the namespace approach, we could store access for each role in a separate entry (instead of eg. an array). As an example: if we want to give role1 and role2 access to systemA, we’d store true in the AccessTable at key role1 and at role2, and also store the file data (eg address of a system) in the SystemTable at role1/systemA and role2/systemA. We’d also need to store all roles having access to systemA in an array somewhere, to be able to update all references when updating the system. This would increase gas cost for registering new files (one time), but keep the gas cost low for accessing files.
  • Unfortunately this approach wouldn’t map as nicely to tables: with namespaces there is a single bytes32 namespace_table key that can be used to refer to the table in Store, while with roles we’d need another concept of the table id (because role1/table and role2/table can’t refer to the same table), and therefore an additional storage access for writing/reading from/to tables.
  • After thinking about the role approach more, I think it would be slightly harder to wrap your head around than the very simple namespace approach. The role approach would make more fine grained access control to a subset of resources easier though.
  • Mostly for the gas efficiency reason I’m personally in favour of the namespace approach, it’s simplicity is a nice bonus.

I’m curious for your thoughts on the general idea of simplifying access control to resources in the World contract, as well as thoughts on the two alternative approaches (or any other alternative approaches that come to mind)

@holic
Copy link
Member

holic commented Mar 6, 2023

We can limit the size of a namespaces selector to bytes16, and the size of a file selector to bytes15, so they can fit into a single bytes32 word when concatenated with a separator character (eg /).

If the names are of a fixed/predetermined length, is the separator all that necessary? We could just use a full bytes16 and bytes16 smashed together, and then make our autogen code, logs, etc. make them more human readable (i.e. separate arguments for namespace/file, adding in a slash when displaying together in logs, etc.)

@alvrs
Copy link
Member Author

alvrs commented Mar 6, 2023

If the names are of a fixed/predetermined length, is the separator all that necessary? We could just use a full bytes16 and bytes16 smashed together, and then make our autogen code, logs, etc. make them more human readable (i.e. separate arguments for namespace/file, adding in a slash when displaying together in logs, etc.)

Agreed! Especially since for passing the full selector from user code we actually can't just do namespace/file but rather need to shift them to the correct locations in the bytes32 to obtain the same hash result as when passing them separately.

@dk1a
Copy link
Contributor

dk1a commented Mar 6, 2023

I like simple namespaces, and they align better with store too. Just disallowing long (>16) route names seems fine to me.
For roles, with the new config they can probably be done well with just accesses lists in typescript

@holic
Copy link
Member

holic commented Mar 6, 2023

@alvrs and I had a quick call to help me better understand the two alternative approaches and trade-offs and I'm all for the first one!

@alvrs
Copy link
Member Author

alvrs commented Mar 6, 2023

Another thought that came from the call with @holic: we can combine the advantages of approach 1 with approach 2 by adding an optional entry point that accepts a role as an additional parameter and first checks whether the caller has this role, and then if the role has access to the namespace. This requires two sloads, but we couldn't find a way to make approach 2 work with tables with less than two sloads anyway.

@ludns
Copy link
Member

ludns commented Mar 7, 2023

I like approach 1 a lot, and it allows us to leverage the standard Solidity dispatcher to not require libraries in order to call system if have an extension of this with a single function per system (and thus namespace + system name is enough to identity a routine being called).
This would be much better calldata wise, and would help with rollups.

world.Namespace_System(arg);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants