Skip to content

Conversation

@jeremyeder
Copy link
Collaborator

DISABLE_AUTH was removed from backend in PR #460 (Dec 2025) as a security fix, but minikube manifests were never updated. This caused all API requests to fail with 401/500 errors because frontend sent mock tokens that backend no longer accepted.

I tested this locally on minikube and it worked fine.

Changes:

  • Makefile: Add _setup-local-dev-auth target to create local-dev-token secret
  • Frontend: Use OC_TOKEN from local-dev-token secret instead of DISABLE_AUTH
  • Backend: Remove DISABLE_AUTH environment variable
  • Grant cluster-admin to local-dev-user for local development

DISABLE_AUTH was removed from backend in PR ambient-code#460 (Dec 2025) as a security
fix, but minikube manifests were never updated. This caused all API requests
to fail with 401/500 errors because frontend sent mock tokens that backend
no longer accepted.

Changes:
- Makefile: Add _setup-local-dev-auth target to create local-dev-token secret
- Frontend: Use OC_TOKEN from local-dev-token secret instead of DISABLE_AUTH
- Backend: Remove DISABLE_AUTH environment variable
- Grant cluster-admin to local-dev-user for local development

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

github-actions bot commented Jan 12, 2026

Claude Code Review

Summary

This PR fixes a critical authentication regression introduced when DISABLE_AUTH was removed in PR #460 (Dec 2025). The minikube manifests were not updated, causing all API requests to fail with 401/500 errors. The fix properly implements ServiceAccount-based authentication for local development using real Kubernetes tokens.

Overall Assessment: Approve with minor recommendations

The changes correctly address the authentication issue and follow established security patterns. The implementation is clean, well-integrated into the Makefile workflow, and aligns with the project security-first approach.


Issues by Severity

Blocker Issues

None - No blocking issues found.


Critical Issues

None - No critical issues found.


Major Issues

  1. Overly Permissive RBAC for Local Development

Location: Makefile:709-711

Granting cluster-admin to local-dev-user contradicts the existing RBAC philosophy in local-dev-rbac.yaml:85-87 which states local-dev-user should have NO ClusterRole bindings.

Impact:

  • Violates principle of least privilege
  • Inconsistent with documented RBAC design
  • May mask RBAC issues that would appear in production

Recommendation:
Remove the cluster-admin binding from the Makefile. The existing namespace-scoped Role in local-dev-rbac.yaml already provides sufficient permissions for local development (AgenticSessions CRUD, ProjectSettings CRUD, Core resources, Jobs management).


Minor Issues

  1. Hard-coded Token Duration (8760h = 1 year) at Makefile:700 - Consider reducing to 24h or 168h for better security hygiene
  2. Secret Deletion Error Suppression at Makefile:705 - Silently suppressing errors can hide legitimate problems
  3. Missing Token Expiration Handling Documentation - No documentation on what happens when token expires
  4. Hard-coded Version String at frontend-deployment.yaml:44 - Not introduced by this PR but worth noting

Positive Highlights

  • Excellent Security Improvement: Completely removes insecure DISABLE_AUTH, uses real Kubernetes RBAC and ServiceAccount tokens
  • Clean Integration: Well-integrated into existing make local-up workflow, idempotent
  • Proper Secret Management: Token stored in Kubernetes Secret, injected via secretKeyRef
  • Follows Project Patterns: Consistent with CLAUDE.md security standards
  • Good Documentation in PR: Clear explanation, references originating PR Add Comprehensive Backend Unit Test Suite with Ginkgo Framework #460
  • Zero Breaking Changes for Users: All changes in local dev manifests only

Recommendations

Priority 1 (Before Merge)

  1. Remove or justify cluster-admin binding - Inconsistent with documented RBAC design
  2. Add inline documentation - Comment explaining why 8760h duration was chosen

Priority 2 (Follow-up PR or Issue)

  1. Reduce token duration - Consider 24h-168h for better security
  2. Document token expiration handling - How to detect/fix expired tokens
  3. Improve secret deletion logic - More informative on first run

Validation Checklist

Based on loaded memory files and project standards:

  • Authentication: Uses real ServiceAccount tokens (check)
  • Authorization: RBAC properly configured (check)
  • Token Security: Token stored in Secret, injected via secretKeyRef (check)
  • Error Handling: Appropriate error handling (check)
  • Code Quality: Clean Makefile additions (check)
  • RBAC Design: cluster-admin binding inconsistent with documented design (warning)
  • Documentation: Clear PR description (check)
  • Testing: Author tested locally (check)

Conclusion

This PR successfully fixes the authentication regression and modernizes the local development authentication flow.

Recommendation: Approve after addressing the cluster-admin RBAC concern (Priority 1). This can be done by either:

  1. Removing the binding (preferred - use existing namespace-scoped Role)
  2. Adding justification and documentation for why cluster-wide access is needed

The minor issues can be addressed in follow-up work.


Review completed by Claude Code
Loaded context: CLAUDE.md, backend-development.md, frontend-development.md, security-standards.md, k8s-client-usage.md, error-handling.md, react-query-usage.md


🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

  1. CLAUDE.md - Master project instructions, development standards
  2. backend-development.md - Go backend, K8s integration patterns
  3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
  4. security-standards.md - Auth, RBAC, token handling
  5. k8s-client-usage.md - User token vs service account patterns
  6. error-handling.md - Consistent error patterns
  7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.

@bobbravo2 bobbravo2 added this to the v0.0.17 milestone Jan 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants