Fix SSH JWT issuer derivation for IDPs with path components

[sgtaziz]

Describe your changes

Fixed SSH JWT authentication for IDPs with path-based issuers (e.g., Authentik).

The buildJWTConfig() function in conversion.go was incorrectly using OR (||) instead of AND (&&) when checking whether to derive the issuer from the token endpoint. This caused it to always override the configured AuthIssuer when deviceFlowConfig exists, even when AuthIssuer was properly set.

The deriveIssuerFromTokenEndpoint() function strips the path from URLs (returns only scheme://host/), which breaks IDPs like Authentik where the issuer includes a path component (e.g., https://auth.example.com/application/o/netbird/).

Changed: Line 372 in management/internals/shared/grpc/conversion.go

// Before
if issuer == "" || deviceFlowConfig != nil {

// After  
if issuer == "" && deviceFlowConfig != nil {

Now the issuer is only derived from the token endpoint when AuthIssuer is not already configured.

Issue ticket number and link

Fixes #4813

Stack

Checklist

• Is it a bug fix
• Is a typo/documentation fix
• Is a feature enhancement
• It is a refactor
• Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

• I added/updated documentation for this change
• Documentation is not needed for this change (explain why)

Reason: This is a bug fix that corrects existing behavior. No new features or configuration options are introduced.

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

N/A

Summary by CodeRabbit

• Bug Fixes
  • Modified JWT issuer derivation logic to apply only when both an empty issuer and device flow configuration are present, refining the conditions for automatic issuer resolution in authentication flows.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[CLAassistant]

All committers have signed the CLA.

[coderabbitai]

Walkthrough

Modified the JWT issuer derivation condition in buildJWTConfig from an OR operator to an AND operator. The issuer is now inferred from the token endpoint only when both the issuer is empty AND a device flow config is present, rather than when either condition is true.

Changes

Cohort / File(s)	Summary
JWT Config Issuer Logic management/internals/shared/grpc/conversion.go	Changed issuer derivation condition in buildJWTConfig from issuer == "" || deviceFlowConfig != nil to issuer == "" && deviceFlowConfig != nil, narrowing the scope of when the issuer is inferred from the token endpoint

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

• Specific areas requiring attention:
  • Verify that the AND logic correctly handles device flow authentication scenarios, particularly for SSH use cases referenced in issue #4813
  • Confirm that removing the OR condition doesn't break existing JWT configurations where deviceFlowConfig is present but issuer is already set
  • Test authentication flows with both empty and pre-configured issuers to ensure proper issuer resolution

Poem

🐰 A logic fix, both small and tight,
From OR to AND, we set things right!
SSH speaks again with auth so true,
Device flows dance, all shiny new! ✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing SSH JWT issuer derivation for IDPs with path components, which directly matches the code modification and purpose.
Description check	✅ Passed	The description follows the template structure with all required sections completed: changes described in detail, issue ticket linked (#4813), bug fix checkbox marked, and documentation requirement addressed with reasoning.
Linked Issues check	✅ Passed	The PR addresses the root cause of issue #4813 by fixing the JWT issuer derivation logic that was breaking SSH authentication for IDPs with path-based issuers like Authentik.
Out of Scope Changes check	✅ Passed	The PR contains only a single logical operator change in the issuer derivation condition, which is directly scoped to fixing the SSH JWT authentication issue without introducing unrelated modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

management/internals/shared/grpc/conversion.go (1)

394-405: Optional: Document the path-stripping behavior.

The function intentionally strips path components from the token endpoint, returning only scheme://host/. While this is appropriate for deriving a fallback issuer, it may be worth adding a comment explaining this behavior to prevent future confusion, especially since it's the reason path-based IDPs were broken by the old call-site logic.

Consider adding a doc comment:

 // deriveIssuerFromTokenEndpoint extracts the issuer URL from a token endpoint
+// Note: This function intentionally strips path components, returning only scheme://host/.
+// It should only be used when no explicit issuer is configured.
 func deriveIssuerFromTokenEndpoint(tokenEndpoint string) string {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 131d7a3 and f25e3c4.

📒 Files selected for processing (1)

• management/internals/shared/grpc/conversion.go (1 hunks)

🔇 Additional comments (1)

management/internals/shared/grpc/conversion.go (1)

372-376: LGTM! The condition fix correctly respects explicitly configured issuers.

The change from OR (||) to AND (&&) is correct and directly addresses the reported SSH failure with IDPs that have path components. Previously, any non-nil deviceFlowConfig would trigger derivation from the token endpoint—overriding an explicitly set AuthIssuer—and since deriveIssuerFromTokenEndpoint() strips path components (returns only scheme://host/), this broke IDPs like Authentik that require paths in the issuer URL.

The new logic only derives the issuer when it's truly unset, preserving the configured value when present. Code search confirms this is the only call site for issuer derivation, and the fix doesn't affect other authentication providers (e.g., Auth0 explicitly requires AuthIssuer to be configured independently).

[pascal-fischer]

Hi @sgtaziz,
Thank you for the fix. You are absolutely right, there should be an AND. I messed this up as part of #4807

Thanks again for the contribution