Optional OIDC bearer-token authentication for the API - #19378
Open
rodchristiansen wants to merge 10 commits into
Open
Optional OIDC bearer-token authentication for the API#19378rodchristiansen wants to merge 10 commits into
rodchristiansen wants to merge 10 commits into
Conversation
…ic SSO)
Adds an `oidc` auth guard layered alongside Passport via `auth:api,oidc`, so an
API caller can present an Authorization: Bearer JWT issued by any OpenID Connect
provider (Entra, Okta, Auth0, Keycloak, Google) instead of a Passport token. The
token is validated against configurable trusted issuer(s)/audience/JWKS and
mapped to an existing Snipe-IT user, whose own permissions then apply unchanged.
- config/oidc.php: env-driven config (issuers, audience, JWKS, username claim,
optional JIT provisioning, leeway). Inert until OIDC_API_ENABLED + issuers +
audience are set, so this is purely additive — Passport, passphrase, and every
existing caller keep working untouched.
- App\Services\Oidc\OidcTokenValidator: firebase/php-jwt (already present via
Passport); RS/ES only (rejects none/HS* — algorithm confusion), iss/aud/exp
validation, JWKS discovery + caching. Returns null on any failure (no error
oracle; details logged server-side).
- App\Services\Oidc\OidcUserResolver: matches a user by username (same active,
not-deleted filter as SAML); optional gated JIT provisioning.
- App\Auth\OidcGuard + Auth::extend('oidc') registration; config/auth.php guard;
api middleware group and RouteServiceProvider switched to auth:api,oidc.
- Feature tests (local RSA keypair, seeded JWKS): valid token authenticates the
matching user; expired/wrong-audience/untrusted-issuer/unknown-user/deactivated
rejected; disabled mode ignores bearers; Passport still authenticates via the
multi-guard.
Design: ADO Projects work item grokability#3721. Pairs with the ReportMate OIDC work.
…llback) The resolver fell back from the admin-configured username_claim to preferred_username/upn/email when the configured claim was absent. Since claims differ in trust and mutability (email especially), a token lacking the trusted claim could authenticate by matching a different one -- an account-confusion / bypass vector, sharper for multi-issuer or less-trusted IdPs. Now matches only the configured claim and rejects when it is absent. Adds a regression test. Found by automated commit security review.
- OidcUserResolver::provision assigned int 1 to User::$activated (typed bool). - OidcApiAuthTest passed an uninitialized typed property by reference to openssl_pkey_export (fatal on PHP 8.4); export into a local, then assign.
…e the guard Two test-only fixes (the guard/validator logic is unchanged): - Inject an OidcTokenValidator whose signingKeys() returns the local public key, so validation runs for real (iss/aud/exp/signature) without a network JWKS fetch. The seeded-cache approach didn't reach the validator inside the request, so every token fell through to a failing fetch and was rejected. - Create a user in setUp: Snipe redirects unauthenticated requests to the setup wizard (302) until a user exists, which masked the intended 401s.
Passport's TokenGuard clears the Authorization header when it fails to validate a bearer (vendor TokenGuard.php:234). With auth:api,oidc, Passport ran first, wiped the header on the (foreign) OIDC JWT, and the oidc guard then saw nothing -- so OIDC bearers were never authenticated (would have failed in production too, not just the test). Reorder to auth:oidc,api: the OIDC guard runs first and never clears the header, so a Passport token still falls through cleanly.
The OIDC guard uses Firebase\JWT\JWT and JWK directly. Until now the package only arrived transitively via laravel/passport and laravel/socialite, so a future dependency change there could pull it out from under us. Declare it explicitly; the resolved version is unchanged.
Up to standards ✅🟢 Issues
|
Member
|
So this PR was obviously AI generated - I'm willing to entertain it tho if you can actually explain to me what benefit this PR provides. I understand the technical details, but AI consistently fails to explain to me why I'd want to introduce this code path into the source. |
Contributor
Author
|
Hi @snipe — the reason is that Passport PATs are effectively permanent ( Nothing to store or rotate. The caller presents a one-hour token instead of a secret that lives in a vault forever. Revocation actually works. Disable the account upstream and API access stops at the next token. Today you can disable a Snipe account and their PATs keep working, because the token is the identity — and there's no reliable way to find which ones they minted. Existing MFA and Conditional Access policy now covers the API, without Snipe implementing any of it. Calls are attributable to a named identity instead of a shared secret. The guard grants nothing on its own — it matches the token to an existing Snipe user by username and that user's permissions apply, exactly as today. No match, no auth. Off unless Pushing a cleanup for the Codacy findings shortly. |
Drop OidcUserResolver::provision() and the OIDC_API_PROVISION config key, so a validated token can only ever resolve to an existing, active Snipe-IT user. The provider can no longer create an account under any configuration, which is what the PR description already claimed. Also clear the six Codacy findings, none of which were in the guard or the validator: initialise $privatePem before the openssl_pkey_export() by-reference write, replace constructor property promotion in the JWKS test double that PHPMD misreads as unused parameters, and suppress UnusedFormalParameter on the two signatures fixed by the framework.
PHPMD binds @SuppressWarnings to the enclosing method, so the annotation on the Auth::extend closure never applied. Move it to boot(), which is the method PHPMD actually reads. In the JWKS test double, use the $issuer parameter instead of suppressing the warning: assert it matches the issuer under test, so key resolution can never be reached for an issuer the validator did not already match against the trusted list.
# Conflicts: # composer.lock
PHPStan parses an unquoted @SuppressWarnings value as an expression and fails on the dot in PHPMD.UnusedFormalParameter. The quoted form is valid PHPMD and parses cleanly, so both analysers pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds an optional, provider-agnostic OIDC bearer-token guard for the API, so a token issued by a trusted identity provider (Entra ID, Okta, Keycloak, Auth0, Google — anything with standards-compliant discovery) can authenticate an API call without minting and storing a long-lived Passport personal access token.
It is off by default and inert until
OIDC_ENABLED=trueis set. With it disabled, nothing about the current auth path changes.How it works
App\Auth\OidcGuard— a stateless guard registered as theoidcdriver. Returnsnullon any failure so the framework produces a normal 401.App\Services\Oidc\OidcTokenValidator— validates the JWT against the configured issuer(s), audience(s) and JWKS.App\Services\Oidc\OidcUserResolver— maps a validated token to an existing Snipe-IT user via a configurable username claim.config/oidc.php— all configuration, env-driven.The API stack becomes
auth:oidc,apirather thanauth:api, which accepts either an OIDC token or a Passport token. Existing Passport tokens keep working exactly as before.Security properties
These were deliberate, and are the parts most worth reviewing:
alg: noneis impossible.issis read only to reject early and to select the right JWKS in a multi-issuer deployment; the signature is still fully verified afterwards.oidcmust precedeapi: Passport'sTokenGuardclears theAuthorizationheader when it fails, which would wipe an OIDC bearer before the OIDC guard ever saw it. This is commented at both call sites.Signing keys are discovered per issuer and cached, so validation doesn't hit the provider on every request.
signingKeys()is isolated so tests can seed a local key without a network round-trip.Type of change
How Has This Been Tested?
tests/Feature/Authentication/OidcApiAuthTest.php— 10 tests covering a valid token, untrusted issuer, wrong audience, expired token, symmetric-algorithm rejection, unknown user, and the disabled-by-default path. JWKS retrieval is stubbed so the suite stays offline.auth:oidc,apimiddleware change doesn't disturb existing Passport authentication.Notes for reviewers
One dependency change:
firebase/php-jwtis now declared incomposer.json. It was already installed transitively vialaravel/passportandlaravel/socialite, and the resolved version is unchanged — the lock diff is only the content hash. Declaring it directly just means a future dependency change in those packages can't pull it out from under this code.Happy to split the config surface differently, rename env vars, or gate this behind a settings-panel toggle instead of env if you'd prefer — the guard itself is independent of how it's configured.