Handle AuthnContextClassRef changes when proxying (#1874)

ghalse · tvdijen · tvdijen · commit a87663808779 · 2024-02-03T00:29:40.000+01:00
* Fix for when no RequestedAuthnContext is provided

PHP Warning:  Trying to access array offset on value of type null

* Handle AuthnContextClassRef changes when proxying

* Perform strict comparisons

---------

Co-authored-by: Tim van Dijen &lt;tvdijen@gmail.com&gt;
diff --git a/docs/simplesamlphp-changelog.md b/docs/simplesamlphp-changelog.md
@@ -56,7 +56,6 @@ Released 2023-10-30
 * Update vulnerable composer (CVE-2023-43655; not affected)
 * Fixed a potential XSS-through-DOM (3x; not affected)
 * Fixed a warning in the RequestedAuthnContextSelector
-* Fixed file logging handler to not fail on the first write after file-creation (#1877)
 
 ## Version 2.0.6
 
diff --git a/modules/saml/src/Auth/Source/SP.php b/modules/saml/src/Auth/Source/SP.php
@@ -886,6 +886,30 @@ public function reauthenticate(array &$state): void
             $response = self::askForIdPChange($state);
             $response->send();
         }
+
+        /*
+         * When proxying AuthnContextClassRef, we also need to check if the
+         * original IdP used a compatible AuthnContext. If not, we may need
+         * need to do step-up authentication (e.g. for requesting MFA). At
+         * the moment this only handles exact comparisons, since handling
+         * better/minimum/maximum would require a prioritised list.
+         */
+        if (
+            $this->passAuthnContextClassRef
+            && isset($state['saml:RequestedAuthnContext'])
+            && $state['saml:RequestedAuthnContext']['Comparison'] === Constants::COMPARISON_EXACT
+            && isset($data['saml:sp:AuthnContext'])
+            && $state['saml:RequestedAuthnContext']['AuthnContextClassRef'][0] !== $data['saml:sp:AuthnContext']
+        ) {
+            Logger::warning(sprintf(
+                "Reauthentication requires change in AuthnContext from '%s' to '%s'.",
+                $data['saml:sp:AuthnContext'],
+                $state['saml:RequestedAuthnContext']['AuthnContextClassRef'][0]
+            ));
+            $state['saml:idp'] = $data['saml:sp:IdP'];
+            $state['saml:sp:AuthId'] = $this->authId;
+            self::tryStepUpAuth($state);
+        }
     }
 
 
@@ -931,6 +955,37 @@ public static function askForIdPChange(array &$state): RedirectResponse
     }
 
 
+    /**
+     * Attempt to re-authenticate using the same identity provider, perhaps
+     * with different requirements (e.g. AuthnContext). Note that this method
+     * is intended for instances of SimpleSAMLphp running as a SAML proxy,
+     * and therefore acting as both an SP and an IdP at the same time.
+     *
+     * @param array The state array
+     * The following keys must be defined:
+     * - 'saml:idp': the IdP to re-authenticate with
+     * - 'saml:sp:AuthId': the identifier of the current authentication source.
+     * @throws \SAML2\Exception\Protocol\NoPassiveException In case the authentication request was passive.
+     */
+    public static function tryStepUpAuth(array &$state): void
+    {
+        Assert::keyExists($state, 'saml:idp');
+        Assert::keyExists($state, 'saml:sp:AuthId');
+
+        if (isset($state['isPassive']) && (bool) $state['isPassive']) {
+            // passive request, we cannot authenticate the user
+            throw new NoPassiveException(
+                Constants::STATUS_REQUESTER . ':  Reauthentication required'
+            );
+        }
+
+        /** @var \SimpleSAML\Module\saml\Auth\Source\SP $as */
+        $as = new Auth\Simple($state['saml:sp:AuthId']);
+        $as->login($state);
+        Assert::true(false);
+    }
+
+
     /**
      * Log the user out before logging in again.
      *
