Skip to content

Conversation

@fangpenlin
Copy link
Contributor

@fangpenlin fangpenlin commented Dec 3, 2025

Description 📣

ref: https://linear.app/infisical/issue/PAM-12/add-support-for-kubernetes-resource-in-pam

Type ✨

  • Bug fix
  • New feature
  • Improvement
  • Breaking change
  • Documentation

Tests 🛠️

Please read desc in this PR:

Infisical/infisical#4981 (comment)

@gitguardian
Copy link

gitguardian bot commented Dec 3, 2025

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@fangpenlin fangpenlin force-pushed the PAM-12-add-k8s-support branch from a92fbc7 to 77c7b6d Compare December 4, 2025 20:52
@fangpenlin fangpenlin force-pushed the PAM-12-add-k8s-support branch from f745441 to 56e8f74 Compare December 5, 2025 20:05
@fangpenlin fangpenlin marked this pull request as ready for review December 5, 2025 21:12
@greptile-apps
Copy link

greptile-apps bot commented Dec 5, 2025

Greptile Overview

Greptile Summary

This PR adds Kubernetes support to the PAM (Privileged Access Management) system, enabling users to access Kubernetes clusters through the Infisical gateway with session auditing. The implementation follows existing patterns for SSH and database PAM proxies.

Key Changes

  • New HTTP/WebSocket proxy handler in packages/pam/handlers/kubernetes/proxy.go for forwarding Kubernetes API requests
  • Local proxy server in packages/pam/local/kubernetes-proxy.go that automatically updates user's kubeconfig
  • Extended session logging to support HTTP events alongside existing terminal events
  • Added infisical pam kubernetes access-account CLI command with duration and port flags

Security & Quality Issues Found

  • DoS vulnerability: Unbounded request body reads at proxy.go:111 could exhaust memory - needs size limits
  • Memory exhaustion: Response bodies buffered entirely in memory for audit logging (proxy.go:210) - kubectl logs or kubectl cp could cause OOM
  • Audit gap: WebSocket traffic (kubectl exec sessions) bypasses audit logging entirely (proxy.go:273)
  • Sensitive data leak: Request/response headers logged without filtering (proxy.go:127)
  • Race condition: Kubeconfig file modified without locking (kubernetes-proxy.go:110) - concurrent writes could corrupt file
  • UX issue: Automatic kubeconfig modification without user opt-out option (kubernetes-proxy.go:109)
  • Documentation: CLI example shows incorrect path prod/ssh/my-k8s-account instead of kubernetes path (pam.go:167)

Positive Aspects

  • Clean integration with existing PAM architecture
  • Proper TLS certificate handling and validation
  • Session expiry and credential caching implemented correctly
  • Encrypted session recording with proper lifecycle management

Confidence Score: 2/5

  • This PR has multiple critical security issues including DoS vulnerabilities and audit logging gaps that need resolution before merging
  • The implementation has good architecture but contains several critical issues: (1) unbounded memory reads create DoS vectors that could crash the gateway, (2) WebSocket exec sessions have no audit logging defeating the PAM system's purpose, (3) race conditions in kubeconfig file writes could corrupt user configs. While the code structure is solid, these security and reliability issues make it unsafe to merge without fixes.
  • Primary attention needed on packages/pam/handlers/kubernetes/proxy.go for DoS vulnerabilities and audit logging gaps, and packages/pam/local/kubernetes-proxy.go for file locking issues

Important Files Changed

File Analysis

Filename Score Overview
packages/pam/handlers/kubernetes/proxy.go 3/5 New Kubernetes proxy handler with HTTP/WebSocket forwarding - has unaddressed DoS risk and missing WebSocket frame parsing for audit logs
packages/pam/local/kubernetes-proxy.go 4/5 Local Kubernetes proxy server with automatic kubeconfig updates - modifies user's kubeconfig without opt-out option
packages/pam/pam-proxy.go 5/5 Adds Kubernetes case to PAM proxy handler - clean integration with existing patterns

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 files reviewed, 7 comments

Edit Code Review Agent Settings | Greptile

Comment on lines 109 to 137
// TODO: we should let the user decide whether if they want to update kubeconfig or not
// TODO: ideally, lock the files to avoid others from writing to it
// TODO: use clientcmd.ModifyConfig instead?
configLoader := clientcmd.NewDefaultClientConfigLoadingRules()
config, err := configLoader.Load()
if err != nil {
log.Fatal().Err(err).Msg("Failed to load kubernetes config")
return
}
clusterName := fmt.Sprintf("infisical-k8s-pam%s%s", actualAccountPath, accountName)

config.Clusters[clusterName] = &k8sapi.Cluster{
Server: fmt.Sprintf("http://localhost:%d", proxy.port),
}
config.AuthInfos[clusterName] = &k8sapi.AuthInfo{}
config.Contexts[clusterName] = &k8sapi.Context{
Cluster: clusterName,
AuthInfo: clusterName,
}
proxy.kubeConfigOriginalContext = config.CurrentContext
config.CurrentContext = clusterName
kubeconfig := configLoader.GetDefaultFilename()
if err = clientcmd.WriteToFile(*config, kubeconfig); err != nil {
log.Fatal().Err(err).Str("kubeconfig", kubeconfig).Msg("Failed to write kubernetes config")
return
}
log.Info().Str("kubeconfig", kubeconfig).Msg("Updated kubeconfig file")
proxy.kubeConfigClusterName = clusterName
proxy.kubeConfigPath = kubeconfig
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Automatic kubeconfig modification without user consent

The code automatically modifies the user's kubeconfig file and switches their context. The TODO on line 109 acknowledges users should be able to opt out, but this isn't implemented. Consider adding a --no-update-kubeconfig flag.

Comment on lines +110 to +111
// TODO: ideally, lock the files to avoid others from writing to it
// TODO: use clientcmd.ModifyConfig instead?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Race condition: kubeconfig not locked during modification

Multiple processes could corrupt the kubeconfig if they write simultaneously. Consider using file locking (e.g., syscall.Flock) before modifying the kubeconfig.

Use: "access-account <account-path>",
Short: "Access Kubernetes PAM account",
Long: "Access Kubernetes via a PAM-managed Kubernetes account. This command automatically launches a proxy connected to your Kubernetes cluster through the Infisical Gateway.",
Example: "infisical pam kubernetes access-account prod/ssh/my-k8s-account --duration 2h",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Example shows incorrect path

The example uses prod/ssh/my-k8s-account but should use prod/kubernetes/... or similar to match the resource type.

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.

3 participants