Framework bundle Kernel: consider the order config files are loaded (security misconfiguration)

[glynnforrest]

As far as I understand it, the security bundle doesn't allow for firewalls to be defined in multiple files - see https://github.com/symfony/symfony/issues/25021, https://github.com/symfony/symfony/issues/22308, https://github.com/symfony/symfony/issues/24461.

When multiple security config files are loaded, an exception should be thrown: You are not allowed to define new elements for path "security.firewalls". Please define all elements for this path in one config file.

However, depending on the order config files are loaded in Kernel::configureContainer(), this process can fail silently and change the security config.

With these config files:

# config/packages/security.yaml
security:
    firewalls:
        main:
            # ...
            remember_me:
                secure: true
            # ...

    access_decision_manager:
        strategy: unanimous
        allow_if_all_abstain: false
    # ...

# config/packages/dev/security.yaml
security:
    firewalls:
        main:
            remember_me:
                secure: false

and this loading order in Kernel::configureContainer() (the recipe default):

protected function configureContainer(ContainerBuilder $container, LoaderInterface $loader)
{
    // ...

    $loader->load($confDir . '/{packages}/*' . self::CONFIG_EXTS, 'glob');
    $loader->load($confDir . '/{packages}/' . $this->environment . '/*' . self::CONFIG_EXTS, 'glob');

    // ...
}

There is no error thrown when the container loads. The config of access_decision_manager is now different between environments:

./bin/console deb:conf security access_decision_manager.strategy
# affirmative

APP_ENV=prod ./bin/console deb:conf security access_decision_manager.strategy
# unanimous

Both environments should be unanimous.

Changing the order of the config file loading does show an error however:

$loader->load($confDir . '/{packages}/' . $this->environment . '/*' . self::CONFIG_EXTS, 'glob');
$loader->load($confDir . '/{packages}/*' . self::CONFIG_EXTS, 'glob');

Is it possible to change the order of these lines in the recipe, or would it break other things?

[stof]

you can reconfigure existing firewalls in new files, but you cannot add new firewalls. That's because the order of firewalls is relevant (the first matching firewall is used) and merging files does not allow to control the order for new keys. the rule is not that the whole config must be in one file.

[stof]

also, access_decision_manager is not part of the security.firewalls key at all, so it is not even impacted at all. Normal merging rules apply.

[glynnforrest]

you can reconfigure existing firewalls in new files, but you cannot add new firewalls. That's because the order of firewalls is relevant (the first matching firewall is used) and merging files does not allow to control the order for new keys. the rule is not that the whole config must be in one file.

Thanks @stof, understood. That makes sense.

also, access_decision_manager is not part of the security.firewalls key at all, so it is not even impacted at all. Normal merging rules apply.

This is what I'm opening the issue about - the access_decision_manager.strategy key changed when config/packages/dev/security.yaml was loaded, even though it didn't have access_decision_manager mentioned anywhere. I thought it was to do with the way the security bundle processed its configuration - in which case, I hoped it would throw an error instead of loading this other file erroneously.

• If I load config/packages/dev/security.yaml, then config/packages/security.yaml, I see an error. Great, that's expected behaviour.
• If I load config/packages/security.yaml, then config/packages/dev/security.yaml, I don't see an error, and access_decision_manager.strategy changes. This is unexpected.

I can create a proof of concept repository, if that would help?

[glynnforrest]

Here's a repository that demonstrates the problem: https://github.com/glynnforrest/symfony-recipes-issue-829

[stof]

OK, found it. there is actually a bug in the config processing of SecurityBundle: https://github.com/symfony/symfony/blob/3c0cfbcb8cf322666d13bbde4e7cd469c360e6c1/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php#L50-L65 is adding a default strategy in each loaded config due to using beforeNormalization. the current implementation works fine without merging, but breaks when merging multiple files.

[nicolas-grekas]

@stof could you please have a look at a fix for this issue you described here?

[nicolas-grekas]

/cc @symfony/mergers in case this inspires any of you?

[yceruto]

This issue was probably fixed in https://github.com/symfony/symfony/pull/43901, @glynnforrest could you confirm? thanks