Cleanup + fixed tests of PR #19

fabiang · Feb 14, 2025 · 52dbc0e · 52dbc0e
1 parent e6d6b0c
commit 52dbc0e
Show file tree

Hide file tree

Showing 11 changed files with 148 additions and 36 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -64,7 +64,16 @@ jobs:
     - name: Wait for XMPP to become available
       uses: iFaxity/wait-on-action@v1
       with:
-        resource: tcp:localhost:5222
+        resource: tcp:localhost:15222
+        timeout: 1800000
+        interval: 10000
+        delay: 60000
+        log: true
+
+    - name: Wait for second XMPP to become available
+      uses: iFaxity/wait-on-action@v1
+      with:
+        resource: tcp:localhost:25222
         timeout: 1800000
         interval: 10000
         delay: 60000

diff --git a/.gitignore b/.gitignore
@@ -3,6 +3,7 @@
 build/
 phpunit.xml
 .phpunit.result.cache
+.phpunit.cache/
 .phpcs-cache
 *.log
 codeclimate.json

diff --git a/behat.yml b/behat.yml
@@ -5,7 +5,9 @@ default:
       contexts:
         - Fabiang\Sasl\Behat\XMPPContext:
           - localhost
-          - 5222
+          - 15222
+          # Extra test server for https://dyn.eightysoft.de/xeps/xep-0474.html
+          - 25222
           - localhost
           - testuser
           - testpass

diff --git a/docker-compose.yml b/docker-compose.yml
@@ -6,8 +6,23 @@ services:
       - "./tests/config/ejabberd/ejabberd.yml:/home/ejabberd/conf/ejabberd.yml"
       - "./tests/config/ejabberd/ejabberd.db:/home/ejabberd/database/ejabberd.db"
     ports:
-      - "5222:5222"
-      - "5223:5223"
+      - "15222:5222"
+      - "15223:5223"
+    environment:
+      - CTL_ON_CREATE=register testuser localhost testpass
+
+  # Extra test server for https://dyn.eightysoft.de/xeps/xep-0474.html
+  xmpp_source:
+    build:
+      context: https://github.com/processone/docker-ejabberd.git#master:ecs
+      args:
+        - VERSION=ceee3d3be1fc1053e61bbc81881bd40dbbbc1e89
+    volumes:
+      - "./tests/config/ejabberd/ejabberd.yml:/home/ejabberd/conf/ejabberd.yml"
+      - "./tests/config/ejabberd/ejabberd.db:/home/ejabberd/database/ejabberd.db"
+    ports:
+      - "25222:5222"
+      - "25223:5223"
     environment:
       - CTL_ON_CREATE=register testuser localhost testpass
 

diff --git a/src/Authentication/AbstractAuthentication.php b/src/Authentication/AbstractAuthentication.php
@@ -103,9 +103,11 @@ protected function generateCnonce()
     /**
      * Generate downgrade protection string
      *
+     * @param string $groupDelimiter
+     * @param string $delimiter
      * @return string
      */
-    protected function generateDowngradeProtectionVerification()
+    protected function generateDowngradeProtectionVerification($groupDelimiter, $delimiter)
     {
         $downgradeProtectionOptions = $this->options->getDowngradeProtection();
 
@@ -119,9 +121,9 @@ protected function generateDowngradeProtectionVerification()
         usort($allowedMechanisms, array($this, 'sortOctetCollation'));
         usort($allowedChannelBindings, array($this, 'sortOctetCollation'));
 
-        $protect = implode(',', $allowedMechanisms);
+        $protect = implode($delimiter, $allowedMechanisms);
         if (count($allowedChannelBindings) > 0) {
-            $protect .= '|' . implode(',', $allowedChannelBindings);
+            $protect .= $groupDelimiter. implode($delimiter, $allowedChannelBindings);
         }
         return $protect;
     }

diff --git a/src/Authentication/SCRAM.php b/src/Authentication/SCRAM.php
@@ -165,17 +165,18 @@ private function generateResponse($challenge, $password)
         $serverMessageRegexp = "#^r=(?<nonce>[\x21-\x2B\x2D-\x7E/]+)"
             . ",s=(?<salt>(?:[A-Za-z0-9/+]{4})*(?:[A-Za-z0-9/+]{3}=|[A-Za-z0-9/+]{2}==)?)"
             . ",i=(?<iteration>[0-9]*)"
-            . "(,(?<additionalAttributes>.*))?$#";
+            . "(?<additionalAttr>(?:,[A-Za-z]=[^,]+)*)$#";
 
         if (!isset($this->cnonce, $this->gs2Header) || !preg_match($serverMessageRegexp, $challenge, $matches)) {
             return false;
         }
 
-        $additionalAttributes = $this->parseAdditionalAttributes($matches);
+        $additionalAttributes = $this->parseAdditionalAttributes($matches['additionalAttr']);
 
         //forbidden by RFC 5802
-        if(isset($additionalAttributes['m']))
+        if (isset($additionalAttributes['m'])) {
             return false;
+        }
 
         $nonce = $matches['nonce'];
         $salt  = base64_decode($matches['salt']);
@@ -191,9 +192,14 @@ private function generateResponse($challenge, $password)
             return false;
         }
 
-        //SSDP hash
-        if (!empty($additionalAttributes['d'])) {
-            if (!$this->downgradeProtection($additionalAttributes['d'])) {
+        if (! empty($additionalAttributes['h'])) {
+            if (! $this->downgradeProtection($additionalAttributes['h'], "\x1f", "\x1e")) {
+                return false;
+            }
+        }
+
+        if (! empty($additionalAttributes['d'])) {
+            if (! $this->downgradeProtection($additionalAttributes['d'], '|', ',')) {
                 return false;
             }
         }
@@ -215,15 +221,22 @@ private function generateResponse($challenge, $password)
 
     /**
      * @param string $expectedDowngradeProtectionHash
+     * @param string $groupDelimiter
+     * @param string $delimiter
      * @return bool
      */
-    private function downgradeProtection($expectedDowngradeProtectionHash)
+    private function downgradeProtection($expectedDowngradeProtectionHash, $groupDelimiter, $delimiter)
     {
         if ($this->options->getDowngradeProtection() === null) {
             return true;
         }
 
-        $actualDgPHash = base64_encode(call_user_func($this->hash, $this->generateDowngradeProtectionVerification()));
+        $actualDgPHash = base64_encode(
+            call_user_func(
+                $this->hash,
+                $this->generateDowngradeProtectionVerification($groupDelimiter, $delimiter)
+            )
+        );
         return $expectedDowngradeProtectionHash === $actualDgPHash;
     }
 
@@ -248,19 +261,26 @@ private function hi($str, $salt, $i)
 
     /**
      * This will parse all non-fixed-position additional SCRAM attributes (the optional ones and the m-attribute)
-     * @param array $matches The array returned by our regex match, MUST contain an 'additionalAttributes' key
+     *
+     * @param string $additionalAttributes 'additionalAttributes' string
      * @return array
      */
-    private function parseAdditionalAttributes($matches)
+    private function parseAdditionalAttributes($additionalAttributes)
     {
-        $additionalAttributes=array();
-        $tail=explode(',', $matches['additionalAttributes']);
-        foreach($tail as $entry)
-        {
-            $entry=explode("=", $entry, 2);
-            $additionalAttributes[$entry[0]] = $entry[1];
+        if ($additionalAttributes == "") {
+            return array();
         }
-        return $additionalAttributes;
+
+        $return = array();
+        $tail = explode(',', $additionalAttributes);
+
+        foreach ($tail as $entry) {
+            $entry = explode("=", $entry, 2);
+            if (count($entry) > 1) {
+                $return[$entry[0]] = $entry[1];
+            }
+        }
+        return $return;
     }
 
     /**
@@ -274,21 +294,22 @@ private function parseAdditionalAttributes($matches)
      */
     public function verify($data)
     {
-        $verifierRegexp = '#^v=((?:[A-Za-z0-9/+]{4})*(?:[A-Za-z0-9/+]{3}=|[A-Za-z0-9/+]{2}==)?)(,(?<additionalAttributes>.*))?$#';
+        $verifierRegexp = '#^v=(?<verifier>(?:[A-Za-z0-9/+]{4})*(?:[A-Za-z0-9/+]{3}=|[A-Za-z0-9/+]{2}==)?)'
+            . '(?<additionalAttr>(?:,[A-Za-z]=[^,]+)*)$#';
 
         $matches = array();
         if (!isset($this->saltedPassword, $this->authMessage) || !preg_match($verifierRegexp, $data, $matches)) {
             // This cannot be an outcome, you never sent the challenge's response.
             return false;
         }
 
-        $additionalAttributes = $this->parseAdditionalAttributes($matches);
+        $additionalAttribute = $this->parseAdditionalAttributes($matches['additionalAttr']);
 
-        //forbidden by RFC 5802
-        if(isset($additionalAttributes['m']))
+        if (isset($additionalAttribute['m'])) {
             return false;
+        }
 
-        $verifier                = $matches[1];
+        $verifier                = $matches['verifier'];
         $proposedServerSignature = base64_decode($verifier);
         $serverKey               = call_user_func($this->hmac, $this->saltedPassword, "Server Key", true);
         $serverSignature         = call_user_func($this->hmac, $serverKey, $this->authMessage, true);

diff --git a/tests/features/bootstrap/AbstractContext.php b/tests/features/bootstrap/AbstractContext.php
@@ -56,12 +56,12 @@ abstract class AbstractContext
     protected $logdir;
     protected $logfile;
 
-    protected function connect()
+    protected function connect($port)
     {
         $errno  = null;
         $errstr = null;
 
-        $connectionString = "tcp://{$this->hostname}:{$this->port}";
+        $connectionString = "tcp://{$this->hostname}:{$port}";
 
         $context = stream_context_create(array(
             'ssl' => array(

diff --git a/tests/features/bootstrap/AbstractXMPPContext.php b/tests/features/bootstrap/AbstractXMPPContext.php
@@ -55,6 +55,9 @@ abstract class AbstractXMPPContext extends AbstractContext implements Context, S
      */
     protected $domain;
 
+    protected $port1;
+    protected $port2;
+
     /**
      * @var string
      */
@@ -81,7 +84,8 @@ abstract class AbstractXMPPContext extends AbstractContext implements Context, S
      * context constructor through behat.yml.
      *
      * @param string  $hostname Hostname for connection
-     * @param integer $port
+     * @param integer $port1
+     * @param integer $port2
      * @param string  $domain
      * @param string  $username Domain name of server (important for connecting)
      * @param string  $password
@@ -90,15 +94,17 @@ abstract class AbstractXMPPContext extends AbstractContext implements Context, S
      */
     public function __construct(
         $hostname,
-        $port,
+        $port1,
+        $port2,
         $domain,
         $username,
         $password,
         $logdir,
         $tlsversion = 'tlsv1.2'
     ) {
         $this->hostname = $hostname;
-        $this->port     = (int) $port;
+        $this->port1     = (int) $port1;
+        $this->port2     = (int) $port2;
         $this->domain   = $domain;
         $this->username = $username;
         $this->password = $password;

diff --git a/tests/features/bootstrap/POP3Context.php b/tests/features/bootstrap/POP3Context.php
@@ -95,7 +95,7 @@ public function __construct($hostname, $port, $username, $password, $logdir)
      */
     public function connectionToPopServer()
     {
-        $this->connect();
+        $this->connect($this->port);
         Assert::assertSame("+OK Dovecot ready.\r\n", $this->read());
     }
 

diff --git a/tests/features/bootstrap/XMPPContext.php b/tests/features/bootstrap/XMPPContext.php
@@ -112,7 +112,16 @@ private function sendStreamStart()
      */
     public function connectionToXmppServer()
     {
-        $this->connect();
+        $this->connect($this->port1);
+        $this->sendStreamStart();
+    }
+
+    /**
+     * @Given Connection to second XMPP server
+     */
+    public function connectionToSecondXmppServer(): void
+    {
+        $this->connect($this->port2);
         $this->sendStreamStart();
     }
 

diff --git a/tests/features/downgrade_protection_source.feature b/tests/features/downgrade_protection_source.feature
@@ -0,0 +1,47 @@
+@xmpp @downgradeProtection
+Feature: Authentication with a second XMPP server
+
+  Background:
+    Given Connection to second XMPP server
+    And Connection is encrypted by STARTTLS
+
+  @scramsha1
+  Scenario: Authenticate with xmpp server through scram-sha-1 authentication with downgrade protection
+    Given xmpp server supports authentication method "SCRAM-SHA-1"
+    And Downgrade protection is properly set up
+    When authenticate with method SCRAM-SHA-1
+    And response to challenge for SCRAM-SHA-1
+    Then should be authenticated at xmpp server with verification
+
+  @scramsha256
+  Scenario: Authenticate with xmpp server through scram-sha-256 authentication with downgrade protection
+    Given xmpp server supports authentication method "SCRAM-SHA-256"
+    And Downgrade protection is properly set up
+    When authenticate with method SCRAM-SHA-256
+    And response to challenge for SCRAM-SHA-256
+    Then should be authenticated at xmpp server with verification
+
+  @scramsha1
+  Scenario: Authenticate with xmpp server through scram-sha-1 authentication with downgrade protection with invalid mechanism
+    Given xmpp server supports authentication method "SCRAM-SHA-1"
+    And Downgrade protection is properly set up
+    And We simulate downgrade protection error by adding an auth mechanism
+    When authenticate with method SCRAM-SHA-1
+    And response to challenge for SCRAM-SHA-1 was invalid
+    Then Server connection should be closed
+
+  @scramsha1
+  Scenario: Authenticate with xmpp server through scram-sha-1 authentication with downgrade protection with invalid channel-binding
+    Given xmpp server supports authentication method "SCRAM-SHA-1"
+    And Downgrade protection is properly set up
+    And We simulate downgrade protection error by adding an unsupported channel-binding
+    When authenticate with method SCRAM-SHA-1
+    And response to challenge for SCRAM-SHA-1 was invalid
+    Then Server connection should be closed
+
+  @scramsha1
+  Scenario: Authenticate with xmpp server through scram-sha-1 authentication with downgrade protection is disabled
+    Given xmpp server supports authentication method "SCRAM-SHA-1"
+    When authenticate with method SCRAM-SHA-1
+    And response to challenge for SCRAM-SHA-1
+    Then should be authenticated at xmpp server with verification