feature#6554 [Security] Added Security\Csrf sub-component with better token generation (bschussek)

fabpot · fabpot · commit ab70528b604a · 2013-09-30T17:35:08.000+02:00
This PR was merged into the master branch.

Discussion
----------

[Security] Added Security\Csrf sub-component with better token generation

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | TODO

**Update September 27, 2013**

This PR simplifies the CSRF mechanism to generate completely random tokens. A random token is generated once per ~~intention~~ token ID and then stored in the session. Tokens are valid until the session expires.

Since the CSRF token generator depends on `StringUtils` and `SecureRandom` from Security\Core, and since Security\Http currently depends on the Form component for token generation, I decided to add a new Security\Csrf sub-component that contains the improved CSRF token generator. Consequences:

* Security\Http now depends on Security\Csrf instead of Form
* Form now optionally depends on Security\Csrf
* The configuration for the "security.secure_random" service and the "security.csrf.*" services was moved to FrameworkBundle to guarantee BC

In the new Security\Csrf sub-component, I tried to improve the naming where I could do so without breaking BC:

* CSRF "providers" are now called "token generators"
* CSRF "intentions" are now called "token IDs", because that's really what they are

##### TODO

- [ ] The documentation needs to be checked for references to the configuration of the application secret. Remarks that the secret is used for CSRF protection need to be removed.
- [ ] Add aliases "csrf_token_generator" and "csrf_token_id" for "csrf_provider" and "intention" in the SecurityBundle configuration
- [x] Make sure `SecureRandom` never blocks for `CsrfTokenGenerator`

Commits
-------

7f02304 [Security] Added missing PHPDoc tag
2e04e32 Updated Composer dependencies to require the Security\Csrf component where necessary
bf85e83 [FrameworkBundle][SecurityBundle] Added service configuration for the new Security CSRF sub-component
2048cf6 [Form] Deprecated the CSRF implementation and added an optional dependency to the Security CSRF sub-component instead
85d4959 [Security] Changed Security HTTP sub-component to depend on CSRF sub-component instead of Form
1bf1640 [Security] Added CSRF sub-component
diff --git a/Firewall/LogoutListener.php b/Firewall/LogoutListener.php
@@ -11,12 +11,12 @@
 
 namespace Symfony\Component\Security\Http\Firewall;
 
-use Symfony\Component\Form\Extension\Csrf\CsrfProvider\CsrfProviderInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\Event\GetResponseEvent;
 use Symfony\Component\Security\Core\SecurityContextInterface;
 use Symfony\Component\Security\Core\Exception\LogoutException;
+use Symfony\Component\Security\Csrf\CsrfTokenGeneratorInterface;
 use Symfony\Component\Security\Http\HttpUtils;
 use Symfony\Component\Security\Http\Logout\LogoutHandlerInterface;
 use Symfony\Component\Security\Http\Logout\LogoutSuccessHandlerInterface;
@@ -34,18 +34,18 @@ class LogoutListener implements ListenerInterface
     private $handlers;
     private $successHandler;
     private $httpUtils;
-    private $csrfProvider;
+    private $csrfTokenGenerator;
 
     /**
      * Constructor.
      *
      * @param SecurityContextInterface      $securityContext
-     * @param HttpUtils                     $httpUtils       An HttpUtilsInterface instance
-     * @param LogoutSuccessHandlerInterface $successHandler  A LogoutSuccessHandlerInterface instance
-     * @param array                         $options         An array of options to process a logout attempt
-     * @param CsrfProviderInterface         $csrfProvider    A CsrfProviderInterface instance
+     * @param HttpUtils                     $httpUtils          An HttpUtilsInterface instance
+     * @param LogoutSuccessHandlerInterface $successHandler     A LogoutSuccessHandlerInterface instance
+     * @param array                         $options            An array of options to process a logout attempt
+     * @param CsrfTokenGeneratorInterface   $csrfTokenGenerator A CsrfTokenGeneratorInterface instance
      */
-    public function __construct(SecurityContextInterface $securityContext, HttpUtils $httpUtils, LogoutSuccessHandlerInterface $successHandler, array $options = array(), CsrfProviderInterface $csrfProvider = null)
+    public function __construct(SecurityContextInterface $securityContext, HttpUtils $httpUtils, LogoutSuccessHandlerInterface $successHandler, array $options = array(), CsrfTokenGeneratorInterface $csrfTokenGenerator = null)
     {
         $this->securityContext = $securityContext;
         $this->httpUtils = $httpUtils;
@@ -55,7 +55,7 @@ public function __construct(SecurityContextInterface $securityContext, HttpUtils
             'logout_path'    => '/logout',
         ), $options);
         $this->successHandler = $successHandler;
-        $this->csrfProvider = $csrfProvider;
+        $this->csrfTokenGenerator = $csrfTokenGenerator;
         $this->handlers = array();
     }
 
@@ -72,7 +72,7 @@ public function addHandler(LogoutHandlerInterface $handler)
     /**
      * Performs the logout if requested
      *
-     * If a CsrfProviderInterface instance is available, it will be used to
+     * If a CsrfTokenGeneratorInterface instance is available, it will be used to
      * validate the request.
      *
      * @param GetResponseEvent $event A GetResponseEvent instance
@@ -88,10 +88,10 @@ public function handle(GetResponseEvent $event)
             return;
         }
 
-        if (null !== $this->csrfProvider) {
+        if (null !== $this->csrfTokenGenerator) {
             $csrfToken = $request->get($this->options['csrf_parameter'], null, true);
 
-            if (false === $this->csrfProvider->isCsrfTokenValid($this->options['intention'], $csrfToken)) {
+            if (false === $this->csrfTokenGenerator->isCsrfTokenValid($this->options['intention'], $csrfToken)) {
                 throw new LogoutException('Invalid CSRF token.');
             }
         }
diff --git a/Firewall/SimpleFormAuthenticationListener.php b/Firewall/SimpleFormAuthenticationListener.php
@@ -13,10 +13,11 @@
 
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\Security\Core\Exception\InvalidCsrfTokenException;
+use Symfony\Component\Security\Csrf\CsrfTokenGeneratorInterface;
 use Symfony\Component\Security\Http\Authentication\AuthenticationFailureHandlerInterface;
 use Symfony\Component\Security\Http\Authentication\AuthenticationSuccessHandlerInterface;
 use Symfony\Component\Security\Core\Authentication\AuthenticationManagerInterface;
-use Symfony\Component\Form\Extension\Csrf\CsrfProvider\CsrfProviderInterface;
 use Symfony\Component\Security\Core\Authentication\SimpleFormAuthenticatorInterface;
 use Symfony\Component\Security\Core\SecurityContextInterface;
 use Symfony\Component\Security\Http\HttpUtils;
@@ -29,7 +30,7 @@
 class SimpleFormAuthenticationListener extends AbstractAuthenticationListener
 {
     private $simpleAuthenticator;
-    private $csrfProvider;
+    private $csrfTokenGenerator;
 
     /**
      * Constructor.
@@ -46,16 +47,16 @@ class SimpleFormAuthenticationListener extends AbstractAuthenticationListener
      * @param LoggerInterface                        $logger                A LoggerInterface instance
      * @param EventDispatcherInterface               $dispatcher            An EventDispatcherInterface instance
      * @param SimpleFormAuthenticatorInterface       $simpleAuthenticator   A SimpleFormAuthenticatorInterface instance
-     * @param CsrfProviderInterface                  $csrfProvider          A CsrfProviderInterface instance
+     * @param CsrfTokenGeneratorInterface            $csrfTokenGenerator    A CsrfTokenGeneratorInterface instance
      */
-    public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler, AuthenticationFailureHandlerInterface $failureHandler, array $options = array(), LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfProviderInterface $csrfProvider = null, SimpleFormAuthenticatorInterface $simpleAuthenticator = null)
+    public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler, AuthenticationFailureHandlerInterface $failureHandler, array $options = array(), LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfTokenGeneratorInterface $csrfTokenGenerator = null, SimpleFormAuthenticatorInterface $simpleAuthenticator = null)
     {
         if (!$simpleAuthenticator) {
             throw new \InvalidArgumentException('Missing simple authenticator');
         }
 
         $this->simpleAuthenticator = $simpleAuthenticator;
-        $this->csrfProvider = $csrfProvider;
+        $this->csrfTokenGenerator = $csrfTokenGenerator;
 
         $options = array_merge(array(
             'username_parameter' => '_username',
@@ -84,10 +85,10 @@ protected function requiresAuthentication(Request $request)
      */
     protected function attemptAuthentication(Request $request)
     {
-        if (null !== $this->csrfProvider) {
+        if (null !== $this->csrfTokenGenerator) {
             $csrfToken = $request->get($this->options['csrf_parameter'], null, true);
 
-            if (false === $this->csrfProvider->isCsrfTokenValid($this->options['intention'], $csrfToken)) {
+            if (false === $this->csrfTokenGenerator->isCsrfTokenValid($this->options['intention'], $csrfToken)) {
                 throw new InvalidCsrfTokenException('Invalid CSRF token.');
             }
         }
diff --git a/Firewall/UsernamePasswordFormAuthenticationListener.php b/Firewall/UsernamePasswordFormAuthenticationListener.php
@@ -11,9 +11,9 @@
 
 namespace Symfony\Component\Security\Http\Firewall;
 
-use Symfony\Component\Form\Extension\Csrf\CsrfProvider\CsrfProviderInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Psr\Log\LoggerInterface;
+use Symfony\Component\Security\Csrf\CsrfTokenGeneratorInterface;
 use Symfony\Component\Security\Http\Authentication\AuthenticationFailureHandlerInterface;
 use Symfony\Component\Security\Http\Authentication\AuthenticationSuccessHandlerInterface;
 use Symfony\Component\Security\Http\Session\SessionAuthenticationStrategyInterface;
@@ -32,12 +32,12 @@
  */
 class UsernamePasswordFormAuthenticationListener extends AbstractAuthenticationListener
 {
-    private $csrfProvider;
+    private $csrfTokenGenerator;
 
     /**
      * {@inheritdoc}
      */
-    public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler, AuthenticationFailureHandlerInterface $failureHandler, array $options = array(), LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfProviderInterface $csrfProvider = null)
+    public function __construct(SecurityContextInterface $securityContext, AuthenticationManagerInterface $authenticationManager, SessionAuthenticationStrategyInterface $sessionStrategy, HttpUtils $httpUtils, $providerKey, AuthenticationSuccessHandlerInterface $successHandler, AuthenticationFailureHandlerInterface $failureHandler, array $options = array(), LoggerInterface $logger = null, EventDispatcherInterface $dispatcher = null, CsrfTokenGeneratorInterface $csrfTokenGenerator = null)
     {
         parent::__construct($securityContext, $authenticationManager, $sessionStrategy, $httpUtils, $providerKey, $successHandler, $failureHandler, array_merge(array(
             'username_parameter' => '_username',
@@ -47,7 +47,7 @@ public function __construct(SecurityContextInterface $securityContext, Authentic
             'post_only'          => true,
         ), $options), $logger, $dispatcher);
 
-        $this->csrfProvider = $csrfProvider;
+        $this->csrfTokenGenerator = $csrfTokenGenerator;
     }
 
     /**
@@ -67,10 +67,10 @@ protected function requiresAuthentication(Request $request)
      */
     protected function attemptAuthentication(Request $request)
     {
-        if (null !== $this->csrfProvider) {
+        if (null !== $this->csrfTokenGenerator) {
             $csrfToken = $request->get($this->options['csrf_parameter'], null, true);
 
-            if (false === $this->csrfProvider->isCsrfTokenValid($this->options['intention'], $csrfToken)) {
+            if (false === $this->csrfTokenGenerator->isCsrfTokenValid($this->options['intention'], $csrfToken)) {
                 throw new InvalidCsrfTokenException('Invalid CSRF token.');
             }
         }
diff --git a/composer.json b/composer.json
@@ -25,10 +25,11 @@
     "require-dev": {
         "symfony/form": "~2.0",
         "symfony/routing": "~2.2",
+        "symfony/security-csrf": "~2.4",
         "psr/log": "~1.0"
     },
     "suggest": {
-        "symfony/form": "",
+        "symfony/security-csrf": "",
         "symfony/routing": ""
     },
     "autoload": {
