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(setcap) #206

Merged
merged 3 commits into from
Jan 5, 2019
Merged

fix(setcap) #206

merged 3 commits into from
Jan 5, 2019

Conversation

hutchic
Copy link
Contributor

@hutchic hutchic commented Dec 21, 2018

moves setcap out of Dockerfile and into the docker entrypoint's

alpine/docker-entrypoint.sh Outdated Show resolved Hide resolved
Copy link

@james-callahan james-callahan left a comment

Choose a reason for hiding this comment

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

I played around with capsh and I think we can do this with ambient capabilities instead.

sudo capsh --caps="cap_net_admin+eip cap_setpcap+ep" --addamb=cap_net_admin -- -c "grep Cap /proc/self/status"

alpine/docker-entrypoint.sh Outdated Show resolved Hide resolved
alpine/docker-entrypoint.sh Outdated Show resolved Hide resolved
@james-callahan
Copy link

:( the capsh that ships with alpine 3.8 is too old to have the --addamb argument. Does anyone know of a program easily available that execs with new ambient capabilities?

chmod 777 /proc/self/fd/1
chmod 777 /proc/self/fd/2

su-exec kong /usr/local/openresty/nginx/sbin/nginx \
Copy link

@james-callahan james-callahan Jan 4, 2019

Choose a reason for hiding this comment

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

This should be exec su-exec. otherwise bash will continue to exist as a parent. (also the script will continue.)

Copy link
Member

Choose a reason for hiding this comment

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

That shouldn't be necessary (https://github.com/ncopa/su-exec/blob/master/su-exec.c#L103). It is also documented in the project's README: https://github.com/ncopa/su-exec#tty--parentchild-handling

Choose a reason for hiding this comment

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

Unless you exec the bash process will live on as a parent.
The readme there is saying su-exec won't stick around; not the parent of su-exec.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. Also, any thoughts on ncopa/su-exec#23?

* auto-detect 'transparent' suffix in listeners to trigger setcap
* ability to force setcap with new `SET_CAP_NET_RAW` env variable
* run master and worker processes as kong with su-exec
    * requires a workaround for /dev/stdout and /dev/stderr output

Unfortunately, `kong prepare` still has to be executed as root (as it is
the case prior to this patch), but trying to execute it as the kong user
doesn't seem to work. Will revisit later.

Co-authored-by: Colin Hutchinson <[email protected]>
@thibaultcha
Copy link
Member

New version pushed. Please see the commit message for details. This has been delayed long enough, and the current implementation already is an improvement over the existing image, plus supports auto-detection of the transparent option as the best compromise we could find.

@hutchic
Copy link
Contributor Author

hutchic commented Jan 5, 2019

I tested the latest push in docker-compose and minikube both environments and setups worked as expected 👍

@thibaultcha thibaultcha merged commit 97d8e93 into master Jan 5, 2019
@thibaultcha thibaultcha deleted the fix/setcap branch January 5, 2019 02:19
@Tieske
Copy link
Member

Tieske commented Feb 2, 2019

shouldn't the same changes be applied to the rhel image?

@james-callahan
Copy link

shouldn't the same changes be applied to the rhel image?

We don't have any way to test rhel.

@subnetmarco
Copy link
Member

Wouldn't this apply to any other distribution too (Ubuntu, Debian, CentOS in addition to RHEL, etc)?

@james-callahan
Copy link

Wouldn't this apply to any other distribution too (Ubuntu, Debian, CentOS in addition to RHEL, etc)?

I'm not sure what you mean by this?

The method is different per distro (su-exec is not that common). This repository only creates docker images for alpine, centos and rhel. rhel is the only one of those for which we have no testing setup.

@subnetmarco
Copy link
Member

What are the next steps to support this feature on any other Docker image we create? @Tieske is right in his observation.

@hutchic
Copy link
Contributor Author

hutchic commented Feb 11, 2019

@Kong/team-fast-track handles the RHEL container but I believe the RHEL container is loosely based on our centos image so it might be fine but probably best to follow up with folks on that team

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.

6 participants