fix(useradd) make sure kong:kong exist and files ownership is correct #457

outsinre · 2022-04-11T14:36:04Z

If the group kong exists before installation, "useradd -U" would not create the kong user.
The file ownership update would also fail.

Firstly create the kong group; then create the kong user on demand.

If the group kong exists before installation, "useradd -U" would not create the kong user. The file ownership update would also fail. Firstly create the kong group; then create the kong user on demand.

hutchic

We have a desire to go the other direction with this in 3.0 #445

At a glance this PR looks fine to go in until then if we desire though (pending the one change)

hutchic · 2022-04-11T16:43:36Z

after-install.sh

  FILES="${FILES} /usr/local/etc/luarocks/"
+  FILES="${FILES} /usr/local/etc/passwdqc/"


these two addition are enterprise only are they not?

currently KBT build kong
kong-distributions builds kong enterprise

that changes in 3.0 (hopefully) with #455

@hutchic , yep, currently it is. I added this two lines to make "kong-distributions" (EE) and "kong-build-tools" (CE) synced.

According to @gszr, we will deprecate "kong-distributions" in favor of "kong-build-tools". So, along this PR, I added the extra two lines.

A similar PR to is created for the enterprise version now: https://github.com/Kong/kong-distributions/pull/773

In that case I recommend changing the:

chown -R kong:kong ${FILE} chmod -R g=u ${FILE}

below to:

chown -f -R kong:kong ${FILE} chmod -f -R g=u ${FILE}

so to avoid potential errors during OSS install.

hutchic

approved with a note that 3.0 moves us from kong:kong to kong:root

gszr · 2022-04-12T13:08:40Z

approved with a note that 3.0 moves us from kong:kong to kong:root

👋 curious what the reason is for the change?

hutchic · 2022-04-16T14:16:48Z

@gszr the desire that the application not be capable of rewriting itself while it runs. It's a highly unlikely attack vector but relatively easy to protect against

Protect Kong files/directories. See #457 (comment)

curiositycasualty · 2022-04-30T03:00:41Z

I'm hoping https://github.com/Kong/kong-build-tools/pull/463/files sets this PR up for success.

hutchic · 2022-05-02T10:39:24Z

Going to approve this and the kong-build-tools PR's bug label them appropriately so we don't merge until 3.0. If we require a fix sooner the original useradd I believe is backwards compatible and could go out sooner FWIW

hutchic

approved pending 3.0 release

outsinre · 2022-05-12T10:33:21Z

@gszr the desire that the application not be capable of rewriting itself while it runs. It's a highly unlikely attack vector but relatively easy to protect against

@hutchic, could you share similar attack cases? Any links are welcome.

dannysauer · 2022-05-12T15:21:25Z

@outsinre

@hutchic, could you share similar attack cases? Any links are welcome.

To start with, it should be root:kong, with the group having read (and execute, in the case of a directory or executable) permission. The files should be owned by a user which is not the same user the process is running as. That allows files to be modified by an administrator who can assume the identity of that user, while still allowing the process (which should be a member of the specified group) to be able to read. In general, this is an example of applying "the principle of least privilege", where each actor has only the minimum set of privileges necessary to complete tasks, and no more. ACLs would really be better, but those are somewhat poorly understood in general, and not universally supported.

For a moderately concrete example, I'll note that this is a very common strategy with web servers and other services that watch their config file for changes. Consider a web service run by the web server. A coding mistake allows a remote user to change the storage path of some data which is normally written to a specific folder dedicated to the app. If only the path can be changed, it's possible that the attacker could change the written path to the web service's configuration file. In the "best" case, this allows a remote attacker to break the server's configuration by writing whatever garbage over the config file. In a worse case, the attacker controls the data and maybe causes this web server to start exposing /etc/passwd or user homedirs or whatever else. Or maybe replaces the service executable with something else. The possibilities are endless! ;)

Notably, services which read their config file at startup and then don't look again often have their config set as root:root (or some other combination which is outside of the process's privilege set). The approach there is to start the process as root. The process reads its config and binds to privileged ports (and whatever other things require increased privileges), then drops privileges by either switching to a lower-permission user or using the relevant capability bounding set syscalls. There's a trade-off there, as the potential race condition of "process is initially running as root" introduces its own issues.

Basically, giving processes more privileges than they need opens up the risk of processes doing more things than they should do, and that risk increases as more entities have access to interact with the process (like, say, a gateway exposed to the Internet). :)

dannysauer · 2022-05-12T15:41:34Z

after-install.sh

  FILES="${FILES} /usr/local/kong/"
  FILES="${FILES} /usr/local/lib/lua/"
  FILES="${FILES} /usr/local/lib/luarocks/"
  FILES="${FILES} /usr/local/openresty/"
  FILES="${FILES} /usr/local/share/lua/"

  for FILE in ${FILES}; do
-    chown -R kong:kong ${FILE}
+    chown -R kong:root ${FILE}
    chmod -R g=u ${FILE}


Implementing the control described in the comment at #457 (comment), the chmod should really be chmod -R g=u,g-w unless the group actually needs write access.

Related: why is this in a for loop? Just run like chown $FILES and let the shell's whitespace splitting work it out; that's already what's happening in the for loop anyway, and there's no sense fork/exec'ing chown & chmod 15 times. ;)

I guess that should be an issue and not a comment in this PR. I'll see myself out. :p

@dannysauer,

I strongly agree with the for loop part. We should just remove the loop.

The conclusion on ownership is root:kong instead of kong:root, right?

Here is my personal observation for discussion.

By default, Linux/Unit package files installed is root:root, while mode is 755 (directories and executables) and 644 (regular files).

That means others (o) can still read the files, but cannot write files. So, we can just leave the ownership as root:root but run Kong as kong. Kong belongs to o, so it can read but cannot write the files.

So probably, we can make it simple. Just make sure user kong exists and optionally group kong exists (for completeness and compatibility). We can omit the chown or chmod parts.

Take the official Nginx for example, the configuration file is root:root because the Nginx (Kong included) master process is ran as root but worker process as nginx (nginx_user = kong kong in our case).

Here is an excerpt of default umask at Redhat:

[ec2-user@ip-172-31-16-59 ~]$ sudo umask -S u=rwx,g=rx,o=rx

It’s 022 by default.

[ec2-user@ip-172-31-16-59 ~]$ ls -l /etc/nginx/ total 68 drwxr-xr-x 2 root root 6 Jul 15 2021 conf.d drwxr-xr-x 2 root root 6 Jul 15 2021 default.d -rw-r--r-- 1 root root 1077 Jul 15 2021 fastcgi.conf -rw-r--r-- 1 root root 1077 Jul 15 2021 fastcgi.conf.default -rw-r--r-- 1 root root 1007 Jul 15 2021 fastcgi_params -rw-r--r-- 1 root root 1007 Jul 15 2021 fastcgi_params.default -rw-r--r-- 1 root root 2837 Jul 15 2021 koi-utf -rw-r--r-- 1 root root 2223 Jul 15 2021 koi-win -rw-r--r-- 1 root root 5231 Jul 15 2021 mime.types -rw-r--r-- 1 root root 5231 Jul 15 2021 mime.types.default -rw-r--r-- 1 root root 2334 Jul 15 2021 nginx.conf -rw-r--r-- 1 root root 2656 Jul 15 2021 nginx.conf.default -rw-r--r-- 1 root root 636 Jul 15 2021 scgi_params -rw-r--r-- 1 root root 636 Jul 15 2021 scgi_params.default -rw-r--r-- 1 root root 664 Jul 15 2021 uwsgi_params -rw-r--r-- 1 root root 664 Jul 15 2021 uwsgi_params.default -rw-r--r-- 1 root root 3610 Jul 15 2021 win-utf

Kong’s runtime data is located at /kong/ or /usr/local/kong/ depending on the start args. If I am right, it’s created on the fly. “kong-build-tools” does not create it, or change the ownership or modes.

We can either:

Keep it as default. That is, root:root. The group is root with higher protection.

We'd better leave the default configuration files /etc/kong/* readable by others o, just as other Linux packages do. Otherwise, a normal user account cannot even check the default configuration of Nginx or Kong.

Set root:kong and chmod g=u,g-w.

Optionally, we can chmod o= to mask others for configuration files /etc/kong/*.

In either method, the runtime data /kong/servroot/* will be generated on the fly as root:root.

One point is that we shouldn't be making new folders at the filesystem root to begin with. But perhaps that's a separate problem. :D

Option 2 is the higher security approach. Using root:root with other access gives access to all processes on the entire machine. Using root:kong with the world access bits removed limits access to only members of the kong group.

The reason nginx provides world access to its config is that nginx is a general-purpose web server. Its use cases include serving user home directories and being run as a proxy by other processes on the system. So it is a service which may need to be run by any user on the system while including its global config. Kong gateway is a different use case - it is intended to be run by only the Kong administrator, and then any non-administrative users who interact with it are expected to only use the API. So, the only users who need to access its files directly are Kong administrators and the process itself.

FWIW: I checked the Fedora, Debian, arch, and SUSE (ok, I was a SUSE release coordinator, so I actually just remembered that one) packaging guides earlier today. They all say root:root/0755 for binaries unless increased security is necessary (a suid or sgid binary, for example). None have a specific guideline for configuration, so I think we're generally free to do what makes most sense to us.

One point is that we shouldn't be making new folders at the filesystem root to begin with. But perhaps that's a separate problem. :D

Maybe, we should let Kong creates runtime data folder dynamically under /run/ or /var/run. It's the de facto. Agree that, this is another topic.

FWIW: I checked the Fedora, Debian, arch, and SUSE (ok, I was a SUSE release coordinator, so I actually just remembered that one) packaging guides earlier today. They all say root:root/0755 for binaries unless increased security is necessary (a suid or sgid binary, for example). None have a specific guideline for configuration, so I think we're generally free to do what makes most sense to us.

Ha, really appreciate the time to do the research. Leaving it as default (root:root) is preferable, as that will make our Kong package as much compatible with different Linux distributions as possible. Leave the security enhancement with our customers. They have different administrators and may have different security guidelines.

outsinre · 2022-04-25T04:11:31Z

after-install.sh

  FILES="${FILES} /usr/local/kong/"
  FILES="${FILES} /usr/local/lib/lua/"
  FILES="${FILES} /usr/local/lib/luarocks/"
  FILES="${FILES} /usr/local/openresty/"
  FILES="${FILES} /usr/local/share/lua/"

  for FILE in ${FILES}; do
-    chown -R kong:kong ${FILE}
+    chown -R kong:root ${FILE}
    chmod -R g=u ${FILE}
  done



@gszr a new commit pushed to change the group from "kong" to "root".

The counterpart of "kong-distributions" is 3fdccc8.

outsinre · 2022-05-13T03:00:16Z

after-install.sh

  FILES="${FILES} /usr/local/kong/"
  FILES="${FILES} /usr/local/lib/lua/"
  FILES="${FILES} /usr/local/lib/luarocks/"
  FILES="${FILES} /usr/local/openresty/"
  FILES="${FILES} /usr/local/share/lua/"

  for FILE in ${FILES}; do
-    chown -R kong:kong ${FILE}
+    chown -R kong:root ${FILE}
    chmod -R g=u ${FILE}


@dannysauer,

I strongly agree with the for loop part. We should just remove the loop.

The conclusion on ownership is root:kong instead of kong:root, right?

Here is my personal observation for discussion.

By default, Linux/Unit package files installed is root:root, while mode is 755 (directories and executables) and 644 (regular files).

That means others (o) can still read the files, but cannot write files. So, we can just leave the ownership as root:root but run Kong as kong. Kong belongs to o, so it can read but cannot write the files.

So probably, we can make it simple. Just make sure user kong exists and optionally group kong exists (for completeness and compatibility). We can omit the chown or chmod parts.

outsinre · 2022-05-13T04:43:07Z

after-install.sh

  FILES="${FILES} /usr/local/kong/"
  FILES="${FILES} /usr/local/lib/lua/"
  FILES="${FILES} /usr/local/lib/luarocks/"
  FILES="${FILES} /usr/local/openresty/"
  FILES="${FILES} /usr/local/share/lua/"

  for FILE in ${FILES}; do
-    chown -R kong:kong ${FILE}
+    chown -R kong:root ${FILE}
    chmod -R g=u ${FILE}


Take the official Nginx for example, the configuration file is root:root because the Nginx (Kong included) master process is ran as root but worker process as nginx (nginx_user = kong kong in our case).

Here is an excerpt of default umask at Redhat:

[ec2-user@ip-172-31-16-59 ~]$ sudo umask -S u=rwx,g=rx,o=rx

It’s 022 by default.

[ec2-user@ip-172-31-16-59 ~]$ ls -l /etc/nginx/ total 68 drwxr-xr-x 2 root root 6 Jul 15 2021 conf.d drwxr-xr-x 2 root root 6 Jul 15 2021 default.d -rw-r--r-- 1 root root 1077 Jul 15 2021 fastcgi.conf -rw-r--r-- 1 root root 1077 Jul 15 2021 fastcgi.conf.default -rw-r--r-- 1 root root 1007 Jul 15 2021 fastcgi_params -rw-r--r-- 1 root root 1007 Jul 15 2021 fastcgi_params.default -rw-r--r-- 1 root root 2837 Jul 15 2021 koi-utf -rw-r--r-- 1 root root 2223 Jul 15 2021 koi-win -rw-r--r-- 1 root root 5231 Jul 15 2021 mime.types -rw-r--r-- 1 root root 5231 Jul 15 2021 mime.types.default -rw-r--r-- 1 root root 2334 Jul 15 2021 nginx.conf -rw-r--r-- 1 root root 2656 Jul 15 2021 nginx.conf.default -rw-r--r-- 1 root root 636 Jul 15 2021 scgi_params -rw-r--r-- 1 root root 636 Jul 15 2021 scgi_params.default -rw-r--r-- 1 root root 664 Jul 15 2021 uwsgi_params -rw-r--r-- 1 root root 664 Jul 15 2021 uwsgi_params.default -rw-r--r-- 1 root root 3610 Jul 15 2021 win-utf

Kong’s runtime data is located at /kong/ or /usr/local/kong/ depending on the start args. If I am right, it’s created on the fly. “kong-build-tools” does not create it, or change the ownership or modes.

We can either:

Keep it as default. That is, root:root. The group is root with higher protection.

We'd better leave the default configuration files /etc/kong/* readable by others o, just as other Linux packages do. Otherwise, a normal user account cannot even check the default configuration of Nginx or Kong.

Set root:kong and chmod g=u,g-w.

Optionally, we can chmod o= to mask others for configuration files /etc/kong/*.

In either method, the runtime data /kong/servroot/* will be generated on the fly as root:root.

This reverts commit b50c155.

This reverts commit d66b848.

…g-kong-group-results-in-kg-owned-by-root-and-no-kong-user-created

outsinre · 2022-05-18T08:53:33Z

The kong:kong to kong:root is reverted to keep consistent with https://github.com/Kong/kong-distributions/pull/773 which is also reverted.

New PR should be filed to care about the file permission issue.

hutchic · 2022-05-24T17:35:02Z

🎉 This PR is included in version 4.27.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

…#457) * fix(useradd) make sure kong:kong exist and files ownership is correct If the group kong exists before installation, "useradd -U" would not create the kong user. The file ownership update would also fail. Firstly create the kong group; then create the kong user on demand. * fix(ownership) change the group to "root" Protect Kong files/directories. See #457 (comment) * fix(package) tests for kong:kong -> kong:root * Revert "fix(ownership) change the group to "root"" This reverts commit b50c155. * Revert "fix(package) tests for kong:kong -> kong:root" This reverts commit d66b848. Co-authored-by: Isa Farnik <isa@konghq.com>

fix(useradd) make sure kong:kong exist and files ownership is correct

1eedeae

If the group kong exists before installation, "useradd -U" would not create the kong user. The file ownership update would also fail. Firstly create the kong group; then create the kong user on demand.

hutchic suggested changes Apr 11, 2022

View reviewed changes

hutchic approved these changes Apr 12, 2022

View reviewed changes

fix(ownership) change the group to "root"

b50c155

Protect Kong files/directories. See #457 (comment)

hutchic added do not merge 3.0 labels May 2, 2022

hutchic approved these changes May 2, 2022

View reviewed changes

fix(package) tests for kong:kong -> kong:root

d66b848

dannysauer reviewed May 12, 2022

View reviewed changes

outsinre commented May 13, 2022

View reviewed changes

outsinre mentioned this pull request May 18, 2022

FTI-3275 partially revert d66b848a b50c155b #468

Closed

outsinre added 3 commits May 18, 2022 16:47

Revert "fix(ownership) change the group to "root""

0caaeaa

This reverts commit b50c155.

Revert "fix(package) tests for kong:kong -> kong:root"

a812124

This reverts commit d66b848.

Merge branch 'master' into FTI-3275-cigna-kg-rpm-install-with-existin…

5813914

…g-kong-group-results-in-kg-owned-by-root-and-no-kong-user-created

fffonion approved these changes May 19, 2022

View reviewed changes

fffonion merged commit b5d163c into master May 19, 2022

fffonion deleted the FTI-3275-cigna-kg-rpm-install-with-existing-kong-group-results-in-kg-owned-by-root-and-no-kong-user-created branch May 19, 2022 11:13

hutchic added the released label May 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(useradd) make sure kong:kong exist and files ownership is correct #457

fix(useradd) make sure kong:kong exist and files ownership is correct #457

outsinre commented Apr 11, 2022

hutchic left a comment

hutchic Apr 11, 2022

outsinre Apr 12, 2022

dndx May 18, 2022

hutchic left a comment

gszr commented Apr 12, 2022

hutchic commented Apr 16, 2022

curiositycasualty commented Apr 30, 2022

hutchic commented May 2, 2022 •

edited

hutchic left a comment

outsinre commented May 12, 2022

dannysauer commented May 12, 2022 •

edited

dannysauer May 12, 2022

dannysauer May 12, 2022

outsinre May 13, 2022

outsinre May 13, 2022 •

edited

dannysauer May 13, 2022

dannysauer May 13, 2022

dannysauer May 13, 2022

outsinre May 18, 2022

outsinre May 18, 2022 •

edited

outsinre Apr 25, 2022

outsinre May 13, 2022

outsinre May 13, 2022 •

edited

outsinre commented May 18, 2022 •

edited

hutchic commented May 24, 2022

		FILES="${FILES} /usr/local/etc/luarocks/"
		FILES="${FILES} /usr/local/etc/passwdqc/"

fix(useradd) make sure kong:kong exist and files ownership is correct #457

fix(useradd) make sure kong:kong exist and files ownership is correct #457

Conversation

outsinre commented Apr 11, 2022

hutchic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hutchic left a comment

Choose a reason for hiding this comment

gszr commented Apr 12, 2022

hutchic commented Apr 16, 2022

curiositycasualty commented Apr 30, 2022

hutchic commented May 2, 2022 • edited

hutchic left a comment

Choose a reason for hiding this comment

outsinre commented May 12, 2022

dannysauer commented May 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outsinre May 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outsinre May 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outsinre May 13, 2022 • edited

Choose a reason for hiding this comment

outsinre commented May 18, 2022 • edited

hutchic commented May 24, 2022

hutchic commented May 2, 2022 •

edited

dannysauer commented May 12, 2022 •

edited

outsinre May 13, 2022 •

edited

outsinre May 18, 2022 •

edited

outsinre May 13, 2022 •

edited

outsinre commented May 18, 2022 •

edited