Installing Samba unexpectedly adds many unknown local users to sambashare group

Status changed to 'Confirmed' because the bug affects multiple users.

Hello Richard and thanks for this bug report. You are right: the samba.postinst script adds all the users in the 'admin' group to the 'sambashare' group:

for USER in `getent group admin | cut -f4 -d:`; do
    adduser "$USER" sambashare \
    || ! getent passwd "$USER" >/dev/null
done

The interesting thing is that I had a look in some Ubuntu systems I have access to and there's no 'admin' group at all. I think the 'admin' group is a legacy thing: the Ubuntu 12.04 release notes [1] report that:

    Up until Ubuntu 11.10, administrator access using the
    sudo tool was granted via the "admin" Unix group. In
    Ubuntu 12.04, administrator access will be granted
    via the "sudo" group.

So in the older Ubuntu releases the 'admin' did always exist as a local group.

The obvious fix here is to have the postinst script add users from the 'sudo' group instead, however I'm not sure I really like this idea. We'll again hardcode a group name in the postinst, and as apparently nobody noticed that the "auto-add admin/sudo users to sambashare" mechanism was broken in all these years, maybe it's not really worth having it in the first place.

Not having it will also make Ubuntu's samba behave like Debian in this respect. Note however that the postinst snippet is present in the Debian package (not a delta), so we should really try to land the fix in Debian, as it would be awkward to have an Ubuntu delta that fixes Ubuntu specific code.

This is worth some more discussion.

[1] https://wiki.ubuntu.com/PrecisePangolin/ReleaseNotes/UbuntuDesktop#PrecisePangolin.2FReleaseNotes.2FCommonInfrastructure.Common_Infrastructure

This is what I propose, see the commit message for the rationale:

https://salsa.debian.org/paride/samba/-/commit/4385c3920afa81e2c30a74d80f2ee0eb04753bb9

I didn't open a MR yet, let's agree on the approach first.

Thanks for the commit, Paride. I left a comment there, but I'm also going to copy it over here:

First of all, thank you for taking the time to investigate and write an insightful commit message explaining the situation.

My first thought when reviewing this was: "where did this excerpt of code came from?" I did some archaeology (`git blame` FTW) and found that this was added by commit fe5cef5014db5b5d6cf55e036583f8f84962e9b2. Unfortunately, there doesn't seem to be much context around the decision to add this code to the postinst script; I couldn't find a related Ubuntu bug, nor a mailing list thread that could provide some explanation here.

Either way, after thinking a little bit, and especially after considering the arguments you raised in the commit message, I find myself aligned with your rationale. While I can understand why one would want to use the members of the `admin` group as the initial members of the `sambashare` group, I also tend to think that this is perhaps "too invasive" without much benefit. The user should be the one deciding who is part of `sambashare` and who isn't, just like it is with the Debian samba package.

Given that this is an Ubuntu-related topic, I will copy this comment to the Launchpad bug and we can continue the discussion there if you want. I'm particularly interested in knowing whether you foresee any problems with this approach (I don't).

Thanks!

Thanks for reviewing Sergio. I did the same archeology and indeed the first line of that commit message is "This reverts commit fe5cef50." :-)

I don't foresee any problem with the approach that commit implements: it just feels strange to fix a bug by dropping functionality, even if broken and apparently not missed by anyone. However as we agree on it I'll go ahead and open a MR on salsa.

Here it is: https://salsa.debian.org/samba-team/samba/-/merge_requests/54

This bug was fixed in the package samba - 2:4.13.5+dfsg-2ubuntu3

---------------
samba (2:4.13.5+dfsg-2ubuntu3) impish; urgency=medium

  * d/samba.postinst: do not populate sambashare from the admin group
    (Debian packaging cherry-pick. LP: #1942195)

 -- Paride Legovini <email address hidden> Wed, 06 Oct 2021 10:31:14 +0200

The upload to the devel release targeted impish, but actually landed in jammy. Added an Impish task for the SRU.

Paride, firstly thanks for picking this up. Looking at another machine I have I notice that the lxd group may have a similar issue. This suggests the problem may affect other packages as well. Is there a way to trigger an audit for other packages that might be affected?

Hi Richard, very interesting, you are right. The Bionic lxd.preinst script has:

# Add each admin user to the lxd group - for systems installed
# before precise
for u in $(getent group admin | sed -e "s/^.*://" -e "s/,/ /g"); do
    adduser "$u" lxd >/dev/null || true
done

I tried to install Bionic from the ISO image and I can confirm the admin group doesn't get automatically created at install time. This only affects Bionic as in >= Focal lxd is a snap, and I'm pretty sure the snap doesn't touch the groups.

I filed this bug against the lxd package for the lxd maintainers to have a look:

https://bugs.launchpad.net/ubuntu/+source/lxd/+bug/1950635

Thanks again for raising the issue!

SRUs to fix this got uploaded to all the supported releases (thanks Sergio!):

 bionic
 focal
 hirsute
 impish

Now it's up to the SRU team to review the change.

Hello Richard, or anyone else affected,

Accepted samba into impish-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.13.14+dfsg-0ubuntu0.21.10.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-impish to verification-done-impish. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-impish. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Hello Richard, or anyone else affected,

Accepted samba into hirsute-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.13.14+dfsg-0ubuntu0.21.04.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-hirsute to verification-done-hirsute. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-hirsute. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Hello Richard, or anyone else affected,

Accepted samba into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.13.14+dfsg-0ubuntu0.20.04.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Hello Richard, or anyone else affected,

Accepted samba into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/samba/2:4.7.6+dfsg~ubuntu-0ubuntu2.24 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

All autopkgtests for the newly accepted samba (2:4.13.14+dfsg-0ubuntu0.21.10.2) for impish have finished running.
The following regressions have been reported in tests triggered by the package:

adsys/0.7.1 (arm64)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/impish/update_excuses.html#samba

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

The failed adsys/0.7.1 autopkgtest is due to a timeout:

*** Test killed with quit: ran too long (11m0s).
FAIL github.com/ubuntu/adsys/internal/policies/ad 660.011s

I'm trying with a simple retrigger for now. I'll do the verification once all the tests pass.

It failed again (total 3 times) in the same way:

https://autopkgtest.ubuntu.com/results/autopkgtest-impish/impish/arm64/a/adsys/20211118_112720_82d83@/log.gz

Verification done per [Test Case].

Note: the adsys autopkgtest failure on impish/arm64 is unrelated to the samba upload:

1. The part of the test that exercises samba passes. The failure is in an unrelated test.
2. The autopkgtest run fails even with adsys from impish-release; the test failure is not a regression caused by the samba upload.
3. The issue was discussed with didrocks (adsys "upstream"), and he agrees there isn't anything pointing at samba in the test failure. Discussion happened in #ubuntu-devel on 2021-11-19 [1].

[1] https://irclogs.ubuntu.com/2021/11/19/%23ubuntu-devel.html

Per team discussion we leave the decision whether adding a hint or triggering a migration-reference/0 autopkgtest run to the SRU team.

It indeed looks to be a bit unreliable, the failure that is. As it has been discussed with adsys upstream, let me hint it before releasing. Thanks!

Oh, and for the future: please always state which exact versions of packages for each series have been tested when performing verification, as other wise - in some cases - the verification can be rejected because of that.

This bug was fixed in the package samba - 2:4.13.14+dfsg-0ubuntu0.21.10.2

---------------
samba (2:4.13.14+dfsg-0ubuntu0.21.10.2) impish; urgency=medium

  * samba.postinst: do not populate sambashare from the Ubuntu admin group
    (LP: #1942195)

 -- Paride Legovini <email address hidden> Fri, 12 Nov 2021 11:17:14 +0100

The verification of the Stable Release Update for samba has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

This bug was fixed in the package samba - 2:4.13.14+dfsg-0ubuntu0.21.04.2

---------------
samba (2:4.13.14+dfsg-0ubuntu0.21.04.2) hirsute; urgency=medium

  * samba.postinst: do not populate sambashare from the Ubuntu admin group
    (LP: #1942195)

 -- Paride Legovini <email address hidden> Fri, 12 Nov 2021 14:44:50 +0100

This bug was fixed in the package samba - 2:4.13.14+dfsg-0ubuntu0.20.04.2

---------------
samba (2:4.13.14+dfsg-0ubuntu0.20.04.2) focal; urgency=medium

  * samba.postinst: do not populate sambashare from the Ubuntu admin group
    (LP: #1942195)

 -- Paride Legovini <email address hidden> Fri, 12 Nov 2021 14:42:02 +0100

This bug was fixed in the package samba - 2:4.7.6+dfsg~ubuntu-0ubuntu2.24

---------------
samba (2:4.7.6+dfsg~ubuntu-0ubuntu2.24) bionic; urgency=medium

  * samba.postinst: do not populate sambashare from the Ubuntu admin group
    (LP: #1942195)

 -- Paride Legovini <email address hidden> Wed, 10 Nov 2021 15:29:48 +0100