External credential providers KEP follow-ups

[enj]

Tracking work from kubernetes/enhancements#1711

Not all of these are required for GA

• Update KEP to include new API struct
• Doc updates, especially around new API changes [WIP] [Placeholder] Update external credential providers docs with GA details website#21626
• Implement new API changes
  • Wire cluster info exec credential provider: wire in cluster info #91192
  • Add install hint exec credential provider: add install hint #91217 / exec credential provider: add install hint #91305
• Deprecate gcp and azure authentication options
• Deprecate v1alpha1 and v1beta1 APIs
• Create v1 API
• Should we add a config knob to allow the plugin to indicate that it never returns a cert to prevent onRotateListWarningLength warning?
• Reach out to conformance working group. We are aiming for good test coverage for GA and not conformance, but it may be good to have a discussion around this.
• Review implementations
  • Make sure they are not hacking around limitations of the API
  • Inform them that they should move to v1
• Address open issues
  • kubecetl client-go exec credentials plugin should use new valid token as soon as stored rather than old expired #87369 / Presence of bearer token should cancel exec action #91745
  • exec auth provider expiration causes problems tilt-dev/tilt#2702 / cache: add error handling to informers #87329 (comment)
  • Verify behavior of exec auth plugin when credentials expire #89114
• Explore options to harden the attack surface around execution of arbitrary binaries:

* Verify that the execution options are not controlled by untrusted users. This validation would be similar to what https://github.com/google/path-auditor preforms.
  
  * Verify that the kubeconfig is only under the control of the exec'ing user and/or root.
  * Verify that the exec'd binary is only under the control of the exec'ing user and/or root.

* Add option to verify that the exec'd binary matches some hash. sudoers configuration allows this. A more complex alternative is to add support for code signing.

* Explore cheap sandboxing options of the exec'd process.

/assign
/sig auth

[ankeesler]

Opened #91217 to specifically track install hint bit. Associated PR is here: #91305.

Open to other ways of tracking this work.

[anderseknert]

Maybe missing something obvious, but having worked for quite some time on a custom exec credential plugin it's not really clear to me how a plugin is meant to communicate failures. Our plugin sends the user to do an interactive MFA, but returning from that the only options are to return either a token or a cert - if the external authentication process returns failures, obligations or anything but a successful response it would make sense to me that I'd be able to pass that along to the exec plugin and have the message displayed in the kubectl output.

[ankeesler]

@anderseknert - could you return a non-0 exit code from your plugin? Looks like today that non-0 exit code should tell kubectl (or some other client-go consumer) that the exec credential plugin failed[0]. At least for kubectl, the stderr output from the plugin shows up on the stderr output stream of the kubectl process[1].

One question I do have, though, is that since stderr of the plugin is printed to stderr of the client-go process, then does that mean that logs that are meant to only contain JSON will now contain non-JSON....?

[0]

kubernetes/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

Lines 325 to 327 in 7b0d784

	if err := cmd.Run(); err != nil {
	return fmt.Errorf("exec: %v", err)
	}

[1]

kubernetes/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

Line 133 in 7b0d784

stderr: os.Stderr,

[anderseknert]

Thanks @ankeesler! Yeah, it's been a while since I worked on our plugin but I think we ended up doing something like that but thinking it didn't feel very "native". Good to know that it is. I think it would make a good addition to the docs on the topic as they currently say nothing about dealing with errors. Also, interesting question!

[enj]

@anderseknert thanks for your feedback. We really appreciate having real world usage data to help us understand if exec is ready to GA.

I think that the KEP fails to outline how to communicate failure correctly.

We describe how standard input is used:

Note that an environment variable is used over passing this information via standard input because standard input is reserved for interactive flows between the user and executable (i.e. to prompt for username and password).

And standard output:

The command is exec'd and outputs structured stdout holding credentials.

But we fail to describe that if cmd.Run() fails per:

// The returned error is nil if the command runs, has no problems
// copying stdin, stdout, and stderr, and exits with a zero exit
// status.

then client-go will fail the overall process. We also do not describe that standard error from the plugin is passed through to client-go's standard error.

Per Andrew's comment, I am wondering if we should store the standard error output in a buffer and just include it in the returned error instead of directly writing to client-go's standard error. This would give the caller more flexibility on how to print the error.

I think an update to the KEP should be made to help provide more clarity here.

[anderseknert]

Appreciate hearing that @enj 👍 If I was to write a new exec credential plugin from scratch today I might have done some things differently knowing what I know now, but the idea at the time was to extend an existing company tool that retrieved credentials (ID token) with an --exec option, which would do pretty much the same but save the credentials to a kubeconfig file and then print them in the expected output format to stdout. Next time it would check the kubeconfig before trying to retrieve them. I remember you recommended against saving the token to the kubeconfig file and so I changed that, somewhat circumventing the issue I've reported from that experience.

This worked fairly well, but a potential problem of kubectl hijacking stdout for everything is that although a terrible practice, some libraries and dependencies may under some circumstances print stuff to stdout. And sometimes, like with us extending an existing app one might not want to change the whole logging logic of that app just for this new use case. AND.. sometimes you even want your app and dependencies to be loud, like when developing and debugging your plugin with the equivalent of kubectl -v 10. This has me thinking that perhaps an option to buffering output to stderr would be to grab stdout, but instead of failing - just pass any non-conforming (i.e. not a ExecCredential object) onwards to stdout and let the plugin log whatever it normally would until an ExecCredential object is caught.. and only then assume control and act on what's presented therein, possibly including an error field and a reason. Does that make sense?

[enj]

but save the credentials to a kubeconfig file and then print them in the expected output format to stdout. Next time it would check the kubeconfig before trying to retrieve them. I remember you recommended against saving the token to the kubeconfig file

Yeah that will be part of the doc updates for GA, per the KEP note:

Docs describe set of best practices (i.e. do not mutate kubeconfig)

This worked fairly well, but a potential problem of kubectl hijacking stdout for everything is that although a terrible practice, some libraries and dependencies may under some circumstances print stuff to stdout. And sometimes, like with us extending an existing app one might not want to change the whole logging logic of that app just for this new use case. AND.. sometimes you even want your app and dependencies to be loud, like when developing and debugging your plugin with the equivalent of kubectl -v 10.

I think part of the issue here is that an exec based model is inherently limited to environment vars + stdin as input, and stdout + stderr as output. Since all these are global to the binary, it is trivial for them to get polluted accidentally. These are probably not the best building blocks for an API, but it is trivial to print a string to stdout, so it made the barrier to entry very small (and it mimics how the GCP plugin works).

I think at some level the belief is that you control the binary you are using, so you can make it do anything. In a sense, instead of extending an existing app with the --exec flag, you would create a new binary (possibly in the same codebase) that only did the exec bits. This prevents the problem of trying to make different use cases agree at the cost of having to distribute a new binary. The Azure folks seem to have gone this route by creating https://github.com/Azure/kubelogin instead of extending https://github.com/Azure/azure-cli.

This has me thinking that perhaps an option to buffering output to stderr would be to grab stdout, but instead of failing - just pass any non-conforming (i.e. not a ExecCredential object) onwards to stdout and let the plugin log whatever it normally would until an ExecCredential object is caught.. and only then assume control and act on what's presented therein, possibly including an error field and a reason. Does that make sense?

I think this approach may muddy up the API contract a bit too much. The exec plugin can already basically do anything with stdin and stderr. It seems fair to leave some level of contractual obligation to only emit ExecCredential on stdout. Even if you do not control the exec binary, you could certainly wrap it with a shell script (possibly inline to the kubeconfig) and force everything that is not an ExecCredential object to stderr.

I think Go locks stdout to a single writer at a time, but I am not sure all languages do this. In particular, I am not sure if it would be possible to get interleaved writes to stdout if you have multiple writers (and buffering).

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

Instructions 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/test-infra repository.