broker: support Entra passwordless and FIDO/security-key MFA - #1723
broker: support Entra passwordless and FIDO/security-key MFA #1723nooreldeenmansour wants to merge 11 commits into
Conversation
37fa20c to
c3b3c69
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1723 +/- ##
==========================================
+ Coverage 84.97% 85.85% +0.88%
==========================================
Files 25 26 +1
Lines 1943 2107 +164
==========================================
+ Hits 1651 1809 +158
- Misses 292 298 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR extends the Microsoft Entra ID broker to support passwordless authentication (probing passwordless methods first and only prompting for a password when required) and adds local security-key (FIDO/WebAuthn) MFA completion using libfido2, integrating new follow-up auth modes into the broker flow.
Changes:
- Add passwordless-first initiation for
entra_password, with explicit fallback to password+MFA when Entra reportsPASSWORD_REQUIRED. - Implement local FIDO/WebAuthn MFA completion (
entra_mfa_fido,entra_mfa_fido_pin) using a newinternal/fidopackage and enhanced libhimmelblau challenge metadata. - Update build/CI/snap packaging to include libfido2 headers/runtime and add extensive broker/unit test coverage for the new flows.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| snap/variants/msentraid/snapcraft.yaml | Adds snap interface + libfido2 build/runtime dependencies for security-key MFA. |
| authd-oidc-brokers/internal/providers/msentraid/msentraid.go | Plumbs optional auth options through provider initiation. |
| authd-oidc-brokers/internal/providers/msentraid/himmelblau/himmelblau.go | Adds passwordless constraints and extracts richer MFA challenge metadata (incl. FIDO). |
| authd-oidc-brokers/internal/providers/msentraid/himmelblau/himmelblau_c.go | Adds C-option mapping and exposes FIDO challenge/allow-list accessors. |
| authd-oidc-brokers/internal/providers/msentraid/himmelblau/himmelblau_c_test.go | Tests new MSAL error mapping and C-auth-option translation. |
| authd-oidc-brokers/internal/providers/msentraid/himmelblau/generate.sh | Uses cargo install --locked for reproducible tool installs. |
| authd-oidc-brokers/internal/providers/msentraid/himmelblau/entrapwd.go | Defines AuthOption + enriches MFAChallengeInfo (FIDO fields) and error classification. |
| authd-oidc-brokers/internal/fido/authenticator.go | Implements libfido2-backed authenticator and assertion ceremony handling. |
| authd-oidc-brokers/internal/fido/authenticator_test.go | Unit tests for mapping libfido2 errors to broker-actionable sentinels. |
| authd-oidc-brokers/internal/fido/assertion.go | Builds Entra-specific assertion JSON and helper decoding/parsing utilities. |
| authd-oidc-brokers/internal/fido/assertion_test.go | Unit tests for assertion JSON, allow-list decoding, and CBOR authData unwrap. |
| authd-oidc-brokers/internal/broker/options_test.go | Exposes test-only option hook to inject a mock FIDO authenticator. |
| authd-oidc-brokers/internal/broker/helper_test.go | Enables FIDO authenticator injection in broker test harness. |
| authd-oidc-brokers/internal/broker/fido_stub.go | Disables FIDO modes in non-withmsentraid builds. |
| authd-oidc-brokers/internal/broker/fido_msentraid.go | Enables default libfido2 authenticator in withmsentraid builds. |
| authd-oidc-brokers/internal/broker/export_test.go | Test hook to shorten the “wait for key insertion” timeout. |
| authd-oidc-brokers/internal/broker/broker.go | Implements passwordless probing, FIDO routing, and new FIDO follow-up modes. |
| authd-oidc-brokers/internal/broker/broker_test.go | Adds comprehensive tests for passwordless + FIDO routing and edge cases. |
| authd-oidc-brokers/internal/broker/authmodes/consts.go | Adds new auth mode IDs/labels for security-key MFA. |
| authd-oidc-brokers/go.sum | Adds go-libfido2 and transitive dependency checksums. |
| authd-oidc-brokers/go.mod | Adds go-libfido2 dependency. |
| authd-oidc-brokers/conf/variants/msentraid/broker.conf | Documents passwordless negotiation behavior for entra_password. |
| .gitmodules | Updates libhimmelblau submodule tracking configuration. |
| .github/workflows/tics-run.yaml | Installs libfido2 headers in TICS workflow environment. |
| .github/workflows/brokers-qa.yaml | Installs libfido2 headers for broker QA/coverage workflows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| NewPassword = "newpassword" | ||
|
|
||
| // EntraPassword is the ID of the Entra ID password + MFA authentication method. | ||
| // EntraPassword is the ID of the Entra ID password/passwordless MFA method. |
There was a problem hiding this comment.
Can we add an e2e-test for Entra ID passwordless authentication?
There was a problem hiding this comment.
Will look into it. If we can make TOTP MFA (similar to current test) passwordless, it should be feasible. Otherwise, I'm not sure if there is a way to automate a passwordless MFA, but I'll do some research and report back here
There was a problem hiding this comment.
Documenting for reference, A potential solution which I need to experiment with is https://learn.microsoft.com/en-us/entra/identity/authentication/howto-authentication-temporary-access-pass, which can work as a passwordless method if there is a way to auotmate its generation in the e2e tests
There was a problem hiding this comment.
Doesn't seem to be the case, unfortunately. https://learn.microsoft.com/en-us/entra/identity/authentication/overview-authentication
Software OATH Tokens is listed as a secondary method only
156e815 to
7bf6a34
Compare
|
@nooreldeenmansour is this ready for another round of review? |
adombeck
left a comment
There was a problem hiding this comment.
Looks good! Just missing some renames and the e2e-test.
a8e5120 to
df16ab0
Compare
| ## Note: If both device_code and entra_auth are disabled, no | ||
| ## authentication will be available and users will not be able to log in. |
There was a problem hiding this comment.
Don't we treat that as invalid config and let the broker fail to start?
There was a problem hiding this comment.
Indeed, I've updated the comment, making it more explicit
| EntraAuthWait: "Waiting for MFA approval", | ||
| EntraAuthCode: "Enter your MFA code", |
There was a problem hiding this comment.
Are these two actually about MFA? If so, we can keep naming them EntraMFAWait / EntraMFACode. If not, we should consider changing the display label.
There was a problem hiding this comment.
These were about MFA indeed, I've changed those back to their older names, and update their references as well
adombeck
left a comment
There was a problem hiding this comment.
Just missing the e2e-test now
2ca583b to
d373efb
Compare
47c74d0 to
a8d9794
Compare
Entra accounts increasingly authenticate without a password (Authenticator number-matching, TAP, security keys). Drive this from the existing Entra auth flow: probe for a passwordless method first and prompt for a password only when Entra reports one is required. Security-key challenges are completed locally with libfido2 (internal/fido) behind new entra_auth_fido and entra_auth_fido_pin modes. A key cannot be attached to a headless or SSH host, so the security-key step waits a bounded time and then falls back to the device code flow instead of blocking the login; FIDO is still advertised to Entra so passwordless FIDO-only accounts are discovered before a key is inserted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Entra direct-auth flow now handles both password and passwordless authentication, so the old entra_password name is misleading. Because the feature has not shipped yet, hard rename it instead of preserving compatibility aliases. Rename the config key, auth mode constant, cached-token marker, e2e suite names, and the remaining Go identifiers consistently to the entra_auth family.
e69760c to
ea1506a
Compare
To prevent CI failures until the changes are reviewed and merged upstream.
A returning passwordless user whose first login already used the app-only Graph fallback is locked out of every later online login when device registration keeps failing: the cached token carries no device registration data, which also disqualifies the local password mode online, so the user is forced back into entra_auth where the fallback was gated on "first login" (oldAuthInfo == nil) and skipped. Gate the fallback on the absence of device registration data instead, which is the condition the surrounding comment always described: the fallback exists for logins that fail registration before any local device state exists, regardless of whether it is the first one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The initial entra_auth layout is a wait-only form (the passwordless probe screen), but the mode was offered to any client whose form layout supports chars_password. authd validates the broker's layout against the client's declared capabilities and rejects undeclared fields, so a client without wait support would see entra_auth offered yet fail every attempt to select it. All in-repo PAM clients declare wait, so this only affects clients that self-declare their capabilities (GDM's JSON protocol, third-party D-Bus consumers). The shared test fixture now declares wait like the real clients do. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
EndSession freed the MFA flow synchronously, so an in-flight AcquireTokenByMFAFlow call could hold MFAFlowState.mu and stall the D-Bus reply, and with it PAM teardown, for the broker HTTP timeout. Release the flow asynchronously while authentication is still running; the idempotent, mutex-guarded free remains synchronous otherwise. Use denyAndClearMFA for terminal denial paths so the flow, password hash, and FIDO PIN are cleared consistently. Clarify that completed-flow replay also covers failure redirects, not only successful MFA transitions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cAuthOptions silently drops unknown AuthOption values, so it can return an empty slice even for non-empty input, and indexing &options[0] unconditionally would panic inside the broker daemon. Unreachable today only because InitiateMFAFlow always prepends NoDAGFallback; use unsafe.SliceData, which yields nil for a nil slice, and the C API accepts NULL with length 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The poll-based and code-entry follow-up modes are only used for MFA challenges, so calling them entra_auth_wait/entra_auth_code was misleading after the main mode was renamed to entra_auth. Restore the MFA-specific naming for these two modes.
parseFlowsConfig already treats having both device_code and entra_auth disabled as an invalid configuration and makes the broker fail to start. Update the comment to match that behavior.
e51c8b3 to
b5ae8b3
Compare
Automated an e2e test for TAP MFA login, as discussed in #1723 (comment). Review the last commit for details (Had to add an application permission to automate the generation of the TAPs) Previously it always fell back to the Entra password prompt; a small libhimmelblau patch was needed to enable TAP MFA, and now it works correctly (Users with a temporary access pass are shown MFA method directly, rather than "Enter your Entra ID password" Passed locally; waiting on CI. Edit: The new test is passing in the last runs, first, second run Latest forced push is to trim the comments verbosity, and to ensure that the temporary access passwords are cleared for the tests that do not require them (if not already cleared) to prevent failures |
b5ae8b3 to
643ee2c
Compare
Test passwordless Entra sign-in via a Temporary Access Pass (TAP) instead of a regular password. Mint and revoke the TAP per-test through the Microsoft Graph API instead of provisioning a dedicated passwordless account, so the existing shared test user can be reused without affecting the password-based tests.
Other Entra e2e tests that share the same test account started failing intermittently, timing out waiting for the Entra password prompt. Entra was showing a Temporary Access Pass challenge instead: once a TAP exists for an account, Entra offers it to any interactive sign-in for that account, regardless of which flow started the session. Since the passwordless test's TAP can outlive its own teardown (or a stray one can come from unrelated manual testing), any other test using the same account can hit it unexpectedly. None of the other Entra tests know about TAP, so add a defensive, best-effort cleanup at the shared entry points that perform a real interactive Entra sign-in, rather than relying solely on the passwordless test's own teardown to keep the account clean. The three release channels run concurrently against the same account, so also gate the cleanup on TAP age: a very fresh TAP may belong to a passwordless test still in flight in another channel, and deleting it out from under that test would trade one kind of cross-test breakage for another.
643ee2c to
9760d97
Compare
Augment the Entra ID authentication flows to support both password/passwordless MFA methods by probing for a passwordless method first and prompt for a password only when Entra reports one is required.
Security-key challenges are completed locally with
libfido2behind newentra_mfa_fidoandentra_mfa_fido_pinmodes.Tested with Yubikey MFA, and Passwordless MS Authenticator MFA
UDENG-10773
UDENG-10774
Relevant Issues and Discussions (WIP)
Closes #832
#595