Verifying jwt signing algo to prevent vulnerability

[ksykulev]

Related to a vulnerability found when working on
#43295
#43295 (comment)

golang-jwt/jwt/v5 library already mitigates this, however, we are using v4 which does not include this check.

Summary by CodeRabbit

• Bug Fixes
  • Enforced RSA-only validation for JWTs used in authentication; tokens signed with non-RSA algorithms are now rejected.
• Tests
  • Added tests to verify that non-RSA and unsigned JWTs are rejected and produce the expected error.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

[qodo-code-review]

Review Summary by Qodo

Validate JWT signing algorithm to prevent substitution attacks

🐞 Bug fix

Walkthroughs

Description

• Add JWT signing algorithm validation to prevent algorithm substitution attacks
• Verify RSA signing method in token parsing callback
• Mitigate vulnerability in golang-jwt/jwt/v4 library

Diagram

flowchart LR
  A["JWT Token Parsing"] --> B["Validate Signing Method"]
  B --> C{Is RSA Method?}
  C -->|Yes| D["Return Public Key"]
  C -->|No| E["Return Error"]
  D --> F["Token Verified"]
  E --> G["Token Rejected"]

Loading

File Changes

1. server/mdm/microsoft/wstep.go 🐞 Bug fix +3/-0

Add JWT signing algorithm validation check

• Add algorithm validation check in JWT token parsing callback
• Verify that token uses RSA signing method before accepting it
• Return error if unexpected signing algorithm is detected
• Prevent algorithm substitution vulnerability in STS token verification

server/mdm/microsoft/wstep.go

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1)   📘 Rule violations (0)   📎 Requirement gaps (0)

🐞\ ⛨ Security (1)

1. RS256 not pinned 🐞 ⛨

Description

GetSTSAuthTokenUPNClaim only checks that the JWT uses an RSA signing method, so it would accept any
RSA JWT alg even though NewSTSAuthToken always issues RS256. This weakens the verifier/issuer
contract and may hide unexpected token variants or configuration drift.

Code

server/mdm/microsoft/wstep.go[R319-321]

+		if _, ok := token.Method.(*jwt.SigningMethodRSA); !ok {
+			return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"])
+		}

Evidence

NewSTSAuthToken always signs STS tokens using RS256, but the updated verifier only enforces the RSA
family, not the specific expected algorithm.

server/mdm/microsoft/wstep.go[194-226]
server/mdm/microsoft/wstep.go[303-323]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GetSTSAuthTokenUPNClaim` now checks the token uses an RSA signing method, but it doesn’t enforce the specific algorithm (`RS256`) that Fleet uses when issuing STS tokens.

## Issue Context
`NewSTSAuthToken` creates tokens with `jwt.GetSigningMethod("RS256")`. To fully lock down the contract between issuer and verifier, the verifier should reject RSA algorithms other than RS256.

## Fix Focus Areas
- server/mdm/microsoft/wstep.go[303-323]
- server/mdm/microsoft/wstep.go[194-226]

## Suggested change
Inside the `keyFunc` for `jwt.ParseWithClaims`, add an explicit check that `token.Method.Alg()` (or equivalent) equals `"RS256"` (and keep the RSA-type check if desired). Update the error message to report the actual method/alg used.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[Copilot]

Pull request overview

This PR hardens Windows MDM JWT validation by explicitly verifying the JWT signing algorithm during STS token parsing, addressing an algorithm-confusion class of vulnerabilities when using github.com/golang-jwt/jwt/v4.

Changes:

• Add a signing-method check in GetSTSAuthTokenUPNClaim to reject non-RSA JWT algorithms before returning the verification key.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c7aa953e-1a4b-4aa8-8eff-c07934036d92

📥 Commits

Reviewing files that changed from the base of the PR and between 0719e03 and 6b7d971.

📒 Files selected for processing (1)

• server/mdm/microsoft/wstep_test.go

---

Walkthrough

The change modifies the GetSTSAuthTokenUPNClaim method in server/mdm/microsoft/wstep.go to add validation that ensures JWT tokens are signed using RSA signing methods. Within the jwt.ParseWithClaims key-supply callback, the code now checks the token's signing method and fails token parsing with an error message that includes the token's alg header value if the method is not RSA. No other functional behavior in the method was altered.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description lacks required checklist items. It references related issues but doesn't follow the template structure with security validation checklist items or testing confirmation boxes.	Fill in the description template with checked boxes for relevant items (security validation, automated tests, manual QA) to document the verification work completed.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: adding JWT signing algorithm verification to prevent a security vulnerability.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch signing-fix

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.91%. Comparing base (83a886b) to head (6b7d971).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #43474   +/-   ##
=======================================
  Coverage   66.90%   66.91%           
=======================================
  Files        2596     2596           
  Lines      208220   208221    +1     
  Branches     9285     9285           
=======================================
+ Hits       139314   139321    +7     
+ Misses      56236    56233    -3     
+ Partials    12670    12667    -3     

Flag	Coverage Δ
backend	68.69% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.