Oauth2 Support

[kickster97]

WHAT is this pull request doing?

Add OAuth2/OpenID Connect authentication to LavinMQ. Clients can now authenticate using JWT tokens (RS256) as the password field. The authenticator validates tokens against the OAuth provider's JWKS endpoint, checks issuer/audience/expiration, and maps scopes to LavinMQ permissions. Supports token refresh via UpdateSecret frame without reconnecting. JWKS public keys are cached with configurable TTL.

Refactors the user system into an abstract User base class with LocalUser (password-based) and OAuthUser (token-based with expiration tracking) implementations. Authentication chain tries local auth first, then OAuth.

fixes #857

HOW can this pull request be tested?

specs to come once we are satisfied with the general logic and structure

[claude]

Claude finished @kickster97's task —— View job

---

OAuth2 Support PR Code Review

Reviewing this PR for bugs, security vulnerabilities, performance issues, missing error handling, and Crystal anti-patterns.

Todo List

• Read repository guidelines (CLAUDE.md)
• Review core OAuth authentication files
• Examine JWT handling and verification logic
• Check user management and security patterns
• Review configuration and error handling
• Post findings

Issues Found

🚨 Critical Security Vulnerability

1. Timing Attack in Public Keys Iteration (src/lavinmq/auth/public_keys.cr:54-58):

   keys.each_value do |key|
     return JWT::RS256Parser.decode(token, key, verify: true)
   rescue JWT::VerificationError
   end

   This creates a timing side channel - successful verification returns immediately while failed verification continues iterating. Attackers could use timing analysis to determine key ordering and potentially extract key information.

🛡️ Security Issues
2. Insufficient Error Context for Security Events (src/lavinmq/auth/jwt_token_verifier.cr:101-103):

• Issuer validation error doesn't log what the actual issuer was, making security incident investigation difficult
• Should log expected vs actual issuer for security monitoring

3. Resource Exhaustion Vulnerability (src/lavinmq/auth/oauth_user.cr:26):

   when timeout(token_lifetime)

   If token_lifetime is negative (expired token), this could cause very short timeouts, potentially creating a busy-loop that consumes CPU resources.

⚠️ Missing Error Handling
4. Uncaught LibCrypto Exceptions (src/lavinmq/auth/jwks_fetcher.cr:73-96):

• Multiple LibCrypto calls that could fail but errors get converted to generic "Failed to create X" messages
• Should have more specific error handling for different OpenSSL failure modes

5. Network Timeout Missing Retry Logic (src/lavinmq/auth/jwks_fetcher.cr:128-139):
   • HTTP client has timeouts but no retry mechanism for transient network failures
   • Could cause authentication failures during temporary network issues

🐛 Potential Bugs
6. Race Condition in Token Update (src/lavinmq/auth/oauth_user.cr:47):

@token_updated.send nil

If multiple token updates happen rapidly, the channel send could block or messages could be lost, potentially leaving the expiration fiber in an inconsistent state.

7. Integer Overflow in Cache TTL (src/lavinmq/auth/jwks_fetcher.cr:60):

   return match[1].to_i.seconds

   No bounds checking on the parsed integer - malicious JWKS response could specify extremely large max-age values.

📈 Performance Issues
8. Inefficient Scope Processing (src/lavinmq/auth/jwt_token_verifier.cr:183-194):

• Creates multiple intermediate arrays during scope filtering
• Could be optimized with lazy evaluation for large scope lists

🔧 Crystal Anti-patterns
9. Mixed Exception Types in Rescue (src/lavinmq/auth/oauth_authenticator.cr:31-33):

rescue ex : Exception

Catching generic Exception after specific JWT exceptions violates Crystal's exception handling patterns.

10. Unsafe Type Casting (src/lavinmq/auth/jwt_token_verifier.cr:66-67):

    exp = payload["exp"]?.try(&.as_i64?)

    Should validate that the exp value is reasonable (not negative, not too far in future) before using it.

[carlhoerberg]

For sure should implement our own JWT parser, it's not many lines of code: https://claude.ai/artifacts/82992d3e-5e0d-4e0e-8205-bb29f7a0dce1

[carlhoerberg]

Is the the TempUsers class really necessary? Can't the original UserStore be used? The only difference between BasicUser and TempUser is the expiration? Can/should the expiration time be nilable and added to the normal User? Or should the old User be kept we just create a new TempUser that inherits from it?

[kickster97]

Is the the TempUsers class really necessary? Can't the original UserStore be used? The only difference between BasicUser and TempUser is the expiration? Can/should the expiration time be nilable and added to the normal User? Or should the old User be kept we just create a new TempUser that inherits from it?

Yes sounds good, Could override the methods that need password in UserStore for TempUsers and let TempUser inherit User. I still think it makes sense for temporary users to have its own class TempUser, due to expire and password etc..

[spuun]

Rename User to BasicUser and introduce an abstract base class User. Rename TempUser to OAuthUser and let it inherit thew new abstract User.

Remove all TempUser handling code in UserStore

[spuun]

Good job!
I've got some questions and suggestions.

[kickster97]

closing here to get a cleaner PR to wrok with in #1632 :)