Add in-memory authserver storage implementation#3298
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3298 +/- ##
==========================================
+ Coverage 64.18% 64.44% +0.25%
==========================================
Files 365 366 +1
Lines 35244 35652 +408
==========================================
+ Hits 22623 22975 +352
- Misses 10810 10860 +50
- Partials 1811 1817 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@claude please do a thorough security review of this, leverage agents if possible. |
|
Claude finished @JAORMX's task in 5m 16s —— View job Security Review Complete ✅I've conducted a thorough security review of the OAuth 2.0 authorization server storage implementation, leveraging specialized agents for deep analysis. Executive SummaryThe implementation demonstrates strong foundational security with excellent RFC awareness and thoughtful design patterns. However, 4 critical security issues and 6 important recommendations were identified that should be addressed before production deployment. Overall Assessment:
Critical Security Issues (Require Immediate Action)1. Incomplete Token Revocation Cascade (RFC 7009 Violation)Severity: 🔴 CRITICAL Issue:
Current Code: func (s *MemoryStorage) RevokeRefreshToken(_ context.Context, requestID string) error {
s.mu.Lock()
defer s.mu.Unlock()
// Only removes refresh tokens - missing access token cascade!
for sig, entry := range s.refreshTokens {
if entry.value.GetID() == requestID {
delete(s.refreshTokens, sig)
}
}
return nil
}Impact: Access tokens remain valid after refresh token revocation, violating OAuth 2.0 security requirements. Fix: Add access token deletion (like // Also invalidate associated access tokens per RFC 7009 Section 2.1
for sig, entry := range s.accessTokens {
if entry.value.GetID() == requestID {
delete(s.accessTokens, sig)
}
}2. Missing Refresh Token BindingSeverity: 🔴 CRITICAL Issue: The // TODO: Store the accessSignature in a refreshToAccess map to enable direct lookup
// during token rotation instead of O(n) scan by request ID in RotateRefreshToken.Impact:
Fix: Implement the TODO: type MemoryStorage struct {
// ... existing fields ...
refreshToAccess map[string]string // refresh signature -> access signature
}
func (s *MemoryStorage) CreateRefreshTokenSession(
_ context.Context,
signature string,
accessSignature string, // Use this parameter!
request fosite.Requester,
) error {
// ... validation ...
s.refreshToAccess[signature] = accessSignature
// ... rest of implementation ...
}3. No PKCE Method Validation (Security Best Practice Violation)Severity: 🟠 HIGH Issue: Storage accepts any PKCE method string without validation. RFC 7636 Section 4.4 states servers MUST NOT allow "plain" method with public clients, and OAuth 2.0 Security BCP deprecates the plain method entirely. Current Code: // PKCEMethod is the PKCE challenge method (must be "S256").
PKCEMethod string // Comment says "must be S256" but no validation!Impact: Defense-in-depth failure. If validation is missed in handlers, weak PKCE could be accepted. Fix: Enforce S256 in if pending.PKCEMethod != "" && pending.PKCEMethod != "S256" {
return fosite.ErrInvalidRequest.WithHint(
"PKCE challenge method must be S256; plain method is not supported",
)
}4. Upstream Token Binding Not ImplementedSeverity: 🟠 HIGH Issue:
Impact: Cross-session token theft is possible if session IDs are predictable or leaked. Attack Scenario:
Fix: Either implement validation in // GetUpstreamTokensWithBinding retrieves upstream tokens and validates binding.
func (s *MemoryStorage) GetUpstreamTokensWithBinding(
ctx context.Context,
sessionID string,
expectedSubject string,
expectedClientID string,
) (*UpstreamTokens, error) {
tokens, err := s.GetUpstreamTokens(ctx, sessionID)
if err != nil {
return nil, err
}
if tokens.Subject != expectedSubject {
return nil, fmt.Errorf("%w: subject mismatch", ErrInvalidBinding)
}
if tokens.ClientID != expectedClientID {
return nil, fmt.Errorf("%w: client ID mismatch", ErrInvalidBinding)
}
return tokens, nil
}Important Security Recommendations5. Client Registration Missing Duplicate CheckSeverity: 🟡 MEDIUM Issue: Fix: func (s *MemoryStorage) RegisterClient(_ context.Context, client fosite.Client) error {
s.mu.Lock()
defer s.mu.Unlock()
if _, exists := s.clients[client.GetID()]; exists {
return fmt.Errorf("%w: client %s already registered",
ErrAlreadyExists, client.GetID())
}
s.clients[client.GetID()] = client
return nil
}6. Missing Audit LoggingSeverity: 🟡 MEDIUM Issue: Limited logging for security-sensitive operations. Only debug-level logs for not-found cases. Recommendation: Add structured audit logging for:
7. No Memory Exhaustion ProtectionSeverity: 🟡 MEDIUM Issue: No limits on storage size. An attacker could exhaust memory via:
Recommendation: Add configurable limits: type MemoryStorage struct {
// ... existing fields ...
maxClients int
maxActiveSessions int
maxPendingAuthorizations int
}8. Session ID Entropy Not ValidatedSeverity: 🟡 MEDIUM Issue: Storage accepts any string as session ID. If session IDs are predictable, Finding #4 becomes more severe. Recommendation: Validate minimum entropy (e.g., 256 bits) for session IDs, or document caller responsibility. 9. Grace Period Not ImplementedSeverity: 🔵 LOW Issue: Recommendation: Document the decision or implement grace period support. 10. Test Coverage GapsIssue: Missing tests for:
Strengths Worth HighlightingThe implementation has several excellent security practices: ✅ Authorization Code Replay ProtectionLines 78-79, 356-374: Separate ✅ JWT Assertion Replay ProtectionLines 277-305: Proper JTI tracking per RFC 7523 with automatic cleanup. Well implemented. ✅ Thread SafetyConsistent use of ✅ Defensive CopyingLines 637-648 (store), 681-688 (retrieve), 731 (slices): Prevents aliasing issues that could lead to data corruption. ✅ Comprehensive DocumentationThe 192-line ✅ Token Lifetime PoliciesAll token lifetimes are RFC-compliant and use secure defaults:
RFC Compliance Summary
Priority Remediation PlanEstimated total time: ~1.5 hours for critical fixes
ConclusionThis is a well-designed implementation with strong security foundations. The authorization code replay protection and JWT assertion tracking are exemplary. However, the 4 critical findings must be addressed before production deployment. The implementation is suitable for development/testing in its current state, but requires remediation for production use. Code Coverage NoteCodecov reports 92.76% coverage with 27 lines missing. The security-critical paths appear to be well-tested based on the comprehensive test suite in References:
|
This is a preparatory commit for the auth-proxy feature. The storage package provides the persistence layer for the OAuth 2.0 authorization server. In the complete branch, this storage is: - Passed to authserver.New() and used by the fosite OAuth2 provider for authorization codes, access tokens, refresh tokens, and PKCE - Used by handlers to validate clients and store pending authorizations while users authenticate with the upstream identity provider - Exposed via Server.IDPTokenStorage() for the token swap middleware, which replaces client JWTs with upstream IDP tokens for backend requests This commit adds: - Storage interface embedding fosite's OAuth2 storage interfaces plus ToolHive-specific interfaces for upstream tokens, pending authorizations, and client registration - MemoryStorage implementation with thread-safe maps and background cleanup - Comprehensive documentation in doc.go explaining fosite's type system (Requester, Session, signature vs request ID) to help developers understand why the API is designed as it is The in-memory implementation is suitable for single-instance deployments and development. Production deployments will use a distributed storage backend (Redis or database).
This commit addresses three review comments from the PR: - Remove authorization code from debug log - Remove internal state from debug logs - Refactor cleanupExpired() to collect-then-delete pattern The original implementation held a write lock while iterating all 8 storage maps, blocking all read and write operations during cleanup. The new implementation: - Acquires READ lock to collect expired keys into slices - Releases read lock and returns early if nothing to delete - Acquires WRITE lock only for the deletion phase This reduces write lock hold time from O(total entries) to O(expired entries), improving concurrency for read-heavy workloads.
jhrozek
force-pushed
the
auth-proxy-pr-8-in-memory-storage
branch
from
4aaf665 to
391bb77
Compare
github-actions
Bot
added
size/XL
github-actions
Bot
dismissed
their stale review
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
2 participants
This is a preparatory commit for the auth-proxy feature. The storage package provides the persistence layer for the OAuth 2.0 authorization server. In the complete branch, this storage is:
This commit adds:
The in-memory implementation is suitable for single-instance deployments and development. Production deployments will use a distributed storage backend (Redis or database).
Large PR Justification
We need to implement all of the required fosite interfaces, doing it in multiple commits would just muddy the waters.