feat: webauthn and PAM session mfa #4896

sheensantoscapadngan · 2025-11-18T18:03:01Z

Description 📣

This PR implements WebAuthn (passkey) support and adds MFA session verification for PAM account access. The implementation adds WebAuthn as a third MFA method alongside email and TOTP, with proper challenge-based verification and replay protection. A new MFA session flow allows users to verify their identity before accessing sensitive PAM accounts.

Type ✨

Tests 🛠️

# Here's some code block to paste some code snippets

I have read the contributing guide, agreed and acknowledged the code of conduct. 📝

maidul98 · 2025-11-18T18:03:49Z

✅ Snyk checks have passed. No issues have been found so far.

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

…ion-mfa

gitguardian · 2025-11-20T17:18:43Z

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
22397427	Triggered	Generic Private Key	`382bd1a`	backend/bdd/pebble/localhost/key.pem	View secret
22397428	Triggered	Generic Private Key	`382bd1a`	backend/bdd/pebble/pebble.minica.key.pem	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

greptile-apps · 2025-11-21T14:25:56Z

Greptile Overview

Greptile Summary

This PR implements WebAuthn (passkey) support and adds MFA session verification for PAM account access. The implementation adds WebAuthn as a third MFA method alongside email and TOTP, with proper challenge-based verification and replay protection. A new MFA session flow allows users to verify their identity before accessing sensitive PAM accounts.

Key changes:

Added WebAuthn credential storage with proper public key cryptography and counter-based replay protection
Implemented MFA session service with Redis-backed temporary sessions (5-minute TTL)
Added requireMfa field to PAM accounts to optionally enforce MFA before granting access
Created dedicated MFA session verification page for out-of-band MFA flows
WebAuthn challenges stored in Redis with 5-minute expiration

Critical security issues found:

MFA bypass vulnerability in PAM account access logic - accounts with requireMfa=false allow access without any MFA verification
Migration defaults requireMfa to false, leaving all existing PAM accounts unprotected until manually enabled
MFA method selection creates email MFA sessions even when users have no MFA configured

Confidence Score: 2/5

This PR has critical security vulnerabilities in the MFA enforcement logic that could allow unauthorized PAM account access
The MFA bypass vulnerability in pam-account-service.ts allows attackers to access PAM accounts without MFA verification when requireMfa=false. Since the migration defaults this to false for all existing accounts, this creates immediate security exposure. The WebAuthn implementation itself is solid, but the integration with PAM access control has fundamental flaws in the security model.
Pay critical attention to backend/src/ee/services/pam-account/pam-account-service.ts (MFA bypass logic) and backend/src/db/migrations/20251119124200_add-require-mfa-to-pam-account.ts (insecure defaults)

Important Files Changed

File Analysis

Filename	Score	Overview
backend/src/services/webauthn/webauthn-service.ts	4/5	WebAuthn service implementation with proper challenge validation, credential storage, and counter-based replay protection
backend/src/services/mfa-session/mfa-session-service.ts	4/5	MFA session service with proper user verification and session ownership checks
backend/src/ee/services/pam-account/pam-account-service.ts	3/5	PAM account service with MFA session support - contains potential session bypass and unclear MFA enforcement logic
backend/src/db/migrations/20251118190904_add-webauthn-support.ts	5/5	WebAuthn credentials table migration with proper foreign keys and unique constraints
backend/src/db/migrations/20251119124200_add-require-mfa-to-pam-account.ts	5/5	Added requireMfa boolean column to PAM accounts, defaults to false
backend/src/server/routes/v2/mfa-session-router.ts	5/5	MFA session routes with proper authentication and rate limiting

greptile-apps

_{62 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

backend/src/ee/services/pam-account/pam-account-service.ts

backend/src/db/migrations/20251119124200_add-require-mfa-to-pam-account.ts

backend/src/ee/services/pam-account/pam-account-service.ts

backend/src/server/routes/v1/user-router.ts

…ion-mfa

x032205 · 2025-11-27T02:30:00Z

backend/src/ee/services/pam-account/pam-account-service.ts

+      // Determine which MFA method to use
+      // Priority: org-enforced > user-selected > email as fallback
+      const orgMfaMethod = org.enforceMfa
+        ? ((org.selectedMfaMethod as MfaMethod | null) ?? MfaMethod.EMAIL)
+        : undefined;
+      const userMfaMethod = actorUser.isMfaEnabled
+        ? ((actorUser.selectedMfaMethod as MfaMethod | null) ?? MfaMethod.EMAIL)
+        : undefined;
+      const mfaMethod = (orgMfaMethod ?? userMfaMethod ?? MfaMethod.EMAIL) as MfaMethod;


Pretty sure this logic is flawed. The line:

((org.selectedMfaMethod as MfaMethod | null) ?? MfaMethod.EMAIL)

will make it so that orgMfaMethod becomes MfaMethod.EMAIL even if org.selectedMfaMethod is null rather than falling back to userMfaMethod. Should probably be ((org.selectedMfaMethod as MfaMethod | null) ?? undefined) or something

x032205 · 2025-11-27T02:37:53Z

backend/src/ee/services/pam-account/pam-account-service.ts

+      // Verify the session is for the same account
+      if (mfaSession.resourceId !== accountId) {
+        throw new BadRequestError({
+          message: "MFA session is for a different resource"


Maybe the error should say its for a different account?

x032205 · 2025-11-27T03:01:33Z

backend/src/server/routes/v1/user-router.ts

+    handler: async (req) => {
+      return server.services.webAuthn.verifyRegistrationResponse({
+        userId: req.permission.id,
+        registrationResponse: req.body.registrationResponse as unknown as RegistrationResponseJSON,


Pretty sure you don't need this type assertion if you make clientExtensionResults have a default like:

clientExtensionResults: z.record(z.unknown()).default({})

x032205 · 2025-11-27T03:02:02Z

backend/src/server/routes/v1/user-router.ts

+    handler: async (req) => {
+      return server.services.webAuthn.verifyAuthenticationResponse({
+        userId: req.permission.id,
+        authenticationResponse: req.body.authenticationResponse as unknown as AuthenticationResponseJSON


Same as above

x032205 · 2025-11-29T05:04:50Z

frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/RotateAccountFields.tsx

This file should not have been edited. rotationCredentialsConfigured should still be passed, otherwise the tooltip functionality is useless. The idea is to grey it out if it's not configured

x032205 · 2025-11-29T05:05:14Z

frontend/src/pages/pam/PamAccountsPage/components/PamAccountForm/PostgresAccountForm.tsx

        <GenericAccountFields />
        <SqlAccountFields isUpdate={isUpdate} />
-        <RotateAccountFields rotationCredentialsConfigured={rotationCredentialsConfigured} />
+        {rotationCredentialsConfigured && <RotateAccountFields />}


rotationCredentialsConfigured should stay as a parameter. We just want to grey out the option rather than fully hide it.

x032205 · 2025-12-09T22:57:46Z

backend/src/services/mfa-session/mfa-session-service.ts

+        message: "MFA session not found or expired"
+      });
+    }
+


Maybe adding a check like if (mfaMethod !== mfaSession.mfaMethod) ... may be beneficial to prevent possible exploit attempts?

x032205 · 2025-12-09T22:59:44Z

backend/src/services/webauthn/webauthn-fns.ts

+export const verifyCredentialOwnership = (userId: string, credentialUserId: string): boolean => {
+  return userId === credentialUserId;
+};


This function feels a little unnecessary... maybe we just use an inline === check?

x032205 · 2025-12-09T23:12:35Z

backend/src/services/webauthn/webauthn-service.ts

+        transports: cred.transports as AuthenticatorTransportFuture[]
+      })),
+      authenticatorSelection: {
+        authenticatorAttachment: "platform",


Is using "platform" here the best choice? From my tiny bit of research I think this only works for platform-based methods like windows hello, touch id, etc. And not stuff like YubiKeys.

x032205 · 2025-12-09T23:32:35Z

frontend/src/hooks/api/mfaSession/queries.tsx

+export enum MfaSessionStatus {
+  PENDING = "PENDING",
+  ACTIVE = "ACTIVE"
+}
+
+export type TMfaSessionStatusResponse = {
+  status: MfaSessionStatus;
+  mfaMethod: MfaMethod;
+};
+
+export type TVerifyMfaSessionRequest = {
+  mfaSessionId: string;
+  mfaToken: string;
+  mfaMethod: MfaMethod;
+};
+
+export type TVerifyMfaSessionResponse = {
+  success: boolean;
+  message: string;
+};


Maybe it'd be better to put the enum and types into dedicated file, and not queries.tsx? Maybe into types or helper file

feat: webauthn support

7fbea4d

sheensantoscapadngan added 7 commits November 19, 2025 03:11

misc: minor connections

c0a5562

misc: updated qc clear to invalidate

c69f070

feat: add pam session gating

019a571

feat: add require mfa to pam account

6061c8f

misc: rename

7ffa4a9

Merge remote-tracking branch 'origin/main' into feat/webauth-and-sess…

382bd1a

…ion-mfa

misc: delete duplicate files

765d578

misc: addressed feedback

08fb784

sheensantoscapadngan changed the title ~~Feat/webauth and session mfa~~ feat: webauthn and PAM session mfa Nov 20, 2025

sheensantoscapadngan marked this pull request as ready for review November 21, 2025 14:21

greptile-apps bot reviewed Nov 21, 2025

View reviewed changes

backend/src/ee/services/pam-account/pam-account-service.ts Outdated Show resolved Hide resolved

backend/src/db/migrations/20251119124200_add-require-mfa-to-pam-account.ts Show resolved Hide resolved

backend/src/ee/services/pam-account/pam-account-service.ts Show resolved Hide resolved

sheensantoscapadngan commented Nov 21, 2025

View reviewed changes

backend/src/server/routes/v1/user-router.ts Show resolved Hide resolved

sheensantoscapadngan added 3 commits November 21, 2025 23:21

misc: address self-review

15e5398

Merge remote-tracking branch 'origin/main' into feat/webauth-and-sess…

4f7a460

…ion-mfa

Merge remote-tracking branch 'origin/main' into feat/webauth-and-sess…

ee0f51b

…ion-mfa

x032205 requested changes Dec 9, 2025

View reviewed changes

feat: webauthn and PAM session mfa #4896

Are you sure you want to change the base?

feat: webauthn and PAM session mfa #4896

Conversation

sheensantoscapadngan commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📣

Type ✨

Tests 🛠️

Uh oh!

maidul98 commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Snyk checks have passed. No issues have been found so far.

Uh oh!

gitguardian bot commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Uh oh!

greptile-apps bot commented Nov 21, 2025

Greptile Overview

Greptile Summary

Confidence Score: 2/5

Important Files Changed

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sheensantoscapadngan commented Nov 18, 2025 •

edited

Loading

maidul98 commented Nov 18, 2025 •

edited

Loading

gitguardian bot commented Nov 20, 2025 •

edited

Loading