Simplify the settings for filesystem paths

changed the description

I'll give credit to ojkelly for coming up with that hack. It is not pretty but it works. I've included it as an optional patch in the CiviCRM Starterkit.

From what I understand, the Smarty templates are compiled, not caches so I don't think it's quite the same. I assume that they don't expire like caches would.

I was looking at your link but I didn't see anything about WPEngine not allowing other .php in webroot. Unless I overlooked it.

Our host (WPEngine) does not allow guest users to write to php files on production environment.

It's about writing to PHP more than locating cache dir in webroot. But I'd say:

Ability to not use filesystem would be an improvement
Not being in webroot would be an improvement
Not having .php ext might be an improvement (provided either of the above - for smarty/civi, .php acts as a way to prevent direct read of personal data, at cost of generating many directly executable files that users have input to)

Agree it would be an improvement to store outside the webroot (and... preferably... moving templates_c shouldn't break all the other URLs).

For changing the extension from .php to .php_ or .txt.... I'm skeptical of the value. Don't get me wrong, it might help for hacking around restrictions on one or two hosts. But:

If httpd config blocks access to templates_c (as expected/tested), then changing the file extension doesn't add any a security.
If httpd config does not block access to templates_c (as expected/tested), then changing the file extension is just as likely to make it worse (by revealing source code).
With or without renaming, any direct invocations should be blocked. With or without renaming, a clever hacker might replace a cache file with malicious code. Renaming just creates the new edge-case where code might become publicly visible.

IMHO, if one wants to kill templates_c, then we have to pursue workflows that pre-compile the templates for civicrm-core (and for any extensions). There are only two opportunities where pre-compilation could be done: the build-process and the install/deploy process.

IMO the httpd_config part (eg .htaccess support or custom webserver configs) is (at least part of) where we are not "cloud native" here. I don't expect these hosting envs to know about CiviCRM's templates_c or support .htaccess

I must admit I hadn't peeked at the actual content of templates_c but I think it's runtime compilation. I can't look right now (no CiviCRM on work laptop at new job!). A good test might be "do any contact names / DB values appear in templates_c?" -

grep -ri andrew templates_c

... if so then that's not a compilation we can do until after deployment.

I agree with you about changing to .txt|.php_|... for the reasons you give (as I was trying to say in my third point).

Oh! That's you commenting Tim. I will happily defer to your knowledge of how things work if you think that's how we could deal with templates_c? If true, I'm surprised CiviCRM even asks for +w there.

Yup, we're in agreement about file extensions. :)

Regarding the last comment, we may have crossed wires -- as of today, the templates use runtime compilation. If the idea is to remove templates_c as a permission/security concern, then I'd vote to work toward pre-compilation (during the build or install phases).

But I have jumped the gun a little -- there were some good links in the first comment about Smarty/Redis. Random reactions:

The patch at https://github.com/freeform/civicrm-drupal-pantheon/blob/4.6.x/patches/smarty-redis-civi-cache.patch looks much more complete/understandable. :) tldr: it continues with on-demand compilation; it writes+reads the PHP code via Redis; and then it eval()s the PHP code at runtime.
I suppose this solves problems with templates_c on a distributed filesystem. Evidently Smarty-on-Redis provides a more consistent experience than Smarty-on-$blackboxDistributedFilesystem.
From a security perspective, templates_c and Redis+eval() are the same thing. Both designs violate the "writable XOR executable" (W^X) principle. Violating W^X means that a less-serious flaw (in file-management or cache-management) can be converted to a more-serious flaw (arbitrary code execution).
The perfect shouldn't prevent the good. It's quite reasonable to fix the distributed-filesystem issue (ie Smarty-on-Redis) without fixing the W^X issue.

There's been a conversation about this on the Pantheon Poweruser list. I think this quote from David Strauss at Pantheon is quite helpful:

It's been years since I was a contributor to CiviCRM, but if the templates work the same way as they did, then:

It's essential to performance that they get generated and cached at the opcode level (then APC, now opcache).

It wouldn't be possible to bypass the PHP generation without breaking the architecture.

More generally, my recommendations for generated content are:

If it doesn't have to be on disk...

Generate the content and cache it in an object cache (Redis on Pantheon) or the proxy/edge cache (Global CDN on Pantheon).

Handle invalidation via query strings, explicit invalidation, or surrogate keys.

If the generated asset has to be written to disk (e.g. PHP)...

Store the generated assets only to the local machine (in something like the temporary directory). If there are multiple servers/containers, it's okay to generate the same thing more than once. Avoid saving generated assets to a network file system.

Handle invalidation by altering the expected filename when the source content changes.

These recommendations aren't relevant just to Pantheon; they're good recommendations for any architecture that needs to support multiple web servers.

I'm guessing currently that templates_c doesn't have any opcode caching.

David does provide a possible good alternative to using Redis, at least for Pantheon: store the files in the tmp directory which is local to each web server. Since it's runtime compilation I'm guessing it will generate the files each time a new web server is spun up.

I'm considering this as alternative to the Redis hack, since it should be much more straightforward (so long as Civi is also patched so that no other system paths are being created based on the templates_c path--which is a different issue).

If the generated asset has to be written to disk (e.g. PHP)... Store the generated assets only to the local machine... Handle invalidation by altering the expected filename...

This is well said.

For newer compiled assets (CachedCiviContainer.*.php and CachedExtLoader.*.php), the most obvious point at which to twiddle the filenames is CRM_Core_Config_Runtime::getId(): update it to include some kind of generational ID like the resCacheCode from CRM_Core_Resources. It probably makes sense to have Smarty use the same identifier in file-paths.

The real problem is that the setting CIVICRM_TEMPLATE_COMPILEDIR hasn't been intuitive -- because it has been used to calculate default paths for unrelated things. A full audit/test would be a proper task, but loosely speaking... you can probably make 50%+ of the issues go away by explicitly setting both CIVICRM_TEMPLATE_COMPILEDIR (to a local/mid-term storage) and $civicrm_paths['civicrm.files'] (to persistent/long-term storage). The new installer should be doing this by default on D7/Backdrop.

I'm guessing currently that templates_c doesn't have any opcode caching.

Opcode caching is a service provided by the PHP runtime. As long as you write+read concrete *.php files, the PHP runtime should automatically do opcode caching. In a vanilla Civi deployment, the templates_c uses opcode-caching. (That's the point of its existence.) However, with smarty-redis-civi-cache.patch (Redis+eval()), there would be no opcode caching.

Disclaimer: I just want to be crystal clear here, because critiques of templates_c are generally grounded in two very different things: (1) compatibility with distributed hosting systems (2) defense-in-depth/security/writable-xor-executable. Using a local temp dir with dynamic filenames is relevant to the first but not the second. IMHO, it is useful to make progress on (1) without addressing (2).

Thanks for clarifying, @totten, about opcode caching. My guess was way off. Learning as I go.

So sounds like if we can make it easier to assign templates_c to /tmp/ this might give us the best of both worlds. Pantheon and WPEngine both recommend using /tmp/ it sounds like. @xurizaemon this might help your WPEngine issue.

Source for WPEngine: http://docs.gantry.org/gantry5/troubleshooting/permissions-error -- "WPEngine recommends that users place cache files within the /tmp/ directory." (I couldn't find a direct quote from WPEngine but Gantry is compiling Twig on WP so they probably know their stuff.)

Tim, I'm happy to help with making CIVICRM_TEMPLATE_COMPILEDIR more intuitive. I had to take a stab at it to make it work on Pantheon here: https://github.com/civicrm/civicrm-core/pull/10513. If I can help get something like this merged then it could eliminate two patches used on Pantheon.

Here's my audit of both baseFilePath() and CIVICRM_TEMPLATE_COMPILEDIR: audit-template_compiledir.txt. Thinking more about this, on Pantheon we need to put compiled templates in /tmp/; private files like logs in /sites/default/files/private/; and public files in /sites/default/files/. So this means setting all three separately.

A few random reactions:

The references to baseFilePath() in CRM_Utils_System_* should become irrelevant if civicrm.settings.php has the $civicrm_paths['civicrm.files']
The references to baseFilePath() in CRM_Utils_File::absoluteDirectory and ::relativeDirectory already appear to be irrelevant within civicrm-core. (The only usage I could find was one which explicitly set its own base.)
The reference to baseFilePath() in CRM_Core_Config_Runtime is more effort. I don't know if it'd work, but my first try would be (a) lookup a path-variable like Civi::paths()->getVariable('civicrm.log', 'path'), (b) declare the variable in Civi\Core\Paths, (c) change the relative boot order of Civi\Core\Paths and CRM_Core_Config_Runtime.
I think CRM/Utils/Cache/SerializeCache.php is unused.
The CRM/Core/IDS.php line feels silly. We should pick one folder! Pointing that at templates_c makes as much sense as uploadDir. (TBH, I'm not sure does anything now that Config.IDS.ini has been killed.)
Civi/Core/Container.php and CRM/Extension/ClassLoader.php are very similar to the Smarty use-case (i.e. writing out ephemeral PHP files to take advantage of opcode caching).

In case of some misconfiguration or predictable file name, I always put private files outside the webroot, e.g. in ../private rather than sites/default/files/private. So private files should be assumed to not be web accessible - but in some cases it seemed like CiviCRM was assuming that private files are web accessible.. so I had to make symlinks.

@totten in the PR https://github.com/civicrm/civicrm-core/pull/10513 you mentioned doing this in phases. So I wonder if in the first phase we need to keep baseFilePath() where we can either use civicrm.files if it is set then and fallback to CIVICRM_TEMPLATE_COMPILEDIR if it isn't.

added needs:concept approval in cloud-native sig:compatibility triaged type:proposal labels

mentioned in issue cloud-native#3 (moved)

In my case of using Pantheon, setting CIVICRM_TEMPLATE_COMPILEDIR to be the /tmp directory has helped quite a bit (ignore any previous hacks). But it still requires more control over the filesystem paths cloud-native#3 (moved)

I think some redesign might be sensible to give direction here.

Other CMS have for example three paths which everything is derivative of:

Temporary - generally not accessible, and don't need to be persistent
Private - not accessible directly, .htaccess protection or outside wwwroot, access may be mediated via CMS
Public - www-accessible

CiviCRM has many fs paths, some may be derived from others, and some have confusing or inconsistent names (from memory).

CiviCRM.log
UploadDir
ImagesDir
I'm on my phone, can you tell?

Should we be deciding first on a new layout, a mapping between old and new (civicrm.log = private + /logs) and whatever flexibility is required to handle different site expectations? Eg in the past when we "fixed" a direct access to private files issue, some sites were relying on that access.

I think that design statement could simplify the GSoC work and ensure an outcome we can utilize.

Might be an interface for extensions / CMS interfaces to adjust paths during boot is called for, but there's a bootstrapin issue there.

Might be an interface for extensions / CMS interfaces to adjust paths during boot is called for, but there's a bootstrapin issue there.

That's the intent of global $civicrm_paths.

If we want to stay sane, then interfaces for manipulating bootstrap have to revolve around one of these:

define('CIVICRM_FOO', ...) (which is something Civi does today)
global $civicrm_foo (which is something Civi does today)
`getenv('CIVICRM_FOO') (which is something Civi doesn't much do today)
Pass parameters to a boot-class/boot-function. (which is technique we don't have in core; CRM_Core_Config::singleton(TRUE,TRUE) is sorta like that but not really; https://github.com/civicrm/cv/blob/master/src/Bootstrap.php and https://github.com/civicrm/cv/blob/master/src/CmsBootstrap.php are better examples, but they're not in core)

This deviates from Civi's general architecture of "settings"+"hooks", but... as you hint at... settings and hooks are unreliable/brittle during bootstrap.

Other CMS have for example three paths which everything is derivative of: Temporary... Private... Public...

CiviCRM has many fs paths... CiviCRM.log... UploadDir... ImagesDir...

I like temp/private/public split as the baseline.

It's not completely fair/apples-to-apples to compare those settings against, say, D7-core settings (ignoring D7-contrib settings), but that's academic, and I mostly agree with the goal. The real question is how to transition away.

For example: IMHO, imageUploadDir, imageUploadURL, extensionsDir, and extensionsURL are silly settings that shouldn't exist... but it's hard to remove something once it's there (without breaking compatibility). What we could do, though, is phase them out... e.g. pull those settings out of the admin UI, put them in config files, and carefully design/test the notification/transition-process. (The process would probably take a few release-cycles.)

This has diverged from "make templates_c work on places where it doesn't" to "resolve the fs path situation". Be good to put together a clear problem statement, possibly enumerate other settings than the above, decide whether this ticket is templates_c or something bigger.

My proposed approach would put those settings.get calls behind a compatibility wrapper:

Civi::settings()->get('imageUploadDir')

if (isset($settings['imageUploadDir'])) {
  // old sites keep their existing settings,
  // maybe upgrader removes the override for
  // sites where values are equivalent
  return $settings['imageUploadDir'];
} else {
  // compatible with existing layout, but defined
  // based on the new model 
  return Civi::settings()->get('fs.private') . '/imageUploadDir';
}

changed title from Smarty templates_c to Simplify the settings for filesystem paths

I renamed this to be closer to what @xurizaemon mentioned.

mentioned in issue #3524 (closed)

I think we should close this and make more specific issues if needed. Some of the random ideas have been done elsewhere already.

closed

Simplify the settings for filesystem paths

Child items ...

Activity