[SecurityBundle] Make security schema deterministic

[MatTheCat]

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix #57463
License	MIT

Currently you cannot put a namespace on custom providers/authenticators because the XmlFileLoader will ignore every element whose namespace is not its parent’s. This means the only way to configure e.g. SymfonyConnect’s provider is

<provider name="connect_memory">
    <connect_memory>
        <user username="whatever">ROLE_ADMIN</user>
    </connect_memory>
</provider>

This requires providers and authenticators xsd:any’s namespace to be ##any, which is no longer possible with libxml ≥ 2.12 (already available on Alpine 3.20 and Arch Linux e.g.) because it will report their content model as non-determinist.

Unable to parse file "/srv/backend/config/packages/security.xml": [ERROR 3070] complex type 'provider': The content model is not determinist. (in file:////srv/backend/vendor/symfony/security-bundle/DependencyInjection/../Resources/config/schema//security-1.0.xsd - line 88, column 0)
[ERROR 3070] complex type 'firewall': The content model is not determinist. (in file:////srv/backend/vendor/symfony/security-bundle/DependencyInjection/../Resources/config/schema//security-1.0.xsd - line 134, column 0) in /srv/backend/config/packages/security.xml (which is being imported from "/srv/backend/src/Kernel.php").

As a result, any XML security config will fail to be loaded once libxml is updated.

Fixing this issue requires to change xsd:anys’ namespace to ##other so that content models become deterministic.
A side-effect is that any custom providers/authenticators XML configuration will become invalid.

Unable to parse file "/srv/backend/config/packages/security.xml": [ERROR 1871] Element '{http://symfony.com/schema/dic/security}connect_memory': This element is not expected. Expected is one of ( {http://symfony.com/schema/dic/security}chain, {http://symfony.com/schema/dic/security}memory, {http://symfony.com/schema/dic/security}ldap, ##other{http://symfony.com/schema/dic/security}* ). (in /srv/backend/public/ - line 13, column 0) in /srv/backend/config/packages/security.xml (which is being imported from "/srv/backend/src/Kernel.php").

To avoid a BC break this PR allows such errors to occur. A deprecation will have to be triggered on 7.2 for users to namespace their custom providers/authenticators elements, e.g.

<provider name="symfony_connect">
    <connect_memory xmlns="whatever">
        <user username="whatever">ROLE_ADMIN</custom:user>
    </connect_memory>
</provider>

Because custom providers/authenticators’ processContents is lax there won’t be any validation if the namespace has no schema. If it has, it will be better to provide it:

<srv:container xmlns="http://symfony.com/schema/dic/security"
               xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
               xmlns:srv="http://symfony.com/schema/dic/services"
+              xmlns:custom="whatever"
               xsi:schemaLocation="http://symfony.com/schema/dic/services
        https://symfony.com/schema/dic/services/services-1.0.xsd
        http://symfony.com/schema/dic/security
        https://symfony.com/schema/dic/security/security-1.0.xsd
+       whatever the/schema.xsd">

    <config>
        <provider name="symfony_connect">
-           <connect_memory xmlns="whatever">
-               <user username="whatever">ROLE_ADMIN</custom:user>
-           </connect_memory>
+           <custom:connect_memory>
+               <custom:user username="whatever">ROLE_ADMIN</custom:user>
+           </custom:connect_memory>
        </provider>
</srv:container>

[wouterj]

Uh-oh, this sounds like a really tricky situation. Breaking changes on an LTS version is a no-go, but we have to fix this somehow.

While searching for xsd:any in the code-base, as I'm sure more places are affected, I came across the Xliff 1.2 schema. It uses <xsd:any maxOccurs="unbounded" minOccurs="0" namespace="##other" processContents="skip"/> (processContent=skip is the relevant bit). Can you maybe verify if this could fix the issue in a BC way?

[MatTheCat]

I clarified the PR description: BC break would impact users configuring custom user providers and/or authenticators using XML, while the issue will impact users configuring security using XML (meaning e.g. XmlCompleteConfigurationTest will always fail once it is tested against an up-to-date libxml).

processContents="skip" is the same than lax when you don’t have an XSD so that wouldn’t change anything (confirmed after testing).

For completeness, I thought about two others solutions which would keep BC (in a sub-optimal way):

• remove all provider and authenticator types from the XSD. You wouldn’t get any validation or code autocompletion anymore but this would allow to keep namespace="##any" while being deterministic.
• disable validation in the XmlFileLoader. No validation => no validation error… Config would still be validated though.

[nicolas-grekas]

If there is no other choice than this BC break, then at least it happens at compile time so that it's going to be easy to spot, isn't it?
Then, we should make the solution super obvious, with clear explanations, before/after example, anything that can help make the situation actionable. Can you think of ways to do that?

[MatTheCat]

I clarified that the two other solutions I thought of (removing types from the XSD or disabling XML validation) would keep BC, so maybe they should be preferred?

If not, the simplest fix is to add a namespace to custom providers/authenticators, like

<provider name="default">
-    <my-provider />
+    <custom:my-provider xmlns:custom="my.custom.namespace" />
</provider>

Thanks to processContents="lax" you don’t need an XSD, but thanks to this PR you could now reference one:

+ xmlns:custom="my.custom.namespace"
  xsi:schemaLocation="http://symfony.com/schema/dic/services
     https://symfony.com/schema/dic/services/services-1.0.xsd
     http://symfony.com/schema/dic/security
-    https://symfony.com/schema/dic/security/security-1.0.xsd">
+    https://symfony.com/schema/dic/security/security-1.0.xsd
+    my.custom.namespace
+    /path/to/my.xsd

    <config>
        <provider name="default">
-          <my-provider />
+          <custom:my-provider  />
        </provider>
    </config>
</srv:container>

Don’t really know how to advertise this; I guess the upgrade guide and a blog post would be a good start. Surely there have been similar cases in the past?

[stof]

Given that any bundle can register extra settings for the security-bundle configuration (it has an extension config) and we were not requiring a custom namespace until now (and they probably never documented using one), I don't think we can require one (especially not in a patch version of an old LTS)

[wouterj]

I'm leaning towards going for "remove all provider and authenticator types from the XSD". This means we'll loose validation and autocompletion of build-in user provider configuration, but it is not a BC break.

Then, maybe we can find a nice upgrade path to reintroduce this in Symfony 8 (e.g. first deprecating custom user providers/authenticators without namespace?).

[stof]

note that we don't loose all validation. We loose the XSD-based validation, but we still process the config with the Configuration class.

[MatTheCat]

In fact if we’re going to disable validation, we can make our XSD valid as well. That way there is no BC break and users are free to make their config valid or not. Updated the PR this way.

It would be even better to check for validation errors related to custom providers/authenticators (and trigger a deprecation?) but I’m not sure it is possible yet.

[stof]

I don't think disabling XML validation in the XmlFileLoader is a good idea (this won't be only for the SecurityBundle configuration but for any configuration). And I don't think the answer about IDEs is right: they would report any configuration for third-party providers as invalid, which will confuse developers.

This would also make it even more likely to have outdated XSD files for our bundle configuration schemas as they would not be validated anymore (which would also lead to bad IDE support for them if they are based on XSD files).

[MatTheCat]

[IDEs] would report any configuration for third-party providers as invalid, which will confuse developers.

Only as long as they stay invalid. Once we offer the possibility to namespace them, developers should update them so that they aren’t invalid anymore. But this means we must support these invalid.

If we can detect these cases when validating, we could allow them and trigger a deprecation or something to tell the user to update its config (for now I managed to detect invalid providers from the exception message, but not authenticators yet).

Would that be a viable solution?

[stof]

@MatTheCat you disabled validation for all XML configuration affecting all bundles of the ecosystem.

[MatTheCat]

@stof yes this is the PR actual state while I’m investigating how to implement the idea I presented.

[MatTheCat]

Okay I re-enabled Symfony’s XML validation while allowing invalid providers/authenticators; these will only trigger a deprecation.

Not sure this is the best way and it’s missing tests, but do you agree with this solution?

[MatTheCat]

Ubuntu will join impacted systems on October 10 when 24.10 is released.

[nicolas-grekas]

LGTM, let's add some tests?

[MatTheCat]

Will add some

[MatTheCat]

Hm not sure what to do because there are changes both in symfony/dependency-injection and symfony/security-bundle 🤔

Should I make symfony/security-bundle a dev dependency of symfony/dependency-injection, or is there a way to restrict the version of symfony/dependency-injection when testing symfony/security-bundle?

[OskarStark]

Just for testing or in general? In this case a conflict rule could help, or do I miss sth?

[MatTheCat]

This PR’s SecurityBundle changes require the DependencyInjection’s (but not the other way around), so I would need a way to force this dependency to be up-to-date, even though those changes are not part of any version yet (I think high and low-deps failures are related) 🤔

[stof]

Bumping the min version of the symfony/dependency-injection requirement in security-bundle is valid as a way to fix the lowest-deps job (this is usually the way to go). The highest-deps job will probably be solved once the change in DI is merged up in 6.4.

[MatTheCat]

Bumping the min version of the symfony/dependency-injection requirement in security-bundle is valid as a way to fix the lowest-deps job (this is usually the way to go).

But here the required changes are not part of any release yet; how can I bump the min version to “this PR”?

[xabbuh]

use the next patch release version (i.e. ^5.4.43|^6.4.11)

[fabpot]

Minor CS issue.

[fabpot]

Thank you @MatTheCat.