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

ichmod should not be allowed to bypass the permission model #6579

Closed
2 tasks done
korydraughn opened this issue Sep 6, 2022 · 11 comments
Closed
2 tasks done

ichmod should not be allowed to bypass the permission model #6579

korydraughn opened this issue Sep 6, 2022 · 11 comments

Comments

@korydraughn
Copy link
Collaborator

korydraughn commented Sep 6, 2022

  • main
  • 4-3-stable

Improvement

ichmod is currently allowed to bypass the permission model when the user adjusting the permissions matches the original owner. See example below.

alice@machine $ itouch foo
alice@machine $ ichmod null alice foo
alice@machine $ ichmod own alice foo   # This should result in an error.

Only users with own permissions or an admin should be allowed to restore alice's permissions.

This makes the behavior around permissions clearer and makes all clients behave the same.

@darthtux
Copy link

I am a big fan of removing ichmod's ability to bypass the permissions model when used by the original object owner. Its presence causes some issues for our workflows and, as already stated, removing this ability would make permissions clearer, especially from an audit standpoint.

@mstfdkmn
Copy link

mstfdkmn commented Sep 16, 2022

Would it be good to think about keeping at least one user's ownership/permission on an object somehow? This could be either by not allowing the 'owner' to remove his/her access completely (not lower than read) or by locking the last remaining acl on an object. Because we are seeing some 'orphaned' objects (no acl exists on) and only admins can search these and make them again visible in the logical namespace by giving permission to the original owner.

A rods user can/cant do:

ils -A alice1.txt
  /kuleuven_tier1_pilot/home/vsc33586/alice1.txt
        ACL - vsc33586#kuleuven_tier1_pilot:own

ichmod null vsc33586 alice1.txt

iquest "select DATA_NAME, DATA_PATH, DATA_OWNER_NAME where DATA_NAME = 'alice1.txt'"
CAT_NO_ROWS_FOUND: Nothing was found matching your query

A rodsadmin can do:

iquest "select DATA_NAME, DATA_PATH, DATA_OWNER_NAME where DATA_NAME = 'alice1.txt'"
DATA_NAME = alice1.txt
DATA_PATH = /irods/a/home/vsc33586/alice1.txt
DATA_OWNER_NAME = vsc33586

ils -A /kuleuven_tier1_pilot/home/vsc33586/data.tar
  /kuleuven_tier1_pilot/home/vsc33586/data.tar
        ACL -

This shows I guess we should monitor the zone to find these types of objects periodically. And admins should put them back in the namespace of regular users by providing required ACLs or they will need to eventually remove those? And this seems prone to be making some mistakes. Or should I stop comparing with linux as the ACLs has already significantly changed with 4.3.0?

@korydraughn
Copy link
Collaborator Author

Requiring that an object have at least one permission on it is an approach. What are we solving by doing that? Admins can always restore permissions as long as they can find the object.

Your issue with orphaned objects needs to be investigated though. If you can determine what is causing them, then we can discuss a fix. That fix could be in the form of additional tooling or some other solution.

I haven't verified this, but I think you can write policy that enforces the objects must always have one permission on them rule. The PEPs of interest are:

  • pep_api_mod_access_control_pre
  • pep_api_atomic_apply_acl_operations_pre
    • You may have to simulate the results of the operation, which shouldn't be too bad

Or should I stop comparing with linux as the ACLs has already significantly changed with 4.3.0?

The permission models are very different. I'm not sure if comparing them provides additional insight as to how we should handle ichmod.

@mstfdkmn
Copy link

mstfdkmn commented Sep 16, 2022

Since I am not fully aware of the roadmap of iRODS (to which direction is evolving), what is written below might be irrelevant. Sorry in advance!

I think the clearest approach must be why a user is be able to get rid of a given permission on a object belongs to him/her without sharing its access with someone else.

Actually there seems theoretical and practical reasons. A strict ACL policy says to me that an object should always have an ACL. I guess the question should be why there is a possibility to see an object might have no ACL on. What is the added value there?

Practical reasons:

  • we will not see any flying objects in space - I mean preventing a data object physically exists/stored in storage but not seen both in the logical and physical namespace by regular users.
  • admins will not have an extra workload/burden to find those lost data objects and will not have to decide on whether to delete these data objects or to think how to restore permissions (to whom and which permission should I give - own/write/read as an admin).
  • Since ACL brings responsibility to all users (creating an object make those a natural owner), we will block possible gaps so that users will not be able to get rid of it easily, meaning someone always has a permission on a data object.

These are only my own insights. I will try the mentioned PEPs to block users not to give themselves null access if there is no other granted access exists. Thanks.

@trel
Copy link
Member

trel commented Sep 17, 2022

So...

1 - default bumpers - prevent removing the last permission on data objects and collections

2 - bumpers - demonstrate a PEP approach to do the same

3 - visibility - provide a default ZMT 'checkfile' that counts / lists any data objects and/or collections with no granted permission

Is there a use case for having a data object without permission?

Does not implementing option 1 above... leave a loophole for users trying to avoid hitting their quota (but still take up space)... a stealth attack on available storage space?

@korydraughn
Copy link
Collaborator Author

(2) and (3) are perfectly reasonable IMO. I understand the reasons for (1), but I'm hesitant to make the server enforce that directly.

My concern is that implementing (1) could block future enhancements in some way we didn't anticipate.

If we decide to go with (1), it would have to be an opt-in solution. I'd be more comfortable with that and it also gives people a chance to experiment.

Does not implementing option 1 above... leave a loophole for users trying to avoid hitting their quota (but still take up space)... a stealth attack on available storage space?

Assuming there is a loophole ... if we remove the permission hall pass from ichmod, the loophole becomes more difficult to use because they will never be able to restore their permissions without an admin.


Ultimately, the topic of "should collections and data objects require at least one permission at all times" deserves its own issue. That will alert more people in the community and allow them to join the discussion if they have opinions.

Let's continue the conversation about maintaining at least one permission in #6596.

MartinFlores751 added a commit to MartinFlores751/irods that referenced this issue Oct 7, 2022
alanking pushed a commit that referenced this issue Oct 7, 2022
@alanking
Copy link
Contributor

alanking commented Oct 7, 2022

@MartinFlores751 - Please cherry-pick to a branch based on 4-3-stable and open a PR against 4-3-stable. Thanks!

MartinFlores751 added a commit to MartinFlores751/irods that referenced this issue Oct 7, 2022
@trel
Copy link
Member

trel commented Oct 10, 2022

first closed issue!

@MartinFlores751
Copy link
Contributor

irods_filesystem unit test still failing, reopening issue.

@trel
Copy link
Member

trel commented Oct 19, 2022

And still need to cherry-pick the second commit to 4-3-stable?

@trel
Copy link
Member

trel commented Oct 20, 2022

I think it's close-able (again) - go for it.

mstfdkmn referenced this issue in irods/irods_client_icommands Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

6 participants