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

[baseimage]: specify gid for redis group. #7249

Merged
merged 4 commits into from
Apr 8, 2021

Conversation

renukamanavalan
Copy link
Contributor

@renukamanavalan renukamanavalan commented Apr 7, 2021

Why I did it

Default groupadd for redis, takes 1000 by default. This forces, subsequently created admin group to get 1001.
As all TACACS users are created with 1000 as their gid, they end up in redis group.

How I did it

Switched the order of group creation. Move redis group creation command after the command that creates admin group.

How to verify it

Check /etc/group & group of RW users via TACACS

   admin@str-s6000-acs-9:~$ cat /etc/group | grep -e "admin\|redis"
    sudo:x:27:admin,user_rw
    docker:x:999:admin,user_rw
    admin:x:1000:                       --- gid ==1000
    redis:x:1001:admin               --- gid != 1000
    admin@str-s6000-acs-9:~$

    admin@str-s6000-acs-9:~$ ls -l /home
    total 4
    drwxr-xr-x 2 admin   admin   66 Apr  7 14:53 admin
    drwxr-xr-x 2 user_rw admin 4096 Apr  7 15:47 user_rw  -- RW user get admin group

Which release branch to backport (provide reason below if selected)

  • 201811
  • [x ] 201911
  • [x ] 202006
  • [x ] 202012

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

By explicilty specifying something other than 1000, it leaves behind 1000 for grab, which is
duly taken by admin group, as required.
BTW, TACACS RW users use, 1000 as gid.

Test result:

    admin@str-s6000-acs-9:~$ cat /etc/group | grep -e "admin\|redis"
    sudo:x:27:admin,user_rw
    docker:x:999:admin,user_rw
    redis:x:1001:admin               --- takes 1001
    admin:x:1000:
    admin@str-s6000-acs-9:~$

    admin@str-s6000-acs-9:~$ ls -l /home
    total 4
    drwxr-xr-x 2 admin   admin   66 Apr  7 14:53 admin
    drwxr-xr-x 2 user_rw admin 4096 Apr  7 15:47 user_rw  -- RW user get admin group
lguohan
lguohan previously approved these changes Apr 7, 2021
@lguohan lguohan changed the title Specify gid for redis group. [baseimage]: specify gid for redis group. Apr 7, 2021
build_debian.sh Outdated
@@ -245,7 +245,7 @@ sudo cp files/docker/docker.service.conf $_
sudo sed -i '/After=/s/$/ containerd.service/' $FILESYSTEM_ROOT/lib/systemd/system/docker.service

## Create redis group
sudo LANG=C chroot $FILESYSTEM_ROOT groupadd -f redis
sudo LANG=C chroot $FILESYSTEM_ROOT groupadd -f -g 1001 redis
Copy link
Collaborator

@qiluo-msft qiluo-msft Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1001 [](start = 51, length = 4)

I think the motivation is not to fix the gid for redis, but fix the gid for admin to 1000.
So suggest

  1. create a group with gid 1000 and name $USERNAME
  2. create a group redis without specifying gid
  3. create user $USERNAME with -g 1000 and also in the groups sudo,docker,redis #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, we need not fix gid for redis. But we need to fix gid for "admin" group.
At the same time, it does not limit/restrict anything by explicitly specifying gid for redis.

This fix is nearly a no-op for all purposes as all that is required is a redis group and we continue to have it as before.

Copy link
Collaborator

@lguohan lguohan Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we pin down the admin group id to 1000 more directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could. Just a groupadd. Not sure, about any side effects. Just trying to make the change close to no-op. This change is ~= NO-OP

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hold on....There could be something simpler

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Just shifted order of creation.

tahmed-dev
tahmed-dev previously approved these changes Apr 7, 2021
@renukamanavalan renukamanavalan dismissed stale reviews from tahmed-dev and lguohan via 6e6e954 April 7, 2021 21:48
build_debian.sh Outdated
# Ensure admin gid 1000 is available
gid_1000=$(sudo LANG=C grep -e ":1000:" $FILESYSTEM_ROOT/etc/group)
if [ -n "${gid_1000}" ]; then
if [ "${gid_1000}" != "admin:x:1000:" ]; then
Copy link
Collaborator

@qiluo-msft qiluo-msft Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

admin [](start = 27, length = 5)

admin is customizable. Use $USERNAME instead.
I guess you can just id -g $USERNAME #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thank you!

@renukamanavalan renukamanavalan merged commit be78973 into sonic-net:master Apr 8, 2021
abdosi pushed a commit that referenced this pull request Apr 8, 2021
Problem:
Default groupadd for redis, takes 1000 by default. This forces, subsequently created admin group to get 1001.
As all TACACS users are created with 1000 as their gid, they end up in redis group.

Fix:
Create redis group *after* admin group is created
Add a check that admin group id is 1000
Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

yxieca pushed a commit that referenced this pull request Apr 8, 2021
Problem:
Default groupadd for redis, takes 1000 by default. This forces, subsequently created admin group to get 1001.
As all TACACS users are created with 1000 as their gid, they end up in redis group.

Fix:
Create redis group *after* admin group is created
Add a check that admin group id is 1000
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-buildimage that referenced this pull request May 23, 2021
Problem:
Default groupadd for redis, takes 1000 by default. This forces, subsequently created admin group to get 1001.
As all TACACS users are created with 1000 as their gid, they end up in redis group.

Fix:
Create redis group *after* admin group is created
Add a check that admin group id is 1000
@renukamanavalan renukamanavalan deleted the redis branch July 19, 2021 16:58
carl-nokia pushed a commit to carl-nokia/sonic-buildimage that referenced this pull request Aug 7, 2021
Problem:
Default groupadd for redis, takes 1000 by default. This forces, subsequently created admin group to get 1001.
As all TACACS users are created with 1000 as their gid, they end up in redis group.

Fix:
Create redis group *after* admin group is created
Add a check that admin group id is 1000
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.

6 participants