Updated: Comment #33

Problem/motivation

In #1929136: Fix override-free context, move global config overrides back to an event listener and #1763640: Introduce config context to make original config and different overrides accessible, @pounard voiced strong concerns that not taking global overrides in case of an override-free context poses security issues. He proposed that would be a security issue. In Drupal 7, if you put something in global $conf, whatever you put in the forms to edit those values, it never took hold, the global $conf set value was trumping everything. In Drupal 8 that is not the case, and we should discuss if this is good or not.

1. Global $conf values were used for different purposes. For example, variable module used them to override values with translations with early versions creeping in translations into original config. That was resolved with workarounds, and Drupal 8 has a native override system that can differentiate between original values and overrides (thanks to variable module experience by Jose :), so this use case might not apply anymore. However, it is easy to imagine people use global $conf values to include dynamic overrides as well. Such as values depending on session information or outside data sources, etc. Dynamic conditions are possible to put into settings.php and we really don't know if certain global $conf values were used as strongarm to avoid certain settings from being modified or were dynamically generated. These are two use cases for global $conf.

2. When different overrides are available, they have different priorities and the stronger one will prevail. As per the current setup in #1929136: Fix override-free context, move global config overrides back to an event listener and prior to that in #1646580: Implement Config Events and Listeners, and storage realms for localized configuration, global overrides were lower priority compared to language overrides. So if you overrode your site name in your global overrides, if you also had translations for your site name, then the translations prevailed. That make sense for the site name use case at least. Again there are likely use cases when you want some global overrides to apply if there are no translations and others to just strongarm on the site and not let people change or override in any other way. The language overrides now prevail for two reasons: (a) their weight in event listener priority is higher compared to global overrides [not actually relevant] (b) they listen to an event fired later (config.load vs. config.init), so they set their overrides later and therefore trump global overrides.

The position that @pounard explained is where global $conf is used as a security enforcement system, and working around it either by making higher priority overrides or making it sometimes not apply in forms is a hazard. We should figure out if we want to make global overrides trump everything (take higher priority) and even apply in override free environments, keep their current lower priority or separate this to two override system, one where you can put your enforced overrides that would always apply even in "override free" contexts and one where you put your soft global overrides. There can be arguments for both.

It should be noted it is already possible to do global overrides that would trump language overrides, it is admittedly not currently possible to do global overrides that would sure-fire apply even in "override free" mode, that is prohibited right now.

Proposed resolution

Make global overrides stick:

  1. Keep the override-free context, since it is used in internal config operations to access the original values for import & staging.
  2. Add the capability to force overrides. The default override handler will take any override either forced or not. The free context will not take any override, even if forced and there is a forced-only context, which only takes forced values.
  3. Add a config.context.only.forced on the DIC to expose this and make the admin forms use it. Made global overrides forced.

Remaining tasks

  • Re-roll patch
  • Agree that the global $conf set value should trump everything

User interface changes

TBD

API changes

TBD

#1929136: Fix override-free context, move global config overrides back to an event listener
#1763640: Introduce config context to make original config and different overrides accessible
#1646580: Implement Config Events and Listeners, and storage realms for localized configuration
#1932336: Use override free configuration context for SystemConfigFormBase

Files: 
CommentFileSizeAuthor
#40 drupal-n1934152-39-conf_ftw.patch4.25 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] 58,897 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#39 drupal-n1934152-38-conf_ftw.patch0 bytesDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#35 global-overrides-forced-35.patch4.25 KBEc1ipsis
PASSED: [[SimpleTest]]: [MySQL] 57,913 pass(es).
[ View ]
#24 interdiff.txt622 bytesGábor Hojtsy
#24 global-overrides-forced-24.patch11.51 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 53,240 pass(es).
[ View ]
#19 global-overrides-forced-19.patch10.91 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Posted this to the security team:

Hey,

In Drupal 8 if you put something in global $conf, that overrides your config by default but it may be ignored if the code specifically asked for an "override free context" or if there are translations for the value, those would trump global overrides. It would be important to get this right and I think there are people using global $conf as a security enforcement system (beyond admin permissions), which is NOT currently possible in Drupal 8 due to how it is applied. I think the way it works now makes sense for use cases where you want a soft fallback default that cannot be edited, but if we consider global $conf overrides as a security measure, we should somehow support that use case too, or maybe restore global $conf to be an enforcement system like it was in Drupal 7 and not have soft overrides or have them in a different way.

Your feedback welcome in http://drupal.org/node/1934152

Gábor

And tweeted this to get more people from the larger community: https://twitter.com/gaborhojtsy/status/308971595563679744 (earlier in https://twitter.com/gaborhojtsy/status/308969857951608832 which was removed due to a spelling error).

Hope this gets us some insight into what people want :)

Issue tags:+security

Issue tags:-security

Being able to override settings per-hostname is extremely important for many developers and many sites. The settings.php $conf array is how I ensure that production sites have the correct production-worthy settings enabled while my local install has all caching disabled, uses alternative API keys, etc. This must be possible. How can I help? :-)

Issue tags:+security

sorry.

@DamienMcKenna: yeah, so overriding for different host names is an interesting use case. Do you mean overriding conditionally in settings.php or just using different settings.php files in different environments? global $conf is still used and applied EXCEPT if the code is in an "override free" environment which is only applied for admin forms. So if you go edit your site setup, your $conf overriden values will not be present there and not saved. Which is logical if you use logic in settings.php to apply to different domains, but probably irrelevant if you just use static settings.php overrides on different setups.

@Gábor: I've used multiple combinations of this:

  • Different values for a "true" multisite install, e.g. different API keys for different Mailchimp accounts.
  • Different values for different instances of the same site, e.g. API keys for the production vs dev Acquia Network accounts to avoid corrupting the production search index :) and different performance tuning settings.

I've had a blog post kicking around in my head for 2+ years on how I handle variables, I really need to write it down..

So the question is if its a bigger problem if those overrides creep into settings forms and get saved as original config on sites, or if its a bigger problem if they don't creep into forms but then they are not applied when editing those config (so eg. if the global override changes the theme that would not be possible on the admin editing form). Which is the bigger problem? :)

I have done many of the same things as @DamienMcKenna - I think the API key use case is one to consider for sure - how to we ensure that "production only" config doesn't leak to staging and vice versa (you don't want to get your real and testing Paypal API keys mixed up, trust me!). If there is not already a good way to ensure this, $conf might be a reasonable solution.

I have also used the case of having multiple hostnames all using sites/default (and the same database/files etc) and using $conf to switch out some key variables (frontpage, language, site name etc) based on the hostname. This is all stuff I could have done (less efficiently) with using sites/hostname or other means however.

Another use case to consider is for automated hosting setups - allowing the host to set the database and reverse proxy configuration dynamically (as Acquia does, for instance), and if necessary change it - it seems preferable if this can be done without needing to trigger a config reload or other operation that may have side effects.

Another use case to consider is for automated hosting setups - allowing the host to set the database and reverse proxy configuration dynamically (as Acquia does, for instance), and if necessary change it - it seems preferable if this can be done without needing to trigger a config reload or other operation that may have side effects.

Anything that can't actually be stored in the database/CMI is in $settings now - this distinguishes between things that are exclusively configured in settings.php vs. overrides. There's likely a grey area where something is used after CMI becomes available but you don't ever want a UI for it, I'd lean towards $settings for those too.

I think it's OK that conf overrides don't show up in forms and aren't saved back, that's arguably a bug fix compared to Drupal 7, there's two things that could use clarifying though:

1. Should we warn people on the form somewhere that values are overridden in $conf and their changes won't take effect?

2. We should put a huge warning on the 'no context' context to make it clear that $conf overrides are disabled for that context. I can't really think of a reason to use it except for the forms and maybe the new drush vget.

@Gábor: With my dev workflow, nothing is changed via the admin UI on production, so these overridden settings being exported to the config files elsewhere doesn't matter, the changes won't be committed unless I've reviewed every single line and given it my blessing.

my 2 cents: I use it a lot to differentiate between environments (all running on top of aegir)

Some things that can be in there besides API keys:

<?php
 
global $conf;
 
$conf['https'] = TRUE;
  unset(
$conf['cache']);          // disable hardcoded caching
 
unset($conf['preprocess_css']); // disable hardcoded css aggregation
 
unset($conf['preprocess_js']);  // disable hardcoded js aggregation
 
$conf['xmlsitemap_submit'] = 0; // disable XML Sitemap for dev.domain
 
$conf['xmlsitemap_update'] = 0; // disable XML Sitemap for dev.domain
?>

I do address a similar issue over in Domain Access, where I have a small hook that can be used to declare which admin forms contain domain-sensitive values. In an ideal world, the config system might detect these hard overrides and flag them for the user via a message.

That would, however, involve some trickery to match $conf values with form config values. Perhaps if we disallowed putting "loose" $conf variables in settings.php and instead allowed for a standard function call for overrides:

e.g. Drupal 7:

<?php
// settings.php
// Custom settings.php lines
$conf['foo'] = 'Bar';
?>

Drupal 8:

<?php
// settings.php
// Load config variable overrides within settings.php
$conf = settings_override();
function
settings_override() {
  return array(
'foo' => 'Bar');
}
?>

Then the config form process could invoke settings_override() and look for matches.

/me stops thinking out loud.

[edit] And notes that catch said something similar in #9 while I was writing this.

I don't think there is a common element in config forms, etc. views, node types and site settings don't have much in common, also form structure is not related to config structure (as in views, or even the 403/404/front path config is entirely differently presented on the form vs. the config file). It was a point of the CMI initiative *not* to deal with automated form generation. D8MI needs to generate such forms, so we have http://drupal.org/project/config_translation which was so far deemed not a fit for Drupal 8 core. There it is possible to mark elements, because all forms are autogenerated from the schema/data combined. I don't see a reliable way to mark elements in Drupal forms overall where original config is edited.

I think it is a big win to disable form elements where the admin's changes would not make any difference. It is not appropriate IMO to offer a non-overidden value for editing without telling the user whats going on. His edit is likely not to make any difference. We have always done it that way and it has always sucked :(

If config_translation project gets us there, perhaps we should consider it for core.

@moshe: I'm not sure such programatic backreferences are possible. In Drupal 7, system_settings_form() pages were known to store their values in variables and their form elements were named equal to the variables. So you could look at the form element name and put on warnings if it was overridden. Eg. variable module used this to put on notes if the variable was multilingual.

In Drupal 8, there is no central store like variables, and it is not really known where in config would the value be stored. So while you can get the form element name, you don't really know where in config it would end up. For example the user account settings page uses two config files to store settings, one for "base" settings like anonymous user name or whether registration is allowed and one for all the user email subjects and bodies. All are edited on one form altogether. Menus are config entities and the menu editing form merges editing the menu itself with menu items, so it is quicker for the user. So to be able to tell for specific form fields whether they are overridden (globally or in some language), we'd need to have this information on each form element. That is lots of added info to form structures, that is not there now. And then finally, the question of how we generally disable an element comes up. There are potentially complex form elements. Eg. if the site logo upload field would have an inline preview of the logo, how would you make that form element disabled? If you enforce date formats to be in a specific setup, then multiple admin pages would be entirely disabled / pointless. You could try to add new formats, but that would not work.

The config initiative specifically opted to not deal with automated form generation and wanted to leave forms as one-off implementations on top of config. Later config entities somewhat structured this for whatever is implemented through config entities, so you kind of have some structure (and config vs. form relation somewhat programatically discoverable), but still then the form item vs. config key relation does not necessarily work. See menus.

What config translation (contrib) module does it takes the config schema, that describes the structure of the config and generates translation forms based on that. It totally ignores the original forms for config, because it just cannot pull out fields from the various views forms or from the middle of a config entity or cherry-pick elements from different levels from the structure of the user account settings form. So that does not even attempt to do this relation, because it looks pointless. Dries spelled out earlier he sees this module as a parallel form API and not a fit for Drupal 8. Also, this module needs to build up its own relation system between admin paths and config keys, so it can put translate tabs on the right places for example (and go and alter random forms of various structures to put in translate operations to summary tables).

OK, you've convinced me that showing disabled form elements is desireable, but impossible. Sigh.

I guess we need an "override free" state so that we don't save translated values of site slogan and config like that. Can't we just neuter the LocaleConfigOverride in this case? In my world, all conf overrides would *always* be in effect, even during admin forms. This preserves D7 behavior and satisfies the security concern described by @pounard.

Yes, it is totally our decision, whether we want to make global overrides always apply for example, if those showing up in forms is a smaller issue vs. those not being applied on some admin forms. If we add a feature so overrides can optionally apply in override free contexts too, then one can do logic based overrides (such as domain based overrides in contrib) with a system separate from global $conf and it is their choice if it applies in this context or not. We will need a better name for this context then too :)

Title:Figure out override priorities and whether we want a kind of global override to stickFigure out override priorities and whether we want global overrides to stick (settings.php overrides don't work on all pages)
Priority:Normal» Critical

I believe I just ran into a version of this, and the current behavior is bizarro broken.

You can look at #1926228-5: Performance page provides incorrect/incomplete information about CSS and JS compression for a really simple patch I tried to write which changes the value of some text on an admin form based on a config setting (one which we encourage people to set in settings.php). The patch didn't work and I assume this "don't apply overrides on admin forms" behavior is the reason?

Or here's a simpler example you can try:

  1. Go into settings.php and uncomment the $conf['system.site']['name'] = 'My Drupal site'; line.
  2. Make sure you aren't using the Seven theme (this step is only required because Seven doesn't actually display the site name on the page).
  3. Navigate around the admin pages of your site and observe how some pages say "My Drupal site" at the top and others don't.

Yeah...

Status:Active» Needs review
StatusFileSize
new10.91 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

@David: sounds like the reason indeed. Looks like one more +1 from you to make global overrides stick. (Considering them creeping into forms not a bug like many people noted but a feature like many other people noted). Here is a possible solution. Will probably not pass tests yet.

1. I kept the override-free context, since it is used in internal config operations to access the original values for import & staging (see code in core/config.inc exclusively, which is not even touched in the patch). With this patch that is the only place where that is used.
3. Added the capability to force overrides. The default override handler will take any override either forced or not. The free context will not take any override, even if forced (may be a possibility for misunderstanding :) and there is a forced only context, which only takes forced values.
2. Added a config.context.only.forced on the DIC to expose this and made the admin forms use it. Made global overrides forced.

Status:Needs review» Needs work

The last submitted patch, global-overrides-forced-19.patch, failed testing.

Status:Needs work» Needs review

@Moshe:

I guess we need an "override free" state so that we don't save translated values of site slogan and config like that. Can't we just neuter the LocaleConfigOverride in this case? In my world, all conf overrides would *always* be in effect, even during admin forms. This preserves D7 behavior and satisfies the security concern described by @pounard.

Domain module will also use this override system. The overrides of domain module are conditional on which domain you are on, so saving them back to the original config might be an issue or at least confusing. There are unconditional overrides like sometimes global $conf (although I've seen conditions in settings.php files when setting config values in $conf), and there are dynamic overrides like translations or domain overrides. Looks like we want some of them apply to forms but not others. So the question is where is that decision made. The patch I proposed puts that decision with the override event listener and only the global overrides decide to "apply globally" in the patch. The domain overrides could then decide in contrib to apply globally (except when truly no overrides apply in internal config operations) or not.

Issue tags:-security, -D8MI, -language-config

#19: global-overrides-forced-19.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+security, +D8MI, +language-config

The last submitted patch, global-overrides-forced-19.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.51 KB
PASSED: [[SimpleTest]]: [MySQL] 53,240 pass(es).
[ View ]
new622 bytes

It helps if I follow the new interface definition in implementations :)

Also as for why the overrides creeping in to forms and therefore current config is a problem, if you have eg. domain module, depending on which domain you save your admin form, the config saved will be different, if the domain conditional config override gets into the form. That will be confusing when you stage / update site config from development, since your live site will show changes while it just flip-flops between different domain based values. The config diff / staging system as well as overrides were built with the idea that the saved configuration is the canonical value and overrides are not used as "even more canonical".

Anybody still interested in resolving this? It has been raised to critical, a solution has been proposed and then no feedback for over two weeks. If that is as much as we care, then I don't see how this would be critical?

I'm not sure how entirely, but I think #1932336: Use override free configuration context for SystemConfigFormBase is related to this.

Yeah, that would also need to be modified to the new context introduced here. I don't think it makes sense to keep updating the patch if nobody cares though.

Priority:Critical» Normal

Sounds to me like if people care so much as to not review the proposed patch for a month, then this should not be anywhere near critical.

Issue tags:+Configuration system

adding tag.

Hah, heh. Any opinions? :D

Priority:Normal» Critical

Talking to people in Portland, they thought this is still very important, they just did not have time to think about this. So this can just as well be back to critical.

i've tried to think of something useful to say about this issue and patch a couple of times, but never quite managed it.

implementation details aside, i'm ok with values set in settings.php being special, so i think i'm ok with the patch in #24. the context system is already insanely hard to reason about, so i don't think another layer of special casing makes much difference, and seems to solve a real use case or ten.

honestly, i'd still rather we rip out the context system, and bake in special handling for settings.php and locale. that would get us more features than D7 (first class locale support), without making it insane.

Issue tags:+Needs reroll

I can no longer duplicate the behavior outlined in #18. However, the idea of making global overrides stick makes sense to me.

It looks like this patch needs a re-roll now that CoreBundle.php has been replaced by CoreServiceProvider.php in #1988508: Stop pretending we support Symfony bundles when we really just have service providers.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Assigned:Unassigned» Ec1ipsis
Issue tags:-Needs reroll

Assigned:Ec1ipsis» Unassigned
StatusFileSize
new4.25 KB
PASSED: [[SimpleTest]]: [MySQL] 57,913 pass(es).
[ View ]

Per my review with @YesCT, this patch is likely in need of a complete rewrite. Maybe @Cottser can help?

There are three files with issues: core/modules/system/system.admin.inc, core/modules/user/user.admin.inc, and core/lib/Drupal/Core/CoreBundle.php. User.admin.inc in particular had ~1,700 lines deleted from it, and as was mentioned above CoreBundle.php was completely removed.

We attempted to find the issue that deleted CoreBundle.php with the command: git log -S'class CoreBundle extends Bundle'. The intention was to find the commit where that particular line was removed, which would tell us when the file was deleted. The result was:

commit b1c684b8f82e103764422acd95cab332dd7904ff
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Fri Jun 28 17:11:57 2013 +0100
    Issue #1988508 by katbailey: Stop pretending we support Symfony bundles when we really just have service providers.
commit e665805ccee0c28c21d613c87ff0d2c23f9f4b7c
Author: Katherine Bailey <katherine@katbailey.net>
Date:   Wed Jul 11 16:47:37 2012 -0700
    Various coding standards fixes and other minor changes in response to Crell's latest patch review

Unfortunately, this didn't provide the information we were after.

The attached patch is the result of applying #comment-7189198 and removing all offending code. File size of new patch has been reduced roughly 60% due to changes being removed during the process.

Anyone able and willing to take this on?

Anyone able and willing to take this on?

Status:Needs review» Postponed

Moshe: I think we should discuss first and foremost what we want. Not many people cared to comment on my patch :/ Also #2098119: Replace config context system with baked-in locale support and single-event based overrides is about to change the config context/override system drastically so not sure there is a point in working on this one *now* until that is committed.

Status:Postponed» Needs review
StatusFileSize
new0 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

A minor reroll.

StatusFileSize
new4.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,897 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

Letmetrythatagain.

Status:Needs review» Postponed

Putting it back on the backlog per #38.

bump.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
Status:Postponed» Needs work

This no longer applies....

$ git apply drupal-n1934152-39-conf_ftw.patch
error: patch failed: core/lib/Drupal/Core/Config/Config.php:275
error: core/lib/Drupal/Core/Config/Config.php: patch does not apply
error: core/lib/Drupal/Core/Config/Context/ConfigContext.php: No such file or directory
error: core/lib/Drupal/Core/Config/Context/ContextInterface.php: No such file or directory
error: core/lib/Drupal/Core/Config/Context/FreeConfigContext.php: No such file or directory
error: core/lib/Drupal/Core/EventSubscriber/ConfigGlobalOverrideSubscriber.php: No such file or directory

Is this still an issue given that the context stuff is gone and this is always applied last in ConfigFactory?

@Berdir The question that remains is: should ConfigFactory::setOverrideState(FALSE) stop overrides in settings.php from applying? People have raised this as a security issue.

Right. I have not managed to get the security team to weigh on this though :/