fix(sso): verify the SAML key pair and keep the private key out of logs

The registration log emitted the resolved provider config with only the client secret and IdP certificate redacted, so enabling encrypted assertions wrote the service provider's private key to a log line. It is redacted now, with a test that fails if it ever reaches one again.

Both halves of the encryption pair are also parsed and compared rather than checked for PEM markers: a valid certificate paired with a valid key from a different pair used to save cleanly and then fail every sign-in. The tests generate real key material per run instead of committing any.

Author: waleedlatif1

apps/sim/app/api/auth/sso/register/route.test.ts

@@ -1,6 +1,11 @@
 /**
  * @vitest-environment node
  */
+
+import { execFileSync } from 'node:child_process'
+import { mkdtempSync, readFileSync, rmSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import path from 'node:path'
 import {
   createMockRequest,
   dbChainMock,
@@ -13,6 +18,7 @@ import {
   setEnv,
   setEnvFlags,
 } from '@sim/testing'
+import { loggerMock } from '@sim/testing/mocks/logger.mock'
 import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
 
 const {
@@ -91,6 +97,17 @@ vi.mock('@/lib/core/security/input-validation.server', () => ({
 
 import { POST } from '@/app/api/auth/sso/register/route'
 
+type MockLogger = { info: { mock: { calls: unknown[][] } } }
+
+/** The logger the route built at import time, so its calls can be inspected. */
+const routeLogger = loggerMock.createLogger.mock.calls.reduce<MockLogger | null>(
+  (found, call, index) =>
+    call[0] === 'SSORegisterRoute'
+      ? (loggerMock.createLogger.mock.results[index].value as MockLogger)
+      : found,
+  null
+)
+
 const OIDC_BODY = {
   providerType: 'oidc' as const,
   providerId: 'acme-oidc',
@@ -364,8 +381,40 @@ describe('POST /api/auth/sso/register', () => {
   })
 
   describe('SAML encrypted assertions', () => {
-    const SP_CERT = `-----BEGIN CERTIFICATE-----\nQUJD\n-----END CERTIFICATE-----`
-    const SP_KEY = `-----BEGIN PRIVATE KEY-----\nREVG\n-----END PRIVATE KEY-----`
+    /**
+     * Real key material, because the route parses both halves and checks they
+     * belong together. Generated per run rather than committed: a private key
+     * in the repository is exactly what this PR is about not doing.
+     */
+    const keyPair = (subject: string) => {
+      const dir = mkdtempSync(path.join(tmpdir(), 'sim-sso-keys-'))
+      execFileSync('openssl', [
+        'req',
+        '-x509',
+        '-newkey',
+        'rsa:2048',
+        '-keyout',
+        path.join(dir, 'key.pem'),
+        '-out',
+        path.join(dir, 'cert.pem'),
+        '-days',
+        '2',
+        '-nodes',
+        '-subj',
+        `/CN=${subject}`,
+      ])
+      const pair = {
+        cert: readFileSync(path.join(dir, 'cert.pem'), 'utf8'),
+        key: readFileSync(path.join(dir, 'key.pem'), 'utf8'),
+      }
+      rmSync(dir, { recursive: true, force: true })
+      return pair
+    }
+
+    const SP = keyPair('sim-test-sp')
+    const OTHER = keyPair('sim-test-other')
+    const SP_CERT = SP.cert
+    const SP_KEY = SP.key
     const samlBody = (overrides: Record<string, unknown> = {}) => ({
       providerType: 'saml' as const,
       providerId: 'acme-saml',
@@ -396,9 +445,29 @@ describe('POST /api/auth/sso/register', () => {
       })
       /** The certificate travels in the metadata document, stripped of its PEM armor. */
       expect(samlConfig.spMetadata.metadata).toContain('use="encryption"')
-      expect(samlConfig.spMetadata.metadata).toContain('QUJD')
+      expect(samlConfig.spMetadata.metadata).toContain(
+        SP_CERT.replace(/-----(BEGIN|END) CERTIFICATE-----/g, '').replace(/\s+/g, '')
+      )
       expect(samlConfig.spMetadata.metadata).not.toContain('BEGIN CERTIFICATE')
-      expect(samlConfig.spMetadata.metadata).not.toContain('REVG')
+      expect(samlConfig.spMetadata.metadata).not.toContain('PRIVATE KEY')
+    })
+
+    it('never writes the private key to a log line', async () => {
+      queueMembers([{ organizationId: 'org1', role: 'owner' }])
+      queueProviders([])
+
+      await POST(
+        request(
+          samlBody({ encryptAssertions: true, spEncryptionCert: SP_CERT, spDecryptionKey: SP_KEY })
+        )
+      )
+
+      /** The route logs its resolved provider config; the key must be redacted there. */
+      const logged = (routeLogger?.info.mock.calls ?? [])
+        .map((call) => JSON.stringify(call))
+        .join('\n')
+      expect(logged).not.toContain('PRIVATE KEY')
+      expect(logged).toContain('[REDACTED]')
     })
 
     it('leaves the metadata and key material alone when encryption is off', async () => {
@@ -441,7 +510,9 @@ describe('POST /api/auth/sso/register', () => {
       const res = await POST(request(samlBody({ encryptAssertions: true, ...overrides })))
 
       expect(res.status).toBe(400)
-      await expect(res.json()).resolves.toMatchObject({ error: expect.stringContaining('PEM') })
+      await expect(res.json()).resolves.toMatchObject({
+        error: expect.stringMatching(/PEM|matching pair/),
+      })
       expect(mockRegisterSSOProvider).not.toHaveBeenCalled()
     })
 

apps/sim/app/api/auth/sso/register/route.ts

@@ -1,3 +1,4 @@
+import { createPrivateKey, createPublicKey } from 'node:crypto'
 import { db, member, ssoDomain, ssoProvider } from '@sim/db'
 import { keepDomainSignInProvider, ssoProviderDomainKey } from '@sim/db/sso-primary-provider'
 import { createLogger } from '@sim/logger'
@@ -92,23 +93,38 @@ function stripPemArmor(pem: string): string {
     .trim()
 }
 
+/** The SubjectPublicKeyInfo of a certificate or private key, for comparing the two. */
+function publicKeyOf(pem: string, kind: 'certificate' | 'private key'): string {
+  const key = kind === 'certificate' ? createPublicKey(pem) : createPublicKey(createPrivateKey(pem))
+  return key.export({ type: 'spki', format: 'pem' }).toString()
+}
+
 /**
- * Names the first problem with an encryption key pair, or null when both look
- * like PEM documents of the right kind. Catching it here turns what would
- * otherwise be a failed sign-in weeks later into an error on the save.
+ * Names the first problem with an encryption key pair, or null when both parse
+ * and belong together. A mismatched pair is the failure worth catching here:
+ * each half is individually valid, so nothing complains until the identity
+ * provider encrypts an assertion Sim cannot read, weeks later at sign-in.
  */
-function describePemProblem(
+function describeKeyPairProblem(
   cert: string | undefined,
   privateKey: string | undefined
 ): string | null {
-  if (!cert?.includes('BEGIN CERTIFICATE')) {
-    return 'Service provider certificate must be a PEM certificate beginning with -----BEGIN CERTIFICATE-----'
+  let certificatePublicKey: string
+  try {
+    certificatePublicKey = publicKeyOf(cert ?? '', 'certificate')
+  } catch {
+    return 'Service provider certificate must be a PEM X.509 certificate beginning with -----BEGIN CERTIFICATE-----'
   }
-  if (!privateKey?.includes('PRIVATE KEY')) {
+
+  let privateKeyPublicKey: string
+  try {
+    privateKeyPublicKey = publicKeyOf(privateKey ?? '', 'private key')
+  } catch {
     return 'Service provider private key must be a PEM private key beginning with -----BEGIN PRIVATE KEY-----'
   }
-  if (!stripPemArmor(cert) || !stripPemArmor(privateKey)) {
-    return 'Service provider certificate and private key cannot be empty'
+
+  if (certificatePublicKey !== privateKeyPublicKey) {
+    return 'Service provider certificate and private key are not a matching pair'
   }
   return null
 }
@@ -599,8 +615,8 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
       }
 
       if (encryptAssertions) {
-        const pemProblem = describePemProblem(spEncryptionCert, decryptionKey)
-        if (pemProblem) return NextResponse.json({ error: pemProblem }, { status: 400 })
+        const keyPairProblem = describeKeyPairProblem(spEncryptionCert, decryptionKey)
+        if (keyPairProblem) return NextResponse.json({ error: keyPairProblem }, { status: 400 })
       }
 
       const computedCallbackUrl =
@@ -711,6 +727,15 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
             ? {
                 ...providerConfig.samlConfig,
                 cert: REDACTED_MARKER,
+                /** The service provider's own private key never reaches a log line. */
+                ...(providerConfig.samlConfig.spMetadata?.encPrivateKey
+                  ? {
+                      spMetadata: {
+                        ...providerConfig.samlConfig.spMetadata,
+                        encPrivateKey: REDACTED_MARKER,
+                      },
+                    }
+                  : {}),
               }
             : undefined,
         },