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

Granularity of permissions #366

Closed
frangio opened this issue Aug 13, 2017 · 7 comments
Closed

Granularity of permissions #366

frangio opened this issue Aug 13, 2017 · 7 comments
Assignees
Labels
breaking change Changes that break backwards compatibility of the public API. contracts Smart contract code.
Milestone

Comments

@frangio
Copy link
Contributor

frangio commented Aug 13, 2017

Some contracts need to have an account with special permissions. An example is MintableToken, which has an address which is the only one allowed to mint tokens. These are currently implemented as Ownable, with the special permissions assigned to the owner (by marking some functions as onlyOwner).

Sometimes a single contract has more than one feature requiring special permissions. Imagine a MintableToken which is also a PausableToken. In such a case, the contract owner can both mint tokens and pause the token.

This goes against the security principle of least privilege, and also makes some requirements harder to implement. As an example of the latter, suppose we have the requirement to disable minting after a crowdsale, but want to retain the ability to stop the contract in an emergency.

For the two reasons stated above, I believe we need to provide greater granularity of permissions. This is kind of doable under the current ownership framework, by setting as owner a contract that can forward calls or not according to some logic. I don't dislike that, but I can see some problems with it.

We might want to provide the granularity directly, i.e. by making each privileged feature have a different address (or set of addresses) with permissions for it. At the same time it sounds like it could lead to an explosion of different permissions and roles which could be hard to manage.

Opening up the topic for discussion. Do you agree that permissions need to be more granular? Have you run into this problem yourself?

@SCBuergel
Copy link

IMO we have to trade-off between granularity (ideally separate access control per-non-constant function) and easy of management (maybe a few roles to which we can subscribe accounts). Why not have a hasRole(x) modifier with some readable enum x and add that to every function? Disadvantage of that enum x is that is has to be known at compile time. Disadvantage of having super dynamic roles which we (or owner) can update at runtime is that security is hard to audit / continuously review.

I have the feeling that this discussion assumes a bit of current-centralised-world thinking: Won't DAO-style votes replace a good bit of hard coded user roles? If the majority (of e.g. investors / voters) wants the DAO to do something then the majority should have the right to do so - and not be restricted by some centralised agent that has the privilege of a specific role. I know, we wont get to that DAO world just tomorrow so a decent role architecture would probably be required for now.

@frangio
Copy link
Contributor Author

frangio commented Aug 13, 2017

The enum approach sounds good for a project-specific contract, and we could recommend that to developers, but I don't think it'll be useful for a general purpose library like OpenZeppelin. Aside from having to be known at compile time, they are also not extensible by subclasses. We would have to have a global enum with all of OpenZeppelin's roles.

Auditability is an important design objective, it's good you brought it up.

With respect to your second point, since a DAO is a smart contract it can be set as the owner itself, and other privileges can also be assigned to the DAO. Then it would have to go through the voting process to call any privileged function. Does that sound like what you had in mind? I think whatever role architecture we come up with will be equally applicable to a world were smart contracts are managed by DAOs.

@SCBuergel
Copy link

TBH I had no perfect solution in mind but having an owner-DAO sounds like a reasonable way to go. It's probably more expensive in terms of gas (since we need another call), so maybe we could derive from that DAO instead of setting it as a (separate) owner account? Either way, having a well structured and abstracted DAO might come with little overhead compared to user roles: propose, vote, resolve (and take action if needed).
A tokenised voting DAO would have the additional benefit of (a) supporting liquid democracy out of the box, (b) also being able to limit voters to a smaller privileged group (by just giving them tokens, which might be non-transferrable).

@foundingnimo
Copy link
Contributor

I think there are multiple scenarios here. Having a distributed authorization model is very interesting and needs some design thinking. However - a simple Role Based Authentication Control is relatively simple to achieve. I have created sample code at https://github.com/duckranger/zeppelin-solidity/tree/rbac.
Inside the contracts/rbac directory you can find the RoleDirectory - which is the main role and user-role relationship repository.
The owner of the RoleDirectory may define system roles, and assign them to users.
There is also a Secured contract which allows for a withRole('..') modifier.

In contracts/examples - I added SampleRoleBasedAccess - which is Secured. Some methods there are then modified with the withRole modifier.

This should be easy enough to add more granular permissions if required.

@frangio
Copy link
Contributor Author

frangio commented Aug 24, 2017

I like it! I had taken a shot at RBAC with a Roles library here. There are a couple of things in your implementation that I like better:

  1. I was planning to reimplement Ownable as a role like the rest, but I agree with your implementation now, in that it should remain a separate simpler ad-hoc role with the permission to manage roles itself.

  2. I hadn't added a central directory of roles and so it wasn't easy for an owner to externally manage roles out of the box.

Do you feel that hierarchical levels are really needed? I think I'd rather keep things simpler, with no hierarchies, just a mapping of role names to role assignments.

@foundingnimo
Copy link
Contributor

foundingnimo commented Aug 26, 2017

Thanks @frangio :)

  1. With owner I suppose you could do either, my concern was with existing code where you'd want to
    plug the RBAC mechanism into - and ensure that things don't suddenly break. However- implementers can do either, so it might be worth it?
  2. I don't think that a hierarchical role structure is absolutely necessary, but it can help in some situations, think team-lead and technical-manager situation for example - the technical-manager may have all the permissions of a team-lead, and some extra permissions. You could give the technical-manager 2 separate roles, or create a hierarchy where the technical-manager role is higher than the team-lead. This might be an overkill in some situations, so I created the 'short circuit' mechanism to make all roles the same level, and then you need to assign each individual role to users.

What do you say?

@frangio
Copy link
Contributor Author

frangio commented Apr 21, 2018

For the record, a solution for this was built in #580 in the form of RBAC. I'm keeping this issue open because the rest of OpenZeppelin still uses Ownable for everything.

@frangio frangio removed the backlog label Jun 15, 2018
@frangio frangio added contracts Smart contract code. breaking change Changes that break backwards compatibility of the public API. and removed kind:improvement labels Aug 1, 2018
@frangio frangio added this to the v2.0 milestone Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that break backwards compatibility of the public API. contracts Smart contract code.
Projects
None yet
Development

No branches or pull requests

5 participants