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

Improve documentation on permission setup #4

Closed
jidanni opened this issue Mar 14, 2019 · 11 comments
Closed

Improve documentation on permission setup #4

jidanni opened this issue Mar 14, 2019 · 11 comments
Assignees

Comments

@jidanni
Copy link

jidanni commented Mar 14, 2019

On https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=922763
they say to tell you directly about those documentation improvements.

@ndim ndim self-assigned this Mar 16, 2019
@ndim
Copy link
Member

ndim commented Mar 17, 2019

This comment is about both the Debian beep package and upstream beep and also the relationship between the two. I am writing this comment without having access to an up to date Debian system with an up to date beep package. The information source this comment is based on are Debian's beep package sources debian-1.4.3 branch and Debian bug #922763.

As I understand Debian bug #922763, the issue is that in the Debian beep package, the information in INSTALL.md partially overlaps with the information in PERMISSIONS.md, and that the contents of INSTALL.md and PERMISSIONS.md do not exactly match the permission setup the Debian beep package uses.

So, let me explain the status quo from the upstream beep point of view, the Debian package status quo and how I would solve some of the problems.

Status Quo: Upstream beep

When I moved beep from the ancient root-only API to using the newer input device file based API, I had to invent a scheme of how to grant users access to the PC speaker input device file. The classic Linux way to solve the permission issue was a user group for the device and adding the users to the device, so I chose that as the example permission setup suggested by upstream beep, and used that very setup in the Fedora beep package.

However, I tried to keep the upstream beep package distinct from the Fedora package and so I wrote redundant information in INSTALL.md and PERMISSIONS.md on purpose.

INSTALL.md describes how to build and install beep starting with a source tree. That kind of information is not useful for someone who is using a distribution's compiled binary package. INSTALL.md describes one way of setting up the permissions which gives users the permissions to beep the PC speaker. This gives both people installing from source tree and distribution packagers an idea about how the permission setup works in principle, but there are literally infinetely many other ways to do the permission setup instead, and the beep executable works will all of them.

If a distribution chooses to adopt the permission setup described by INSTALL.md, most of the permission setup will be done by the distribution's binary package and thus needs no description for the person having installed said binary package. PERMISSIONS.md then describes what the person having installed a beep distro package needs to do.

So the PERMISSIONS.md in upstream beep is an example file which describes how to set up the permissions to the user of a compiled binary package which happens to work exactly if that binary package uses the permission setup suggested by upstream beep.

As mentioned in the PACKAGING.md file section Documentation files, the INSTALL.md file is not intended to be shipped in a compiled binary package.

If a distribution's packager has decided for the distribution's binary package to use another way to set up permissions, PERMISSIONS.md does not apply any more and needs to be removed, changed, or rewritten completely. I tried to describe that in PACKAGING.md.

So much for the status quo in upstream beep.

Status Quo: The Debian Package

From what I can see, the Debian package does not use user groups for the permission setup. Instead, it uses TAG+="uaccess" to give all local users access to the PC speaker and leaves the actual permission setup to the udev subsystem and its integration with the login system. So the Debian package should not ship a PERMISSIONS.md file describing something not found in the Debian package at all.

The TAG+="uaccess" based approach has the great advantage that there is no need to add users to a beep group. So for many people, the permission setup will not require any sysadmin intervention at all after installing Debian's beep package.

The disadvantage of the TAG+="uaccess" approach is that for remote users (e.g. logged in via ssh) and for users not currently using the active virtual console, beeping does not work. So I would have expected the Debian package to not ship upstream beep's PERMISSIONS.md and to ship its own PERMISSIONS.Debian instead, describing how sysadmins are supposed to set up permissions for the users on their Debian systems who are not covered by TAG+="uaccess".

Possible Solution

The TAG+="uaccess"based approach is a very nice way to do the default permission setup, so I would like to make it part of the way upstream beep suggests permission setup in INSTALL.md.

However, for non-local users and users not logged into the currently active virtual console, classical user/group permission setup must still be performed. There are a number of interesting use cases for beep requiring speaker access for non-local users or users not owning the current virtual console. So more comprehensive (i.e. explicitly user/group based) permission setup must at least be described, if not prepared by the distro package.

And this is where we arrive at the very same setup beep upstream is suggesting and which the Fedora package implements: The beep distro package provides a beep system user group to which users can be added, and a udev script to add beep group write access to the input device special file.

The group access could be provided by either

  • GROUP="beep", MODE="0620" in the udev rule to change the input device special file's group from the default input to beep, which might have implications for some input group process

  • RUN=".../setfacl -m g:beep:w ..." in the udev rule which requires setfacl from the acl package

I personally think every distro package should also offer the group based permission setup as a finished solution where the sysadmin only needs to add a few users to the beep group, but distro packagers are more familiar with what is suitable for their respective distro and whether or how they want to support non-local users and non-current-console-owner users.

The PERMISSIONS.md could also well include a few explanations as to why some maybe kind of obvious ways of setting up permissions maybe actually are a bit less obvious ways to shoot yourself in the foot.

I will implement those changes in the next few days.

Comments welcome.

@ndim ndim changed the title Documentation improvements Improve documentation on permission setup Mar 17, 2019
@kanliot
Copy link

kanliot commented Mar 20, 2019

I believe that beep is no longer SUID in debian testing. but rather BEEP is still testing for uaccess, does that mean this repo is specialized for debian?

The disadvantage of the TAG+="uaccess" approach is that for remote users (e.g. logged in via ssh) and for users not currently using the active virtual console, beeping does not work.

Yeah, is why I'm here. Looking forward to getting beeps back. I have to wonder if there's a different way to make the beep signal handler resistant to reentrant exploit, although, notification beeps do tend to all finish at the same time anyhow. Good luck

@ndim
Copy link
Member

ndim commented Mar 22, 2019

On the "finish at the same time" issue: There is only one PC speaker hardware. So parallel runs of beep will finish beeping when the first beep process issues the "stop beeping" command because its beep is done. The other beep processes will issue their respective "stop beeping" command later, which will not change the fact that the PC speaker has been silent since the first "stop beeping" command.

There is no way to change that.

@ndim
Copy link
Member

ndim commented Mar 23, 2019

@kanliot beep does not test for any permissions. beep opens the /dev/input/by-path/platform-pcspkr-event-spkr device special file for writing. The kernel will allow that depending on the standard file permissions that file has.

To set up those file permissions, we define udev rules. Most distributions today come with systemd-udev and a bunch of freedesktop.org based conventions built on that. One such convention is that appending the uaccess tag will mark a device as belonging to the user currently logged in locally. This is not Debian or Fedora specific, and will provide a good out of the box experience for many.

For other users, I suggest a beep group, adding the users to that group and then using either GROUP="beep" or setfacl in the udev rule to allow group write access.

Still working on boiling down the facts into a simple and flexible set of conventions and instructions.

@kanliot
Copy link

kanliot commented Mar 26, 2019

device as belonging to the user currently logged in locally.

My use case for /bin/beep is scripting. Rather than "checking" a job to see if it's still running, the jobs finish by calling beep.

So I might log in several times, and each login would need to have access to beep.

Also,
$ beep & beep
does behave like there's serial access, but it's very possible to play two beeps at the same time in Debian, it's just not consistent ( which is good, since I want jobs to actually finish)

@ndim
Copy link
Member

ndim commented Mar 29, 2019

This command will show you that there is no serialized access to the PC speaker:

[user@host ~]$ beep -f 220 -l 4000 & (sleep 1; beep -f 440 -l 100) & wait

The first beep is supposed to run for 4 seconds (4000), but is interrupted after 1 second (1) by a short (100) beep one octave higher. About 1.1 seconds after hitting enter, the PC speaker will be and remain silent. The first beep process will continue to wait for another 2.9 seconds before silencing the already silent PC speaker.

@ndim ndim closed this as completed in 0c061db Mar 29, 2019
@ndim
Copy link
Member

ndim commented Mar 29, 2019

Please reopen this if you spot something I should have adressed when rewriting the documentation.

Non-complete summary of what I did:

  • PERMISSIONS.md now describes permission setup, whether you are installing from source, using a distro package, use the suggested way of permission setup (TAGS+="uaccess" for local users and beep user group for anyone else) or use a different way.

  • INSTALL.md describes how to install from source. For the permission setup, it refers to PERMISSIONS.md.

  • PACKAGING.md describes how to package beep for distributions. Regarding permission setup, it now refers to PERMISSIONS.md.

  • README.md now lists all documentation files with a short description of their contents.

No udev rules files are shipped any more, so packagers need to provide their own, preferably by following the suggested permission setup from PERMISSIONS.md.

If there are no corrections or suggestions within the next few days, I plan to release this as 1.4.4 soon.

@jidanni
Copy link
Author

jidanni commented Apr 3, 2019

Well all I know is maybe ask the Debian maintainer (Rhonda) what she thinks.

@ndim
Copy link
Member

ndim commented Apr 3, 2019

@rhonda Any comments on permission setup and the related documentation?

Summarizing:

  • INSTALL.md should not be installed by binary distro packages. Building the distro package already entails all the steps laid out in INSTALL.md.

  • PERMISSIONS.md now contains everything a user/sysadmin needs to know about permission setup. It suggests TAG+="uaccess" for the local user plus a beep system user group for all other users. This covers all the use cases I can think of, and those described by users in Improve documentation on permission setup #4 and Running under sudo is not synonymous with the program having extra privileges #5.

    Distribution packagers can ship whatever permission setup fits their distribution best. If some user's use case differs from the ones anticipated by the distro package, PERMISSIONS.md should help them achieve their goals, and documentation specific to the distro package (such as README.Debian or README.fedora) can contain short cuts or specific instructions. beep.1.in contains a line suitable for sed substitution with a reference to README.Distro.

  • PACKAGING.md now reflects those changes.

Oh, and BTW, the comment in Debian's beep package's 70-pcspkr-beep.rules does not match what the udev rule actually does. Not being a Debian user, I do not know where I would report that.

Reopening this to allow for more discussion.

@ndim ndim reopened this Apr 3, 2019
@ndim
Copy link
Member

ndim commented Apr 8, 2019

FTR: While this upstream issue #4 has been opened in connection with Debian bug #922763, Debian beep package maintainer @rhonda makes their point of view quite clear in Debian bug #922667 as to how they want the permission setup by the Debian beep package:

  • Default access for locally logged in users (that is the TAG+="uaccess" part)
  • Everything else like setting up a beep group, installing the acl package, etc. is left to the sysadmin

I can understand that avoiding a dependency on an additional package (the acl package) can be useful to keep distributions small, even though I was surprised that a 2019 distribution can be designed to work without ACLs.

However, I personally think beep can and should be more friendly to users. That is why I, in my role as beep upstream, suggest setting up the beep group by default, and in my role as Fedora packager, implement that in the beep Fedora package. Then sysadmins only have to add users to the beep group, which is relatively easy. But different distributions have different perspectives on such things.

I hope the current PERMISSIONS.md explains the permission setup well enough such that people for whom their distribution package's permission setup does not work can help themselves. That is all I can do as upstream.

@ndim
Copy link
Member

ndim commented May 26, 2019

As witnessed by six weeks without any discussion, this issue appears not to raise any more comments.

Closing this issue.

@ndim ndim closed this as completed May 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants