Adds support for exec credential plugin

[motymichaely]

Hey!

This PR offers a partial implementation of the Kube Config ExecCredential plugin (#362).

This was Inspired by: https://github.com/kubernetes/client-go/blob/master/plugin/pkg/client/auth/exec/exec.go

Things to consider:

1. Add support for Client Certificate and Key data fields.
2. Credentials refresh
3. Caching (with token expiration support)

[cben]

Neat! 👏
Relevant documentation: https://kubernetes.io/docs/reference/access-authn-authz/authentication/#client-go-credential-plugins

Havent reviewed in detail yet.

I wonder if this can/should be opt-in (a separate method like fetch_user_auth_options_unsafe or an UnsafeConfig subclass of Config, or something like that).
I worry that someone might take config file from untrusted source (file writable by others, or worst case is accepting config over the network). This is questionable idea, and README advises against it, and last time I searched github it seemed people don't really do this.
But anyway let's say someone did it. #334 plugged one direct risk I knew about. There are still some read-external-files risks. But now this will throw the gates wide open, elevating the problem to arbitrary command execution...

[cben]

Close-cycling to re-run tests fixed by #364.

[motymichaely]

@cben I believe we should add a warning in README. According to this client-go issue comment, exec credential will eventually replace any Azure, GCP and alike credential providers (which makes sense).

I am currently trying to figure out a way to implement token refreshment mechanism. There will be cases in which the client is initialized with a token that expires after an hour and a refreshing mechanism should come into play before the next API call is made. BTW - I think this is also true for GoogleApplicationDefaultCredentials token.

Any idea?

[rhodrid]

I need this for EKS on AWS which uses the aws-iam-authenticator helper. For now I've forked but would like to see this in master.

[cben]

Sorry for silence, I'm trying to land some discovery related stuff before fully context-switching to this...

exec credential will eventually replace any Azure, GCP and alike credential providers (which makes sense).

I need this for EKS on AWS which uses the aws-iam-authenticator helper

Thanks for the context. Yeah, this is important.
I'd still like to see an opt-out — IMHO a library should not offer dangerous functionality (especially "run arbitrary processes") without also offering a safe "just parse" functionality.
Let's see. As currently written config.context(name) will automatically do everything, including running exec commands. With the passive-sounding .context name, I find that a bit ... surprising :-(
Perhaps opt-in like .context(name, obtain_auth: true) might be better from purely "does what it says" perspective?

Now, about refreshment.That's a bigger effort, may be better to land this first.
It opens some questions about object lifetime (which we punted on in #213).
So far Client is effectively immutable. Do you want ability to update a single Client with new auth? Do you want it to happen automatically?

One idea I have is for Client instead of taking a static auth_options: hash, could take an active object with a method Client would invoke for every request asking for currently valid auth. That way, you can construct many clients (which are currently a frequent need for multiple API groups) from single config and not have to construct them again nor actively updating them. And it would support any static, or manually renewed, or automatically renewed, or BYO auth... What do you think?

[cben]

(sent too soon, edited now ^^)

[cben]

These string exceptions should become some exception class.
KubeException is deprecated because it's in global namespace, we've documented "The gem raises Kubeclient::HttpError or subclasses now." But Kubeclient::HttpError is inappropriate here.
This is not very important, until we actually try to remove KubeException.

What's more interesting is understand when this can be raised. If we'll support auto-renewal, then ANY request such as get_pods would potentially also raise these!
What exceptions does Config raise presently? I see KeyError, and one string 'Unknown kubeconfig version'.
You're adding some ArgumentError and strings.

Perhaps add Kubeclient::ConfigError for all config problems?

BTW, if we add opt-out, need to choose error for refusing to exec commands too.

[motymichaely]

@cben I agree, KubeClient::ConfigError can be a good candidate for configuration errors.

[motymichaely]

@cben For refreshing, I was thinking about the following:

1. Having bearer_token accept callable method which will return a valid token. We can then change this line to a block, that will be called here:

def bearer_token(bearer_token)
  token = bearer_token.respond_to?(:call) ? bearer_token.call : bearer_token
  @headers ||= {}
  @headers[:Authorization] = "Bearer #{token}"
end

2. Then, adding a retry mechanism to handle_exception when a 403 exception occurs (?). The retry mechanism can then refresh the token by calling bearer_token(@auth_options[:bearer_token]) again and retry the call:

def handle_exception
  retry_count = 0
  begin
    yield
  rescue RestClient::Exception => e
    if e.http_code == 403 
      retry_count += 1
      retry if retry_count < (@auth_options[:max_retries] || 1)
    end
    json_error_msg = begin
      JSON.parse(e.response || '') || {}
    rescue JSON::ParserError
      {}
    end
    err_message = json_error_msg['message'] || e.message
    error_klass = e.http_code == 404 ? ResourceNotFoundError : HttpError
    raise error_klass.new(e.http_code, err_message, e.response)
  end
end

Thoughts?

[cben]

Do we need retry, or do we have sufficient info to renew before it expires?

https://kubernetes.io/docs/reference/access-authn-authz/authentication/#input-and-output-formats

• If an expiry is included, the bearer token and TLS credentials are cached until the expiry time is reached, or if the server responds with a 401 HTTP status code, or when the process exits.
• If an expiry is omitted, the bearer token and TLS credentials are cached until the server responds with a 401 HTTP status code or until the process exits.

OK, we'll need ability to retry.

Is it always bearer tokens?

No, executed plugin might choose to return clientCertificateData & clientCertificateData. We don't know which it will be until we run it.
=> Can't replace bearer_token with a block. It's not even enough to allow whole auth_config to be an active object, because client certs come via ssl_options?
=> The PR as currently written assumes it's always ExecCredentials.token. (I'm fine with merging this part first if you want.)

Does caching belong in Client or elsewhere?

It should be possible to use one config shared by many Client objects (for different API groups) and obtain / renew the credentials only once.
So let's say we have a new auth object.
I feel Client should ask the auth object for connection options every time (in case it expired or was renewed via another Client).
handle_exception will still try to handle 401 but it'd do something like @auth_config.renew if @auth_config.respond_to?(:renew).
TODO: consider thread safety (requests from different threads hitting expiration or 401 simultaneously).

Are we OK with plugins blocking for TTY input?

When run from an interactive session, stdin is exposed directly to the plugin. Plugins should use a TTY check to determine if it’s appropriate to prompt a user interactively.

I guess that's OK.

[cben]

Did I mention that it's simpler to first land this without renewal? 😉

[f4tq]

Hi,

I just pulled this PR and rebased up and it works great on all my istio 1.0.4 eks (k8 10) & aks (k8 11.0.4) clusters.

Without this PR, kubeclient doesn't work with eks - period.

Kindly, take this PR and perfect it later.

👍

[cben]

note for future (not blocker): if command is relative path, the Go implemetation resolves it relative to the kubeconfig file
https://github.com/kubernetes/kubernetes/pull/59495/files/6463e9efd9ba552e60d2555a3e6526ef90196473#diff-4c107b9e9f7f10a98e5c52f66b952e01R562

[cben]

Sorry, should have merged before. Though I wanted to make some changes — at least document — before releasing.
I've discovered we already have a documented way to opt-out from any external-file lookups — specify nil as base directory — albeit the opt-out didn't actually work :-/ — fixed in #372.
So I felt I must extend it to external-file execution as well.
And implemented distinction between cmd and dir/cmd like in Go client.
=> merging this and opening followup PR => #375.

I'm not converting string exceptions to class(es). The Config code already raises a lot of non-KubeException/HttpError errors (tweaked README to not sound like it should)

[cben]

If you want to see this released, it'd help if someone can review my followup #375.

(at the moment I'm focused on fixing an already released regression (#376) but hopefully finishing that soon)