Reviewed SAML SLS changes for ADFS, #2902

- Migrated env usages to config. - Removed potentially unneeded config options or auto-set signed options based upon provision of certificate. - Aligned SP certificate env option naming with similar IDP option. Tested via AFDS on windows server 2019. To test on other providers.
BookStackApp · Oct 23, 2021 · 98072ba · 98072ba
1 parent 2e9ac21
commit 98072ba
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 20 deletions.
diff --git a/.env.example.complete b/.env.example.complete
@@ -232,11 +232,8 @@ SAML2_ONELOGIN_OVERRIDES=null
 SAML2_DUMP_USER_DETAILS=false
 SAML2_AUTOLOAD_METADATA=false
 SAML2_IDP_AUTHNCONTEXT=true
-SAML2_SP_CERTIFICATE=null
-SAML2_SP_PRIVATEKEY=null
-SAML2_SP_NAME_ID_Format=null
-SAML2_SP_NAME_ID_SP_NAME_QUALIFIER=null
-SAML2_RETRIEVE_PARAMETERS_FROM_SERVER=false
+SAML2_SP_x509=null
+SAML2_SP_x509_KEY=null
 
 # SAML group sync configuration
 # Refer to https://www.bookstackapp.com/docs/admin/saml2-auth/

diff --git a/app/Auth/Access/Saml2Service.php b/app/Auth/Access/Saml2Service.php
@@ -9,6 +9,7 @@
 use BookStack\Exceptions\UserRegistrationException;
 use Exception;
 use OneLogin\Saml2\Auth;
+use OneLogin\Saml2\Constants;
 use OneLogin\Saml2\Error;
 use OneLogin\Saml2\IdPMetadataParser;
 use OneLogin\Saml2\ValidationError;
@@ -59,17 +60,20 @@ public function login(): array
      *
      * @throws Error
      */
-    public function logout(): array
+    public function logout(User $user): array
     {
         $toolKit = $this->getToolkit();
         $returnRoute = url('/');
 
         try {
-            $email = auth()->user()['email'];
-            $nameIdFormat = env('SAML2_SP_NAME_ID_Format', null);
-            $nameIdSPNameQualifier = env('SAML2_SP_NAME_ID_SP_NAME_QUALIFIER', null);
-
-            $url = $toolKit->logout($returnRoute, [], $email, null, true, $nameIdFormat, null, $nameIdSPNameQualifier);
+            $url = $toolKit->logout(
+                $returnRoute,
+                [],
+                $user->email,
+                null,
+                true,
+                Constants::NAMEID_EMAIL_ADDRESS
+            );
             $id = $toolKit->getLastRequestID();
         } catch (Error $error) {
             if ($error->getCode() !== Error::SAML_SINGLE_LOGOUT_NOT_SUPPORTED) {
@@ -128,10 +132,13 @@ public function processAcsResponse(string $requestId, string $samlResponse): ?Us
     public function processSlsResponse(?string $requestId): ?string
     {
         $toolkit = $this->getToolkit();
-        $retrieveParametersFromServer = env('SAML2_RETRIEVE_PARAMETERS_FROM_SERVER', false);
-
-        $redirect = $toolkit->processSLO(true, $requestId, $retrieveParametersFromServer, null, true);
 
+        // The $retrieveParametersFromServer in the call below will mean the library will take the query
+        // parameters, used for the response signing, from the raw $_SERVER['QUERY_STRING']
+        // value so that the exact encoding format is matched when checking the signature.
+        // This is primarily due to ADFS encoding query params with lowercase percent encoding while
+        // PHP (And most other sensible providers) standardise on uppercase.
+        $redirect = $toolkit->processSLO(true, $requestId, true, null, true);
         $errors = $toolkit->getErrors();
 
         if (!empty($errors)) {

diff --git a/app/Config/saml2.php b/app/Config/saml2.php
@@ -1,6 +1,7 @@
 <?php
 
 $SAML2_IDP_AUTHNCONTEXT = env('SAML2_IDP_AUTHNCONTEXT', true);
+$SAML2_SP_x509 = env('SAML2_SP_x509', false);
 
 return [
 
@@ -78,10 +79,11 @@
             // represent the requested subject.
             // Take a look on lib/Saml2/Constants.php to see the NameIdFormat supported
             'NameIDFormat' => 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress',
+
             // Usually x509cert and privateKey of the SP are provided by files placed at
             // the certs folder. But we can also provide them with the following parameters
-            'x509cert'   => env('SAML2_SP_CERTIFICATE', ''),
-            'privateKey' => env('SAML2_SP_PRIVATEKEY', ''),
+            'x509cert'   => $SAML2_SP_x509 ?: '',
+            'privateKey' => env('SAML2_SP_x509_KEY', ''),
         ],
         // Identity Provider Data that we want connect with our SP
         'idp' => [
@@ -147,9 +149,11 @@
             // Multiple forced values can be passed via a space separated array, For example:
             // SAML2_IDP_AUTHNCONTEXT="urn:federation:authentication:windows urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport"
             'requestedAuthnContext' => is_string($SAML2_IDP_AUTHNCONTEXT) ? explode(' ', $SAML2_IDP_AUTHNCONTEXT) : $SAML2_IDP_AUTHNCONTEXT,
-            'logoutRequestSigned'   => env('SAML2_LOGOUT_REQUEST_SIGNED', false),
-            'logoutResponseSigned'  => env('SAML2_LOGOUT_RESPONSE_SIGNED', false),
-            'lowercaseUrlencoding'  => env('SAML2_LOWERCASE_URLENCODING', false),
+            // Sign requests and responses if a certificate is in use
+            'logoutRequestSigned'   => (bool) $SAML2_SP_x509,
+            'logoutResponseSigned'  => (bool) $SAML2_SP_x509,
+            'authnRequestsSigned'   => (bool) $SAML2_SP_x509,
+            'lowercaseUrlencoding'  => false,
         ],
     ],
 

diff --git a/app/Http/Controllers/Auth/Saml2Controller.php b/app/Http/Controllers/Auth/Saml2Controller.php
@@ -37,7 +37,7 @@ public function login()
      */
     public function logout()
     {
-        $logoutDetails = $this->samlService->logout();
+        $logoutDetails = $this->samlService->logout(auth()->user());
 
         if ($logoutDetails['id']) {
             session()->flash('saml2_logout_request_id', $logoutDetails['id']);