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

Fix various permission issues #3216

Merged
merged 2 commits into from
Mar 18, 2018

Conversation

ymartin59
Copy link
Contributor

@ymartin59 ymartin59 commented Mar 10, 2018

Grant GROUP (sc-download) permission to browse subfolder in Share Folder
Conditionally enlist service user into "users" group to access contents

Linked issues: #3192 #3215 #3212 #3185

@Safihre
Copy link
Contributor

Safihre commented Mar 10, 2018

So we would use this on all download related packages or just processing packages like Sonarr?
Because if we add all packages, then why use sc-download at all? Add everything to users and we have no problems at all.
If we only add to processing packages, the objective is just backwards compatibility right? Because for example the sc-nzbget user wouldn't be part of users, only sc-download.

@ymartin59
Copy link
Contributor Author

Here is my idea:

  • service user is member of "users" to read existing content coming from other users
  • download services write in there own folder, which requires to be in "sc-download" to access them "by default"
  • protocol service will run in no specific group, neither "users" nor "sc-download" to avoid "leaking" content

If user want "download" folders (from transmission for instance) to be accessible to everyone, they can add "users" read permission on it.

@ymartin59
Copy link
Contributor Author

I just wonder if it is a good idea to set as "owner" folders in Shared... when package is removed, some strange effects may happen:

xpenology52> synoacltool -get /volume1/downloads                       
ACL version: 1 
Archive: has_ACL,is_support_ACL 
Owner: `������� Y��(user) not found 
--------------------- 
	 [0] group:administrators:allow:rwxpdDaARWc--:fd--  (level:0)
	 [1] group:sc-download:allow:rwxpdDaARWcC-:fd--  (level:0)

@ymartin59 ymartin59 force-pushed the 3192-share-permissions-fix branch from 185a657 to 42b03ba Compare March 10, 2018 16:47
@ymartin59 ymartin59 changed the title PREVIEW Fix various permission issues WIP Fix various permission issues Mar 10, 2018
@ymartin59 ymartin59 force-pushed the 3192-share-permissions-fix branch from 42b03ba to f88e098 Compare March 10, 2018 17:07
@Safihre
Copy link
Contributor

Safihre commented Mar 10, 2018

download services write in there own folder, which requires to be in "sc-download" to access them "by default"

If all services (both downloaders and professors) are already in users, we could just remove sc-download. Don't you think?

@ymartin59
Copy link
Contributor Author

Maybe I have guessed what DrBean idea was:

  • segregate service accounts from standard end-users account
  • "sc-download" group permission for file producers/writers
  • "sc-media" group permission for content consumers/readers

Probably sc-download/sc-media split is too complex for most end-users... But I think it is relevant for security and some advanced users (like some small companies for instance) to keep permission segregation.

If a service account is in "users", it does not necessarily mean it will write files readable for "users". My idea is to enlist service account into "users" to be able to read "public" files (readable to anyone)

@ymartin59
Copy link
Contributor Author

I have even imagine in framework to propose users to choose group name from wizard thanks to SERVICE_WIZARD_GROUP... I do not remember who rejected this idea, but it would allow to choose between old-style "users" (open-bar) and standard segration (sc-download) or even custom segregation with one group per application (at worst)
For application with both producer and consumer roles, two groups are required... as service B may read content from another service A and write content to service C. That is exactly our user case at the moment, with "users" group to read content and "sc-download" to provide it to others.

@Safihre
Copy link
Contributor

Safihre commented Mar 10, 2018

I am pretty sure it was me who suggested to remove the group option from wizard. Because I think SynoCommunity package's should be here to make things easier for regular end-users.. Not expect them to be permissions and command line experts.

I don't think I completely understand how the proposed approach would work in practice: how things can be just readable for one group but read and writeable for the other. Doesn't a consumer like Sonarr need also to write (remove) files?
But if it works and makes things easier for the end-users: let's do it :)

@ymartin59
Copy link
Contributor Author

I agree with you... but I really think there is no reason to give any application full access to "users" group. I am not sure if it makes things clearer but I wrote a concept as a specification/design: #3217

@the-lazy-fox
Copy link

Being part of a group like media or download is pretty fine and convenient.

@ymartin59 ymartin59 force-pushed the 3192-share-permissions-fix branch from f88e098 to 29f14ca Compare March 17, 2018 09:27
@ymartin59 ymartin59 changed the title WIP Fix various permission issues Fix various permission issues Mar 18, 2018
Grant GROUP (sc-download) permission to browse subfolder in Shared
Allow to enlist service user into "users" group to access contents
Increase package versions to publish
@ymartin59
Copy link
Contributor Author

@Safihre Are you OK with this PR ?

@Safihre
Copy link
Contributor

Safihre commented Mar 18, 2018

Yes looks good to me.
Now we only add the to users when upgrading 👍
One final improvement would be to detect non-ACL shares and warn users before installing. If this is possible?
But that's for another PR!

@Safihre Safihre merged commit 8060b1c into SynoCommunity:master Mar 18, 2018
@ymartin59
Copy link
Contributor Author

@Safihre I agree. I have no idea how to check and test it, as I only have recent DSM version where Share Folder have ACL enabled by default... We may ask @BenjV for tips about it - with hope its system may still contain non-ACL-enabled Share Folder.

@ymartin59 ymartin59 deleted the 3192-share-permissions-fix branch March 18, 2018 17:46
@ymartin59
Copy link
Contributor Author

@Safihre @BenjV synoshare command has no convert action, so my best bet is probably to test error return from --list_acl if any

@BenjV
Copy link

BenjV commented Mar 19, 2018

synoshare --list_acl does not give you this info.
You have to use: synoshare --get [sharename]
This wil produce a datadump and the [ACL] attribute will tell you with a Yes or No if the share has acl capabilities.
IfNothen the user has to convert it via the wizard.

I did not find a way to convert it with a command so the script could do it, I also don't think it should be done via the script, the warnings in the wizard are there for good reasons.

@Safihre
Copy link
Contributor

Safihre commented Mar 19, 2018

Doesn't synoacltool also show it? For all paths?

@BenjV
Copy link

BenjV commented Mar 19, 2018

Not to my knowledge.
Can you give an example of the synoacltool command you think would work so I can try it on my last "Non-Acl" share.

@ymartin59
Copy link
Contributor Author

@BenjV May you please report output of synoacltool -get /volume1/downloads if you have a "downloads" non-ACL Share Folder on "volume1".

@BenjV
Copy link

BenjV commented Mar 20, 2018

I am sorry, I made a mistake.
On my two Nasses I have a share with the same name and when I was testing yesterday I must have mistakingly used the wrong Nas for my test with the synoacltool -get command.

You were right it gives this output when the share has no ACL support:
(synoacltool.c, 359)It's Linux mode
and the return code is 255

@Safihre
Copy link
Contributor

Safihre commented Mar 20, 2018

That's pretty general.. I also see that sometimes on DSM6 when old-style packages did some chown commands. 😢

@BenjV
Copy link

BenjV commented Mar 20, 2018

Yes it is general but enough to determine if the share has to be converted to support ACL's.
If I give that command on a share that is converted, I get a whole list of ACL's and a return code of 0.

@ZeroZorro999
Copy link

Sorry to intrude, but reading this thread, will this ACL-requirement, when implemented, has as side-effect that external devices (usbshares) won't be able to be the destination shared-folder for Sonarr/Radarr?

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

Successfully merging this pull request may close these issues.

5 participants