Mariadb rewrite, fixes mysqld running as root #191
Conversation
chmod confuses me. This invocation matches other Dockerfiles here, and confuses me the least.
As in the "official" MariaDB Dockerfile, and mysql-prepare-db-dir in Fedora (used by systemd service file).
Follow official docs, "official" Dockerfile etc.
1. We don't need to worry about a pre-existing version of $USER. Testing shows the only user created by mysql_db_install is "root". 2. '$USER'@'%' already includes '$USER'@'localhost'
mysqld is the only process on localhost. Unprivileged remote mysql users should not be able to spoof as localhost. If they can, something has gone very wrong, and it's quite possible they could bypass the mysql root password anyway. It might still be useful to have a separate root user, and it doesn't look like it hurts to leave it around. Closes issue fedora-cloud#190.
After considering the effects of a blank root password - let us limit the privileges of $USER to the named database only. For example, this will prevent them using mysqld to read or create arbitrary files using the global FILE privilege. I don't have a particular risk in mind (once we've configured mysql correctly to run as the mysql user). I just think that if you _want_ the remote user to do stuff like that, that's unusual and it's probably best if you set that up yourself. E.g. just add some mysql commands in your own setup script.
Upgrading to this image is likely to break due to file permissions. As discussed in github issues, Fedora-Dockerfiles is expected to be used for learning purposes, not directly in production. There's a similar breaking change I really want to get out there (set a fixed UID), and I'm not eager to add a `chown` command because it would defeat the point of that. We don't need to put USER in the Dockerfile. Actually for learning purposes it's much friendlier if users can run bash in the container without remembering to pass `-u root` to docker. And also be able to run our scripts as root without causing permissions problems. This is the approach used by "official" images on the docker hub. http://stackoverflow.com/questions/36137953/why-do-docker-official-images-not-use-user-as-required-by-best-practices/36137954#36137954 This oversight was probably because a) it's common to have config files that tell mysqld to drop privileges b) Fedora doesn't, and relies on systemd starting it as the user mysql instead. People should really read the .service files when they port to Docker :-P.
Self-deleting scripts are hinky. If we preserve config_mariadb.sh, you can edit-run-debug to your hearts content. Also it shortens the code.
Functions were not re-used. They didn't have meaningful names either. They did print a heading when we'd finished waiting for mysqld to start. However the following commit will fix the 10 seconds where we wait and do nothing, so even that won't be useful anymore.
Poll for readiness, like everyone else does. This bloats the code back up & it got rather harder to read. So I added some comments, in the form of progress and error messages.
This is my second breaking change. This aims to promote the idea of fixed UIDs. The alternative to fixing UIDs is to add a script which runs chown, to handle when the base image gains a new user. There's nothing particularly wrong with that for mysqld, but it becomes sub-optimal as volumes grow to have large numbers of files. Paying attention to UIDs is the "correct" answer to that. I've not seen anyone suggest a drawback to it, except that it means choosing a uid. "Add our user and group first to make sure their IDs get assigned consistently" returns 900+ results on google. People think they're doing the right thing. However we don't have documentation to say Fedora base images are guaranteed to allocate exactly N UIDs. Some images use uid 999, because on debian (`useradd`) this is the last "system" uid to be allocated. On Fedora (`adduser`), uid 999 is the first to be allocated. So uid 1000 is a less confusing choice. If the uid is already allocated, we should see the build fail with an error that mentions uids. As opposed to failing at runtime with a blanket "Permission denied" :-).
This requires 1. Add custom mariadb-server.cnf 2. Set user= instead of passing -u each time 3. Drop mysqld_safe. It doesn't support logging to stdout/stderr (really). 4. Finally, drop log-file= to enable logging to stdout/stderr Frankly it's arguable. The first 3 points aren't bad. And it means we show errors directly, instead of hiding them somewhere. The disadvantage is that setting up mysqld is very noisy; mysqld is started *four times* when you first run this container. I think the main argument for pushing this is that we're making a nice example container for popular software, showing how you can follow the docker ideals / "best practice" document etc. (Albeit the nice-ness is limited by how nice the software is to start with). It'd be a shame to stop half-way without a specific reason.
|
I should have mentioned, I also resolved my complaint #190 by dropping the password for the mysql root user altogether. Again, to see how I rationalize my questionable decisions, you should read the commit message :). |
|
@praiskup thanks for notice.. |
|
@hhorak I'm not sure what should have been done earlier to prevent this. I can imagine why it happens, and it's no problem for me if the code as it stands is actually dead. It just wasn't much fun to make sense of. I've rewritten this at least once. It's getting late so I'm just going to post and be damned. I hope there's some constructive use to it, and I apologize in advance for all the fighty-ness I left in. Italic is for the one constructive point I was able to highlight, not intended as shouting or anything. The Fedora-Dockerfiles repo here is claiming to welcome contributions, and I didn't notice any mention of these OpenShift requirements. For some reason I couldn't find the relevant docs at first (a link would have been helpful to show specifically what you meant!), but it looks like it's possible to start from https://docs.openshift.com/enterprise/3.0/creating_images/guidelines.html. E.g. I haven't noticed the required numeric USER statements in Fedora-Dockerfiles. It's an idea I sympathize with, but it's quite non-obvious and confusing to someone coming from the Docker docs + official images! If you're right about what this repo wants, then My understanding is that Fedora-Dockerfiles were supposed to be a nice simple way to get started, hacking up your own images. IMO the SCL image doesn't look like it was written with that in mind. It doesn't "look like" the current images in Fedora-Dockerfiles. I guess some of the complexity is around supporting replication, even though you didn't mention that. I'd be happy to look at the official image and port any missing features like replication from there. I'm not sure I'm interested in the alternative, working with the SCL image as an upstream. It's already quite confusing, to have to refactor it into something I could quickly understand. (E.g. it's not as clear having code split between common.sh in one directory, and the entrypoints in a different directory). And the repo doesn't look as welcoming to contributions as (I assumed) this one was. The OpenShift stuff looks interesting but it seems like a really big constraint. The docs say one of the points is to "prevent running images that are trying to run as root", and you're not allowed to bind to low ports for this reason. In which case it would be better to name this repo OpenShift-Dockerfiles. (I'm grumping a bit, but again, it would have been nice if you'd given some reference for how significant the OpenShift constraints are considered to be). I wasn't planning on learning something like OpenShift, so it doesn't sound like I'm going to be able to test against its requirements. I would not have said the SCL image "looks like" the official one. I guess you mean the env vars should be named the same? I suppose it wouldn't hurt to change the names, but it's still not going to allow for drop-in replacement. Personally I'd prefer not to change things to look similar without an explicit compatibility story. (I don't want to require |
|
@sourcejedi you're right in many things, especially that there is a lot of information missing in the documentation, especially in http://docs.projectatomic.io/container-best-practices/ that is mentioned in the README as guidelines for contributing to this repo. Anyway, I've realized there already was variant of mysql image that I've created, but at that time this repository was divided into branches based on the Fedora version and from some reason it didn't make it to master, so we lost those changes once other branches disappeared. I've just opened a new PR #193 with the same changes as I used before and as I state in the PR -- I'd like to use that one more as showing in practice what I was talking about and we can collaborate on even better image together. |
|
About the documentation - Atomic Host doesn't use Openshift, so of course their guidelines don't explain the Openshift-friendly subset of Docker :P. I did have some comments about the details of the Openshift image, so it's nice to have somewhere I can put them. As I say, I don't feel comfortable developing on it myself, unless there's a convenient way to test against the Openshift requirements. Or if the code was obvious enough that (in most cases) it could just be validated against a (provided) checklist. To be clear where my expectations came from, the package description in downstream fedora-dockerfiles is this:
|
Summary:
sleep 10. One gets replaced withwait. The other is replaced with a polling loop, because mariadb hasn't got--daemonizeyet.docker run -i mariadb). Runtime errors will still be saved, but using docker's logging feature instead of a second data volume.We stop using mysqld_safe because it hates logging to stdout/stderr.
TODO: copy upgrade handling from mariadb.service
EDIT: should have mentioned I resolved my complaint #190 by dropping the password for the mysql root user altogether. Again, to see how I rationalize my questionable decisions you should read the commit message :)..