Merge pull request #704 from DirectoryTree/BUG-669

Bug 669 - [Bug] ldap_start_tls(): Unable to start TLS: Local error
DirectoryTree · Mar 27, 2024 · a9eb49e · a9eb49e
2 parents 4f28001 + cd28b77
commit a9eb49e
Show file tree

Hide file tree

Showing 7 changed files with 108 additions and 14 deletions.
diff --git a/src/Auth/Guard.php b/src/Auth/Guard.php
@@ -80,7 +80,7 @@ public function bind(?string $username = null, ?string $password = null): void
         // Prior to binding, we will upgrade our connectivity to TLS on our current
         // connection and ensure we are not already bound before upgrading.
         // This is to prevent subsequent upgrading on several binds.
-        if ($this->connection->isUsingTLS() && ! $this->connection->isBound()) {
+        if ($this->connection->isUsingTLS() && ! $this->connection->isSecure()) {
             $this->connection->startTLS();
         }
 

diff --git a/src/HandlesConnection.php b/src/HandlesConnection.php
@@ -23,10 +23,15 @@ trait HandlesConnection
     protected mixed $connection = null;
 
     /**
-     * The bound status of the connection.
+     * Whether the connection is bound.
      */
     protected bool $bound = false;
 
+    /**
+     * Whether the connection is secured over TLS or SSL.
+     */
+    protected bool $secure = false;
+
     /**
      * Whether the connection must be bound over SSL.
      */
@@ -61,6 +66,14 @@ public function isBound(): bool
         return $this->bound;
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function isSecure(): bool
+    {
+        return $this->secure;
+    }
+
     /**
      * {@inheritdoc}
      */
@@ -139,6 +152,16 @@ public function getExtendedError(): ?string
         return $this->getDiagnosticMessage();
     }
 
+    /**
+     * Handle the bind response.
+     */
+    protected function handleBindResponse(LdapResultResponse $response): void
+    {
+        $this->bound = $response->successful();
+
+        $this->secure = $this->secure ?: $this->bound && $this->isUsingSSL();
+    }
+
     /**
      * Convert warnings to exceptions for the given operation.
      *

diff --git a/src/Ldap.php b/src/Ldap.php
@@ -150,7 +150,7 @@ public function setRebindCallback(callable $callback): bool
      */
     public function startTLS(): bool
     {
-        return $this->executeFailableOperation(function () {
+        return $this->secure = $this->executeFailableOperation(function () {
             return ldap_start_tls($this->connection);
         });
     }
@@ -276,7 +276,7 @@ public function bind(?string $dn = null, ?string $password = null, ?array $contr
 
         $response = $this->parseResult($result);
 
-        $this->bound = $response && $response->successful();
+        $this->handleBindResponse($response);
 
         return $response;
     }

diff --git a/src/LdapInterface.php b/src/LdapInterface.php
@@ -108,6 +108,11 @@ public function isUsingTLS(): bool;
      */
     public function isBound(): bool;
 
+    /**
+     * Determine if the connection is secure over TLS or SSL.
+     */
+    public function isSecure(): bool;
+
     /**
      * Determine if the connection has been created.
      */

diff --git a/src/Testing/LdapFake.php b/src/Testing/LdapFake.php
@@ -330,7 +330,7 @@ public function getOption(int $option, mixed &$value = null): mixed
      */
     public function startTLS(): bool
     {
-        return $this->resolveExpectation(__FUNCTION__, func_get_args());
+        return $this->secure = $this->resolveExpectation(__FUNCTION__, func_get_args());
     }
 
     /**
@@ -368,7 +368,7 @@ public function bind(?string $dn = null, ?string $password = null, ?array $contr
     {
         $result = $this->resolveExpectation(__FUNCTION__, func_get_args());
 
-        $this->bound = $result->successful();
+        $this->handleBindResponse($result);
 
         return $result;
     }

diff --git a/tests/Unit/Auth/GuardTest.php b/tests/Unit/Auth/GuardTest.php
@@ -58,7 +58,7 @@ public function test_bind_does_not_rebind_with_configured_user()
 
         $guard = new Guard($ldap, new DomainConfiguration());
 
-        $this->assertNull($guard->bind('user', 'pass'));
+        $guard->bind('user', 'pass');
     }
 
     public function test_bind_allows_null_username_and_password()
@@ -69,7 +69,7 @@ public function test_bind_allows_null_username_and_password()
 
         $guard = new Guard($ldap, new DomainConfiguration());
 
-        $this->assertNull($guard->bind());
+        $guard->bind();
     }
 
     public function test_bind_always_throws_exception_on_invalid_credentials()
@@ -96,7 +96,7 @@ public function test_bind_as_administrator()
             'password' => 'bar',
         ]));
 
-        $this->assertNull($guard->bindAsConfiguredUser());
+        $guard->bindAsConfiguredUser();
     }
 
     public function test_binding_events_are_fired_during_bind()
@@ -231,4 +231,25 @@ public function test_sasl_bind()
 
         $this->assertTrue($ldap->isBound());
     }
+
+    public function test_tls_is_only_upgraded_once_on_subsequent_binds()
+    {
+        $ldap = (new LdapFake())->expect([
+            LdapFake::operation('bind')->once()->with('admin', 'password')->andReturnResponse(),
+            LdapFake::operation('startTLS')->once()->andReturnTrue(),
+            LdapFake::operation('bind')->once()->with('foo', 'bar')->andReturnResponse(1),
+        ]);
+
+        $ldap->tls();
+
+        $this->assertFalse($ldap->isSecure());
+
+        $guard = new Guard($ldap, new DomainConfiguration([
+            'username' => 'admin',
+            'password' => 'password',
+        ]));
+
+        $this->assertFalse($guard->attempt('foo', 'bar'));
+        $this->assertTrue($ldap->isSecure());
+    }
 }
diff --git a/tests/Unit/LdapTest.php b/tests/Unit/LdapTest.php
@@ -88,11 +88,10 @@ public function test_can_change_passwords()
 
     public function test_set_options()
     {
-        $ldap = (new LdapFake())
-            ->expect([
-                LdapFake::operation('setOption')->once()->with(1, 'value')->andReturnTrue(),
-                LdapFake::operation('setOption')->once()->with(2, 'value')->andReturnTrue(),
-            ]);
+        $ldap = (new LdapFake())->expect([
+            LdapFake::operation('setOption')->once()->with(1, 'value')->andReturnTrue(),
+            LdapFake::operation('setOption')->once()->with(2, 'value')->andReturnTrue(),
+        ]);
 
         $ldap->setOptions([1 => 'value', 2 => 'value']);
     }
@@ -105,4 +104,50 @@ public function test_get_detailed_error_returns_null_when_error_number_is_zero()
 
         $this->assertNull($ldap->getDetailedError());
     }
+
+    public function test_is_secure_after_binding_with_an_ssl_connection()
+    {
+        $ldap = (new LdapFake())->expect([
+            LdapFake::operation('bind')->once()->andReturnResponse(),
+        ]);
+
+        $ldap->ssl();
+
+        $this->assertFalse($ldap->isSecure());
+
+        $ldap->bind('foo', 'bar');
+
+        $this->assertTrue($ldap->isSecure());
+    }
+
+    public function test_is_secure_after_starting_tls()
+    {
+        $ldap = (new LdapFake())->expect([
+            LdapFake::operation('startTLS')->once()->andReturnTrue(),
+        ]);
+
+        $this->assertFalse($ldap->isSecure());
+
+        $ldap->startTLS();
+
+        $this->assertTrue($ldap->isSecure());
+    }
+
+    public function test_is_secure_after_starting_tls_but_failing_bind()
+    {
+        $ldap = (new LdapFake())->expect([
+            LdapFake::operation('startTLS')->once()->andReturnTrue(),
+            LdapFake::operation('bind')->once()->andReturnResponse(1),
+        ]);
+
+        $this->assertFalse($ldap->isSecure());
+
+        $ldap->startTLS();
+
+        $this->assertTrue($ldap->isSecure());
+
+        $ldap->bind('foo', 'bar');
+
+        $this->assertTrue($ldap->isSecure());
+    }
 }