From 98072ba4a963449c82933bff4781c77bdeda3337 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 23 Oct 2021 17:26:01 +0100 Subject: [PATCH] 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. --- .env.example.complete | 7 ++---- app/Auth/Access/Saml2Service.php | 25 ++++++++++++------- app/Config/saml2.php | 14 +++++++---- app/Http/Controllers/Auth/Saml2Controller.php | 2 +- 4 files changed, 28 insertions(+), 20 deletions(-) diff --git a/.env.example.complete b/.env.example.complete index a29afaafd..683db703c 100644 --- 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 index 58f999709..ad889faf7 100644 --- a/app/Auth/Access/Saml2Service.php +++ b/app/Auth/Access/Saml2Service.php @@ -9,6 +9,7 @@ use BookStack\Exceptions\StoppedAuthenticationException; 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 @@ class Saml2Service * * @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 @@ class Saml2Service 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 index 3c4319100..44d06c5b2 100644 --- a/app/Config/saml2.php +++ b/app/Config/saml2.php @@ -1,6 +1,7 @@ '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 @@ return [ // 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 index 871abf59f..bd3b25da7 100644 --- a/app/Http/Controllers/Auth/Saml2Controller.php +++ b/app/Http/Controllers/Auth/Saml2Controller.php @@ -37,7 +37,7 @@ class Saml2Controller extends Controller */ public function logout() { - $logoutDetails = $this->samlService->logout(); + $logoutDetails = $this->samlService->logout(auth()->user()); if ($logoutDetails['id']) { session()->flash('saml2_logout_request_id', $logoutDetails['id']);