[Security] fixed issue where x509 authentication clears unrelated tokens #8283

alcaeus · 2013-06-14T18:19:34Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#8226
License	MIT
Doc PR	symfony/symfony-docs#2741

TODO:

Feedback on change to make sure security is not affected
Fix other authentication listeners (they suffer the same problem)
Write unit tests for bug and maybe a few listener classes as well

This pull request is the summary of the problem mentioned in the ticket above.
It only fixes the "disappearing token" problem for one authentication provider, not all. If acceptable, the change needs to be applied to all authentication listeners since they always clear all tokens from the security context.

Any help or feedback is greatly appreciated.

fabpot · 2013-06-17T07:17:01Z

I think it makes sense.

alcaeus · 2013-06-19T17:24:16Z

I updated all other authentication listeners that clear security tokens. I also hardened the check to also check for the provider key.
There is one minor issue that could arise: the AbstractAuthenticationListener class is responsible for handling authentication failure, including clearing the token. Since it could be used by any custom authentication listener which could set a token other than UsernamePasswordToken I'm not sure it makes a lot of sense to only clear those tokens. Any ideas on that?
Other than that I'm done with the changes, maybe one or two people could also just test it in their environments to make sure it works.

stof · 2013-06-19T18:07:16Z

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

@@ -181,7 +181,10 @@ private function onFailure(GetResponseEvent $event, Request $request, Authentica
            $this->logger->info(sprintf('Authentication request failed: %s', $failed->getMessage()));
        }

-        $this->securityContext->setToken(null);
+        $token = $this->securityContext->getToken();
+        if (null !== $token && $token instanceof UsernamePasswordToken && $this->providerKey == $token->getProviderKey()) {


you should use a strict comparison

and you can remove null !== $token. null is not an instance of UsernamePasswordToken

alcaeus · 2013-06-19T18:17:19Z

Done. Thank you!

… tokens This commit fixes an issue where authentication listeners clear all security tokens in case of authentication failure. This behavior makes it impossible to combine certain authentication mechanisms, notably x509 with form-based login.

alcaeus · 2013-06-22T13:06:12Z

From my end this one is ready, I've also tried to test various combinations of authentication listeners which produced the desired results.
I've also squashed the commits into one.

fabpot · 2013-07-02T13:03:16Z

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

@@ -38,10 +38,6 @@ public function __construct(SecurityContextInterface $securityContext, Authentic

    protected function getPreAuthenticatedData(Request $request)
    {
-        if (!$request->server->has($this->userKey)) {


Why have you removed this code?

@alcaeus Can you tell us more about this code removal? It does not look like it is related to the goal of this PR.

Hi! Sorry about the late reply, I've been struck down with the flu for the past week and wasn't able to do anything.
To be honest, that was the first piece of code I removed when trying to fix it. The exception is not caught in the abstract listener, which caused the original problem of cleared tokens.
On second thought, it probably should be left in so that code can react to the different cases ("no certificate provided" and "no data in certificate"). Can you give me another couple of days to check it out? I'll put the code back in and make sure the abstract listener reacts correctly to the exception.

alcaeus · 2013-07-10T15:59:39Z

The BadCredentialsException is back in (it really makes more sense this way), the AbstractPreAuthenticatedListener now takes care of handling that case. The exception handling is necessary to allow the authentication process to fall through to the next listener.

fabpot · 2013-07-10T16:20:38Z

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

+    /**
+     * Clears a PreAuthenticatedToken for this provider (if present)
+     *
+     * @return void


this should be removed as we don't use mention the return value when it is null

alcaeus · 2013-07-19T05:55:16Z

Fabien, is this one good to merge or is there anything else that should be changed?

fabpot · 2013-07-19T09:31:43Z

@alcaeus Looks good to me except that I did not see that you based your patch on 2.1, which is not maintained anymore. Can you reopen a new PR on 2.2 instead?

alcaeus · 2013-07-19T09:46:24Z

Dang, yeah. Will do. Do you need one on 2.3 as well?

fabpot · 2013-07-19T10:26:20Z

No, just 2.2.

alcaeus · 2013-07-19T22:24:03Z

Replaced by the above PR.

This PR was merged into the 2.2 branch. Discussion ---------- [Security] fixed issue where x509 authentication clears unrelated tokens | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #8226 | License | MIT | Doc PR | symfony/symfony-docs#2825 | Notes | Replaces PR #8283 TODO: - [x] Feedback on change to make sure security is not affected - [x] Fix other authentication listeners (they suffer the same problem) - [x] Write unit tests for bug and maybe a few listener classes as well This pull request is the summary of the problem mentioned in the ticket above. It only fixes the "disappearing token" problem for one authentication provider, not all. If acceptable, the change needs to be applied to all authentication listeners since they always clear all tokens from the security context. Commits ------- 2317443 [Security] fixed issue where authentication listeners clear unrelated tokens

alcaeus mentioned this pull request Jun 19, 2013

[WCM] Fix example code to reflect changes for #8226 symfony/symfony-docs#2741

Closed

stof reviewed Jun 19, 2013
View reviewed changes

fabpot reviewed Jul 2, 2013
View reviewed changes

fabpot reviewed Jul 10, 2013
View reviewed changes

Re-add BadCredentialsException if no X509 certificate was provided

72a484f

alcaeus closed this Jul 19, 2013

alcaeus deleted the ticket_8226 branch July 19, 2013 22:19

alcaeus mentioned this pull request Jul 19, 2013

[Security] fixed issue where x509 authentication clears unrelated tokens #8528

Merged

3 tasks

Uh oh!

[Security] fixed issue where x509 authentication clears unrelated tokens #8283

[Security] fixed issue where x509 authentication clears unrelated tokens #8283

Uh oh!

Conversation

alcaeus commented Jun 14, 2013

Uh oh!

fabpot commented Jun 17, 2013

Uh oh!

alcaeus commented Jun 19, 2013

Uh oh!

stof Jun 19, 2013

Choose a reason for hiding this comment

Uh oh!

stof Jun 19, 2013

Choose a reason for hiding this comment

Uh oh!

alcaeus commented Jun 19, 2013

Uh oh!

alcaeus commented Jun 22, 2013

Uh oh!

fabpot Jul 2, 2013

Choose a reason for hiding this comment

Uh oh!

fabpot Jul 8, 2013

Choose a reason for hiding this comment

Uh oh!

alcaeus Jul 8, 2013

Choose a reason for hiding this comment

Uh oh!

alcaeus commented Jul 10, 2013

Uh oh!

fabpot Jul 10, 2013

Choose a reason for hiding this comment

Uh oh!

alcaeus commented Jul 19, 2013

Uh oh!

fabpot commented Jul 19, 2013

Uh oh!

alcaeus commented Jul 19, 2013

Uh oh!

fabpot commented Jul 19, 2013

Uh oh!

alcaeus commented Jul 19, 2013

Uh oh!

Uh oh!