[management, client] Fix SSH server audience validator#5105
Conversation
📝 WalkthroughWalkthroughReplaces single Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)client/ssh/server/server.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/ssh/server/server.go (1)
426-441: Potential panic ifAudiencesslice is empty.Line 440 accesses
config.Audiences[0]without verifying the slice is non-empty. IfAudiencesis nil or has zero length, this will cause a panic during JWT validator initialization.🐛 Proposed fix
if config == nil { return fmt.Errorf("JWT config not set") } + + if len(config.Audiences) == 0 { + return fmt.Errorf("JWT config has no audiences configured") + } log.Debugf("Initializing JWT validator (issuer: %s, audiences: %v)", config.Issuer, config.Audiences)
🧹 Nitpick comments (1)
management/internals/shared/grpc/conversion.go (1)
432-441: Audiences slice may contain duplicates or empty strings.When
CLIAuthAudienceis empty,audienceequalsAuthAudience, resulting in[AuthAudience, AuthAudience, ""]. When set, the slice becomes[CLIAuthAudience, AuthAudience, CLIAuthAudience]with a duplicate.Consider deduplicating and filtering empty strings to avoid potential validation issues or confusion in logs.
♻️ Suggested fix
audience := config.AuthAudience if config.CLIAuthAudience != "" { audience = config.CLIAuthAudience } + + audiences := []string{} + seen := make(map[string]bool) + for _, a := range []string{audience, config.AuthAudience, config.CLIAuthAudience} { + if a != "" && !seen[a] { + audiences = append(audiences, a) + seen[a] = true + } + } + return &proto.JWTConfig{ Issuer: issuer, Audience: audience, - Audiences: []string{audience, config.AuthAudience, config.CLIAuthAudience}, + Audiences: audiences, KeysLocation: keysLocation, }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
shared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
client/internal/engine_ssh.goclient/ssh/server/jwt_test.goclient/ssh/server/server.gomanagement/internals/shared/grpc/conversion.goshared/management/proto/management.proto
🧰 Additional context used
🧬 Code graph analysis (1)
client/ssh/server/server.go (1)
shared/auth/jwt/extractor.go (2)
ClaimsExtractorOption(40-40)WithAudience(43-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: JS / Lint
🔇 Additional comments (4)
shared/management/proto/management.proto (1)
249-259: LGTM! Backward-compatible proto schema extension.The addition of
audiences(field 5) while retaining the deprecatedaudiencefield ensures backward compatibility with older clients. The deprecation comment clearly indicates the migration path.client/ssh/server/jwt_test.go (1)
44-48: LGTM! Test configurations correctly updated for multi-audience support.All test cases consistently use
Audiences: []string{...}instead of the oldAudiencefield. The test coverage for JWT enforcement, detection, fail-close, and authentication scenarios looks comprehensive.client/internal/engine_ssh.go (1)
74-83: LGTM! Correctly maps proto audiences to SSH server config.The change from
GetAudience()toGetAudiences()aligns with the proto schema update. Note thatserver.goaccessesAudiences[0]without a bounds check—ensure the management server always populates at least one audience.client/ssh/server/server.go (1)
476-479: Good improvement to error diagnostics.The updated error message clearly shows the expected audiences (from config) versus actual values from the token, which aids debugging authentication failures.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
mlsmaycon
changed the title
lixmal
changed the title
|
3 participants
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.