[Security] add "lazy_authentication" mode to firewalls #27817

nicolas-grekas · 2018-07-03T08:38:37Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#26769 et al.
License	MIT
Doc PR	-

Wouhou, here is my first significant PR on the Security component :)

This PR is aimed at allowing unauthenticated access to resources that don't read the user token. It applies to stateful firewalls only. Right now, when a stateful firewall is configured, a user token is always hydrated from the session. This has the effect of making the response uncacheable.

When access control defines no specific roles for a resource and when no further is_granted() checks are made when computing it, one doesn't really need statefulness.

This PR allows differentiating the "anonymous" mode from the "unauthenticated" one: "anonymous" is stateful and allows e.g. tracking the journey of a user on a website even if we don't know who this is. "unauthenticated" on the other end is anonymous+stateless - i.e. we don't track navigation.

When "lazy_authentication" is enabled, all security listeners are called lazily only when the user token is actually read from the token storage.

A visible side effect is that ESI fragments can be more easily cached (see linked issue). Another one is that the debug toolbar will report as "unauthenticated" when browsing such a page. And a last one is that once #27806 is merged, TokenProcessor won't add the username on the logs when a resource didn't start the authentication stack.

The next step should be to enable this rule by default in https://github.com/symfony/recipes/blob/master/symfony/security-bundle/3.3/config/packages/security.yaml:

security:
    firewalls:
        main:
            anonymous: true
            lazy_authentication: true

TODO:

add tests
doc PR

WDYT?

xavismeh · 2018-07-03T08:55:24Z

Really interesting feature! Still, the web debug toolbar should force the display of authenticated status even when not necessary on the page since it could be very confusing for DX. But I have no relevant suggestion to make on how to deal with this...

Taluu · 2018-07-03T09:37:24Z

src/Symfony/Bundle/SecurityBundle/Resources/config/collectors.xml

@@ -9,7 +9,7 @@

        <service id="data_collector.security" class="Symfony\Bundle\SecurityBundle\DataCollector\SecurityDataCollector">
            <tag name="data_collector" template="@Security/Collector/security.html.twig" id="security" priority="270" />
-            <argument type="service" id="security.token_storage" on-invalid="ignore" />
+            <argument type="service" id="security.actual_token_storage" />


this name feels weird (I don't have a better suggestion though)

Taluu · 2018-07-03T09:38:50Z

src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml

@@ -21,11 +21,14 @@
        </service>
        <service id="Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface" alias="security.authorization_checker" />

-        <service id="security.token_storage" class="Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage" public="true">
+        <service id="security.token_storage" class="Symfony\Component\Security\Core\Authentication\Token\Storage\LazyTokenStorage" public="true">


how about implementing a decoration for the actual_token_storage here, instead ? So that you can rename it token_storage and let this service decorate the token_storage ?

linaori · 2018-07-03T11:04:42Z

A few questions:

Does this mean that if my minimal authentication level is IS_AUTHENTICATED_ANONYMOUSLY, this option is useless? This will be quite some work to configure properly in an application, because you can't wild-card anymore with exceptions in access control (it is always triggered).
Is this actually useful if you have even 1 isGranted anywhere? Let's say I have a small part where I always check if you're allowed to login or not, it would have to be rewritten to always check if there's a token first. But if I get the token, it will initialize, correct? Meaning this would make ESI impossible if you were to only check if you're allowed to login.

nicolas-grekas · 2018-07-03T12:14:50Z

the web debug toolbar should force the display of authenticated status

that's impossible, by the very definition of what this feature achieves.

implementing a decoration for the actual_token_storage here

we need to access both the lazy and the actual storage services independently, so decoration cannot work here.

Does this mean that if my minimal authentication level is IS_AUTHENTICATED_ANONYMOUSLY, this option is useless?

yes, because this asks for an anonymous token.

Is this actually useful if you have even 1 isGranted anywhere?

as soon as you have one is_granted, the state is created, that's the purpose yes.

EmmanuelVella · 2018-07-03T12:31:36Z

@nicolas-grekas 👍 won't the esi cache issue be fixed in the 3.4 branch ?

nicolas-grekas · 2018-07-03T12:51:10Z

There is no issue in the 3.4 branch finally: enabling the anonymous firewall asks for the session. You'd need to fix your config here.

weaverryan · 2018-07-03T14:05:39Z

iltar: Does this mean that if my minimal authentication level is IS_AUTHENTICATED_ANONYMOUSLY, this option is useless?

nicolasgrekas yes, because this asks for an anonymous token.

I think this is a problem, because IS_AUTHENTICATED_ANONYMOUSLY can be used under access_control to basically mean "this URL requires NO security". But, we could fix this by adding some new way of doing this - e.g. roles: false under the access_control.

xavismeh: the web debug toolbar should force the display of authenticated status

nicolasgrekas: that's impossible, by the very definition of what this feature achieves.

This feature makes so much sense, that it should probably be on by default (and would, "effectively" be on by default thanks to the recipe update). But... this part IS a DX problem. Imagine if you're working on your authentication system, and it appears that it's not working, but really, it is.

If ContextListener never loaded the token, could we store the serialized token in the profiler data, and unserialize it there? This "laziness" should really be an invisible feature... and other than the profiler, I think it would be (e.g. because as soon as you try to "use" security, it would initialize itself).

linaori · 2018-07-03T14:18:44Z

@weaverryan it feels hacky though, but I think the main problem is the separation of the access_control rules and the firewall config. If that would be merged, it might solve problems (more than just this one). I'm currently just not able to figure out a viable alternative for this.

nicolas-grekas · 2018-07-03T19:20:04Z

IS_AUTHENTICATED_ANONYMOUSLY basically mean "this URL requires NO security"

as you know that's no totally accurate, hence this PR :)

e.g. roles: false

roles: [] - or just no "roles" entry at all.

could we store the serialized token in the profiler data, and unserialize it there?

nope we can't: the session is the storage and we don't have direct access to the backend.

I don't think showing the truth is against DX. Actually, trying to hide the truth is the not-DX thing to me.
So here, we could display something that tells what the situation is: security is enabled but has not been used so we don't know who is here, anonymous or not.

stof · 2018-07-11T07:34:12Z

as you know that's no totally accurate, hence this PR :)

but currently, there is no way to have an access_control rule requiring no security at all. So we indeed need to add a new feature.

stof · 2018-07-11T07:34:58Z

So here, we could display something that tells what the situation is: security is enabled but has not been used so we don't know who is here, anonymous or not.

if the profiler also displays whether authentication was attempted or no, it could indeed help.

nicolas-grekas · 2018-07-30T17:23:59Z

I found a better way, see #28089.

nicolas-grekas · 2018-08-31T14:54:32Z

Reopening as there might be a reason to prefer this over #28089: here, the session lock is not created unless needed. But I would remove the lazy_authentication config entry and replace it with anonymous: lazy instead. See #28325

nicolas-grekas · 2019-09-23T19:46:13Z

Continued in #33676

…colas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [Security] add "anonymous: lazy" mode to firewalls | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fixes #26769 et al. | License | MIT | Doc PR | - Contains #33663 until it is merged. This PR allows defining a firewall as such: ```yaml security: firewalls: main: anonymous: lazy ``` This means that the corresponding area should not start the session / load the user unless the application actively gets access to it. On pages that don't fetch the user at all, this means the session is not started, which means the corresponding token neither is. Lazily, when the user is accessed, e.g. via a call to `is_granted()`, the user is loaded, starting the session if needed. See #27817 for previous explanations on the topic also. Note that thanks to the logic in #33633, this PR doesn't have the drawback spotted in #27817: here, the profiler works as expected. Recipe update pending at symfony/recipes#649 Commits ------- 5cd1d7b [Security] add "anonymous: lazy" mode to firewalls

nicolas-grekas added this to the next milestone Jul 3, 2018

carsonbot added Status: Needs Review Security labels Jul 3, 2018

Taluu reviewed Jul 3, 2018

View reviewed changes

EmmanuelVella mentioned this pull request Jul 3, 2018

Esi cache broken ? (started session / private response) #26769

Closed

nicolas-grekas mentioned this pull request Jul 3, 2018

[DI] Allow autoconfiguring bindings #27806

Merged

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Jul 5, 2018

nicolas-grekas force-pushed the sec-lazy branch from ff128ce to 9f032d7 Compare July 26, 2018 09:20

[Security] add "lazy_authentication" mode to firewalls

cad381f

nicolas-grekas force-pushed the sec-lazy branch from 9f032d7 to cad381f Compare July 26, 2018 11:47

nicolas-grekas mentioned this pull request Jul 30, 2018

[Security] Make statefull pages NOT turn responses private when the token is not used #28089

Closed

nicolas-grekas closed this Jul 30, 2018

nicolas-grekas deleted the sec-lazy branch July 30, 2018 17:24

nicolas-grekas mentioned this pull request Aug 31, 2018

Close session after fetching token in ContextListener #28325

Closed

nicolas-grekas restored the sec-lazy branch August 31, 2018 14:53

nicolas-grekas reopened this Aug 31, 2018

nicolas-grekas changed the base branch from master to 4.4 June 2, 2019 20:04

nicolas-grekas mentioned this pull request Sep 23, 2019

[Security] add "anonymous: lazy" mode to firewalls #33676

Merged

nicolas-grekas closed this Sep 23, 2019

nicolas-grekas deleted the sec-lazy branch September 23, 2019 19:46

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] add "lazy_authentication" mode to firewalls #27817

[Security] add "lazy_authentication" mode to firewalls #27817

nicolas-grekas commented Jul 3, 2018 •

edited

Loading

xavismeh commented Jul 3, 2018

Taluu Jul 3, 2018

Taluu Jul 3, 2018

linaori commented Jul 3, 2018

nicolas-grekas commented Jul 3, 2018

EmmanuelVella commented Jul 3, 2018

nicolas-grekas commented Jul 3, 2018

weaverryan commented Jul 3, 2018

linaori commented Jul 3, 2018

nicolas-grekas commented Jul 3, 2018 •

edited

Loading

stof commented Jul 11, 2018

stof commented Jul 11, 2018

nicolas-grekas commented Jul 30, 2018

nicolas-grekas commented Aug 31, 2018 •

edited

Loading

nicolas-grekas commented Sep 23, 2019

[Security] add "lazy_authentication" mode to firewalls #27817

[Security] add "lazy_authentication" mode to firewalls #27817

Conversation

nicolas-grekas commented Jul 3, 2018 • edited Loading

xavismeh commented Jul 3, 2018

Taluu Jul 3, 2018

Choose a reason for hiding this comment

Taluu Jul 3, 2018

Choose a reason for hiding this comment

linaori commented Jul 3, 2018

nicolas-grekas commented Jul 3, 2018

EmmanuelVella commented Jul 3, 2018

nicolas-grekas commented Jul 3, 2018

weaverryan commented Jul 3, 2018

linaori commented Jul 3, 2018

nicolas-grekas commented Jul 3, 2018 • edited Loading

stof commented Jul 11, 2018

stof commented Jul 11, 2018

nicolas-grekas commented Jul 30, 2018

nicolas-grekas commented Aug 31, 2018 • edited Loading

nicolas-grekas commented Sep 23, 2019

nicolas-grekas commented Jul 3, 2018 •

edited

Loading

nicolas-grekas commented Jul 3, 2018 •

edited

Loading

nicolas-grekas commented Aug 31, 2018 •

edited

Loading