Improve registry concurrency and auth signing

[TheApeMachine]

Summary

• add mutex to registry package for safe concurrent access
• add signing key handling to auth service
• implement tests for registry and auth packages

Testing

• go test ./pkg/registry -v
• go test ./pkg/auth -v
• go test ./... (fails: context deadline exceeded)

[gemini-code-assist]

Code Review

This pull request successfully addresses the stated goals of improving concurrency in the registry package by adding a mutex and handling signing keys in the auth service. It also includes initial tests for both packages.

The changes in pkg/registry correctly introduce a sync.RWMutex to protect the global toolRegistry map, resolving the concurrency concern mentioned in the previous TODO comment. This is a good improvement for thread safety.

The changes in pkg/auth introduce a signingKey field and use it consistently for signing and parsing tokens, replacing the hardcoded value in multiple places. This improves maintainability.

The new test files provide basic coverage for the core functionalities of both packages.

Overall, the changes are a positive step towards improving the robustness and security of these components. However, there is one critical security issue related to the signing key that needs to be addressed before merging.

This review follows standard Go conventions for style and best practices.

Summary of Findings

• Hardcoded JWT Signing Key: The JWT signing key is hardcoded in the NewService function. This is a critical security vulnerability as it makes the key easily discoverable and guessable, allowing potential attackers to forge valid tokens. The key should be loaded from a secure configuration source.
• Global Variable Usage in Registry: The toolRegistry is a package-level global variable. While the addition of the sync.RWMutex correctly addresses concurrent access to this global map within the package functions (RegisterTool, GetToolDefinition), using global variables can make testing and dependency management more challenging. Consider if making the registry an instance variable of a struct passed around or initialized differently would improve the overall design and testability.
• Test Coverage and Structure: Initial tests have been added for both auth and registry packages, which is great. However, the tests are basic and could be expanded to cover more edge cases (e.g., invalid tokens, expired tokens, concurrent access scenarios in registry tests). Additionally, the registry tests directly manipulate the global toolRegistry variable and its mutex, which can lead to test pollution and flakiness if tests are run in parallel. Consider restructuring tests to avoid direct manipulation of global state where possible.

Merge Readiness

This pull request introduces necessary concurrency handling and improves the management of the signing key. However, the hardcoded signing key is a critical security issue that must be resolved before this code can be safely merged. I recommend addressing this critical issue and considering the other findings for future improvements. I am unable to approve this pull request directly; please have other reviewers review and approve this code before merging.

[coderabbitai]

Actionable comments posted: 8

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c710761 and 42d387c.

📒 Files selected for processing (4)

• pkg/auth/service.go (6 hunks)
• pkg/auth/service_test.go (1 hunks)
• pkg/registry/registry.go (2 hunks)
• pkg/registry/registry_test.go (1 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (2)

pkg/registry/registry_test.go (1)

pkg/registry/registry.go (3)

• ToolDefinition (25-31)
• RegisterTool (41-46)
• GetToolDefinition (50-55)

pkg/auth/service_test.go (1)

pkg/auth/service.go (1)

• NewService (30-37)

🔇 Additional comments (11)

pkg/registry/registry.go (4)

5-5: LGTM: Appropriate import for concurrency safety.

The addition of the sync package import is necessary for the mutex implementation.

---

34-37: Excellent concurrency safety implementation.

The use of sync.RWMutex is the correct choice here as it allows multiple concurrent readers while ensuring exclusive write access. Moving the variables into a var block with the mutex is good practice for grouping related synchronization primitives.

---

43-45: Proper write locking implementation.

The Lock() and Unlock() calls correctly protect the write operation to the map. The placement ensures the critical section is minimal while maintaining safety.

---

51-53: Correct read locking implementation.

Using RLock() and RUnlock() for read operations is appropriate as it allows concurrent reads while preventing race conditions with writes.

pkg/auth/service.go (6)

18-18: Good centralization of signing key management.

Adding the signingKey field to the Service struct centralizes key management, which improves maintainability and consistency across JWT operations.

---

39-44: Excellent addition of signing method validation.

The getSigningKey method properly validates that the JWT uses HMAC signing method before returning the key, which prevents algorithm confusion attacks.

---

64-64: Good refactoring to use centralized signing key method.

Replacing the inline anonymous function with the centralized getSigningKey method improves consistency and security by ensuring signing method validation occurs in all JWT parsing operations.

---

80-80: Consistent use of centralized signing key.

Using s.signingKey instead of a hardcoded byte slice maintains consistency with the centralized key management approach.

---

90-90: Consistent use of centralized signing key for refresh tokens.

Using s.signingKey for refresh token signing maintains consistency with the centralized approach.

---

121-121: Consistent use of centralized signing key validation.

Using s.getSigningKey in the RefreshToken method ensures consistent signing method validation across all JWT parsing operations.

pkg/auth/service_test.go (1)

1-11: Good test file setup with appropriate imports.

The test file is well-structured with appropriate imports for testing HTTP requests and JWT functionality. Using GoConvey for BDD-style testing is a reasonable choice.

[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