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

Change hardcoded www-data:www-data #249

Merged
merged 4 commits into from
Dec 21, 2017

Conversation

akolnoochenko
Copy link
Contributor

PHP Apache image allow to run Apache process from user different from default www-data. Its set in ENVs APACHE_RUN_USER and APACHE_RUN_GROUP (see docker-library/php#14)

I propose to use this vars in such scenario:

  wordpress:
    image: wordpress
    volumes:
      - ./www:/var/www/html
      - /etc/passwd:/etc/passwd:ro
      - /etc/group:/etc/group:ro
    environment:
      APACHE_RUN_USER: localuser
      APACHE_RUN_GROUP: localgroup

In that case folder www will have access rights as localuser:localgroup

@@ -31,6 +31,7 @@ if [[ "$1" == apache2* ]] || [ "$1" == php-fpm ]; then
( set -x; ls -A; sleep 10 )
fi
tar cf - --one-file-system -C /usr/src/wordpress . | tar xf -
chown -R ${APACHE_RUN_USER:-www-data}:${APACHE_RUN_GROUP:-www-data} $PWD
Copy link
Member

Choose a reason for hiding this comment

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

See #74 for where a chown similar to this one was previously removed very intentionally.

I think for this to work, we'll need to instead use the --no-same-owner and/or --user/--group flags of tar in order to control the ownership of the copied bits directly instead.

@tianon
Copy link
Member

tianon commented Nov 14, 2017

This is probably the sort of thing we'll have to do in order to ditch the current hard-coding of www-data:

diff --git a/docker-entrypoint.sh b/docker-entrypoint.sh
index 2e92d65..a034c2f 100755
--- a/docker-entrypoint.sh
+++ b/docker-entrypoint.sh
@@ -24,13 +24,22 @@ file_env() {
 }
 
 if [[ "$1" == apache2* ]] || [ "$1" == php-fpm ]; then
+	: "${APACHE_RUN_USER:-www-data}"
+	: "${APACHE_RUN_GROUP:-www-data}"
+	export APACHE_RUN_USER APACHE_RUN_GROUP
+
 	if ! [ -e index.php -a -e wp-includes/version.php ]; then
 		echo >&2 "WordPress not found in $PWD - copying now..."
 		if [ "$(ls -A)" ]; then
 			echo >&2 "WARNING: $PWD is not empty - press Ctrl+C now if this is an error!"
 			( set -x; ls -A; sleep 10 )
 		fi
-		tar cf - --one-file-system -C /usr/src/wordpress . | tar xf -
+		tar --create \
+			--file - \
+			--one-file-system \
+			--directory /usr/src/wordpress \
+			--owner "$APACHE_RUN_USER:$APACHE_RUN_GROUP" \
+			. | tar --extract --file -
 		echo >&2 "Complete! WordPress has been successfully copied to $PWD"
 		if [ ! -e .htaccess ]; then
 			# NOTE: The "Indexes" option is disabled in the php:apache base image
@@ -46,7 +55,7 @@ if [[ "$1" == apache2* ]] || [ "$1" == php-fpm ]; then
 				</IfModule>
 				# END WordPress
 			EOF
-			chown www-data:www-data .htaccess
+			chown "$APACHE_RUN_USER:$APACHE_RUN_GROUP" .htaccess
 		fi
 	fi
 
@@ -115,7 +124,7 @@ if (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROT
 }
 
 EOPHP
-			chown www-data:www-data wp-config.php
+			chown "$APACHE_RUN_USER:$APACHE_RUN_GROUP" wp-config.php
 		fi
 
 		# see http://stackoverflow.com/a/2705678/433558

@akolnoochenko
Copy link
Contributor Author

akolnoochenko commented Nov 15, 2017

Well, I think it's a good point and your solution is more nice in case mounted folders inside /var/www/html.

So I'm agree with your solution. I've also tested it and found one error:

--owner "$APACHE_RUN_USER:$APACHE_RUN_GROUP"

should be

--owner "$APACHE_RUN_USER" –-group "$APACHE_RUN_GROUP"

@akolnoochenko
Copy link
Contributor Author

I think the same could also be done in such way:

chown -R "$APACHE_RUN_USER:$APACHE_RUN_GROUP" /usr/src/wordpress && tar cf - --one-file-system -C /usr/src/wordpress . | tar xf -

In that case we'll split files files permissions management and copying to two specific operations. It could be more readable and more linux way. @tianon what do you think?

@tianon
Copy link
Member

tianon commented Dec 21, 2017

We cannot modify /usr/src/wordpress within the image or we'll needlessly create image layer changes.

So, if we're going to adjust this, it needs to be through tar so that only the destination directory gets the adjustment (and so that we do not backtrack on #74).

@akolnoochenko
Copy link
Contributor Author

@tianon, nice, in that case 6a9cb14 is ok.

@tianon
Copy link
Member

tianon commented Dec 21, 2017

In reviewing this I realized we have two additional wrinkles:

  1. php-fpm variants (which do not respect these environment variables)
  2. arbitrary --user values (which haven't been supported before now in wordpress, but have in php to some degree, especially for php-fpm variants)

To that end, I've applied 5af0b0c, and then rebased and ran update.sh to apply these changes to all variants. Once Travis agrees, I think we're good to merge. 👍

@tianon
Copy link
Member

tianon commented Dec 21, 2017

Here's an example running WordPress+Apache as user 1000 directly with this PR!

$ docker run -it --rm --user 1000:1000 --sysctl net.ipv4.ip_unprivileged_port_start=0 --tmpfs /run/apache2:uid=1000 --tmpfs /var/www/html:uid=1000 24bebfa4eb3b
WordPress not found in /var/www/html - copying now...
Complete! WordPress has been successfully copied to /var/www/html
AH00558: apache2: Could not reliably determine the server's fully qualified domain name, using 172.18.0.19. Set the 'ServerName' directive globally to suppress this message
AH00558: apache2: Could not reliably determine the server's fully qualified domain name, using 172.18.0.19. Set the 'ServerName' directive globally to suppress this message
[Thu Dec 21 23:18:59.782526 2017] [mpm_prefork:notice] [pid 1] AH00163: Apache/2.4.25 (Debian) PHP/7.2.0 configured -- resuming normal operations
[Thu Dec 21 23:18:59.782551 2017] [core:notice] [pid 1] AH00094: Command line: 'apache2 -D FOREGROUND'

@yosifkit yosifkit merged commit 1b48b4b into docker-library:master Dec 21, 2017
tianon added a commit to infosiftr/stackbrew that referenced this pull request Dec 23, 2017
- `mariadb`: 10.1.30
- `mysql`: pass extra `mysqld` flags to `mysql_install_db` (docker-library/mysql#358)
- `nextcloud`: redis 3.1.5 (nextcloud/docker#205)
- `ruby`: bundler 1.16.1
- `wordpress`: adjust hard-coded `www-data` to allow for arbitrary user support (docker-library/wordpress#249)
@ejo
Copy link

ejo commented Mar 8, 2018

How do I make this work if I'm operating in a Docker for Windows environment and Windows Powershell? (i know, i know... my next priority will be just moving everything back into Linux but I was really trying to get this to work out so that I could do dev work on my Windows photo editing workstation without dual-boot or a full VM).

I don't have /etc/passwd and /etc/group in this environment so I don't see how to phrase the APACHE_RUN_USER and APACHE_RUN_GROUP env vars in a way that will work. I tried just plugging in my uid and gid from Windows' own id command but that didn't go.

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