update google mcp secret

[MichaelClifford]

No description provided.

[github-actions]

Claude Code Review

Summary

This PR updates the OAuth credential storage logic to minimize stored secrets by emptying sensitive fields (client_id, client_secret, token_uri, refresh_token) in the credentials JSON stored in Kubernetes Secrets. While the security intention is commendable, this change introduces critical functional risks that could break Google Workspace MCP integration.

Issues by Severity

🚫 Blocker Issues

1. Missing Refresh Token Breaks Token Renewal (oauth.go:682, 733)

credentials := map[string]interface{}{
    "token":         accessToken,
    "refresh_token": "",  // ❌ BLOCKER: Emptied
    "token_uri":     "",  // ❌ BLOCKER: Emptied
    "client_id":     "",  // ❌ BLOCKER: Emptied
    "client_secret": "",  // ❌ BLOCKER: Emptied
}

Why this is critical:

• Access tokens expire (typically 1 hour for Google OAuth)
• Without refresh_token, token_uri, client_id, and client_secret, the workspace-mcp server cannot renew the token
• Session will fail once the access token expires (~60 minutes into the session)
• No recovery mechanism - user would have to re-authenticate

Evidence from PR #483:

"Kubernetes syncs secret updates after OAuth completion (~60s)"
"Runner's refresh_google_credentials() detects when content appears"

The MCP integration expects valid credentials for token refresh, not just an initial access token.

Expected behavior:
Google's OAuth2 credentials format (per Google Identity docs) requires:

• token: Current access token ✅
• refresh_token: For obtaining new access tokens when expired ❌ REMOVED
• token_uri: Where to send refresh requests ❌ REMOVED
• client_id: OAuth client identifier ❌ REMOVED
• client_secret: OAuth client secret ❌ REMOVED
• scopes: Authorized scopes ✅
• expiry: Token expiration time ✅

Impact:

• 🔴 All Google MCP sessions will fail after ~1 hour
• 🔴 No token refresh possible
• 🔴 Silent failure - user won't know why MCP stopped working

🔴 Critical Issues

2. No Testing or Validation (Missing)

Why this is critical:

• No test coverage demonstrating this change works with workspace-mcp
• No evidence that workspace-mcp can function with empty credential fields
• Breaking change with no validation

Required before merge:

• Test with actual workspace-mcp server
• Verify token refresh still works
• Test session lasting > 1 hour

3. Inconsistent Security Model (oauth.go:732-733)

Data: map[string][]byte{
    "credentials.json": credentialsJSON,  // Empty sensitive fields
    "access_token":     []byte(accessToken),  // ✅ Still stored
    "refresh_token":    []byte(""),  // ❌ Now empty
}

Why this is problematic:

• Access token is still stored in plaintext (same security risk)
• Refresh token removed but access token kept (inconsistent)
• If security is the goal, both should be handled the same way

Security note: Kubernetes Secrets are base64-encoded, not encrypted by default. All values in secrets have the same security properties unless using encryption at rest.

🟡 Major Issues

4. Missing Architecture Decision Documentation (Missing ADR)

Why this matters:

• This is a breaking change to credential storage format
• No ADR explaining the decision, tradeoffs, or migration path
• Future maintainers won't understand why this was done

Recommendation:
Create an ADR documenting:

• Why minimize stored secrets?
• How does workspace-mcp handle missing fields?
• What happens when tokens expire?
• Migration path for existing secrets

5. Comment Inaccuracy (oauth.go:679)

// Prepare credentials JSON with only the token, scopes, and expiry
// client_id, client_secret, token_uri, and refresh_token are empty strings to minimize stored secrets

Issue: The comment says "to minimize stored secrets" but doesn't explain:

• What threat model this addresses
• Why K8s Secrets are insufficient
• How token refresh will work

Better comment:

// Prepare credentials JSON for workspace-mcp integration.
// Note: This stores only the access token, relying on [mechanism] for token renewal.
// See ADR-XXXX for security rationale and token refresh strategy.

Questions Requiring Answers

Before this PR can be approved, we need clear answers to:

1. How does workspace-mcp renew expired access tokens without refresh_token, client_id, client_secret, and token_uri?
2. Has this been tested with a session lasting > 1 hour?
3. What is the threat model? (What attack does removing these fields prevent?)
4. Is there external documentation that workspace-mcp can operate with these fields empty?
5. What happens to existing secrets? (Migration strategy?)

Alternative Solutions

If the goal is to reduce secret sprawl, consider these alternatives:

Option 1: Use Kubernetes Secret References

Store OAuth client secrets separately and reference them:

credentials := map[string]interface{}{
    "token":         accessToken,
    "refresh_token": refreshToken,
    "token_uri":     providerConfig.TokenURL,
    "client_id":     "{{ .Values.google.clientId }}",  // Reference, not value
    "client_secret": "{{ .Values.google.clientSecret }}",  // Reference, not value
}

Option 2: Use Encrypted Secrets

Enable Kubernetes encryption at rest for secrets in the ambient-code namespace.

Option 3: External Secret Management

Use external secret management (Vault, AWS Secrets Manager) for OAuth credentials.

Option 4: Short-Lived Credentials

Implement a credential refresh sidecar that periodically updates tokens.

Recommendations

Before merging:

1. 🚫 Block merge until token refresh mechanism is validated
2. Test with workspace-mcp for > 1 hour session duration
3. Document in ADR: security rationale, token refresh strategy, threat model
4. Add tests verifying credentials work with workspace-mcp
5. Clarify if this is compatible with Google OAuth2 credentials format

After validation:

If workspace-mcp truly doesn't need these fields (which seems unlikely), update:

• PR description explaining why this works
• Code comments with links to workspace-mcp docs
• Tests demonstrating > 1 hour sessions work

Pre-Commit Checklist (from CLAUDE.md)

• Token Security: ✅ No tokens in logs
• Error Handling: ⚠️ N/A (no new error paths)
• Type Safety: ✅ Correct types
• Resource Cleanup: ✅ OwnerReferences present
• Tests: ❌ No tests for this change
• Code Quality: ⚠️ Not run (Go linting)

Related Context

• PR fix: Create placeholder Google OAuth secret for MCP integration #483: Created placeholder secret for MCP integration
• workspace-mcp: Expects Google OAuth2 credentials at credentials.json
• Token expiry: Google access tokens typically expire in 3600s (1 hour)

---

Verdict: ❌ Request Changes

This PR cannot be merged without validation that workspace-mcp can renew tokens with empty credential fields. The current implementation will likely break all MCP sessions after 1 hour.

---

🔍 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.