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
Update fix_permissions, better sync with desimodel, eliminate special config files #109
Conversation
Bug: group execute was added to More subtle: original files had group=sjbailey with write permission, and got converted to group=desi while leaving the write permission, i.e. expanding who could edit this file. In #108 we originally said "fix_permissions.sh should not change the group-writeability bit", but I think what we should have said was "don't change who can write this file". i.e. if the desi group already had write access, leave it that way. And if the desi group doesn't already have write access (either because group write wasn't set, or because it wasn't in the desi group), then change it to the desi group (to ensure read access) but remove group write so that we don't accidentally expand who can write the file. I also see that it removed world read permissions. We hadn't specified what to do for world bits, but I think that we should either leave those alone, or at least provide an option to leave those alone. Another test: Current master:
This branch:
The current master sets ACLs or something such that after running fix_permissions.sh, any new files created in that directory will also have the correct group=desi with read access (good). This is not the case for this branch. |
Could you please run all of the tests in verbose mode? |
Test 1:
Test 2 current master:
Test 2 this branch:
To ease cutting and pasting if we need to run this again:
|
@sbailey, are you really certain that you are running the tests on the correct branch? On this branch, fix_permissions only runs setfacl if adding permissions for the apache user, but the setfacl command appears in "Test 2 this branch". |
Also note, I found the bug that was causing executable bits to be set on files. But that has nothing to do with whether setfacl is run by fix_permissions.sh. |
Indeed, it appears that I ran the verbose version of "test 2 this branch" with the wrong branch. I edited the comment above to have the the correct branch (permissions-and-desimodel). It now shows the (undesired) behavior of files created after running fix_permissions.sh having the old incorrect group. |
OK, I was trying to simplify fix_permissions.sh so it was orthogonal in some sense to the automated permissions "wrangling" (to use Lisa's term) that NERSC is running. In other words, let the automation deal with things like setting the desi pseudo-user access and the setgid bit (which controls the group id of new files). However maybe that is not the desired design choice, especially if you don't want to wait for the automated wrangling. Do we want fix_permissions.sh to be exactly the same as the automated wrangling? Also,
"desi group doesn't already have write access, ... remove group write". This is actually not that easy to implement, because at minimum it would involve passing the results of one find command to another, and might even require replacing fix_permissions.sh with a Python script. |
Comments on comments/questions:
Yes. I think the main purpose of the automated wrangling is to catch cases where people didn't run fix_permissions.sh in the first place. And the motivation for a user to run fix_permissions.sh instead of waiting for the automated update would be "if you don't want to wait for the automated wrangling".
How about making the default be "change to group=desi and remove group write" and if users really do want the desi group to have write access, they can followup with "chmod -R g+w dirname"? |
Update: we should also avoid the situation where the user grants write permission and then a subsequent run of fix_permissions.sh removes it again. i.e. if it is already group=desi and group writeable, leave it that way. I think that logic is possible with find. |
@sbailey, that is the number one point of this update to fix_permissions.sh! I am trying very hard to satisfy that, and I am reasonably confident that it does. I'm starting to think you're not actually reading the code. |
I feel like we're starting to make up requirements on the DESI data permissions piecemeal. Let's back up and review https://desi.lbl.gov/trac/wiki/Computing/NerscFileSystem#Requirements. Let's expand that, and explain the reasoning behind every step. Then write a script that enforces those requirements. Here are some specific things to consider:
|
My "Update" comment was a clarification that my previous suggestion "change to group=desi and remove group write" was not complete, since we don't want to remove group write if the file was already in the desi group. However, not touching the group write bit does not completely solve the group-write bit problem since that bit interplays with enforcing the requirement that group=desi. One of the wiki page requirements is "Group-writeable is optional". I suggest that what we really mean by that is "fix_permissions.sh will not change whether the desi group has write access to a file", which is different from saying "fix_permissions.sh will not change the group write bit". Returning to "Test 1":
In this case,
(I'm not sure about the 's'). i.e. it should enforce that group=desi but it shouldn't inadvertently change the write access of the desi group in the process. |
OK, I'm much more clear on this now, but I think this distinction will not matter to most people since (I maintain) most users set permissions completely carelessly. So again, we're back to the question: are we effectively supporting carelessness? |
Many users do set permissions carelessly, and the purpose of |
As I already noted:
So do you actually want me to try to implement this? When you previously responded, my interpretation of the response was that you didn't think it was necessary. That will add quite a bit more work. Also that gets pretty far away from the concept of "fix_permissions.sh does the exact same thing as the automated permission-wrangling script". Also could you please respond to the comment beginning:
None of the bullet points in that comment have been addressed. |
I may be missing something, but I think we can implement "change to group=desi but don't grant the desi group write access if it didn't already have it" via
Responding to the other bullet points:
Thanks for pointing out that wiki page. I had read those requirements and agreed with them, modulo the interpretation of "Group-writeable is optional". I was trying to expand on that in this thread since wikis are not great for conversation threads before finalizing something. In particular, I interpret it as "either group write or no write is fine, but fix_permissions shouldn't change whether the desi group has write access (which is different than whether it should change the group write bit, since it may be changing the group).
I don't consider "that bit is not touched" to have "solved the group-write bit problem", because of the example I have shown of inadvertently granting write permission to the desi group when it didn't previously have it. That is a form of accidentally enforcing group write instead of leaving it as optional.
I'm inclined to have fix_permissions leave world bits alone, but could also see a case for removing world readability by default. It certainly shouldn't grant world readability by default. There may be good cases for sharing some directory with non-desi colleagues (e.g. pointing a NERSC person at an installation of a public github repo) |
@sbailey, I've updated https://desi.lbl.gov/trac/wiki/Computing/NerscFileSystem with a more rigorous definition of file system permission requirements. Let's converge on that, then I'll apply the consensus requirements to the fix_permissions script. |
@weaverba137 I added one question and an extended comment to that wiki page. In particular, I added a table of suggested before/after cases for |
ACLs: ugh. desi user: if we have to choose between granting the desi user write access and having "obvious" interpretation of the bits from "ls -l", let's favor the more end-user friendly "ls -l". I added that as a proposed requirement on the wiki. As long as the desi group has read access, we can still do disk inventories and invoke NERSC consulting if we need to request something to be deleted, but if the permissions reported by "ls -l" don't match what can actually be done, a lot of users will waste time being very confused. apache user: is the core problem of ACLs and non-obvious "ls -l" specific to the fact that "desi" is both a user and a group, or does it apply to the apache user too? i.e. is it possible to grant read-access to the apache user without mucking up what "ls -l" reports? Note: If the core problem is that "desi" is both a user and a group, we could change the name of that production account user. I fear it goes deeper than that though. |
I'm pretty sure that the Can we please think about this carefully though before we undo all the work that I've done and that NERSC has already done? Personally I think it is far, far weirder that world-writeability determines group-writeability (in the proposed configuration) than |
Let's put a pause on this and discuss it at the data telecon. github + wiki are not proving efficient for converging on this. |
Agreed. In the meantime, I have confirmed that it's the write permission for the desi user that causes the "phantom" group-write permission. When I apply (only) the read-only permissions for the apache user, the phantom group-write does not appear. |
Updates from desi data call today: Having "ls -l" provide meaningful results trumps having ACLs that give the desi pseudo-user write access. If we can't have both, prioritize "ls -l". We currently think that means dropping the desi-user write access ACLs. Proposed update to what to do with group write bits:
We didn't explicitly discuss world bits today, but I propose the following:
Put another way:
If that last suggestion is controversial, removing world read/write no matter what could also be reasonable and then revisit this if we find cases where we really do want world read and don't want |
@sbailey, I've read the latest summary. Will get a new version of fix_permissions out soon. |
Updating my previous pseudo-code example, comments welcome.
|
I agree with that pseudo-code. Thanks. |
@sbailey, I'm satisfied with the current version of |
Almost there.
When I run
|
The only way to enforce permission bits is to use a default ACL, which we were using until we started down the desi pseudo-user path. The setgid bit enforces group ownership not group permission bits. Could you please tell me which directory at NERSC your are running |
PS, if you want your files to never be group-writeable, you should set your umask accordingly. There basically is no solution that will aggressively enforce permission bits on all files created into the indefinite future, except very strict ACLs, and then we get back to the problem of |
I was running this on edison in /scratch2/scratchdirs/sjbailey/temp/blat . I was hoping that fix_permissions.sh would still be able to set the default ACL entries (like current master does) without messing up 'ls -l' output. But if that is another rabbit hole, ok, we can just recommend that people use umask 22 if they don't want group write for new files and move on. |
Could you run |
I edited my copy of fix_permissions.sh to also "echo hello" to confirm that it is indeed the one being called, and I still get the "chgrp: missing operand after 'Dir9' |
OK, then could you please run |
Offlist we sorted out that the test_fix_permissions.sh problem was another tcsh/bash difference, i.e. whether $GROUP is set or not. It works now. I'm ok to merge this as-is as a step in the right direction, though when I see @weaverba137 in person next I'd like to revisit the default ACL question. |
This PR: