SynoCommunity / spksrc

Cross compilation framework to create native packages for the Synology's NAS
https://synocommunity.com
Other
3.04k stars 1.24k forks source link

Package framework Permission management design #3217

Closed ymartin59 closed 2 years ago

ymartin59 commented 6 years ago

This is discussion room to review concept designed at https://github.com/SynoCommunity/spksrc/wiki/Permission-Concept @m4tt075 @Safihre @BenjV What are your opinion about ? Too complex ? WTF most don't care about security ? Defaults I propose are "too permissive" from my point of view but wizard will allow to restrict, so I am OK with that.

BenjV commented 6 years ago

@ymartin59 I don't understand your tekst about the Previous situation. You are writing:

•If an application mixes these two roles, both "input groups" and "ouput groups" can be configured separately.

But you defined both as sc-download in the paragraph above, should the last one of them not be sc-media?

Furthermore I really don't like this line:

Because of transition from previous situation, installation wizard proposes to enlist application in both sc-download and users groups by defaults.

BenjV commented 6 years ago

For some reason I cannot write anymore to the post before so I go on in this one.

On a linux system the group "users" is there for system management purpose and not for granting privileges to files. An application is not a user. Lets not make the same mistake that Synology made in the pre DSM 6 systems to put everything in the user group. If someone want to do that let them do it themselves in via DSM and not via the package installer.

Safihre commented 6 years ago

@BenjV agreed, nothing about this is the way it actually should be. Both due to old SynoCommunity approach and apparently how DSM did it. So what would be a reasonable fix? You have seen the various topics here with frustrated users that suddenly have broken packages. Should we really break things just to force things that they probably don't care about?

Personally, I would not add package user to users by default. Because what we have seen in the test packages for new platforms (#3138), is that for fresh installs the sc-download approach works exactly as designed. So why not use the syno_user_add_to_legacy_group function to only add a service user to users if the old-style user (without sc- prefix) was in users? So only for upgrades of old packages.

ymartin59 commented 6 years ago

@BenjV I agree with you about "users" group... As SynoCommunity packages delivered for DSM 5 (and before) relies on "users" group to access content on Shared Folder, human users got used to this "open-bar" situation. That is why (to avoid too many issues/support request) to keep both "sc-download" and "users" as defaults in wizard step - for permissions to be OK for people who don't care about security, with a notice to recommend to remove "users" from field values.

As implementation details, "input groups" will be used to enlist application technical user account as group membership, so that it can read content granted to that groups On the contrary, "output groups" will be used to set RW group permissions on "download" application specific folders ("complete" target directory of "transmission" for instance) By defaults, "sc-download" is recommended for both, but DSM administrator may change values and use "sc-media" for "input groups" for instance. Of course documentation will (try to) help administrator to do proper decision according to its DSM usage.

ymartin59 commented 6 years ago

@Safihre Your idea make sense and is easier than my "highly-configurable" proposal... Just a "but": for users having already installed published packages (hopefully thanks to my slowliness, only: transmission, radarr, sonarr), old-style user is already gone (busybox userdel) and we can no longer test if it is member of "users"...

BenjV commented 6 years ago

I don't agree with this proposal. This way we keep this situation alive forever.

Another way could be to let the package installer add the sc-download group to all the old package users of other installed package on that system so everything keeps working. Then when a package is upgraded we remove the old username and use sc-

This will ask for some programming to get a list of installed packages and to add the sc-dowload to their old-style username.

Safihre commented 6 years ago

@BenjV we would only keep it allive for upgrading users. As you saw in those issues, even with perfectly correctly applied permissions they still have problems. On top of that files created previously by old Sonarr user cannot be accessed by new Sonarr user since we don't force permissions on those like we do for download programs.

BenjV commented 6 years ago

Most people only upgrade so we will keep this alive forever.

The fact that old files cannot be accessed by newly installed or upgraded packages cannot be avoided without keeping the system of adding everything to the users group alive. Giving the correct advise to add group permissions of sc-download to those files should be sufficient. We should create a in depth wiki page for how to do that because people don't read the whole issue thread so we keep telling the same thing to lots of people

The biggest problem was that ACL's were used which made things very complicated, because of the mix of ACL and non-ACL shares on a NAS.

Safihre commented 6 years ago

Yes, a cleae wiki would help a lot. I can help with that! But as I mentioned a couple of times now (@ymartin59) we need to have all packages out at once, we can't have the download packages like nzbget and sabnzbd use non-ACL while Sonarr/Radarr use ACL. But then we need to be 100% sure about the approach of course.

BenjV commented 6 years ago

I vote to get ride of the ACL's, they create more problems then solutions. DSM is a Linux platform and not a Windows platform

Safihre commented 6 years ago

@BenjV What do you mean? How would this work? Even the simplest thing is a problem: if a package-user is not part of users, when it creates a folder, you can't even open it from File Station. Because the Administrators group doesn't have permissions for this package-created-folder. If you then use File Station to give yourself permission to a new folder, it will automatically apply ACL. That's why before everything was just part of users.

BenjV commented 6 years ago

If that's true then we have another problem. Old shares (created before DSM 5) do not have ACL setting and won't work with ACL's at all.

I keep forgetting that administrators are not root on DSM 5 and later.

Safihre commented 6 years ago

Guys, what are we going to do? There is truly no ideal solution, so which one do we take? If I summarize correctly we have 3 options:

  1. Use syno_user_add_to_legacy_group to add package-user to users on upgrade (if present before).
  2. Check if ACL is active during preinstall and force users to use them first before allowing the installation?
  3. Abandon the ACL approach and just use old-style Linux permissions (resulting in still every package user having access to all files, similar to 2).

I vote for 1.

BenjV commented 6 years ago

In my view are 1 and 3 no options and 2 is not possible, because the package does not always know which share will be used and the ACL setting is done on a per share base.

So maybe we should try option 2 and give the advice to convert all non ACL shares on the NAS. I think that most users are not even aware there could be two different types of shares on their NAS.

m4tt075 commented 6 years ago

Leaning towards 2 like @BenjV , as 1 will just open up everything again which is what we wanted to avoid in the first place. Preferring 2 over 3 as I believe that ACL permissions "overrule" the Unix ones, and users might want to use ACL permissions, independently of what our package installers do, which would create issues again...

ymartin59 commented 6 years ago

@m4tt075 @Safihre @BenjV I have documented permission management specifically for our usage: https://github.com/SynoCommunity/spksrc/wiki/Permission-Management What do you think about it? Do not hesitate to propose improvements

m4tt075 commented 6 years ago

@ymartin59 Thanks a lot. My thoughts: Firstly, I take that you don't want to revert to broad open access either, which from my perspective makes sense. Secondly, I think the documentation is clear. But then again, we have all probably spent waaaaay too much time with this stuff already. Eventually, only regular user feedback will let us know. But that's fine and we will see over time as soon as we will redirect users to your Wiki entry. Lastly, you always refer to sc-download permissions, but there might be others you want to grant, like to sc-media. Probably this needs to be generalized or at least mentioned. That's it. Thanks again.

ymartin59 commented 6 years ago

I will implement proposal of @Safihre to know: if previous user account running service (before migrating to either synouser on DSM 5 or SDK privilege on DSM 6) is member of users, then new account is added as group member too. It may help. @Safihre decided to rename sc-media of some non-published packages into sc-download to simplify. My idea is to only rely on a single group sc-download.

Safihre commented 6 years ago

So we will do option 1 then? It's the most user friendly indeed (so I like it), but opposed by @BenjV and @m4tt075 😉

m4tt075 commented 6 years ago

If two people always have the same opinion, one of them is redundant! That's fine for me. Make a call, move forward, live with it. There are packages to publish... :smirk:

BenjV commented 6 years ago

I agree, it is important to have these discussions because this is the time to do it even if it means that one has to change one's view on the matter. When everything is implemented it will be hard to change things.

BenjV commented 6 years ago

@ymartin59 Nice tekst but maybe add some explanation of the need to convert the shares to ACL for people who upgrades them for older DSM versions.

I did it on my own NAS and made some screenshot you can use in the wiki. I put them on my github, you can find them here.

https://github.com/BenjV/SYNO-packages/raw/master/Shared%20Folders.JPG https://github.com/BenjV/SYNO-packages/raw/master/Conversion%20Wizard.JPG https://github.com/BenjV/SYNO-packages/raw/master/Warning.JPG

ymartin59 commented 6 years ago

@BenjV Thanks but I think you are mistaken - Windows ACL support is not required by our design - DSM provides Linux ACL support by defaults on any Share Folder. If a folder is in "Linux mode", synoacl command will "upgrade" it with ACL support. As far as I understand, Synology has created its own file system and ACL support is probably different from what we are used to with setfacl.

According to #3212, it sounds like proper documentation has benefits !

ymartin59 commented 6 years ago

@Safihre I am not sure I can reuse syno_user_add_to_legacy_group to test "users" group member for a "legacy user" created by BusyBox. Does synogroup work as expected ?

ymartin59 commented 6 years ago

@Safihre Another question about syno_user_add_to_legacy_group code. Is it really require to remove LEGACY_USER from LEGACY_GROUP when busybox delgroup and deluser are invoked just after? My aim is to reuse generic syno_user_add_to_group as both are closed.

BenjV commented 6 years ago

@ymartin59 You are mistaken. There is no difference in Linux ACL and Windows ACL on DSM.

If you have an old installation with shares created before DSM 5 and not converted, then ACL's don't work at all on those shares. This will only happen is you have a system with shares created in DSM 4 (or maybe an early DSM 5). This will be very confusing because most people don't even know they have those shares and what the difference is.

ymartin59 commented 6 years ago

Thanks for explanations. I have included your screenshots to documentation: https://github.com/SynoCommunity/spksrc/wiki/Permission-Management

Safihre commented 6 years ago

@ymartin59 I think we do need to remove the user, as I remember seeing errors in the synopkg.log where deluser says something like "Can't delete user X, it is still member of a group". But might be good to verify this, as it would indeed simplify the code.

synogroup seems to work as expected, since this code succeeded in upgrading "old-style" to "new-style" packages and adding them to the old-group again.

ymartin59 commented 6 years ago

@BenjV @Safihre @m4tt075 Everybody OK to close this discussion ? In recent issues, feedback seems positive concerning documentation about permission settings. I propose to add link to documentation in installation and upgrade wizards like this: https://github.com/SynoCommunity/spksrc/pull/3230/files#diff-dfe13e790db1a06a5114a984f9cb3984R4 (it would be even a relevant improvement to generate wizards to avoid such copy/paste pattern !)

m4tt075 commented 6 years ago

+1

Safihre commented 6 years ago

Hope this does the trick!

BenjV commented 6 years ago

+1

Diaoul commented 6 years ago

I believe we should keep sc-media and sc-download as usually those define two distinct use cases. Also there's currently a difference as Sonarr uses sc-media but Radarr uses sc-download.

The idea was that download packages use the sc-download group to put files to some download folder. The sc-media group is dedicated for media managers and usually nead read (and write) to some media library directory. They also might need read permissions for the download folder. Separation of concerns is the idea behind this. I like my downloaders (transmission and nzbget) not being able to access my media libraries and my media managers not being able to write to the download folder.

If we put all packages in the sc-download group then we should rename it to something more generic. Sonarr, Radarr and others don't actually download anything.

Safihre commented 6 years ago

@Diaoul but that's based on the assumption that media managers only need read permissions. That's a wrong assumption since almost all of them (Sonarr/CouchPotato/Sickrage) move media files to new locations or rename files, otherwise they would have very limited functionality. For example tvheaded is a real media manager in that respect, but all of the others need direct read and write access to download folders to function.

Diaoul commented 6 years ago

That's true, unless you use copy (torrent users I imagine). But still, transmission doesn't need access to my media library. I won't argue more than that, if y'all think that's a good idea to keep a single group to ease permission management let's use that. I switched to ACL and there's really no pain in setting permissions correctly now. However in that case I think the name of the group is not the best ^^

ymartin59 commented 6 years ago

Here is a proposal to get it straight forward for users: transmission or nzbget packages may create "download location" folders with write permissions for both sc-download and sc-media groups. But then, end users should not care about sc-download group on their files, but about sc-media ! What do you think about it?

Safihre commented 6 years ago

How do we enforce that? Without making the user (nzbget/transmission) a member of both groups and thus giving them access to everything?

BenjV commented 6 years ago

There is actualy only one way to do this.

The normal flow would be something like this in for example the use of torrents. Indexers (Sonarr, Radarr, SickRage etc) look at the internet and create .torrents files The downloaders (DownloadStation, Transmission) must read those files(and delete them) and create downloaded files. Then the Indexers pick them up again and do renaming, moving etc with them. As you can see all need to be in common groups, so in my opinion it is desirable (like Diaoul said) to separate the privs but in practice it is not possible. Everybody would end up giving access to both groups and so I propose to stick to only one group namely sc-media. Otherwise it will get too complicated.

Diaoul commented 6 years ago

Actually, Sonarr or Radarr use the Transmission and Nzbget API to download, no torrent file is created anywhere by them on the filesystem. No permissions required here.

What is true is that media need access to the download folder at least in read for copy or write for move. However, downloaders don't need access to the media library so by using a single group we loose granularity.

I suggest we keep those separated even if they share a good part of their permissions so we have one group per role.

This should only be done in the package setup so power users can run a few commands and tweak the groups as they like afterwards. Maybe we should even leave the option to use a group or not in the wizard but then it's getting messy.

What needs to be documented is the ACL part, I have recently done a new setup and with ACLs I haven't had any permission issue. This needs to be properly setup with inheritance. Also file ownership may require fixing.

I don't know if that used to be the case but I noticed with ACL I can grant individual permissions for system users. However it's not possible to add system users to a group from DSM. This provides the finest granularity possible.

To sum up

Solutions

No group

Users can give individual permissions. For the brave.

One global group

For the lazy.

One group per "role"

Opinionated but clean approach IMO. Both granular and user friendly.

Whatever we end up choosing, this must not be enforced and be reversible. i.e. if I deleted the sc-media group because I chose the brave approach, package should work and not recreate the group in an update for example.

BenjV commented 6 years ago

Not all Indexer packages use API and depending on what downloader is used they have to fallback to the "Black Whole" methode, so the per "role" approach would end up giving all the roles to all packages.

I would vote for one group and if users wants more granular approach, they can use ACL's themselves. If we make it too complicated for the majority of users it they just won't understand it and the tech-savvy users will do whatever they want and could remove the group.

It is a good idea to just set those groups just in the postinstall phase and not in the postupgrade phase so changes by the users will not be revoked.

Safihre commented 6 years ago

I agree with @BenjV, as I am also not sure how we could manage the perfect situation of @Diaoul.

It is a good idea to just set those groups just in the postinstall phase and not in the postupgrade phase so changes by the users will not be revoked.

Problem here is that we still need to correct the "old-package" situation, so we can't detect if it's new-style to new-style or old-style to new-style upgrade.

BenjV commented 6 years ago

You could use the old package version (SYNOPKG_OLD_PKGVER) which is available in the preupgrade phase for that detection.

ymartin59 commented 6 years ago

If I come back with my concept where administrator can select one or multiple "input" and "output" groups for each service, A wizard script (only available for DSM >= 6, as fallback static wizard for DSM 5.2) may be use to investigate current system situation: fresh install or upgrade, service user in group or not... And so generate appropriate options to select: no group, one group, role groups. But I fear it may not be ready soon.

BenjV commented 6 years ago

I would not do this group choosing during installation.. This forum will be swamped with questions why things are not working and the reason would be, that during installation they made the wrong choices for the groups because they don't understand what the implication is.

I still feel that just one group is the best approach. Don't make things too complicated and don't forget that a normal user cannot change this because the package user in not visible in DSM so they cannot add to group or delete from a group.

Also after thinking about it some more, I would say that adding the group during upgrade should be done always. If tech-savvy users don't like to use the group they should not remove that group but just manage the access rights of that group via DSM (remove read/wright access) and not delete that group.

ymartin59 commented 6 years ago

@BenjV I agree. Packages go on with sc-download by default, with permission granted to configured folder (download or watch location typically) Users have documentation to grant permissions to any other location for applications to be able to read/write when configuring media library locations (or any other file types).

For advanced administrators, permissions can be applied per package with "local user". It is possible to remove local service user accounts from sc-download group membership from command line (may be documented - but has to be re-applied after each upgrade).

The concept of "per role group" can be implemented thanks to command lines too (add/remove local service group memberships). As recommendation, a custom shell script to run regularly for applying its own "security concept" on all installed package service users should be convenience enough.

Thanks all for your ideas. Even if default group name choice sc-download is not perfect, I prefer to leave it as is to keep pace with users - by the way, most of packages creating folder for storage are "downloaders".

I will convert my previous "permission concept" documentation with audience to "advanced usage" with this "custom security concept" by script/command-line.

Diaoul commented 6 years ago

Have you noticed that system users can be granted permissions to shares ? I don't think we should document creating a custom user.

Rather, assign the system user to the group at install time only, leaving the liberty for power users to change it afterwards or even leave it there and manage permissions with the above-mentioned feature.

Le jeu. 22 mars 2018 à 10:42, Yves Martin notifications@github.com a écrit :

@BenjV https://github.com/benjv I agree. Packages go on with sc-download by default, with permission granted to configured folder (download or watch location typically) Users have documentation to grant permissions to any other location for applications to be able to read/write when configuring media library locations (or any other file types).

For advanced administrators, permissions can be applied per package with "local user". It is possible to remove local service user accounts from sc-download group membership from command line (may be documented - but has to be re-applied after each upgrade).

The concept of "per role group" can be implemented thanks to command lines too (add/remove local service group memberships). As recommendation, a custom shell script to run regularly for applying its own "security concept" on all installed package service users should be convenience enough.

Thanks all for your ideas. Even if default group name choice sc-download is not perfect, I prefer to leave it as is to keep pace with users - by the way, most of packages creating folder for storage are "downloaders".

I will convert my previous "permission concept" documentation with audience to "advanced usage" with this "custom security concept" by script/command-line.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SynoCommunity/spksrc/issues/3217#issuecomment-375235942, or mute the thread https://github.com/notifications/unsubscribe-auth/AATe9Fm2Mw6CXmIYw0fdwwquvMCPy7-7ks5tg3IMgaJpZM4Slfpe .

ymartin59 commented 6 years ago

@Diaoul Yes, I am aware system user can be grant permission either on Share, or on folder level. But on large system with many packages, probably it may became hard to manage.

Documenting creation of "custom groups" is for the "per role group" option and would be only for advanced users. I consider it a convenient to set permissions on multiple Share or folders, thanks to a grouping many applications - moreover if admin prefer to manage permission at folder level inside a single Share, instead of having many Share to distribute on volumes. When adding an application, enlisting its service user into relevant group and permissions are immediately OK. By the way, I consider it only a small documentation effort (for both per user/share and custom groups) as I will reuse the permission concept alternative I proposed few weeks ago.

BenjV commented 6 years ago

To summarize everything:

If we are only using one group, maybe we should just choose another name to avoid confusion with older packages were the sc-media and sc-download are already used.

I propose to name the group synocom

ymartin59 commented 6 years ago

Some details:

@BenjV I really think like @Safihre it is too late to rename sc-download group... Many packages are already out with that group name #3138 and I imagine troubles in case of rename now.

Diaoul commented 6 years ago

I agree not to do the renaming right now but that must be taken into account for future updates maybe? Also, I insist that the group assignment stuff should only happen when migrating from the "old way". Here is an example on how to manage incremental migrations. This way if say I want to rename the groups myself or assign the user to other groups I won't have to worry about upgrades breaking my permission setup.