Skip to content

keep backward compatibility with debian images #51

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

Merged
merged 1 commit into from
Nov 2, 2018

Conversation

delgod
Copy link
Contributor

@delgod delgod commented Oct 29, 2018

asked in this discussion -
docker-library/percona#68 (comment)

Copy link
Collaborator

@EvgeniyPatlan EvgeniyPatlan left a comment

Choose a reason for hiding this comment

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

lgtm

@tianon
Copy link

tianon commented Oct 30, 2018

Using sudo is definitely going to get some eyebrow raises in review at https://github.com/docker-library/official-images -- having setuid binaries is a much bigger security issue than initially running as root and stepping down during startup (especially with such a permissive configuration like NOPASSWD: ALL).

When you consider that no-new-privileges works with the "start as root, step down to non-root during startup" but does not work with "start as non-root and explicitly gain privileges during startup", it becomes even more compelling IMO (since you then get a pretty firm guarantee that the container can't become root again without a pretty severe kernel level exploit).

@glensc
Copy link

glensc commented Oct 30, 2018

please ensure this approach also will supports --user argument:

https://github.com/docker-library/docs/tree/edb189b2ee5fa42b48334794c6f6ea4aba763be9/mysql#running-as-an-arbitrary-user

If you know the permissions of your directory are already set appropriately (such as running against an existing database, as described above) or you have need of running mysqld with a specific UID/GID, it is possible to invoke this image with --user set to any value (other than root/0) in order to achieve the desired access/configuration:

$ mkdir data
$ ls -lnd data
drwxr-xr-x 2 1000 1000 4096 Aug 27 15:54 data
$ docker run -v "$PWD/data":/var/lib/mysql --user 1000:1000 --name some-mysql -e MYSQL_ROOT_PASSWORD=my-secret-pw -d mysql:tag

@tianon i also noticed the sudo use here, but as they change user in Dockerfile, rather in ENTRYPOINT, they have not much other choices.

@delgod delgod force-pushed the debian-backward-compatibility branch 3 times, most recently from 926edcd to 50f77f8 Compare October 30, 2018 21:35
@delgod
Copy link
Contributor Author

delgod commented Oct 30, 2018

Hi @tianon,
Yes, you right. I have removed sudo command and created another revision, which has UID identical to current DockerHub UID.

Hi @glensc,
We are not going to support --user argument.
Yes, I understand why it is needed, but we don't want to run ENTRYPOINT from root privileges and also don't want to use sudo.
I hope for your understanding. Security is always a hard choice, and it is better to be blamed due to lack of usability than lack of security.
in the new version of this review, I am using 999 UID (as the initial dockerhub image)

&& rm -rf /var/cache/yum /var/lib/mysql

# purge and re-create /var/lib/mysql with appropriate ownership
RUN /usr/bin/install -m 0775 -o 999 -g 0 -d /var/lib/mysql /var/run/mysqld /docker-entrypoint-initdb.d \
Copy link

Choose a reason for hiding this comment

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

minor nitpick: you can user -o mysql -g root as the user/group already created in this very Dockerfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

COPY ps-entry.sh /docker-entrypoint.sh
ENTRYPOINT ["/docker-entrypoint.sh"]

USER 999
Copy link

Choose a reason for hiding this comment

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

as the user is created in this very dockerfile, i find it more elegant to use name than uid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@delgod delgod force-pushed the debian-backward-compatibility branch from 50f77f8 to 2a9e9be Compare October 31, 2018 17:38
&& echo "THP_SETTING=never" >> /etc/sysconfig/mysql \
# allow to change config files
&& chown -R mysql:root /etc/my.cnf /etc/my.cnf.d \
&& chmod -R ug+rw /etc/my.cnf /etc/my.cnf.d
Copy link

Choose a reason for hiding this comment

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

not related to this PR, but you may want to add X for directories, not to rely on previous state or umask:

	&& chmod -R ug+rwX /etc/my.cnf /etc/my.cnf.d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@delgod delgod force-pushed the debian-backward-compatibility branch from 2a9e9be to 7d14c12 Compare November 1, 2018 05:49
@delgod delgod merged commit 140df21 into master Nov 2, 2018
@delgod delgod deleted the debian-backward-compatibility branch November 2, 2018 14:23
evanelias added a commit to skeema/tengo that referenced this pull request Nov 26, 2018
DockerizedInstance.SourceSQL() now specifically adds "-u root" to the mysql
client command-line. This is necessary in recent Percona releases, which
recently changed Docker user to "mysql" in PR
percona/percona-docker#51. This means `docker exec`
also uses "mysql" user, which in turn acts as the default for mysql
connections too if no user is specified.

DockerizedInstance.Stop() increased timeout from 3sec to 10sec. In cases where
the timeout is hit, the instance may not be able to be started again, and this
seems to happen more frequently with 5.7 images (mysql or percona) on MacOS
for unknown reasons.
evanelias added a commit to skeema/skeema that referenced this pull request Nov 27, 2018
This commit updates dep github.com/skeema/tengo to bring in a couple Docker
fixes, solving these problems:

* --workspace=docker with --flavor=mysql:5.7 or percona:5.7 on MacOS would
  sometimes fail to stop an image properly, requiring it to be manually reset
  to regain functionality.

* Integration tests (especially in the applier package) would sometimes hit
  issues with stopping 5.7 images on MacOS, causing flakey test failures.

* Integration tests would fail with percona:5.6 and percona:5.7 if the image
  was fetched in the past few weeks, due to a username change in
  percona/percona-docker#51
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.

5 participants