WIP: Improve SAML response generation and attribute mapping

- Refactored `SamlIdpService` to enhance SAML NameID handling, adding support for configurable formats and sources with `SamlNameIdFormatEnum`. - Enabled custom attribute mapping with dynamic claims resolution, supporting app-level configurations. - Standardized SAML namespace usage across all element creation methods to ensure compliance with SAML 2.0 specifications. - Improved cryptographic signature handling with Exclusive Canonicalization (C14N) and proper DOM node positioning. - Added new unit tests to verify SAML responses and cryptographic signature validation.
2026-06-08 13:20:01 +00:00 · 2026-06-08 13:20:01 +00:00 · 2140c12b10
commit 2140c12b10
parent aaa258903c
2 changed files with 146 additions and 55 deletions
--- 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<string, mixed> $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<string, mixed> $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(),
--- 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);
+});