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

fix(ownership): change Kong permissions to be kong:root #632

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Mar 8, 2023

replaces #546

@Tieske Tieske force-pushed the perm branch 7 times, most recently from da8e75d to 2819a1c Compare March 8, 2023 14:22
@Tieske Tieske changed the title WIP fix(ownership): change Kong permissions to be kong:root Mar 8, 2023
@Tieske Tieske force-pushed the perm branch 2 times, most recently from 98732de to d55ca5a Compare March 8, 2023 14:38
@Tieske Tieske marked this pull request as ready for review March 8, 2023 14:41
@Tieske Tieske requested a review from gszr March 8, 2023 14:42
@Tieske
Copy link
Member Author

Tieske commented Mar 8, 2023

@gszr I updated the old PR, can you have a look?

&& mkdir -p "${KONG_PREFIX}" \
&& chown -R kong:0 ${KONG_PREFIX} \
&& chown kong:0 /usr/local/bin/kong \
&& chown -R root:kong ${KONG_PREFIX} \
Copy link
Member

Choose a reason for hiding this comment

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

@Tieske Is there more context on the motivation for this change? IIRC, the owner of kong files was changed to kong fairly recently, so I am afraid of changing it back to root.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the original pr by Colin; Changing the ownership to root but giving the Kong user access prevents the Kong user from modifying the files.

Copy link
Contributor

@fffonion fffonion Mar 10, 2023

Choose a reason for hiding this comment

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

generally, everything under a common shared path like /usr/local/lib, /usr/local/share should always be owned by root:root because technically other packages will also write to it. this applies to also /etc/kong/kong.conf; only
/usr/local/kong (and /usr/local/openresty, if we mark our package a conflict with openresty) should be owned by
kong:kong. (well, technically, even under /usr/local/kong, subdirs like /usr/local/kong/lib, /usr/local/kong/include` should be owned by root:root as well)

Copy link
Member

Choose a reason for hiding this comment

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

The course of action is IMO:

  • Consolidate what the permissions should be (I tend to agree with most of what @fffonion said)
  • Move permission-related code to post-install scripts (alpine is an exception since it's not really a distro package) as suggested by Wangchong. I cannot remember of any particular issues around this explaining why they were placed in the docker images only.

@fffonion
Copy link
Contributor

file permission of deb and rpm should be set by postinstall script so that users install binary packages directory
should have the same behaviour of docker images.

@Tieske
Copy link
Member Author

Tieske commented Mar 10, 2023

@fffonion not sure what you're saying. What needs to change about this PR? all should be root:root?

@Tieske
Copy link
Member Author

Tieske commented Mar 16, 2023

ping @gszr @fffonion

@fffonion
Copy link
Contributor

fffonion commented Mar 17, 2023

@Tieske sry was typing on phone. i was mentioning to move the chowns to here https://github.com/Kong/kong/blob/master/build/package/postinstall.sh, for deb and rpm; alpine packages don't take postinstall scripts, so leave the chowns here at docker-kong is fine. so that we don't end up having two different states of files ownership when user are installing binary packages directly, vs using docker.

and beside of that, me and @gszr both agree that most files we shipped should be owned by root:root, with the exception of /usr/local/kong directory. the runtime directory can be owned by kong:kong or kong:root whichever fits our needs.

@hbagdi
Copy link
Member

hbagdi commented Apr 5, 2023

ping @Tieske @gszr

@gszr
Copy link
Member

gszr commented Apr 6, 2023

and beside of that, me and @gszr both agree that most files we shipped should be owned by root:root, with the exception of /usr/local/kong directory. the runtime directory can be owned by kong:kong or kong:root whichever fits our needs.

Could this break custom plugin installs attempted with the kong user?

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

Successfully merging this pull request may close these issues.

4 participants