Restrict OAuth scope to prevent console and kubectl access #220
Conversation
Changed OAuth proxy scope from 'user:full' to 'user:info' to allow users to authenticate to vTeam via Google OAuth while blocking access to the OpenShift console and kubectl/oc CLI commands. Changes: - Updated frontend-deployment.yaml OAuth proxy scope - Added security documentation to OPENSHIFT_OAUTH.md - Created deployment guide in OAUTH_SCOPE_RESTRICTION.md This provides app-only authentication without granting cluster privileges. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Pull Request Review: OAuth Scope RestrictionSummaryThis PR implements an important security improvement by restricting OAuth scope from Code Quality: ✅ ExcellentStrengths
Security Analysis: ✅ StrongPositive Security Improvements
Potential ConsiderationsCritical: Verify
|
Relocated OAUTH_SCOPE_RESTRICTION.md to docs/ for better organization alongside other documentation files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Pull Request Review: OAuth Scope RestrictionSummaryThis PR implements an important security improvement by restricting the OAuth scope from ✅ Strengths1. Security Enhancement
2. Excellent Documentation
3. Minimal Code Change
4. Good Commit Hygiene
🔍 Observations & Recommendations1. Backend Service Account Separation
|
Created comprehensive integration tests to validate that the OAuth scope restriction (user:info) allows vTeam application authentication while blocking OpenShift console and kubectl access. Test coverage includes: - User authentication with user:info scope - AgenticSession CRUD operations via API - Cluster-wide access blocking (namespaces, nodes, cluster roles) - Namespace isolation verification - OAuth delegate URLs functionality Test structure: - tests/integration/helpers.go - Test utilities and configuration - tests/integration/oauth_scopes_test.go - OAuth scope restriction tests Run tests with: TEST_NAMESPACE=vteam-test make test-integration Addresses PR ambient-code#220 review comments requesting integration tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: OAuth Scope RestrictionI've reviewed PR #220 which implements OAuth scope restriction from ✅ Strengths1. Security Improvement
2. Comprehensive Test Coverage
3. Code Quality
🔍 Issues to AddressHIGH PRIORITY1. Variable Shadowing in Test Loop (oauth_scopes_test.go:202)for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := tc.operation() // ⚠️ 'tc' shadows the outer TestConfig variableProblem: The loop variable Fix: Rename the loop variable: for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
err := testCase.operation()
if err != nil {
assert.True(t, errors.IsForbidden(err), "Expected Forbidden error, got: %v", err)
t.Logf("✓ Cluster operation correctly blocked: %s", testCase.name)
} else {
t.Errorf("⚠️ Cluster operation succeeded when it should be blocked: %s", testCase.name)
}
})
}Location: components/backend/tests/integration/oauth_scopes_test.go:202-214 MEDIUM PRIORITY2. Hard-Coded Sleep for RBAC PropagationMultiple instances of
Problem: Hard-coded sleeps are brittle and can cause flaky tests in slow environments. Recommendation: Implement a retry-with-backoff pattern: // WaitForRBACPropagation retries an operation until it succeeds or times out
func (tc *TestConfig) WaitForRBACPropagation(t *testing.T, ctx context.Context, checkFunc func() error) error {
t.Helper()
timeout := 30 * time.Second
interval := 1 * time.Second
deadline := time.Now().Add(timeout)
for time.Now().Before(deadline) {
if err := checkFunc(); err == nil {
return nil
}
time.Sleep(interval)
}
return fmt.Errorf("RBAC propagation timed out after %v", timeout)
}Usage: // Instead of time.Sleep(2 * time.Second)
err := tc.WaitForRBACPropagation(t, ctx, func() error {
allowed := tc.PerformSelfSubjectAccessReview(t, ctx, "agenticsessions", "list", tc.Namespace)
if !allowed {
return fmt.Errorf("RBAC not ready")
}
return nil
})
require.NoError(t, err, "RBAC failed to propagate")3. Incomplete Namespace CleanupThe Current Code (helpers.go:220-248): func (tc *TestConfig) Cleanup(t *testing.T, ctx context.Context) {
// ... deletes SA and RB only
t.Logf("Cleanup completed")
}Recommendation: Add namespace cleanup option: func (tc *TestConfig) Cleanup(t *testing.T, ctx context.Context, cleanupNamespace bool) {
t.Helper()
if !tc.CleanupEnabled {
t.Logf("Cleanup disabled, keeping test resources in namespace: %s", tc.Namespace)
return
}
t.Logf("Cleaning up test resources in namespace: %s", tc.Namespace)
// Delete RoleBindings
for _, rbName := range tc.RoleBindings {
err := tc.K8sClient.RbacV1().RoleBindings(tc.Namespace).Delete(ctx, rbName, metav1.DeleteOptions{})
if err != nil && !errors.IsNotFound(err) {
t.Logf("Warning: Failed to delete role binding %s: %v", rbName, err)
}
}
// Delete ServiceAccounts
for _, saName := range tc.ServiceAccounts {
err := tc.K8sClient.CoreV1().ServiceAccounts(tc.Namespace).Delete(ctx, saName, metav1.DeleteOptions{})
if err != nil && !errors.IsNotFound(err) {
t.Logf("Warning: Failed to delete service account %s: %v", saName, err)
}
}
// Optionally delete namespace if it was created for testing
if cleanupNamespace {
err := tc.K8sClient.CoreV1().Namespaces().Delete(ctx, tc.Namespace, metav1.DeleteOptions{})
if err != nil && !errors.IsNotFound(err) {
t.Logf("Warning: Failed to delete namespace %s: %v", tc.Namespace, err)
} else {
t.Logf("Deleted test namespace: %s", tc.Namespace)
}
}
t.Logf("Cleanup completed")
}Note: Update defer calls: 4. Missing Context Timeout in Test FunctionsTests use Recommendation: Add timeouts to test contexts: func TestOAuthScopeRestriction(t *testing.T) {
if testing.Short() {
t.Skip("Skipping integration test in short mode")
}
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
defer cancel()
tc := NewTestConfig(t)
defer tc.Cleanup(t, ctx)
// ... rest of test
}LOW PRIORITY (Nitpicks)5. Token Unused in testVerifyAgenticSessionAccessThe Current: token := tc.GetServiceAccountToken(t, ctx, saName)
require.NotEmpty(t, token, "Failed to get service account token")
// token is never used againOptions:
6. Inconsistent Comment StyleSome functions have detailed godoc comments, others don't. Examples:
Recommendation: Add comprehensive godoc comments to all exported test functions. 7. Hard-Coded CRD ReferencesThe test assumes specific ClusterRoles exist ( Current (oauth_scopes_test.go:102): tc.CreateRoleBinding(t, ctx, "test-session-edit-binding", "ambient-project-edit", saName)Recommendation: Add validation or skip tests if CRDs/ClusterRoles don't exist: // Check if required ClusterRole exists
_, err := tc.K8sClient.RbacV1().ClusterRoles().Get(ctx, "ambient-project-edit", metav1.GetOptions{})
if errors.IsNotFound(err) {
t.Skip("Required ClusterRole 'ambient-project-edit' not found, skipping test")
}
require.NoError(t, err, "Failed to check for ClusterRole")📋 Code Quality ChecklistBased on CLAUDE.md guidelines:
🔒 Security ConsiderationsValidated ✅
Additional Recommendations
📊 Performance Considerations
📝 Documentation ReviewOAUTH_SCOPE_RESTRICTION.md ✅
OPENSHIFT_OAUTH.md ✅
🎯 Recommendations SummaryMust Fix (Blocking)
Should Fix (Before Merge)
Nice to Have (Post-Merge)
✨ Overall AssessmentQuality: 8.5/10 This is a well-implemented security improvement with excellent test coverage and documentation. The code follows established patterns from CLAUDE.md and demonstrates good understanding of Kubernetes RBAC testing. Main concerns:
Once the variable shadowing is fixed and RBAC propagation is made more robust, this PR will be in excellent shape for merge. Review conducted by Claude Code following vTeam standards from CLAUDE.md |
|
Haven't tested it. |
|
Tracked in Jira: https://issues.redhat.com/browse/RHOAIENG-39131 |
|
I just don't have a way to test this. |
3 participants
Changed OAuth proxy scope from 'user:full' to 'user:info' to allow users to authenticate to vTeam via Google OAuth while blocking access to the OpenShift console and kubectl/oc CLI commands.
Changes:
This provides app-only authentication without granting cluster privileges.
🤖 Generated with Claude Code