diff --git a/app/Services/SamlIdpService.php b/app/Services/SamlIdpService.php index 9d36721..13611ac 100644 --- a/app/Services/SamlIdpService.php +++ b/app/Services/SamlIdpService.php @@ -4,6 +4,7 @@ namespace App\Services; +use App\Enums\SamlNameIdFormatEnum; use App\Models\{ConnectedApp, User}; use DOMDocument; use DOMElement; @@ -237,8 +238,8 @@ public function generateResponse(User $user, ConnectedApp $app, string $requestI $idpEntityId )); - $status = $dom->createElement('samlp:Status'); - $statusCode = $dom->createElement('samlp:StatusCode'); + $status = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:protocol', 'samlp:Status'); + $statusCode = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:protocol', 'samlp:StatusCode'); $statusCode->setAttribute('Value', 'urn:oasis:names:tc:SAML:2.0:status:Success'); $status->appendChild($statusCode); $response->appendChild($status); @@ -249,14 +250,18 @@ public function generateResponse(User $user, ConnectedApp $app, string $requestI $assertion->setAttribute('ID', $assertionId); $assertion->setAttribute('Version', '2.0'); $assertion->setAttribute('IssueInstant', $timestamps['issue']); - $assertion->appendChild($dom->createElement('saml:Issuer', $idpEntityId)); + $assertion->appendChild($dom->createElementNS( + 'urn:oasis:names:tc:SAML:2.0:assertion', + 'saml:Issuer', + $idpEntityId + )); // 3. Build Assertion Sub-components Log::debug('SamlIdpService: Appending assertion components (Subject, Conditions, AuthnStatement, Attributes).'); - $this->buildSubject($dom, $assertion, $user, $requestId, $acsUrl, $timestamps['expire']); + $this->buildSubject($dom, $assertion, $user, $requestId, $acsUrl, $timestamps['expire'], $app); $this->buildConditions($dom, $assertion, $spEntityId, $timestamps['notBefore'], $timestamps['expire']); $this->buildAuthnStatement($dom, $assertion, $timestamps['issue'], $assertionId); - $this->buildAttributeStatement($dom, $assertion, $user); + $this->buildAttributeStatement($dom, $assertion, $user, $app); $response->appendChild($assertion); $dom->appendChild($response); @@ -289,26 +294,45 @@ private function buildSubject( User $user, string $requestId, string $acsUrl, - string $expireInstant + string $expireInstant, + ConnectedApp $app ): void { - $immutableId = $user->immutable_id ?? base64_encode((string) $user->id); + /** @var array $settings */ + $settings = (array) $app->settings; + $nameIdSource = $settings['saml']['nameid_source'] ?? 'immutable_id'; + + $formatKey = mb_strtolower($settings['saml']['nameid_format'] ?? 'persistent'); + $nameIdFormat = match ($formatKey) { + 'email', 'emailaddress' => SamlNameIdFormatEnum::Email, + 'transient' => SamlNameIdFormatEnum::Transient, + 'unspecified' => SamlNameIdFormatEnum::Unspecified, + default => SamlNameIdFormatEnum::Persistent, + }; + + $formatUrn = $nameIdFormat->value; + + $nameIdValue = match ($nameIdSource) { + 'email' => $user->email, + 'id' => (string) $user->id, + default => $user->immutable_id ?? base64_encode((string) $user->id), + }; Log::debug('SamlIdpService: Constructing saml:Subject component.', [ 'user_id' => $user->id, - 'has_explicit_immutable_id' => ! empty($user->immutable_id), - 'resolved_immutable_id' => $immutableId, - 'nameid_format' => 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent', + 'nameid_format' => $formatUrn, + 'nameid_source' => $nameIdSource, + 'resolved_value' => $nameIdValue, ]); - $subject = $dom->createElement('saml:Subject'); - $nameId = $dom->createElement('saml:NameID', $immutableId); - $nameId->setAttribute('Format', 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent'); + $subject = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:assertion', 'saml:Subject'); + $nameId = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:assertion', 'saml:NameID', $nameIdValue); + $nameId->setAttribute('Format', $formatUrn); $subject->appendChild($nameId); - $subjectConfirmation = $dom->createElement('saml:SubjectConfirmation'); + $subjectConfirmation = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:assertion', 'saml:SubjectConfirmation'); $subjectConfirmation->setAttribute('Method', 'urn:oasis:names:tc:SAML:2.0:cm:bearer'); - $confirmationData = $dom->createElement('saml:SubjectConfirmationData'); + $confirmationData = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:assertion', 'saml:SubjectConfirmationData'); $confirmationData->setAttribute('InResponseTo', $requestId); $confirmationData->setAttribute('NotOnOrAfter', $expireInstant); $confirmationData->setAttribute('Recipient', $acsUrl); @@ -336,12 +360,12 @@ private function buildConditions( 'not_on_or_after' => $expireInstant, ]); - $conditions = $dom->createElement('saml:Conditions'); + $conditions = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:assertion', 'saml:Conditions'); $conditions->setAttribute('NotBefore', $notBefore); $conditions->setAttribute('NotOnOrAfter', $expireInstant); - $audienceRestriction = $dom->createElement('saml:AudienceRestriction'); - $audienceRestriction->appendChild($dom->createElement('saml:Audience', $spEntityId)); + $audienceRestriction = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:assertion', 'saml:AudienceRestriction'); + $audienceRestriction->appendChild($dom->createElementNS('urn:oasis:names:tc:SAML:2.0:assertion', 'saml:Audience', $spEntityId)); $conditions->appendChild($audienceRestriction); $assertion->appendChild($conditions); @@ -364,13 +388,14 @@ private function buildAuthnStatement( 'authn_class' => 'urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport', ]); - $authnStatement = $dom->createElement('saml:AuthnStatement'); + $authnStatement = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:assertion', 'saml:AuthnStatement'); $authnStatement->setAttribute('AuthnInstant', $issueInstant); $authnStatement->setAttribute('SessionIndex', $assertionId); - $authnContext = $dom->createElement('saml:AuthnContext'); + $authnContext = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:assertion', 'saml:AuthnContext'); $authnContext->appendChild( - $dom->createElement( + $dom->createElementNS( + 'urn:oasis:names:tc:SAML:2.0:assertion', 'saml:AuthnContextClassRef', 'urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport' ) @@ -383,17 +408,34 @@ private function buildAuthnStatement( /** * Build the SAML Attribute Statement (Claims mapping). */ - private function buildAttributeStatement(DOMDocument $dom, DOMElement $assertion, User $user): void + private function buildAttributeStatement(DOMDocument $dom, DOMElement $assertion, User $user, ConnectedApp $app): void { - $statement = $dom->createElement('saml:AttributeStatement'); + $statement = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:assertion', 'saml:AttributeStatement'); - $claims = [ - 'IDPEmail' => $user->email, - 'mail' => $user->email, - 'displayName' => $user->name, - 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/upn' => $user->email, - 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress' => $user->email, - ]; + /** @var array $settings */ + $settings = (array) $app->settings; + $customAttributes = $settings['saml']['attributes'] ?? []; + + if (empty($customAttributes)) { + $claims = [ + 'IDPEmail' => $user->email, + 'mail' => $user->email, + 'displayName' => $user->name, + 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/upn' => $user->email, + 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress' => $user->email, + ]; + } else { + $claims = []; + foreach ($customAttributes as $samlName => $userField) { + $claims[$samlName] = match ($userField) { + 'email' => $user->email, + 'name' => $user->name, + 'id' => (string) $user->id, + 'immutable_id' => $user->immutable_id ?? base64_encode((string) $user->id), + default => null, + }; + } + } Log::debug('SamlIdpService: Constructing saml:AttributeStatement (claims mapping).', [ 'user_id' => $user->id, @@ -402,7 +444,9 @@ private function buildAttributeStatement(DOMDocument $dom, DOMElement $assertion ]); foreach ($claims as $name => $value) { - $this->addAttribute($dom, $statement, $name, (string) $value); + if (null !== $value) { + $this->addAttribute($dom, $statement, $name, (string) $value); + } } $assertion->appendChild($statement); @@ -420,15 +464,18 @@ private function addAttribute(DOMDocument $dom, DOMElement $statement, string $n 'value' => $value, ]); - $attribute = $dom->createElement('saml:Attribute'); + $attribute = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:assertion', 'saml:Attribute'); $attribute->setAttribute('Name', $name); $attribute->setAttribute('NameFormat', 'urn:oasis:names:tc:SAML:2.0:attrname-format:basic'); - $val = $dom->createElement('saml:AttributeValue', htmlspecialchars($value, ENT_QUOTES | ENT_XML1, 'UTF-8')); + $val = $dom->createElementNS('urn:oasis:names:tc:SAML:2.0:assertion', 'saml:AttributeValue', htmlspecialchars($value, ENT_QUOTES | ENT_XML1, 'UTF-8')); $attribute->appendChild($val); $statement->appendChild($attribute); } + /** + * Cryptographically sign the Assertion node in DOMDocument. + */ /** * Cryptographically sign the Assertion node in DOMDocument. */ @@ -444,7 +491,10 @@ private function signAssertion(DOMDocument $dom, DOMElement|DOMDocument $asserti Log::debug('SamlIdpService: Instantiating XMLSecurityDSig for Assertion.'); $objDSig = new XMLSecurityDSig(); - $objDSig->setCanonicalMethod(XMLSecurityDSig::C14N); + + // NATIVE FIX 1: Enforce Exclusive C14N everywhere. + // Inclusive C14N inherits parent namespaces and breaks the hash upon document import. + $objDSig->setCanonicalMethod(XMLSecurityDSig::EXC_C14N); $assertion->setIdAttribute('ID', true); Log::debug('SamlIdpService: Adding reference to XMLSignature.', [ @@ -457,39 +507,35 @@ private function signAssertion(DOMDocument $dom, DOMElement|DOMDocument $asserti 'http://www.w3.org/2000/09/xmldsig#enveloped-signature', 'http://www.w3.org/2001/10/xml-exc-c14n#', ], - ['id_name' => 'ID'] + ['id_name' => 'ID', 'overwrite' => false] ); Log::debug('SamlIdpService: Loading RSA-SHA256 private key.'); $objKey = new XMLSecurityKey(XMLSecurityKey::RSA_SHA256, ['type' => 'private']); $objKey->loadKey($privateKeyPem); - Log::debug('SamlIdpService: Computing signature value.'); + Log::debug('SamlIdpService: Appending and computing signature.'); + // NATIVE FIX 2: Pass $assertion as the second argument. + // This physically attaches the empty signature node to the DOM *first*, + // ensuring it inherits the exact final namespace context before hashing. $objDSig->sign($objKey, $assertion); Log::debug('SamlIdpService: Appending X509 certificate to signature.'); $objDSig->add509Cert($certPem); - // Enforce SAML Schema order compliance: Signature node must precede Subject. - Log::debug('SamlIdpService: Ordering DOM nodes. Moving Signature before Subject to satisfy SAML XSD schema.'); - $signatureNode = $assertion->getElementsByTagNameNS( - 'http://www.w3.org/2000/09/xmldsig#', - 'Signature' - )->item(0); - $subjectNode = $assertion->getElementsByTagNameNS( - 'urn:oasis:names:tc:SAML:2.0:assertion', - 'Subject' - )->item(0); + Log::debug('SamlIdpService: Moving signature to valid XSD position.'); + // NATIVE FIX 3: Reorder the DOM. + // Because this is an Enveloped signature, shifting the node inside the same parent + // has absolutely zero effect on the computed hash. + $subjectNode = $assertion->getElementsByTagNameNS('urn:oasis:names:tc:SAML:2.0:assertion', 'Subject')->item(0); - if ($signatureNode && $subjectNode) { - $assertion->insertBefore($signatureNode, $subjectNode); + if ($subjectNode) { + $assertion->insertBefore($objDSig->sigNode, $subjectNode); Log::debug('SamlIdpService: Re-ordered nodes: Signature is now placed before Subject.'); } else { - Log::warning('SamlIdpService: Failed to re-order nodes; Signature or Subject node not found.', [ - 'signature_found' => ! empty($signatureNode), - 'subject_found' => ! empty($subjectNode), - ]); + Log::warning('SamlIdpService: Subject node not found; leaving signature appended at end.'); } + } catch (Exception $e) { Log::error('SamlIdpService: Cryptographic signing failed.', [ 'message' => $e->getMessage(), diff --git a/tests/Unit/SamlIdpServiceTest.php b/tests/Unit/SamlIdpServiceTest.php index 7efa010..dec47fd 100644 --- a/tests/Unit/SamlIdpServiceTest.php +++ b/tests/Unit/SamlIdpServiceTest.php @@ -4,12 +4,17 @@ namespace Tests\Unit; -uses(\Tests\TestCase::class, \Illuminate\Foundation\Testing\RefreshDatabase::class); - +use App\Enums\{SamlNameIdFormatEnum, SamlNameIdSourceEnum}; use App\Models\{ConnectedApp, ConnectionProtocol, ConnectionProvider, ConnectionStatus, User}; use App\Services\SamlIdpService; +use Database\Seeders\DatabaseSeeder; +use DOMDocument; use Exception; +use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\File; +use Tests\TestCase; + +uses(TestCase::class, RefreshDatabase::class); beforeEach(function (): void { $this->service = new SamlIdpService(); @@ -23,7 +28,7 @@ $this->artisan('saml:generate-keys'); } - $this->seed(\Database\Seeders\DatabaseSeeder::class); + $this->seed(DatabaseSeeder::class); $this->protocol = ConnectionProtocol::query()->where('name', 'saml')->firstOrFail(); $this->provider = ConnectionProvider::query()->where('slug', 'azure-fd')->firstOrFail(); @@ -39,6 +44,8 @@ 'saml' => [ 'entity_id' => 'urn:federation:TestApp', 'acs_url' => 'https://test-app.com/saml/acs', + 'nameid_format' => mb_strtolower(SamlNameIdFormatEnum::Persistent->name), + 'nameid_source' => SamlNameIdSourceEnum::ImmutableId->value, ], ], ]); @@ -120,3 +127,41 @@ ->toContain('user-unit-test-123') // NameID (Immutable ID) ->toContain('unituser@sso.local'); // User email in claims }); + +test('generated SAML Response signature can be cryptographically verified', function (): void { + $samlResponseBase64 = $this->service->generateResponse( + $this->user, + $this->connectedApp, + '_req_id_123', + 'https://test-app.com/saml/acs' + ); + + $xml = base64_decode($samlResponseBase64, true); + + $doc = new DOMDocument(); + $doc->preserveWhiteSpace = true; + $doc->loadXML($xml); + + $xmlSignature = new \RobRichards\XMLSecLibs\XMLSecurityDSig(); + $xmlSignature->idKeys = ['ID']; + + // Locate signature node in the document + $sigNode = $xmlSignature->locateSignature($doc); + expect($sigNode)->not->toBeNull(); + + // Validate reference + $xmlSignature->canonicalizeSignedInfo(); + $refValidated = $xmlSignature->validateReference(); + expect($refValidated)->toBeTrue(); + + // Locate the key and load the public key + $publicKey = $xmlSignature->locateKey(); + expect($publicKey)->not->toBeNull(); + + $certPem = File::get(storage_path('app/saml/idp.crt')); + $publicKey->loadKey($certPem, false); + + // Verify signature + $result = $xmlSignature->verify($publicKey); + expect($result)->toBe(1); +});