Add client-go credential plugin allowlist to kuberc#134870
Add client-go credential plugin allowlist to kuberc#134870k8s-ci-robot merged 1 commit intokubernetes:masterfrom
Conversation
|
Hi @pmengelbert. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/sig auth |
|
/ok-to-test |
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: bee30272d37f8c9f44904d85752b91c789574cbd |
|
/remove-label tide/merge-method-squash |
| } | ||
| } | ||
|
|
||
| func (a *Authenticator) checkAllowlist() error { |
There was a problem hiding this comment.
if the allowlist already contains the exact command, I think we can return early/fast (since LookPath would resolve them the same way)
- cmd="baz", allowlist=["foo","bar","baz"] can return true fast without needing to call LookPath at all
- cmd="baz", allowlist=["/usr/bin/foo","/usr/bin/bar","/usr/bin/baz"] has to call LookPath to resolve cmd, then can see that exists verbatim in the allowlist already, and doesn't need to call LookPath at all
- cmd="baz", allowlist=["../foo","../bar","../baz"] has to call LookPath to resolve cmd, and then LookPath on allowlist items to see if they match (but should probably check the one whose basename matches the cmd first to avoid calling LookPath a ton of times
| // will be called on both, and the resulting strings must be equal. If | ||
| // either call to `exec.LookPath` results in an error, the `Name` check | ||
| // will be considered a failure. | ||
| Name string `json:"name,omitempty"` |
There was a problem hiding this comment.
make sure the internal types are non-serializable with json:"-"
| import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| import ( | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| clientcmdapi "k8s.io/client-go/tools/clientcmd/api" |
There was a problem hiding this comment.
serializable v1beta1/types.go should only be referencing serializable versioned types, not internal types. I think it will be clearer to define these locally in staging/src/k8s.io/kubectl/pkg/config/types.go and staging/src/k8s.io/kubectl/pkg/config/v1beta1/types.go and translate them into internal clientcmd policy types in one spot in applyPluginPolicy
| // at the explicit path `/usr/local/bin/my-plugin`. | ||
| // +optional | ||
| // +listType=atomic | ||
| CredentialPluginAllowlist []clientcmdapi.AllowlistEntry `json:"credentialPluginAllowlist,omitempty"` |
There was a problem hiding this comment.
This is currently referencing types defined in internal packages and counting on them having json tags that make them serializable... that's pretty confusing.
Keep the kuberc types all defined under kubectl/pkg/config/types.go and kubectl/pkg/config/v1beta1/types.go where they can follow normal behavior for json tags
|
/retest |
2 similar comments
|
/retest |
|
/retest |
| ) | ||
|
|
||
| // compile-time check that these two types are aligned | ||
| var _ = clientcmdapi.AllowlistEntry(config.AllowlistEntry{}) |
There was a problem hiding this comment.
I'm surprised this works… did you make sure this flags struct differences if they occur?
|
/lgtm this lgtm, will add milestone once exception request is approved |
|
LGTM label has been added. DetailsGit tree hash: 44f560a44870fb672691d164a7dd6ef00b82aeb0 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, pmengelbert, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Remove reference to internal types in kuberc types * Remove unserialized types from public APIs Also remove defaulting * Don't do conversion gen for plugin policy types Because the plugin policy types are explicitly allowed to be empty, they should not affect conversion. The autogenerated conversion functions for the `Preference` type will leave those fields empty. * Remove defaulting tests Comments and simplifications (h/t jordan liggitt) Signed-off-by: Peter Engelbert <[email protected]>
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 7ef9367f4abe1ed346e105255bd50b818102ce0e |
|
/milestone v1.35 |
|
(exception was approved) |
| // will be called on both, and the resulting strings must be equal. If | ||
| // either call to `exec.LookPath` results in an error, the `Name` check | ||
| // will be considered a failure. | ||
| Name string |
There was a problem hiding this comment.
sigh... reviewing the docs PR, I just noticed this doesn't match the field name in kubeconfigs for exec plugins ... those use command
I think I got confused with the name field of kubelet image credential providers.
I think it's too late to rework this for 1.35, but let's make a note to rename this field to command in the v1 promotion to match kubeconfig
There was a problem hiding this comment.
Based on my understanding, I think there should also be a ticket to update the documentation, right? @soltysh
| } | ||
| entryResolvedPath, err := exec.LookPath(entry.Name) | ||
| if err != nil { | ||
| klog.V(5).ErrorS(err, "resolving credential plugin allowlist", "name", entry.Name) |
There was a problem hiding this comment.
Is this an error that someone has to see and react to or is this a debug message?
A helper method in client-go should not use the global klog. You don't know in which context it gets called and whether the caller wants this log output.
If a helper method uses logging, make the caller aware by having klog.Logger or context.Context in the API of the helper.
There was a problem hiding this comment.
Asking more broadly: what is the user experience if this happens? Can it happen?
There was a problem hiding this comment.
Also, should resolving the certificate in roundTripper.RoundTrip (= r.a.getCreds()) honor the cancellation of the context of the http.Request?
It seems so, because getCreds under the hood does some blocking and potentially long-running operations, like invoking a command, right?
Unfortunately that ripples up into the public client-go API 😢
Here's an incomplete patch (note the context.TODO when setting tlsConfig.GetClientCertificate):
diff --git a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go
index f21ca89bb16..e074569f905 100644
--- a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go
+++ b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go
@@ -18,6 +18,7 @@ package exec
import (
"bytes"
+ "context"
"crypto/tls"
"crypto/x509"
"errors"
@@ -364,7 +365,7 @@ func (r *roundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
}
ctx := req.Context()
- creds, err := r.a.getCreds()
+ creds, err := r.a.getCreds(ctx)
if err != nil {
return nil, fmt.Errorf("getting credentials: %v", err)
}
@@ -377,7 +378,7 @@ func (r *roundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
return nil, err
}
if res.StatusCode == http.StatusUnauthorized {
- if err := r.a.maybeRefreshCreds(creds); err != nil {
+ if err := r.a.maybeRefreshCreds(ctx, creds); err != nil {
klog.FromContext(ctx).Error(err, "Refreshing credentials failed")
}
}
@@ -391,15 +392,15 @@ func (a *Authenticator) credsExpired() bool {
return a.now().After(a.exp)
}
-func (a *Authenticator) cert() (*tls.Certificate, error) {
- creds, err := a.getCreds()
+func (a *Authenticator) cert(ctx context.Context) (*tls.Certificate, error) {
+ creds, err := a.getCreds(ctx)
if err != nil {
return nil, err
}
return creds.cert, nil
}
-func (a *Authenticator) getCreds() (*credentials, error) {
+func (a *Authenticator) getCreds(ctx context.Context) (*credentials, error) {
a.mu.Lock()
defer a.mu.Unlock()
@@ -407,7 +408,7 @@ func (a *Authenticator) getCreds() (*credentials, error) {
return a.cachedCreds, nil
}
- if err := a.refreshCredsLocked(); err != nil {
+ if err := a.refreshCredsLocked(ctx); err != nil {
return nil, err
}
@@ -416,7 +417,7 @@ func (a *Authenticator) getCreds() (*credentials, error) {
// maybeRefreshCreds executes the plugin to force a rotation of the
// credentials, unless they were rotated already.
-func (a *Authenticator) maybeRefreshCreds(creds *credentials) error {
+func (a *Authenticator) maybeRefreshCreds(ctx context.Context, creds *credentials) error {
a.mu.Lock()
defer a.mu.Unlock()
@@ -427,12 +428,12 @@ func (a *Authenticator) maybeRefreshCreds(creds *credentials) error {
return nil
}
- return a.refreshCredsLocked()
+ return a.refreshCredsLocked(ctx)
}
// refreshCredsLocked executes the plugin and reads the credentials from
// stdout. It must be called while holding the Authenticator's mutex.
-func (a *Authenticator) refreshCredsLocked() error {
+func (a *Authenticator) refreshCredsLocked(ctx context.Context) error {
interactive, err := a.interactiveFunc()
if err != nil {
return fmt.Errorf("exec plugin cannot support interactive mode: %w", err)
@@ -455,7 +456,7 @@ func (a *Authenticator) refreshCredsLocked() error {
env = append(env, fmt.Sprintf("%s=%s", execInfoEnv, data))
stdout := &bytes.Buffer{}
- cmd := exec.Command(a.cmd, a.args...)
+ cmd := exec.CommandContext(ctx, a.cmd, a.args...)
cmd.Env = env
cmd.Stderr = a.stderr
cmd.Stdout = stdout
@@ -463,7 +464,7 @@ func (a *Authenticator) refreshCredsLocked() error {
cmd.Stdin = a.stdin
}
- err = a.updateCommandAndCheckAllowlistLocked(cmd)
+ err = a.updateCommandAndCheckAllowlistLocked(ctx, cmd)
incrementPolicyMetric(err)
if err != nil {
return err
@@ -578,14 +579,14 @@ func (a *Authenticator) wrapCmdRunErrorLocked(err error) error {
// according to the credential plugin policy. If the plugin is allowed, `nil`
// is returned. If the plugin is not allowed, an error must be returned
// explaining why.
-func (a *Authenticator) updateCommandAndCheckAllowlistLocked(cmd *exec.Cmd) error {
+func (a *Authenticator) updateCommandAndCheckAllowlistLocked(ctx context.Context, cmd *exec.Cmd) error {
switch a.execPluginPolicy.PolicyType {
case "", api.PluginPolicyAllowAll:
return nil
case api.PluginPolicyDenyAll:
return fmt.Errorf("plugin %q not allowed: policy set to %q", a.cmd, api.PluginPolicyDenyAll)
case api.PluginPolicyAllowlist:
- return a.checkAllowlistLocked(cmd)
+ return a.checkAllowlistLocked(ctx, cmd)
default:
return fmt.Errorf("unknown plugin policy %q", a.execPluginPolicy.PolicyType)
}
@@ -593,7 +594,7 @@ func (a *Authenticator) updateCommandAndCheckAllowlistLocked(cmd *exec.Cmd) erro
// `checkAllowlistLocked` checks the specified plugin against the allowlist,
// and may update the Authenticator's allowlistLookup set.
-func (a *Authenticator) checkAllowlistLocked(cmd *exec.Cmd) error {
+func (a *Authenticator) checkAllowlistLocked(ctx context.Context, cmd *exec.Cmd) error {
// a.cmd is the original command as specified in the configuration, then filepath.Clean().
// cmd.Path is the possibly-resolved command.
// If either are an exact match in the allowlist, return success.
@@ -626,7 +627,7 @@ func (a *Authenticator) checkAllowlistLocked(cmd *exec.Cmd) error {
}
// There is no verbatim match
- a.resolveAllowListEntriesLocked(cmd.Path)
+ a.resolveAllowListEntriesLocked(ctx, cmd.Path)
// allowlistLookup may have changed, recheck
if a.allowlistLookup.Has(cmdResolvedPath) {
@@ -639,7 +640,7 @@ func (a *Authenticator) checkAllowlistLocked(cmd *exec.Cmd) error {
// resolveAllowListEntriesLocked tries to resolve allowlist entries with LookPath,
// and adds successfully resolved entries to allowlistLookup.
// The optional commandHint can be used to limit which entries are resolved to ones which match the hint basename.
-func (a *Authenticator) resolveAllowListEntriesLocked(commandHint string) {
+func (a *Authenticator) resolveAllowListEntriesLocked(ctx context.Context, commandHint string) {
hintName := filepath.Base(commandHint)
for _, entry := range a.execPluginPolicy.Allowlist {
entryBasename := filepath.Base(entry.Name)
@@ -649,7 +650,7 @@ func (a *Authenticator) resolveAllowListEntriesLocked(commandHint string) {
}
entryResolvedPath, err := exec.LookPath(entry.Name)
if err != nil {
- klog.V(5).ErrorS(err, "resolving credential plugin allowlist", "name", entry.Name)
+ klog.FromContext(ctx).V(5).Info("Resolving credential plugin allowlist", "name", entry.Name, "err", err)
continue
}
if entryResolvedPath != "" {
diff --git a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec_test.go b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec_test.go
index ada512c66a6..c39399104bd 100644
--- a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec_test.go
+++ b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec_test.go
@@ -18,6 +18,7 @@ package exec
import (
"bytes"
+ "context"
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rand"
@@ -46,6 +47,7 @@ import (
"k8s.io/client-go/pkg/apis/clientauthentication"
"k8s.io/client-go/tools/clientcmd/api"
"k8s.io/client-go/transport"
+ "k8s.io/klog/v2/ktesting"
testingclock "k8s.io/utils/clock/testing"
)
@@ -794,6 +796,7 @@ func TestRefreshCreds(t *testing.T) {
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
+ _, ctx := ktesting.NewTestContext(t)
c := test.config
if c.Command == "" {
@@ -817,7 +820,7 @@ func TestRefreshCreds(t *testing.T) {
a.stderr = stderr
a.environ = func() []string { return nil }
- if err := a.refreshCredsLocked(); err != nil {
+ if err := a.refreshCredsLocked(ctx); err != nil {
if !test.wantErr {
t.Errorf("get token %v", err)
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
@@ -966,6 +969,7 @@ func TestPluginPolicy(t *testing.T) {
tests := matrix.makeTests(t, testdataDir, path)
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
+ _, ctx := ktesting.NewTestContext(t)
c := test.config
a, err := newAuthenticator(newCache(), func(_ int) bool { return false }, c, &clientauthentication.Cluster{})
if err != nil {
@@ -982,7 +986,7 @@ func TestPluginPolicy(t *testing.T) {
a.stderr = stderr
a.environ = func() []string { return nil }
- if err := a.refreshCredsLocked(); err != nil {
+ if err := a.refreshCredsLocked(ctx); err != nil {
if !test.wantErr {
t.Fatalf("unexpected allows plugin error: %v", err)
} else if !strings.Contains(err.Error(), test.wantErrSubstr) {
@@ -1354,7 +1358,7 @@ func TestAuthorizationHeaderPresentCancelsExecAction(t *testing.T) {
// UpdateTransportConfig returns error on existing TLS certificate callback, unless a bearer token is present in the
// transport config, in which case it takes precedence
- cert := func() (*tls.Certificate, error) {
+ cert := func(context.Context) (*tls.Certificate, error) {
return nil, nil
}
tc := &transport.Config{TLS: transport.TLSConfig{Insecure: true, GetCertHolder: &transport.GetCertHolder{GetCert: cert}}}
@@ -1542,6 +1546,7 @@ func TestInstallHintRateLimit(t *testing.T) {
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
+ _, ctx := ktesting.NewTestContext(t)
c := api.ExecConfig{
Command: "does not exist",
APIVersion: "client.authentication.k8s.io/v1beta1",
@@ -1561,7 +1566,7 @@ func TestInstallHintRateLimit(t *testing.T) {
count := 0
for i := 0; i < test.calls; i++ {
- err := a.refreshCredsLocked()
+ err := a.refreshCredsLocked(ctx)
if strings.Contains(err.Error(), c.InstallHint) {
count++
}
diff --git a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics_test.go b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics_test.go
index b50ac89b668..cbd5a7568d0 100644
--- a/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics_test.go
+++ b/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/metrics_test.go
@@ -25,6 +25,7 @@ import (
"github.com/google/go-cmp/cmp"
"k8s.io/client-go/tools/clientcmd/api"
"k8s.io/client-go/tools/metrics"
+ "k8s.io/klog/v2/ktesting"
"k8s.io/utils/ptr"
)
@@ -112,6 +113,7 @@ func (f *mockCallsMetricCounter) Increment(exitCode int, errorType string) {
}
func TestCallsMetric(t *testing.T) {
+ _, ctx := ktesting.NewTestContext(t)
const (
goodOutput = `{
"kind": "ExecCredential",
@@ -149,7 +151,7 @@ func TestCallsMetric(t *testing.T) {
// Run refresh creds twice so that our test validates that the metrics are set correctly twice
// in a row with the same authenticator.
refreshCreds := func() {
- if err := a.refreshCredsLocked(); (err == nil) != (exitCode == 0) {
+ if err := a.refreshCredsLocked(ctx); (err == nil) != (exitCode == 0) {
if err != nil {
t.Fatalf("wanted no error, but got %q", err.Error())
} else {
@@ -179,7 +181,7 @@ func TestCallsMetric(t *testing.T) {
t.Fatal(err)
}
a.stderr = io.Discard
- if err := a.refreshCredsLocked(); err == nil {
+ if err := a.refreshCredsLocked(ctx); err == nil {
t.Fatal("expected the authenticator to fail because the plugin does not exist")
}
wantCallsMetrics = append(wantCallsMetrics, mockCallsMetric{exitCode: 1, errorType: "plugin_not_found_error"})
@@ -210,6 +212,7 @@ func (f *mockPolicyCallsMetricCounter) Increment(status string) {
}
func TestPolicyCallsMetric(t *testing.T) {
+ _, ctx := ktesting.NewTestContext(t)
const (
goodOutput = `{
"kind": "ExecCredential",
@@ -282,7 +285,7 @@ func TestPolicyCallsMetric(t *testing.T) {
}
a.stderr = io.Discard
- err = a.refreshCredsLocked()
+ err = a.refreshCredsLocked(ctx)
if err != nil && !test.wantDenied {
t.Fatalf("wanted no error, but got %q", err.Error())
}
diff --git a/staging/src/k8s.io/client-go/transport/cache_test.go b/staging/src/k8s.io/client-go/transport/cache_test.go
index 54705276d03..36040cf0428 100644
--- a/staging/src/k8s.io/client-go/transport/cache_test.go
+++ b/staging/src/k8s.io/client-go/transport/cache_test.go
@@ -69,7 +69,7 @@ func TestTLSConfigKey(t *testing.T) {
// Make sure config fields that affect the tls config affect the cache key
dialer := net.Dialer{}
- getCert := &GetCertHolder{GetCert: func() (*tls.Certificate, error) { return nil, nil }}
+ getCert := &GetCertHolder{GetCert: func(ctx context.Context) (*tls.Certificate, error) { return nil, nil }}
uniqueConfigurations := map[string]*Config{
"proxy": {Proxy: func(request *http.Request) (*url.URL, error) { return nil, nil }},
"no tls": {},
diff --git a/staging/src/k8s.io/client-go/transport/config.go b/staging/src/k8s.io/client-go/transport/config.go
index d8a3d64b309..37528f73c1f 100644
--- a/staging/src/k8s.io/client-go/transport/config.go
+++ b/staging/src/k8s.io/client-go/transport/config.go
@@ -156,5 +156,5 @@ type TLSConfig struct {
// GetCertHolder is used to make the wrapped function comparable so that it can be used as a map key.
type GetCertHolder struct {
- GetCert func() (*tls.Certificate, error)
+ GetCert func(ctx context.Context) (*tls.Certificate, error)
}
diff --git a/staging/src/k8s.io/client-go/transport/transport.go b/staging/src/k8s.io/client-go/transport/transport.go
index 8fdcc5700b5..62950aeedd5 100644
--- a/staging/src/k8s.io/client-go/transport/transport.go
+++ b/staging/src/k8s.io/client-go/transport/transport.go
@@ -187,7 +187,7 @@ func TLSConfigFor(c *Config) (*tls.Config, error) {
return dynamicCertLoader()
}
if c.HasCertCallback() {
- cert, err := c.TLS.GetCertHolder.GetCert()
+ cert, err := c.TLS.GetCertHolder.GetCert(context.TODO())
if err != nil {
return nil, err
}There was a problem hiding this comment.
We chose against using exec.CommandContext because many interactive authentication schemes can take longer than the request timeout, and we don't want users to get stuck if the plugin gets terminated before it has a chance to cache credentials after the initial slow login.
There was a problem hiding this comment.
We chose against using exec.CommandContext because many interactive authentication schemes can take longer than the request timeout, and we don't want users to get stuck if the plugin gets terminated before it has a chance to cache credentials after the initial slow login.
There was a problem hiding this comment.
We chose against using exec.CommandContext because many interactive authentication schemes can take longer than the request timeout, and we don't want users to get stuck if the plugin gets terminated before it has a chance to cache credentials after the initial slow login.
So you keep a goroutine in the client running which waits for the slow command after the request has already timed out? Or does RoundTrip keep running beyond its deadline?
What type of PR is this?
/kind feature
What this PR does / why we need it:
It permits the user to specify a client-go credential plugin allowlist as a security knob. The user may specify it in kuberc. See design details in KEP-3104
Which issue(s) this PR is related to:
KEP: kubernetes/enhancements#3104
Special notes for your reviewer:
Does this PR introduce a user-facing change?
yes.
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: