Please review the Core patch #1908440: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination mentioned in #5.

As described #1897712-2: Drush with D8 - I reported problems with MTimeProtected files. I don't know what they are, but @moshe wanted a new issue to address it.

$ drush dl devel
mkdir(): Permission denied MTimeProtectedFastFileStorage.php:150                                                                                                              [warning]
chmod(): Permission denied MTimeProtectedFastFileStorage.php:152                                                                                                              [warning]
file_put_contents(/DRUAL8/sites/default/files/php/service_container/.htaccess): failed to open stream: Permission denied                                [warning]
MTimeProtectedFastFileStorage.php:154

So was this just me, or is there a bigger Drush or D8 issue that needs to be addressed here?

Comments

clemens.tolboom’s picture

Tested with latest

commit 764ab31d5876fcdcf7a58777691e8e60d58e67c7
Author: Jonathan Hedstrom <jhedstrom@gmail.com>
Date:   Thu Jan 31 18:03:58 2013 -0800

    Issue #1881380 by jhedstrom | lotyrin: Fixed .info rewriting broken for modules without a release.

Step to reproduce are

Drush leading

# sudo rm -r files
rm -r files/
chmod u+w .
drush site-install --yes
drush @drupal.d8 uli

Open the link: WSOD

Twig cannot write it's template file(s) as the sites/default/files/php are owned by current (drush) user.

Drupal leading

drush sql-drop
sudo rm -r files/ # files/php/ files
mkdir files
chmod 777 files
chmod 666 settings.php 

http://drupal.d8/core/install.php?langcode=en&profile=standard

After filling in the settings.

drush dl coder
mkdir(): Permission denied  TimeProtectedFastFileStorage.php:150 [warning]
chmod(): Permission denied MTimeProtectedFastFileStorage.php:152 [warning]
... etc
Project coder (8.x-2.x-dev) downloaded to /Users/clemens/Sites/drupal/d8/www/sites/default/modules/coder. [success]
Project coder contains 6 modules: coder_upgrade, bad, bad2, drupalcs, coder_review, coder.

The root cause is Drupal\Component\PhpStorage\* classes

class MTimeProtectedFastFileStorage extends FileStorage {
...
class FileStorage implements PhpStorageInterface {
  public function save($name, $code) {
...
   mkdir(dirname($path), 0700, TRUE);

which make the directory owned by the current user. That is either Drush or the webserver. We cannot mix those can we?

[edit:] http://randyfay.com/content/drush-file-permissions-web-servers-and-comin... [/edit]

Elijah Lynn’s picture

Confirmed in 8.x-6.x-dev.

clemens.tolboom’s picture

I think the trouble is Core 8 is protecting it's config + twig files too much.

We must solve this on the user chmod bits of the files/php + twig I guess. I tried sticky bit on group www-data which was a bad plan :(

franskuipers’s picture

My feeling is drush is not "user-aware". Drupal core needs to protect twig & configuration files, we can't relax the protection there.

Best solution I see is to give ownership to the webserver-user after commands that possibly write into the /files directory. This means the user requires the sudo permissions (local and on the remote server).

sudo chgrp R www-data sites/[default|domain]/files

in drush_shutdown() + a variable webserber_user function would solve it.

Is this a direction we want to go? What are the security concerns?

clemens.tolboom’s picture

Discussing with @helmo lead to the following core patch #1908440: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination.

That probably leads to a lot of discussion as it loosens the file modifications and applies umask. But on my mac drush is happy again afaik.

clemens.tolboom’s picture

Fabianx’s picture

Update, please use FileStorage as described here:

http://drupal.org/node/1908440#comment-7105572

moshe weitzman’s picture

Discussed with chx and Fabianx and here is the plan:

  1. Patch Drush to always use a NULL backend instead of MTimeStorage for its requests. For now, we can use the existing FileReadOnlyStorage class as thats effectively NULL for us
  2. With this, we effectively fixed the problem but the cc all command won't work as expected. For that we, need core to add a feature where it looks for a state flag and if TRUE, calls updateContainer(). If the update fails (as it would on a drush request), the flag is not reset. Someone needs to create an issue a new issue and patch for this. Once core supports this flag, cc all needs to set this flag.
clemens.tolboom’s picture

In #17 of #1908440-17: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination a first step is available indeed but we need feedback on the patch over #1908440: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination

That is should we use a 0y00 or 0yy0 or even 0yyy permissions pattern. Who dares?

clemens.tolboom’s picture

I was about to create the new core issue mentioned in #8 but I don't like that solution. I think it should be Drupals responsibility to decide what to do when invoked through the CLI and take measures. It otherwise make custom script builders puzzled what to do when.

What happens using a drush php-script to pre-seed cache or pre-render imagecache?

Or even drush migrate-import?

(Does that makes sense or am I missing something?)

moshe weitzman’s picture

@Clemens - the public files directory doesn't use MTimeProtected so we would not be overriding it so the imagecache and migrate-import examples would work fine. It is possible that *something* breaks with our plan but its the best we have so far. I agree that its a bit hackish so more thought is welcome.

greg.1.anderson’s picture

I think that all of this is just an uncomfortable symptom of a larger problem.

- In a D6 site, it was almost always cool to run Drush as the non-webserver user (I use www-admin on live, and my personal user account on dev; I'll say 'www-admin' hereafter). Drush dl can write module files that www-data can't touch. Occasionally, you have problems because Drush can't delete user-uploaded files written by www-data in the files directory, but that can almost be considered a feature.

- In a D7 site, you sometimes run into problems with commands such as drush image-flush, but this is fairly easily rectified by calling these commands with sudo.

- In a D8 site, the permissions problem becomes much more complicated, because, as #1 points out, it now matters very much whether Drush or Drupal touches a configuration file first, as the process that creates a file will set its owner. Using "0yy0" permission patterns, as suggested in #9, is a good start. However, you still cannot get around the fact that if Drush creates a config file, it will be owned by www-admin instead of www-data. If Drupal tries to change the permissions on a file that is owned by another user, it will fail, as on Linux, only the file owner can change file permissions. It is my fear that if we rely on special flags, and Drupal knowing whether it is being called in cli mode vs webserver mode, etc., that eventually we'll paint ourselves into a corner and the extra complexity will be for naught anyway.

I see only two solutions.

1. By convention, all config files must be set to "660" permissions when they are created. Both Drush and Drupal must follow this convention, and neither must complain if @chmod fails (or make a wrapper that does nothing if the mode is already set correctly, so that it could properly fail if the mode is set wrong). This might be a little fragile, as it requires a consistent cooperation in core and contrib to work in a mode (cli) that not everyone uses. Additionally, third party libraries used by Drupal may include chmod in a different way, and we may not be at liberty to change these references easily. (This is essentially #1908440: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination.)

2. Drush commands must become aware of whether they are an "admin-type" command (e.g. drush dl) or a "user-type" (webserver) command (e.g. anything that writes to config files). The former run as they always have, whereas the later must sudo to the webserver user before execution. Commands like site-install that work in both modes would need to split out their "user-type" operations into separate Drush commands, and call them with drush_invoke_process(). This is a little complicated; Drush historically does not manage 'sudo', and would need to be configured to know what the web user was. There would also be cross-platform compatibility issues.

3. Drush commands must know when to re-set the owner and permissions of Drupal after execution completes, using #990812: Add a "permissions" subcommand to fix/set all file permissions or something similar. This is solution "3 out of 2" because it still uses sudo, and therefore doesn't seem any better than 2, so perhaps should not be counted.

One way or another, in the age of CMI and more frequent use of Drupal by files to store information that formerly lived in the database, I believe that we are going to have to work out a lasting solution to the permissions problem. (2) would be a big shift for Drush, but I think it might be the best route long-term.

moshe weitzman’s picture

Issue tags: +SprintWeekend2013

The proposal in #8 could fail, but I see no better option at this moment. We may have to fail in order to force core to change. Therefore ...

I committed part 1 of #8. Drush requests no longer throw errors, but web requests still show a warning after having used site-install to install. That error is:

Warning: mkdir(): Permission denied in Drupal\Component\PhpStorage\MTimeProtectedFastFileStorage->ensureDirectory() (line 150 of core/lib/Drupal/Component/PhpStorage/MTimeProtectedFast

I'm not sure why Drupal could create subdirs of files directory in the past but seemingly can't do so here.

Drupal also throws a Notice about not being to dump the container but thats harmless and only shown when Drush is in verbose mode. Least of our problems.

I opened an issue for part 2: #1938402: Rebuild Container based on a new state flag. Once that is committed, we need to set the new state() during cache-clear command.

effulgentsia’s picture

I'm not sure why Drupal could create subdirs of files directory in the past but seemingly can't do so here.

Is this still true? At one time, D8 did chmod sites/default/files itself to remove all group and world permissions, but I don't think it does any more.

One way or another, in the age of CMI..., I believe that we are going to have to work out a lasting solution to the permissions problem.

Are there any CMI-related problems left? I just now newly installed D8 HEAD, and on my machine, looks like the sites/default/files/config_.../active directory has group write permission. Are there any known problems if some files in there are owned by the web user and others by the drush user? This issue is currently titled to be focused on PhpStorage, so I'm curious if CMI, CSS/JS, or image style issues still exist.

greg.1.anderson’s picture

Okay, let me tell you what I was thinking in #12. While I am not active with D8 and can't speak to how things are now, there were some places in some of our dependent libraries that were making chmod calls. I can't quite remember if it was 'chmod' (bad, as it fails if a cli tool created the file) or '@chmod' (probably okay, if they picked 660), but I think it was mostly in twig, which Drush for the most part shouldn't call. My concern was therefore more along the lines of "anything could happen", and if our upstream providers made bad (for Drush) chmod calls, it could be awkward or slow to get the fixes in.

Per #14, though, it seems that things are getting worked out rationally (similar to option (1) in #12), and if I've followed everything correctly, it looks like folks are moving in the direction of insuring that we are using 660 / 770 permissions for files / directories that should be webserver-writable. (Hopefully, no one is keeping separate caches for the web server and for Drush--from other comments, it appears that this is not being done, but I did not confirm.) I didn't do any practical tests, but it looks like the drupal_chmod wrapper function is doing the right thing (unless someone passes in a bad $mode parameter -- and I also wonder about that call to watchdog, which probably should at least check to see if the current permissions of the file are the same as the desired permissions before logging the error). It does seem that things are moving in the right direction, even though there are a few minor worrisome bits of code here and there. For example, in drupal_unlink, there is a 'chmod(..., 0600)' that should be '@chmod(..., 0660)', and similarly, drupal_rmdir uses 'chmod(..., 0700)' instead of '@chmod(..., 0770)'. I did not open issues for any of these things which might be a problem, but don't necessarily seem to be showing any symptoms, nor did I search the issue queue to see if there are pending fixes about ready to go in -- I just pulled the head of today's 8.x branch and did a couple of quick searches.

In short, I'm satisfied that, one others who are using D8 right now are satisfied with the way things are working, we can just close these issues as 'fixed' and put off #12-(2) for another time, and another issue. Perhaps as folks get more used to to dual webserver / cli environment, the calls to chmod will become more consistently correct.

juampynr’s picture

The Symfony documentation has some suggestions on how to set this up so both users (the console user and the web server user) have rights to write on those directories.

Please have a look at Setting up permissions at this page http://symfony.com/doc/master/book/installation.html

clemens.tolboom’s picture

@juampy thanks for the pointer. Do you mind to review and apply this document to #1908440: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination?

greg.1.anderson’s picture

I think that we need to review #16 here first, before adding it to the core issue.

Current situation:

Drupal will create a directory, and then create files inside of them. The php chmod command is used to make the files non-writable. Later, when the file needs to change, php chmod is used to make the file writable again, and its contents are changed. This can cause a problem if the later operation is done via Drush, because the php chmod will fail if it is owned by another user (e.g. the web server user).

Proposal in #1908440: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination:

Delete the file first and rewrite it; then only the directory permissions are important, and these should allow deletion. (Additional file permission mode changes also suggested -- see references issue for details).

Symfony solution:

The suggestion in the Symfony docs might help here. It presumes that either chmod +a or setfacl is available on the target system. I do not know what provides chmod +a, but this is not available by default on Ubuntu/Debian. setfacl, on the other hand, is pre-installed on Ubuntu/Debian. This might indicate that these commands are prevalent enough to consider using as a solution, but it would exclude Windows users from easy commandline use. This may or may not be acceptable of Drush; however, it certainly would not be acceptable to call chmod +a or setfacl from within Drupal code, because we need to support Windows from Drupal core.

The question, then, is whether or not this is a real solution. In order for it to work, the following sequence would have to work:

1. Set up ACL's as described in Symfony docs on Drupal root
2. Create folder and chmod it via php chmod (web user)
3. Create file and chmod it via php chmod to non-writable state (web user)
4. php chmod the file to a writable state (shell user)

If step 1 allows step 4 for files created and chmod'ed via php, and if we do not care to support Windows cli users, then we could make a note in #1908440 that said solution is not necessary, as we have a way to solve the problem in Drush.

I'm not sure if this will work out or not; I have not had time to investigate it very much myself yet. It is certainly an interesting possibility, and I very much appreciate the pointer.

clemens.tolboom’s picture

I ran into another related drush write error while running

drush config-import

getting

WD config_import: Drupal\Core\Config\StorageException: Failed to write configuration file: sites/site-x/files/config_hash/active/image.style.medium.yml in Drupal\Core\Config\FileStorage->write()

while working on #2012586: config-import is broken due to removal of config_sync_get_changes.

I'm not sure how this relates to this very issue but guess this is close enough to report in here. (also reported in #1908440-72: Relax MTimeProtectedFileStorage permissions for DX, drush integration and world domination)

moshe weitzman’s picture

Thats not related. The config directory does not use MTimeProtected.

juampynr’s picture

I tried the following (tested in my Ubuntu 12) I took from the Symfony Permissions doc I mentioned at #16 and after that I can clear caches with Drush and the user interface without errors:

sudo setfacl -R -m u:www-data:rwX -m u:`whoami`:rwX sites/default/files

I have asked at #symfony what do Windows users need to do and someone told me that as long as they are using Apache nothing needs to be done. We would need someone with a Windows box to verify that. The Symfony2 documentation says nothing for Windows users so this may actually be true.

Owen Barton’s picture

Not all Linux/Unix systems support ACLs (ours don't, and some need root access to enable or install the utilities), although perhaps enough do that this is still a reasonable solution.

I still need to read the other issue in more detail, and could be missing some details - so I'll throw this out here first - couldn't the web user and the cli user each get their own directories/namespaces - they can each load the config themselves, and Drupal could use some flag (perhaps just the most recent mtime) to determine the active namespace? This way, neither needs to mess with the others stuff (each would still need readability of the other, but that seems an easier problem).

moshe weitzman’s picture

Status: Active » Closed (duplicate)
moshe weitzman’s picture

@Owen - Drush needs to cache-clear the directories that belong to the web server.

moshe weitzman’s picture

FYI, this got resolved in Drupal during Drupalcon Prague.

moshe weitzman’s picture

Issue summary: View changes

Added link to the core patch