[WIP] [Security] Fix #2172 User switching is not available for pre-authenticated users (oldest open bug!) #19059

pasdeloup · 2016-06-15T09:09:39Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	maybe
Deprecations?	no
Tests pass?	no (!)
Fixed tickets	#2172
License	MIT
Doc PR	no

TODO

gather feedback for my changes
fix the test according to feedback

For my first PR, I tried to fix the Symfony's oldest still open bug (2011!)
New tests have been added to reproduce the bug and are now passing.

To be reviewed carefully as it deals with security.

From my understanding, the problem comes from the AbstractPreAuthenticatedListener that is not stateless: whenever a token is passed, it has to preauthenticate again.
Problem, even if another type of token is passed (like a UsernamePasswordToken), it still tries to authenticate it and as it failed it creates a new one with the current user.
This behaviour doesn't work when switching user because the new artificial user token is automatically replaced by the current preauthenticated token.

So the patch bypass this check if (and only if) a token is already there (given by context), it is already correctly authenticated, and it is of another class than PreAuthenticatedToken (to limit risks of BC break).

Problem: this new behaviour breaks (by design) the following test:
Symfony\Component\Security\Http\Tests\Firewall\AbstractPreAuthenticatedListenerTest::testHandleWhenAuthenticationFailsWithDifferentToken

To be discussed: is this new behaviour correct (it may fix other bugs), or is there potential security issues? Should it be considered as a BC break?

…token, and so allow switching users

linaori · 2016-06-15T10:04:03Z

src/Symfony/Component/Security/Http/Firewall/AbstractPreAuthenticatedListener.php

+        $token = $this->tokenStorage->getToken();
+        if(null !== $token && false === ($token instanceof PreAuthenticatedToken) && $token->isAuthenticated()) {
+            if (null !== $this->logger) {
+                $this->logger->debug('An authenticated token is already provided, bypass PreAuthenticatedListener.', array('token' => (string) $this->tokenStorage->getToken()));


I'd personally prefer to see the to string method being called here in case of refactoring one day

You mean in the debug log?
I kept the same syntax than following logs.
But I realized I should use the $token variable in the log, don't need to recall the getter.

ro0NL · 2016-06-15T12:32:27Z

src/Symfony/Bundle/SecurityBundle/Tests/Functional/SwitchUserPreAuthenticatedTest.php

+
+    public static function setUpBeforeClass()
+    {
+        parent::deleteTmpDir('StandardFormLogin');


Why parent:: and not self:: or static::?

That's the way all the tests have already been wrotten, I just kept the same syntax.
In this context, it's the same, and maybe the more obvious to understand.

If it's convention.. i guess it's okay then.

From my personal POV i think it's just weird... why bypass the current class layer?

pasdeloup · 2016-06-15T13:59:13Z

Any feedback about this idea of bypassing the PreAuthenticatedListener check in case there is already an authenticated token provided in the context?

fsevestre · 2016-06-15T14:14:03Z

src/Symfony/Component/Security/Http/Firewall/AbstractPreAuthenticatedListener.php

@@ -54,6 +54,15 @@ public function __construct(TokenStorageInterface $tokenStorage, AuthenticationM
     */
    final public function handle(GetResponseEvent $event)
    {
+        $token = $this->tokenStorage->getToken();
+        if(null !== $token && !$token instanceof PreAuthenticatedToken && $token->isAuthenticated()) {


Missing space if (null !== ... instead of if(null !==

Oups, I missed this one. Thanks.
Done!

nicolas-grekas · 2016-06-15T14:39:54Z

I don't know the Security component enough to have an opinion here.
Yet, I think you should investigate the test failure, it might help understanding any implications (and make tests green :) )

stof · 2016-06-15T15:24:36Z

This PR breaks BC by changing the behavior of pre-authenticated listeners when they find an existing token. So adding this in 2.7 is just a no-go. and regarding your question about the change being correct, I don't think it is. It can introduce other bugs.

Regarding the user-switching, the issue is that pre-authenticated listeners are meant to be used for stateless auth methods where the client sends the credentials again with each request and our user switching system works only for stateful authentication where the client authenticates once and then the auth is kept in the session (allowing to impersonate someone and have this info stored for subsequent requests).

If your project uses a stateless auth method, an impersonation feature would probably have to be implemented in a stateless way too (where the client remembers the fact that it wants to impersonate someone instead of the server remembering it).
This could be done for instance by sending a header X-Impersonate: nicolas-grekas in the request alongside your credentials. The impersonation would then happen when this header is here (and without redirecting after handling the impersonation of course, as we talk about a stateless feature where we impersonate for the current request).
The current system cannot work for stateless auth, and hacking other auth methods to make them behave in a stateful way would actually break them.

so I'm voting 👎 here

nicolas-grekas · 2016-06-15T15:29:46Z

@stof any clue to help @pasdeloup fix the linked issue? Or do you consider it invalid?

pasdeloup · 2016-06-15T16:11:21Z

So it would mean the issue itself should be closed?

It has been opened almost 5 years ago by developers using "non-stateless authentication with x509", so I suppose they mean they use a session so there must be a way to do something and the feature seems interesting.

However, I personnaly don't need it (for now), I just took the oldest issue and tried to understand and fix it, so if you say the issue itself is irrelevant, I won't spend more time on it and will pick up something else.

pasdeloup · 2016-06-22T08:00:56Z

@stof @nicolas-grekas so what do we do? I close the PR and you close the corresponding issue or I try to work on a workaround without BC Break?

fabpot · 2016-06-22T08:02:03Z

@pasdeloup I would add a note in the docs saying that impersonification is not supported for pre-auth and close both this PR and the issue.

pasdeloup · 2016-06-22T08:55:55Z

Ok, I just submitted the PR #6673 for documentation.

nicolas-grekas · 2016-06-22T10:10:29Z

Thanks for your help @pasdeloup. This wasn't merged but it's been definitely helpful. Waitinf for your next PR now :-)

lisachenko · 2017-11-14T08:22:13Z

It's 2017 year and looks like we still need this feature :)

Let me describe our case why it's needed. We have a lot of internal SF2 services that provide API and UI for our internal users (not clients). All users use x509 certificates with eToken devices to securely access our system with given privileges.

But sometime we experience a situation when there is an issue with our service and this issue is reproducing only for one single user. We can't just go and check it locally because we are international company with many offices across the world.

To reproduce this issue we currently have two options: remote connect via terminal like TeamViewer/RAdmin (it can be painfully slow connection speed for example for Nigeria) or we need to request same rights for our developer on production server to reproduce an issue which is not good, because production environment contains sensitive information that should not be available for all of our developers.

This is why I'm bumping this old PR. In my opinion, user switching could be a nice feature for stateless environment too, this can be either cookie (yes, for stateless authentication!) or a GET-parameter in the URL like ?_switch=Some.Username. Of course, this feature should be protected by separate privilege or role or even one-time tokens for developers.

Without user switching we are doing dirty hack: in app.php file we check for presence of special parameter that should contain certificate and override global variables like SSL_CLIENT_I_DN, SSL_CLIENT_S_DN to temporary have impersonation enabled. I don't like that raw ugly solution, so maybe we could bring this PR back to Security component?

Best regards, Alexander

pasdeloup added 2 commits June 14, 2016 18:30

add functionnal tests

d6038f7

fix AbstractPreAuthenticatedListener to allow sharing other types of …

ee8528e

…token, and so allow switching users

carsonbot added Status: Needs Review Security Bug labels Jun 15, 2016

pasdeloup added 2 commits June 15, 2016 11:13

fix coding standards

6fb6a56

fix array syntax to be compatible with PHP 5.3

7b48ebb

linaori reviewed Jun 15, 2016
View reviewed changes

pasdeloup added 2 commits June 15, 2016 12:13

fix coding standards

e61a29f

improve logs: use directly , no need to recall getter

123a1d7

ro0NL reviewed Jun 15, 2016
View reviewed changes

fix code style

1c3cb0b

fsevestre reviewed Jun 15, 2016
View reviewed changes

fix code style

9f56ae0

pasdeloup closed this Jun 22, 2016

Uh oh!

[WIP] [Security] Fix #2172 User switching is not available for pre-authenticated users (oldest open bug!) #19059

[WIP] [Security] Fix #2172 User switching is not available for pre-authenticated users (oldest open bug!) #19059

Uh oh!

Conversation

pasdeloup commented Jun 15, 2016 • edited by nicolas-grekas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linaori Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

pasdeloup Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

ro0NL Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

pasdeloup Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

ro0NL Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

pasdeloup commented Jun 15, 2016

Uh oh!

fsevestre Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

pasdeloup Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jun 15, 2016

Uh oh!

stof commented Jun 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jun 15, 2016

Uh oh!

pasdeloup commented Jun 15, 2016

Uh oh!

pasdeloup commented Jun 22, 2016

Uh oh!

fabpot commented Jun 22, 2016

Uh oh!

pasdeloup commented Jun 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jun 22, 2016

Uh oh!

lisachenko commented Nov 14, 2017

Uh oh!

Uh oh!

pasdeloup commented Jun 15, 2016 •

edited by nicolas-grekas

Loading

stof commented Jun 15, 2016 •

edited

Loading

pasdeloup commented Jun 22, 2016 •

edited

Loading