Skip to content

fix: explicitly look up MDM client identity for TLS auth challenges - #2732

Closed
ConnorCallison wants to merge 1 commit into
manaflow-ai:mainfrom
ConnorCallison:fix/device-policy-auth-keychain-lookup
Closed

ConnorCallison wants to merge 1 commit into
manaflow-ai:mainfrom
ConnorCallison:fix/device-policy-auth-keychain-lookup

Conversation

@ConnorCallison

@ConnorCallison ConnorCallison commented Apr 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Replaces blanket .performDefaultHandling with explicit SecIdentityCopyPreferred keychain lookup for NSURLAuthenticationMethodClientCertificate challenges
  • Fixes Microsoft Entra ID "this device needs to be under policy" error in browser pane on MDM-enrolled Macs
  • Applied to both BrowserNavigationDelegate and PopupNavigationDelegate for parity

Problem

PR #806 added webView(_:didReceive:completionHandler:) returning .performDefaultHandling to fix Microsoft managed device authentication. While this works for server trust evaluation (validating MDM-installed root CAs), it does not trigger the system keychain lookup needed for client certificate challenges in WKWebView.

Safari's web content process has special entitlements that allow .performDefaultHandling to automatically search the keychain for matching client identities. Third-party apps' WKWebViews lack these entitlements, so .performDefaultHandling for NSURLAuthenticationMethodClientCertificate effectively sends no certificate — making it functionally identical to the pre-#806 behavior for this specific challenge type.

Fix

When a NSURLAuthenticationMethodClientCertificate challenge arrives, explicitly call SecIdentityCopyPreferred(_:_:_:) with the server's host and acceptable CA distinguished names. This runs in the app process (which has keychain access) and returns the MDM device identity certificate. The credential is then provided to WebKit via .useCredential.

  • Non-MDM machines are unaffected — SecIdentityCopyPreferred returns nil when no matching identity exists, and the fallback to .performDefaultHandling preserves all other authentication flows (server trust, NTLM, Kerberos, SSO extensions, etc.)

Test plan

  • On an MDM-enrolled Mac, open cmux browser and navigate to github.com (or any Microsoft Entra ID-protected SSO)
  • Sign in with corporate credentials — should no longer show "this device needs to be under policy"
  • Verify regular HTTPS sites still load normally
  • Verify non-MDM Macs are unaffected (graceful nil fallback)
  • Verify popup windows (OAuth flows) also handle client cert challenges

🤖 Generated with Claude Code


Summary by cubic

Explicitly resolve the MDM client certificate from the system keychain for TLS client‑cert challenges in WKWebView, fixing Microsoft Entra ID “this device needs to be under policy” errors. Applied to both the main browser and popup windows; other auth flows remain unchanged.

  • Bug Fixes
    • For NSURLAuthenticationMethodClientCertificate, use SecIdentityCopyPreferred with the host and acceptable CA DNs, then pass the identity via .useCredential.
    • Fall back to .performDefaultHandling when no identity is found, preserving server trust, NTLM/Kerberos, and SSO behavior across BrowserNavigationDelegate and PopupNavigationDelegate.

Written for commit 9c35c8d. Summary will update on new commits.

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced client certificate authentication handling in browser panels and popup windows.
    • Implemented intelligent certificate selection based on host and issuer requirements when accessing sites that require client certificates.

WKWebView's .performDefaultHandling does not search the system keychain
for client identities the way Safari does — Safari's web content process
has special entitlements that third-party apps lack. On MDM-enrolled
Macs, Microsoft Entra ID (Conditional Access) issues a TLS client-
certificate challenge to verify device compliance. The previous fix
(manaflow-ai#806) returned .performDefaultHandling, which works for server trust
evaluation but does not trigger the keychain lookup needed for client
certificate challenges.

Use SecIdentityCopyPreferred to explicitly find the preferred client
identity matching the server's host and acceptable CA distinguished
names. This is the same lookup Safari performs internally. Non-MDM
machines are unaffected — SecIdentityCopyPreferred returns nil when
no matching identity exists, and the fallback to .performDefaultHandling
preserves all other authentication flows.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Apr 8, 2026

Copy link
Copy Markdown

@ConnorCallison is attempting to deploy a commit to the Manaflow Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Apr 8, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1ea4b6f9-4e16-4417-8bbe-16a4b216335f

📥 Commits

Reviewing files that changed from the base of the PR and between 1d81b74 and 9c35c8d.

📒 Files selected for processing (2)
  • Sources/Panels/BrowserPanel.swift
  • Sources/Panels/BrowserPopupWindowController.swift

📝 Walkthrough

Walkthrough

The changes enhance client-certificate authentication handling in two browser navigation delegate implementations. Rather than uniformly deferring to WebKit's default challenge handling, the code now attempts to locate a matching client identity from the system keychain using SecIdentityCopyPreferred before falling back to default behavior if no identity is found.

Changes

Cohort / File(s) Summary
Client-Certificate Authentication
Sources/Panels/BrowserPanel.swift, Sources/Panels/BrowserPopupWindowController.swift
Added in-process identity selection for NSURLAuthenticationMethodClientCertificate challenges via SecIdentityCopyPreferred. Scoped by host and acceptable CA distinguished names. Falls back to default handling if lookup fails. Introduced import Security in popup controller.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 A rabbit hops through keychains deep,
Finding certificates we need to keep,
No more defaults when certs align,
Just the right identity, by design! 🔐
Secure the web with a hop and a bound!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: adding explicit MDM client identity lookup for TLS authentication challenges, which is the core purpose of the changeset.
Description check ✅ Passed The description provides comprehensive context including problem statement, implemented fix, and test plan, though the testing checklist items are not checked off and some manual testing details are incomplete.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps

greptile-apps Bot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR replaces the blanket .performDefaultHandling response for TLS authentication challenges with an explicit SecIdentityCopyPreferred keychain lookup for NSURLAuthenticationMethodClientCertificate challenges, applied identically in both BrowserNavigationDelegate and PopupNavigationDelegate. The fix correctly addresses the fundamental entitlement gap between Safari's web content process and third-party WKWebView contexts on MDM-enrolled Macs.

Confidence Score: 5/5

Safe to merge — targeted, correct fix with no regressions on the non-MDM path.

All changes are P2 or lower. The API usage is correct: SecIdentityCopyPreferred parameters match the expected types, nil certificates in URLCredential is the documented pattern for identity-only credentials, Security is properly imported in both files, and the fallback preserves existing behavior for all other challenge types.

No files require special attention.

Vulnerabilities

No security concerns identified. The explicit keychain lookup scopes the identity search to the challenged host and the server-supplied issuer list, avoiding over-broad certificate selection. Credentials are scoped to .forSession, which is appropriate. The fallback to .performDefaultHandling on nil correctly preserves all other auth flows without granting unintended access.

Important Files Changed

Filename Overview
Sources/Panels/BrowserPanel.swift Replaces performDefaultHandling with explicit SecIdentityCopyPreferred lookup for client certificate challenges; Security framework was already imported; fallback path preserved.
Sources/Panels/BrowserPopupWindowController.swift Parity update matching BrowserPanel; adds missing import Security and applies the same SecIdentityCopyPreferred pattern correctly.

Sequence Diagram

sequenceDiagram
    participant Server as MDM-Protected Server
    participant WKWebView as WKWebView
    participant Delegate as NavigationDelegate
    participant Keychain as System Keychain

    Server->>WKWebView: TLS ClientCertificate challenge
    WKWebView->>Delegate: didReceive challenge
    alt authMethod == ClientCertificate
        Delegate->>Keychain: SecIdentityCopyPreferred(host, nil, issuers)
        alt Identity found
            Keychain-->>Delegate: SecIdentity
            Delegate->>WKWebView: .useCredential(identity)
            WKWebView->>Server: Client certificate presented
            Server-->>WKWebView: Auth success (device compliant)
        else No matching identity (non-MDM)
            Keychain-->>Delegate: nil
            Delegate->>WKWebView: .performDefaultHandling
        end
    else Other challenge (serverTrust, NTLM, SSO)
        Delegate->>WKWebView: .performDefaultHandling
    end
Loading

Reviews (1): Last reviewed commit: "fix: explicitly look up MDM client ident..." | Re-trigger Greptile

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

@teamleaderleo

Copy link
Copy Markdown
Collaborator

Thanks for this! Browser client-certificate sign-in (with a certificate picker) landed on main in #7040. You opened this first, so you got there first. Closing since main covers it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants