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

in place upgrade issue - base-passwd debconf interative question asks 'Remove group "qubes"'? #1105

Closed
adrelanos opened this Issue Aug 6, 2015 · 26 comments

Comments

Projects
None yet
4 participants
@adrelanos
Member

adrelanos commented Aug 6, 2015

This issue is affecting in place upgrades for Debian templates. Usability issue. (Might also break the whole template when answering 'yes'?)

Package configuration                                                                                                 
 ┌───────────────────────────────────────────┤ Configuring base-passwd ├───────────────────────────────────────────┐  
 │                                                                                                                 │  
 │ update-passwd has found a difference between your system accounts and the current Debian defaults.  It is       │  
 │ advisable to allow update-passwd to change your system; without those changes some packages might not work      │  
 │ correctly.  For more documentation on the Debian account policies, please see                                   │  
 │ /usr/share/doc/base-passwd/README.                                                                              │  
 │                                                                                                                 │  
 │ The proposed change is:                                                                                         │  
 │                                                                                                                 │  
 │ Remove group "qubes" (98)                                                                                       │  
 │                                                                                                                 │  
 │ If you allow this change, a backup of modified files will be made with the extension .org, which you can use    │  
 │ if necessary to restore the current settings.  If you do not make this change now, you can make it later with   │  
 │ the update-passwd utility.                                                                                      │  
 │                                                                                                                 │  
 │ Do you want to remove the group qubes?                                                                          │  
 │                                                                                                                 │  
 │                                 <Yes>                                    <No>                                   │  
 │                                                                                                                 │  
 └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘  

What should one answer? I guess 'no'?

Also happens on jessie -> stretch upgrades. Therefore unspecific to Whonix.

(This has initially been reported at @Whonix tracker)

@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Aug 6, 2015

Member

Yes, the answer should be 'no'. I guess the fix would be to change 'qubes' GID to something over 100 (but less than 1000), right?

Member

marmarek commented Aug 6, 2015

Yes, the answer should be 'no'. I guess the fix would be to change 'qubes' GID to something over 100 (but less than 1000), right?

@adrelanos

This comment has been minimized.

Show comment
Hide comment
@adrelanos

adrelanos Aug 6, 2015

Member

I don't know. I haven't figured out how to manually trigger it yet. Let alone to understand/fix it.

Member

adrelanos commented Aug 6, 2015

I don't know. I haven't figured out how to manually trigger it yet. Let alone to understand/fix it.

@adrelanos

This comment has been minimized.

Show comment
Hide comment
@adrelanos

adrelanos Aug 6, 2015

Member

debian/qubes-core-agent.preinst looks to be the cause indeed.
https://github.com/marmarek/qubes-core-agent-linux/blob/567a045bcdb44e1069c4005bc575ebb0627b466a/debian/qubes-core-agent.preinst#L40-L41

    groupadd --force --system --gid 98 qubes
    groupadd --force --system sudo

This is problematic. I speculate.

Member

adrelanos commented Aug 6, 2015

debian/qubes-core-agent.preinst looks to be the cause indeed.
https://github.com/marmarek/qubes-core-agent-linux/blob/567a045bcdb44e1069c4005bc575ebb0627b466a/debian/qubes-core-agent.preinst#L40-L41

    groupadd --force --system --gid 98 qubes
    groupadd --force --system sudo

This is problematic. I speculate.

@adrelanos

This comment has been minimized.

Show comment
Hide comment
@adrelanos

adrelanos Aug 6, 2015

Member

Using unusual ways to do that.

sudo grep -r useradd /var/lib/dpkg -> qubes-only
vs
sudo grep -r adduser /var/lib/dpkg -> many packages

Suggestion:

  • replace useradd with adduser.
  • groupadd -> adduser
  • usermod -> ?
Member

adrelanos commented Aug 6, 2015

Using unusual ways to do that.

sudo grep -r useradd /var/lib/dpkg -> qubes-only
vs
sudo grep -r adduser /var/lib/dpkg -> many packages

Suggestion:

  • replace useradd with adduser.
  • groupadd -> adduser
  • usermod -> ?
@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Aug 6, 2015

Member

According to /usr/share/doc/base-passwd/README, GID range 0-100 is reserved for base-passwd package. Don't know why static GID used here. @nrgaway why static GID here?
Also I don't see group sudo used anywhere (beside the fact that user is added there).

Member

marmarek commented Aug 6, 2015

According to /usr/share/doc/base-passwd/README, GID range 0-100 is reserved for base-passwd package. Don't know why static GID used here. @nrgaway why static GID here?
Also I don't see group sudo used anywhere (beside the fact that user is added there).

@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Aug 6, 2015

Member

useradd is a standard utility, adduser is Debian-specific wrapper. Since this is debian postinst script, I think its okay to use adduser, but I don't see need for change. Only get rid of static GID.

Member

marmarek commented Aug 6, 2015

useradd is a standard utility, adduser is Debian-specific wrapper. Since this is debian postinst script, I think its okay to use adduser, but I don't see need for change. Only get rid of static GID.

@nrgaway

This comment has been minimized.

Show comment
Hide comment
@nrgaway

nrgaway Aug 6, 2015

On 5 August 2015 at 22:13, Marek Marczykowski-Górecki <
notifications@github.com> wrote:

According to /usr/share/doc/base-passwd/README, GID range 0-100 is
reserved for base-passwd package. Don't know why static GID used here.
@nrgaway https://github.com/nrgaway why static GID here?
Also I don't see group sudo used anywhere (beside the fact that user is
added there).

I installed it as a system account (distribution based). Looks like Debian
assumes system accounts are own by it. Maybe there is another
configuration file to state that qubes is a valid system account, otherwise
if we could just dynamically allocate it moving forward, but not change it
for existing installations as it would mess up permissions bad.

nrgaway commented Aug 6, 2015

On 5 August 2015 at 22:13, Marek Marczykowski-Górecki <
notifications@github.com> wrote:

According to /usr/share/doc/base-passwd/README, GID range 0-100 is
reserved for base-passwd package. Don't know why static GID used here.
@nrgaway https://github.com/nrgaway why static GID here?
Also I don't see group sudo used anywhere (beside the fact that user is
added there).

I installed it as a system account (distribution based). Looks like Debian
assumes system accounts are own by it. Maybe there is another
configuration file to state that qubes is a valid system account, otherwise
if we could just dynamically allocate it moving forward, but not change it
for existing installations as it would mess up permissions bad.

@nrgaway

This comment has been minimized.

Show comment
Hide comment
@nrgaway

nrgaway Aug 6, 2015

On 5 August 2015 at 22:11, Patrick Schleizer notifications@github.com
wrote:

Using unusual ways to do that.

sudo grep -r useradd /var/lib/dpkg -> qubes-only
vs
sudo grep -r adduser /var/lib/dpkg -> many packages

Suggestion:

  • replace useradd with adduser.
  • groupadd -> adduser
  • usermod -> ?

I think @HW42 made the change from adduser to useradd. If there is no
issue with it, why replace it?

nrgaway commented Aug 6, 2015

On 5 August 2015 at 22:11, Patrick Schleizer notifications@github.com
wrote:

Using unusual ways to do that.

sudo grep -r useradd /var/lib/dpkg -> qubes-only
vs
sudo grep -r adduser /var/lib/dpkg -> many packages

Suggestion:

  • replace useradd with adduser.
  • groupadd -> adduser
  • usermod -> ?

I think @HW42 made the change from adduser to useradd. If there is no
issue with it, why replace it?

@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Aug 6, 2015

Member

On Thu, Aug 06, 2015 at 01:04:54PM -0700, nrgaway wrote:

I installed it as a system account (distribution based). Looks like Debian
assumes system accounts are own by it.

No, it doesn't assumes that. It only assumes it own static pool of
them, 0-99. Not the whole "system accounts pool" (0-999). The issue
here is you've chosen GID from that 0-99 pool. IMO there is no need to use
a static one here.

But if we need a static one (why?), it should be properly requested from
base-passwd package maintainers.

Maybe there is another
configuration file to state that qubes is a valid system account, otherwise
if we could just dynamically allocate it moving forward, but not change it
for existing installations as it would mess up permissions bad.

Yes, needs to be changed to dynamically allocated (from system pool,
keep the --system option). And for existing installation provide some
instructions how to fix it. Any idea? Change it manually (including
appropriate permissions fix)? Leave it as it is, answer 'no' for that
debconf question?

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Member

marmarek commented Aug 6, 2015

On Thu, Aug 06, 2015 at 01:04:54PM -0700, nrgaway wrote:

I installed it as a system account (distribution based). Looks like Debian
assumes system accounts are own by it.

No, it doesn't assumes that. It only assumes it own static pool of
them, 0-99. Not the whole "system accounts pool" (0-999). The issue
here is you've chosen GID from that 0-99 pool. IMO there is no need to use
a static one here.

But if we need a static one (why?), it should be properly requested from
base-passwd package maintainers.

Maybe there is another
configuration file to state that qubes is a valid system account, otherwise
if we could just dynamically allocate it moving forward, but not change it
for existing installations as it would mess up permissions bad.

Yes, needs to be changed to dynamically allocated (from system pool,
keep the --system option). And for existing installation provide some
instructions how to fix it. Any idea? Change it manually (including
appropriate permissions fix)? Leave it as it is, answer 'no' for that
debconf question?

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

@adrelanos

This comment has been minimized.

Show comment
Hide comment
@adrelanos

adrelanos Aug 6, 2015

Member

nrgaway:

If there is no issue with it, why replace it?

Doing what's commonly done by many packages vs doing something special
without need. Going the common way is less likely to cause issues such
as this one.

Member

adrelanos commented Aug 6, 2015

nrgaway:

If there is no issue with it, why replace it?

Doing what's commonly done by many packages vs doing something special
without need. Going the common way is less likely to cause issues such
as this one.

@nrgaway

This comment has been minimized.

Show comment
Hide comment
@nrgaway

nrgaway Aug 6, 2015

On 6 August 2015 at 17:00, Patrick Schleizer notifications@github.com
wrote:

nrgaway:

If there is no issue with it, why replace it?

Doing what's commonly done by many packages vs doing something special
without need. Going the common way is less likely to cause issues such
as this one.

The issue here is the user group being created as static. The reason
useradd was selected over adduser is because useradd is more low
level and only configures what specificly instructed compared to adduser
which sets up some defalult stuff like home directory, which in this case
we do not want.

Also here is the commit made by @HW42. At one point there was an issue
with group not being added properly.

     #
--------------------------------------------------------------------------
     # User add / modifications
     #
--------------------------------------------------------------------------
-    id -u 'user' || {
-        groupadd -f user
-        useradd -g user -G
dialout,cdrom,floppy,sudo,audio,dip,video,plugdev -m -s /bin/bash user
+    id -u 'user' >/dev/null 2>&1 || {
+        useradd -U -G dialout,cdrom,floppy,sudo,audio,dip,video,plugdev -m
-s /bin/bash user
     }
-    id -u 'tinyproxy' || {
-        groupadd -f tinyproxy
-        useradd -g tinyproxy -r -M --home /run/tinyproxy --shell
/bin/false tinyproxy
+    id -u 'tinyproxy' >/dev/null 2>&1 || {
+        useradd -U -r -M --home /run/tinyproxy --shell /bin/false tinyproxy
     }
     usermod -p '' root
     usermod -L user

nrgaway commented Aug 6, 2015

On 6 August 2015 at 17:00, Patrick Schleizer notifications@github.com
wrote:

nrgaway:

If there is no issue with it, why replace it?

Doing what's commonly done by many packages vs doing something special
without need. Going the common way is less likely to cause issues such
as this one.

The issue here is the user group being created as static. The reason
useradd was selected over adduser is because useradd is more low
level and only configures what specificly instructed compared to adduser
which sets up some defalult stuff like home directory, which in this case
we do not want.

Also here is the commit made by @HW42. At one point there was an issue
with group not being added properly.

     #
--------------------------------------------------------------------------
     # User add / modifications
     #
--------------------------------------------------------------------------
-    id -u 'user' || {
-        groupadd -f user
-        useradd -g user -G
dialout,cdrom,floppy,sudo,audio,dip,video,plugdev -m -s /bin/bash user
+    id -u 'user' >/dev/null 2>&1 || {
+        useradd -U -G dialout,cdrom,floppy,sudo,audio,dip,video,plugdev -m
-s /bin/bash user
     }
-    id -u 'tinyproxy' || {
-        groupadd -f tinyproxy
-        useradd -g tinyproxy -r -M --home /run/tinyproxy --shell
/bin/false tinyproxy
+    id -u 'tinyproxy' >/dev/null 2>&1 || {
+        useradd -U -r -M --home /run/tinyproxy --shell /bin/false tinyproxy
     }
     usermod -p '' root
     usermod -L user
@adrelanos

This comment has been minimized.

Show comment
Hide comment
@adrelanos

adrelanos Aug 7, 2015

Member

adduser works for all packages that are installed in my Debian template. It supports --no-create-home also.

And for existing installation provide some instructions how to fix it. Any idea? Change it manually (including appropriate permissions fix)?

Please have a look if this helps:
http://unix.stackexchange.com/questions/33844/change-gid-of-a-specific-group

Specifically at the accepted answer:
http://unix.stackexchange.com/a/33874

It sounds difficult at first. See the following command to find files owned by the that gid. (Consider appending 2>/dev/null.)

sudo find / -gid 98 ! -type l -exec echo {} \;

It doesn't find any relevant files. (If you replace the gid with 1, then it finds some files. So the command should be correct.)

Please leave feedback if the following approach to fix this is worth investigating further.

if ! cat /etc/group | grep -q 98 ; then
   if ! cat /etc/group | grep -q 980 ; then
      groupmod -g 980 qubes
   fi
fi

Leave it as it is, answer 'no' for that debconf question?

As a fall back if this gets too difficult, then there is no other way, yes. But please do not preseed this debconf question. Such a workaround could actually introduce more issues.

Member

adrelanos commented Aug 7, 2015

adduser works for all packages that are installed in my Debian template. It supports --no-create-home also.

And for existing installation provide some instructions how to fix it. Any idea? Change it manually (including appropriate permissions fix)?

Please have a look if this helps:
http://unix.stackexchange.com/questions/33844/change-gid-of-a-specific-group

Specifically at the accepted answer:
http://unix.stackexchange.com/a/33874

It sounds difficult at first. See the following command to find files owned by the that gid. (Consider appending 2>/dev/null.)

sudo find / -gid 98 ! -type l -exec echo {} \;

It doesn't find any relevant files. (If you replace the gid with 1, then it finds some files. So the command should be correct.)

Please leave feedback if the following approach to fix this is worth investigating further.

if ! cat /etc/group | grep -q 98 ; then
   if ! cat /etc/group | grep -q 980 ; then
      groupmod -g 980 qubes
   fi
fi

Leave it as it is, answer 'no' for that debconf question?

As a fall back if this gets too difficult, then there is no other way, yes. But please do not preseed this debconf question. Such a workaround could actually introduce more issues.

@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Aug 7, 2015

Member

On Fri, Aug 07, 2015 at 09:14:57AM -0700, Patrick Schleizer wrote:

It doesn't find any relevant files. (If you replace the gid with 1, then it finds some files. So the command should be correct.)

Indeed no persistent files are there - only some in /run and /dev,
which will have permissions set correctly at next VM startup (after GID
change).
Should be care about them? Maybe just verbose chgrp, just after
groupmod (so it would be called only when really changing GID)?

Please leave feedback if the following approach to fix this is worth investigating further.

if ! cat /etc/group | grep -q 98 ; then

This doesn't look correct: it would match any group with GID 98 (for
example when such would be introduced by passwd-base package). Also it
would match "980". There is also logic inverted. I suggest rather this:

if grep -q ^qubes:x:98: /etc/group; then
  if ! grep -q :980: /etc/group; then
    groupmod -g 980 qubes
  fi
fi

BTW Another minor issue: sudo group is already provided by passwd-base
package, no need to create it.

Member

marmarek commented Aug 7, 2015

On Fri, Aug 07, 2015 at 09:14:57AM -0700, Patrick Schleizer wrote:

It doesn't find any relevant files. (If you replace the gid with 1, then it finds some files. So the command should be correct.)

Indeed no persistent files are there - only some in /run and /dev,
which will have permissions set correctly at next VM startup (after GID
change).
Should be care about them? Maybe just verbose chgrp, just after
groupmod (so it would be called only when really changing GID)?

Please leave feedback if the following approach to fix this is worth investigating further.

if ! cat /etc/group | grep -q 98 ; then

This doesn't look correct: it would match any group with GID 98 (for
example when such would be introduced by passwd-base package). Also it
would match "980". There is also logic inverted. I suggest rather this:

if grep -q ^qubes:x:98: /etc/group; then
  if ! grep -q :980: /etc/group; then
    groupmod -g 980 qubes
  fi
fi

BTW Another minor issue: sudo group is already provided by passwd-base
package, no need to create it.

@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Aug 7, 2015

Member

Above code should read:

if grep -q ^qubes:x:98: /etc/group; then
  if ! grep -q :980: /etc/group; then
    groupmod -g 980 qubes
  fi
fi
Member

marmarek commented Aug 7, 2015

Above code should read:

if grep -q ^qubes:x:98: /etc/group; then
  if ! grep -q :980: /etc/group; then
    groupmod -g 980 qubes
  fi
fi
@adrelanos

This comment has been minimized.

Show comment
Hide comment
@adrelanos

adrelanos Aug 7, 2015

Member

Good points.

Should be care about them?

I don't know.

Maybe just verbose chgrp, just after groupmod (so it would be called only when really changing GID)?

Done, please have a look at the following code which is tested from a script (not from the actual postinst).

if grep -q ^qubes:x:98: /etc/group ; then
  if ! grep -q :980: /etc/group ; then
    if groupmod -g 980 qubes ; then
      find / -gid 98 ! -type l -exec chgrp --verbose qubes {} \; 2>/dev/null || true
    fi
  fi
fi

As a stylistic question, do you prefer each line commented, a summary comment on top or no comments at all?

Member

adrelanos commented Aug 7, 2015

Good points.

Should be care about them?

I don't know.

Maybe just verbose chgrp, just after groupmod (so it would be called only when really changing GID)?

Done, please have a look at the following code which is tested from a script (not from the actual postinst).

if grep -q ^qubes:x:98: /etc/group ; then
  if ! grep -q :980: /etc/group ; then
    if groupmod -g 980 qubes ; then
      find / -gid 98 ! -type l -exec chgrp --verbose qubes {} \; 2>/dev/null || true
    fi
  fi
fi

As a stylistic question, do you prefer each line commented, a summary comment on top or no comments at all?

@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Aug 7, 2015

Member

Looks good. Will you make pull request, or should I copy&paste above to
the script?

As a stylistic question, do you prefer each line commented, a summary comment on top or no comments at all?

For not obvious things, summary comment on top ("why" the code is there).
In case of large or tricky code, some additional comments for that lines.
Generally take a look here:
http://www.qubes-os.org/doc/CodingStyle/#general-programming-style-guidelines

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Member

marmarek commented Aug 7, 2015

Looks good. Will you make pull request, or should I copy&paste above to
the script?

As a stylistic question, do you prefer each line commented, a summary comment on top or no comments at all?

For not obvious things, summary comment on top ("why" the code is there).
In case of large or tricky code, some additional comments for that lines.
Generally take a look here:
http://www.qubes-os.org/doc/CodingStyle/#general-programming-style-guidelines

Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

@unman

This comment has been minimized.

Show comment
Hide comment
@unman

unman Aug 7, 2015

Member

Why not:
if grep -w ^qubes98 /etc/group ?

On the adduser/useradd question, useradd is posix and universal.
adduser is a debian specific perl wrapper to useradd.
I'd keep as is. (It may help when porting to other distros)

Member

unman commented Aug 7, 2015

Why not:
if grep -w ^qubes98 /etc/group ?

On the adduser/useradd question, useradd is posix and universal.
adduser is a debian specific perl wrapper to useradd.
I'd keep as is. (It may help when porting to other distros)

@adrelanos

This comment has been minimized.

Show comment
Hide comment
@adrelanos

adrelanos Aug 7, 2015

Member

@unman Graphics ended up in your code snippet. Please properly format your code snippets. Put them into code. Or

code
Member

adrelanos commented Aug 7, 2015

@unman Graphics ended up in your code snippet. Please properly format your code snippets. Put them into code. Or

code
@adrelanos

This comment has been minimized.

Show comment
Hide comment
@adrelanos

adrelanos Aug 7, 2015

Member

I can do a pull request.

Member

adrelanos commented Aug 7, 2015

I can do a pull request.

@adrelanos

This comment has been minimized.

Show comment
Hide comment
@adrelanos

adrelanos Aug 7, 2015

Member

@unman When you fixed your post, we can consider to add your suggestion on top. Or provide a pull request.

Member

adrelanos commented Aug 7, 2015

@unman When you fixed your post, we can consider to add your suggestion on top. Or provide a pull request.

@unman

This comment has been minimized.

Show comment
Hide comment
@unman

unman Aug 7, 2015

Member
if grep -w ^qubes:x:98 /etc/group ?
Member

unman commented Aug 7, 2015

if grep -w ^qubes:x:98 /etc/group ?
@adrelanos

This comment has been minimized.

Show comment
Hide comment
@adrelanos

adrelanos Aug 7, 2015

Member

if grep -w ^qubes:x:98 /etc/group
if grep -q ^qubes:x:98: /etc/group

Member

adrelanos commented Aug 7, 2015

if grep -w ^qubes:x:98 /etc/group
if grep -q ^qubes:x:98: /etc/group

@adrelanos

This comment has been minimized.

Show comment
Hide comment
@adrelanos

adrelanos Aug 8, 2015

Member

Updated pull request.

Member

adrelanos commented Aug 8, 2015

Updated pull request.

@marmarek

This comment has been minimized.

Show comment
Hide comment
@marmarek

marmarek Aug 8, 2015

Member

@adrelanos can you confirm it is fixed?

Member

marmarek commented Aug 8, 2015

@adrelanos can you confirm it is fixed?

@adrelanos

This comment has been minimized.

Show comment
Hide comment
@adrelanos

adrelanos Aug 9, 2015

Member

Yes. Haven't seen the upgrade has been installed. cat /etc/groups | grep qubes also looks good. (Set to gid 980.)

Member

adrelanos commented Aug 9, 2015

Yes. Haven't seen the upgrade has been installed. cat /etc/groups | grep qubes also looks good. (Set to gid 980.)

@adrelanos adrelanos closed this Aug 9, 2015

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