[Core/Install] Improve UNIX permission usage #5323

johnsaigle · 2019-10-16T17:21:57Z

Brief summary of changes

This PR improves the way UNIX permissions are used in LORIS.

Summary

Old permissions

lorisadmin owns all files
lorisadmin has sudo
www-data/apache2 selectively owns and is the group for web-writable files
- e.g. drwxrwxrwx 2 www-data www-data 4096 Oct 9 16:30 user_uploads
Uses the same permissions as lorisadmin
Typical permission codes: 775, for directories, 664 for files
Result: On certain files and folders, Apache gets to do everything lorisadmin can do

New permissions

lorisadmin owns all files
lorisadmin has sudo
lorisadmin is the group for all files
- e.g. drwxrwxrwx 2 lorisadmin lorisadmin 4096 Oct 9 16:30 user_uploads
www-data/apache2 is part of the lorisadmin group
Typical permission codes: 755 for directories, 644 for files.
Result: Certain group permissions on files can be used to limit www-data/apache2 access.

Why?

More details in #5297. In short:

Simplifies install - no need to selectively chgrp to apache2 or www-data user
Prerequisite for creating a Debian package - the lintian tool causes LORIS to fail because of odd permission codes like 775 and 664. See Change to UNIX file permissions on LORIS install #5057
Easier maintenance - everything is now lorisadmin.lorisadmin. Permissions can be tracked using GitHub rather than changed manually.
This is how UNIX permissions are usually used.
Brings us closer to the principle of least-privilege.

Testing instructions

Test the install according to the new instructions. Ensure that LORIS still works.

Links to related tickets (GitHub, Redmine, ...)

johnsaigle · 2019-10-16T17:37:53Z

If this is merged, the installation readmes (CentOS, "Install in depth", macOS) should be updated.

Since at this time there are many open PRs dealing with these same files, it will be easier to make these changes once those PRs go through.

maltheism

Awesome this PR solves a "changing permission error" I face with the current install.sh script.

driusan

not actually a change request, just so this doesn't show up in "approved" until the discussion is resolved..

driusan · 2019-10-25T14:33:53Z

tools/install.sh

-    mkdir -p ../modules/document_repository/user_uploads
-    mkdir -p ../modules/data_release/user_uploads
+    mkdir -p -m 770 ../modules/document_repository/user_uploads
+    mkdir -p -m 770 ../modules/data_release/user_uploads


This doesn't seem to do what the PR description says (not this line, the lines below it where things are still being chown'd to apache.apache)

You're right. I'll remove them

Even if group would have RWX on the directories, files created by Apache would have ownership of www-data:www-data and Lorisadmin would not be able to modify/erase them and possibly read them.

Ideally, these .../user_uploads directories should be moved outside of /var/www/$projectname.

gdevenyi · 2019-10-25T14:37:15Z

I strongly suggest symbolic rather than numeric permissions to make things easier to read/interpret at a glance. https://en.wikipedia.org/wiki/Chmod#Symbolic_modes

driusan · 2019-10-25T15:01:58Z

I disagree.. when talking about absolute permissions, numeric ones are far more common and, as a result, easier to read

PapillonMcGill · 2019-10-28T13:44:25Z

If linst file uploaded by a user get 644 permission, LORIS will not be able to edit (read only).
Is this desired?
Files (linst or otherwise) created by LORIS (apache) would get www-data:www-data ownership and won't be readable by lorisadmin (without sudo).
This would be a security improvement but is it desired? (Do lorisadmin user need access to these files?)

johnsaigle · 2019-10-28T15:55:30Z

If linst file uploaded by a user get 644 permission, LORIS will not be able to edit (read only).
Is this desired?

The 6 permission at the beginning means that Apache will be able to read and write the file (since, as you said, the ownership is www-data:www-data). As far as I know the execute permission doesn't need to be set for editing LINST files in the instrument builder. Let me know if I'm wrong or missing something.

Files (linst or otherwise) created by LORIS (apache) would get www-data:www-data ownership and won't be readable by lorisadmin (without sudo).
This would be a security improvement but is it desired? (Do lorisadmin user need access to these files?)

I think it's alright. I can't really see a scenario where you'd need to do this and wouldn't have sudo. Maybe on some production servers? But in this case it makes sense to have an extra measure against deleting user's files.

It's more of an administration question since files uploaded to LORIS shouldn't be under /var/www/loris/ anyway, but rather some other folder like /data/ that is outside the code base. Whoever is administering this on the back-end (dev or sysadmin) should have the powers to delete or read a file if they really need to anyway.

PapillonMcGill · 2019-10-28T15:59:56Z

@johnsaigle The 6 is for the owner of the files (user=>lorisadmin), not for the group. Apache would have group permission (4) and not be able to modify the file.

As for the 2nd point, entirely approve. and yes, data should not be with the code. I am just asking if the behavior would cause problem to some process (like shell script).

johnsaigle · 2019-10-28T17:38:17Z

I believe www-data will be the owner, as you said here:

Files (linst or otherwise) created by LORIS (apache) would get www-data:www-data ownership and won't be readable by lorisadmin (without sudo).

Requested changes were incorporated

README.md

requested changes applied

ridz1208 · 2019-11-29T22:25:34Z

@johnsaigle Before I comment on the code, i have a few questions

If we use lorisadmin as a group including the apache user, wouldn't it get confusing (especially internally) for devs ? we already use lorisadmin right now just for the user, shouldn't we use a different name for the group? like webgroup or something that would encompass lorisadmin and apache ? I can just see people altering the permissions anyways thinking that it got misconfigured or wtv...
when you say "permissions are going to be tracked on github" aren't they already tracked on github ?
shouldn't lorisadmin not have sudo post-installation step ?

ridz1208 · 2019-11-29T23:56:48Z

@johnsaigle to make this easier
#5815

lingma

This pull request should be rejected. Though some work makes improvement such as "775 to 755" modification, the whole idea is NOT at all acceptable security practice in my eyes. I see really no reason why this modification should go ahead. Moreover, www-data should never be part of lorisadmin, which contains a real user and sudo user, way too dangerous.

PapillonMcGill · 2019-12-02T18:32:11Z

Moreover, www-data should never be part of lorisadmin, which is already a sudo user, way too dangerous.

@lingma Do not confuse the user lorisadmin with the group lorisadmin.

johnsaigle · 2019-12-02T18:34:44Z

Moreover, www-data should never be part of lorisadmin, which is already a sudo user, way too dangerous.

As @PapillonMcGill mentioned above, the group called lorisadmin will not have the same permissions as the user lorisadmin. www-data will be limited to group permissions of lorisadmin. That's why changing e.g. 755 to 775 make a difference - it restricts the access that the web server has to those files.

johnsaigle · 2019-12-06T21:36:46Z

Blocked by #5815

johnsaigle · 2019-12-06T21:39:10Z

Well @lingma I think we'll have to agree to disagree here.

I am confident that this change simplifies the development experience and doesn't pose a security issue. I've responded to your security concern to the best of my ability and thinking it through carefully I do not think there is a serious risk here.

I am happy to leave it to another reviewer to weigh in here and hopefully we'll reach a group consensus with some more input.

lingma · 2019-12-07T00:15:40Z

Loris as a software product has nothing to do with group permission, this is unnecessary and creating possible problems. Just change all read only files to 644, folders to 755. This will not create any problem in development. I think it will be perfect to put all those web server writable folders some where and tell users what to do with them with a guide.

Of course your solution works. I tested without issue, but the idea itself is much out of my taste.

johnsaigle · 2020-01-08T15:57:59Z

Adding Needs Documentation as a note that I should update the CHANGELOG

driusan · 2020-01-08T16:00:28Z

wouldn't "Add To Release Notes" be a better reminder?

ridz1208 · 2020-01-08T16:52:19Z

I ain't touching this one, i think it's getting enough TLC

PapillonMcGill

changes applied earlier have been reverted.

HenriRabalais · 2020-02-13T21:46:09Z

The amount of labels on this PR is impressive.

Co-Authored-By: PapillonMcGill <34311645+PapillonMcGill@users.noreply.github.com>

This was referenced Oct 16, 2019

Update install documents with new UNIX permissions #5324

Closed

[WIKI] Install - Rewrite "Installing Loris in Depth" for new Wiki #5253

Merged

maltheism previously approved these changes Oct 22, 2019

View reviewed changes

maltheism added the Passed manual tests PR has been successfully tested by at least one peer label Oct 22, 2019

driusan previously requested changes Oct 25, 2019

View reviewed changes

johnsaigle added the State: Needs work PR awaiting additional work by the author to proceed label Oct 25, 2019

johnsaigle dismissed maltheism’s stale review via 65bb4db October 28, 2019 17:37

johnsaigle removed the State: Needs work PR awaiting additional work by the author to proceed label Oct 28, 2019

PapillonMcGill previously requested changes Nov 25, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

johnsaigle added the State: Needs work PR awaiting additional work by the author to proceed label Dec 2, 2019

lingma reviewed Dec 2, 2019

View reviewed changes

johnsaigle added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Dec 2, 2019

johnsaigle added the Release: Add to release notes PR whose changes should be highlighted in the release notes label Jan 8, 2020

cmadjar assigned johnsaigle Jan 10, 2020

johnsaigle removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) State: Needs work PR awaiting additional work by the author to proceed State: Needs documentation PR that needs a more exhaustive documentation to proceed labels Jan 29, 2020

PapillonMcGill suggested changes Feb 3, 2020

View reviewed changes

PapillonMcGill added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Feb 10, 2020

johnsaigle and others added 7 commits February 14, 2020 11:32

Improve UNIX permission usage

cbc75b5

lorisadmin already belongs to its own group

55980b9

improve readme formatting

ffd9bf3

Revert change to create-project.sh

d7916c9

Add changelog notes

1534423

Update README.md

09abf37

Update README.md

19c8643

Co-Authored-By: PapillonMcGill <34311645+PapillonMcGill@users.noreply.github.com>

johnsaigle removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Feb 14, 2020

johnsaigle removed their assignment Feb 14, 2020

PapillonMcGill approved these changes Feb 17, 2020

View reviewed changes

driusan merged commit fbea18e into aces:master Feb 26, 2020

ridz1208 added this to the 23.0.0 milestone Feb 27, 2020

[Core/Install] Improve UNIX permission usage #5323

[Core/Install] Improve UNIX permission usage #5323

Uh oh!

Conversation

johnsaigle commented Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Brief summary of changes

Summary

Old permissions

New permissions

Why?

Testing instructions

Links to related tickets (GitHub, Redmine, ...)

Uh oh!

johnsaigle commented Oct 16, 2019

Uh oh!

maltheism left a comment

Choose a reason for hiding this comment

Uh oh!

driusan left a comment

Choose a reason for hiding this comment

Uh oh!

driusan Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

johnsaigle Oct 25, 2019

Choose a reason for hiding this comment

Uh oh!

PapillonMcGill Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdevenyi commented Oct 25, 2019

Uh oh!

driusan commented Oct 25, 2019

Uh oh!

PapillonMcGill commented Oct 28, 2019

Uh oh!

johnsaigle commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PapillonMcGill commented Oct 28, 2019

Uh oh!

johnsaigle commented Oct 28, 2019

Uh oh!

Uh oh!

Uh oh!

ridz1208 commented Nov 29, 2019

Uh oh!

ridz1208 commented Nov 29, 2019

Uh oh!

lingma left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PapillonMcGill commented Dec 2, 2019

Uh oh!

johnsaigle commented Dec 2, 2019

Uh oh!

johnsaigle commented Dec 6, 2019

Uh oh!

johnsaigle commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lingma commented Dec 7, 2019

Uh oh!

johnsaigle commented Jan 8, 2020

Uh oh!

driusan commented Jan 8, 2020

Uh oh!

ridz1208 commented Jan 8, 2020

Uh oh!

PapillonMcGill left a comment

Choose a reason for hiding this comment

Uh oh!

HenriRabalais commented Feb 13, 2020

Uh oh!

Reviewers

Assignees

Labels

johnsaigle commented Oct 16, 2019 •

edited

Loading

PapillonMcGill Nov 25, 2019 •

edited

Loading

johnsaigle commented Oct 28, 2019 •

edited

Loading

lingma left a comment •

edited

Loading

johnsaigle commented Dec 6, 2019 •

edited

Loading