Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client-go: add an exec-based client auth provider #59495

Merged
merged 2 commits into from Mar 1, 2018

Conversation

@ericchiang
Copy link
Member

ericchiang commented Feb 7, 2018

Updates kubernetes/enhancements#541
Implements kubernetes/community#1503
Closes #57164

client-go: alpha support for exec-based credential providers

/sig auth
/kind feature

@caesarxuchao

This comment has been minimized.

Copy link
Member

caesarxuchao commented Feb 8, 2018

@ericchiang

This comment has been minimized.

Copy link
Member Author

ericchiang commented Feb 8, 2018

// Response defines metadata about a failed request, including HTTP status code and
// response headers.
type Response struct {
// HTTP header returned by the server.

This comment has been minimized.

@php-coder

php-coder Feb 8, 2018

Contributor

s/header/headers/

@@ -0,0 +1,4 @@
#!/bin/sh -e

This comment has been minimized.

@php-coder

php-coder Feb 8, 2018

Contributor

I'd rather name it test-plugin.sh.

@@ -0,0 +1,4 @@
#!/bin/sh -e

(>&2 echo "$KUBERNETES_EXEC_INFO")

This comment has been minimized.

@php-coder

php-coder Feb 8, 2018

Contributor

Why do we need a sub-shell here? And how it differs from echo >&2 "$KUBERNETES_EXEC_INFO" that is easier to understand IMHO.

This comment has been minimized.

@ericchiang

ericchiang Feb 8, 2018

Author Member

No reason. Updated.

@ericchiang ericchiang force-pushed the ericchiang:client-auth-exec branch 2 times, most recently from 4aeb04b to ffcd361 Feb 8, 2018

@ericchiang

This comment has been minimized.

Copy link
Member Author

ericchiang commented Feb 8, 2018

Flaking on #59426

/test pull-kubernetes-unit

@@ -119,6 +119,9 @@ type AuthInfo struct {
// AuthProvider specifies a custom authentication plugin for the kubernetes cluster.
// +optional
AuthProvider *AuthProviderConfig `json:"auth-provider,omitempty"`
// Exec specifies a custom exec-based authentication plugin for the kubernetes cluster.
// +optional
Exec *ExecConfig `json:"exec,omitempty"`

This comment has been minimized.

@mikedanese

mikedanese Feb 9, 2018

Member

Why did you decide not to implement this as another AuthProvider?

This comment has been minimized.

@ericchiang

ericchiang Feb 9, 2018

Author Member

Was called out in the proposal. We know the structure and map[string]string is awkward for expressing things like a list of key/value environment variables.

https://github.com/kubernetes/community/blob/master/contributors/design-proposals/auth/kubectl-exec-plugins.md#kubeconfig-changes

This comment has been minimized.

@mikedanese
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// +genclient

This comment has been minimized.

@mikedanese

mikedanese Feb 9, 2018

Member

Do we need this?

)

func Convert_clientauthentication_ExecCredentials_To_v1alpha1_ExecCredentials(in *clientauthentication.ExecInfo, out *ExecInfo, s conversion.Scope) error {
return autoConvert_clientauthentication_ExecInfo_To_v1alpha1_ExecInfo(in, out, s)

This comment has been minimized.

@mikedanese

mikedanese Feb 9, 2018

Member

Why do we need this if it just calls autoConvert?

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// ExecCredentials are credentials returned by the plugin.
type ExecCredentials struct {

This comment has been minimized.

@mikedanese

mikedanese Feb 9, 2018

Member

Request/response non-storage APIs have used a single type and spec/status in the past. Any reason to deviate from that convention?

This comment has been minimized.

@ericchiang

ericchiang Feb 9, 2018

Author Member

Good point, I don't have a good reason. Switched to spec/status.

func (c *cache) get(s string) (*Authenticator, bool) {
c.mu.Lock()
defer c.mu.Unlock()
a, ok := c.m[s]

This comment has been minimized.

@mikedanese

mikedanese Feb 9, 2018

Member

return c.m[s]

This comment has been minimized.

@ericchiang

ericchiang Feb 9, 2018

Author Member

Doesn't seem like that works https://play.golang.org/p/2dzFfrrGBkR

c.mu.Lock()
defer c.mu.Unlock()
if len(c.m) == 0 {
c.m = make(map[string]*Authenticator)

This comment has been minimized.

@mikedanese

mikedanese Feb 9, 2018

Member

why not expect this to happen on cache initialization?

)

func cacheKey(c *api.ExecConfig) string {
h := fnv.New32a()

This comment has been minimized.

@mikedanese

mikedanese Feb 9, 2018

Member

The hash map is already hashing. Do we need to double hash?

a.group, schema.GroupVersion{Group: gvk.Group, Version: gvk.Version})
}

if !creds.Expiry.IsZero() {

This comment has been minimized.

@mikedanese

mikedanese Feb 9, 2018

Member

isn't this if block the same as a.exp = creds.Expiry.Time?

@ericchiang ericchiang force-pushed the ericchiang:client-auth-exec branch 3 times, most recently from da3b7e9 to 08fedf5 Feb 9, 2018

@nckturner

This comment has been minimized.

Copy link
Contributor

nckturner commented Feb 27, 2018

LGTM

@ericchiang

This comment has been minimized.

Copy link
Member Author

ericchiang commented Feb 27, 2018

/assign @smarterclayton

For final approval

type ExecCredentialStatus struct {
// ExpirationTimestamp indicates a time when the provided credentials expire.
// +optional
ExpirationTimestamp metav1.Time `json:"expirationTimestamp,omitempty"`

This comment has been minimized.

@liggitt

liggitt Feb 28, 2018

Member

this has to be a pointer if you actually want it omitted from serialization

}

a := &Authenticator{
cmd: config.Command,

This comment has been minimized.

@liggitt

liggitt Feb 28, 2018

Member

it seems like there are three possibilities for this path:

  • relative (e.g. ./foo)
  • absolute (e.g. /path/to/foo)
  • $PATH-based (e.g. curl)

Was there discussion about how to resolve the referenced command here?

There is already code for resolving files referenced from kubeconfig files so that relative paths are resolved relative to the kubeconfig file (see ResolvePaths and GetAuthInfoFileReferences).

My instinct would be to resolve explicitly relative paths ./foo in GetAuthInfoFileReferences, leave absolute paths alone, and resolve bare commands (e.g. curl) using $PATH. Thoughts?

This comment has been minimized.

@ericchiang

ericchiang Feb 28, 2018

Author Member

I like keeping the relative path logic the same as other files in kubeconfig files. Let me update this.

This comment has been minimized.

@liggitt

liggitt Feb 28, 2018

Member

I guess if you want $PATH behavior you can invoke a local script

This comment has been minimized.

@ericchiang

ericchiang Feb 28, 2018

Author Member

I guess if you want $PATH behavior you can invoke a local script

exec.Command already does that: https://golang.org/pkg/os/exec/#Command

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 28, 2018

no other comments from me

@ericchiang ericchiang force-pushed the ericchiang:client-auth-exec branch from e89b3c0 to 36d94c5 Feb 28, 2018

ericchiang added some commits Feb 7, 2018

@ericchiang ericchiang force-pushed the ericchiang:client-auth-exec branch from 36d94c5 to 01801ae Feb 28, 2018

return []*string{&authInfo.ClientCertificate, &authInfo.ClientKey, &authInfo.TokenFile}
s := []*string{&authInfo.ClientCertificate, &authInfo.ClientKey, &authInfo.TokenFile}
// Only resolve exec command if it isn't PATH based.
if authInfo.Exec != nil && strings.ContainsRune(authInfo.Exec.Command, filepath.Separator) {

This comment has been minimized.

@ericchiang

ericchiang Feb 28, 2018

Author Member

Relative logic added

This comment has been minimized.

@liggitt

liggitt Feb 28, 2018

Member

SGTM... make sure this nuance makes it into the docs ("./foo is relative to the kubeconfig file, foo looks in the $PATH")

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 28, 2018

/lgtm

@ericchiang

This comment has been minimized.

Copy link
Member Author

ericchiang commented Feb 28, 2018

(nits addressed)

@ericchiang ericchiang added the lgtm label Feb 28, 2018

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Mar 1, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 1, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ericchiang, liggitt, mikedanese, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Mar 1, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Mar 1, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit cb9d6b5 into kubernetes:master Mar 1, 2018

13 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-verify
Details
cla/linuxfoundation ericchiang authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Context retired. Status moved to "pull-kubernetes-integration".
pull-kubernetes-verify Job succeeded.
Details
@rajatjindal

This comment has been minimized.

Copy link
Contributor

rajatjindal commented Mar 24, 2018

Hi @ericchiang

is there any documentation that I can refer to to use this. I tried with 1.10 RC kubectl, but it does not seem to call my external command at all.

cben added a commit to cben/kubeclient that referenced this pull request Dec 9, 2018

Credential plugins: lookup command relative to config's dir, nil = op…
…t out

Matches Go client's behavior:
kubernetes/kubernetes#59495 (comment)
Distinguish 3 cases:
- absolute (e.g. /path/to/foo)
- $PATH-based (e.g. curl)
- relative to config file's dir (e.g. ./foo)

If base dir explicitly set to nil, refuse to execute external commands,
matching existing opt-out from file lookups (abonas#372).

Document security aspects of credential plugins.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.