Reduce the lifetime of exported kubecfg credentials #9593

johngmyers · 2020-07-18T06:09:49Z

/approve cancel

johngmyers · 2020-07-18T06:10:46Z

/approve cancel

johngmyers · 2020-07-18T06:17:24Z

pkg/kubeconfig/create_kubecfg.go

+			Signer: fi.CertificateIDCA,
+			Type:   "client",
+			Subject: pkix.Name{
+				CommonName:   "kubecfg",


Perhaps we should use user.Current().Username instead?

Yeah, that's not a bad change to make at the same time. Or maybe "admin-kubecfg-${USER}", just in case of usage inside a container where I expect most of these will end up as "root" or "jenkins" which might be confusing.

cmd/kops/export_kubecfg.go

hakman · 2020-07-22T07:12:02Z

pkg/kubeconfig/create_kubecfg.go

 	"k8s.io/kops/upup/pkg/fi"
 )

-func BuildKubecfg(cluster *kops.Cluster, keyStore fi.Keystore, secretStore fi.SecretStore, status kops.StatusStore, configAccess clientcmd.ConfigAccess, admin bool, user string) (*KubeconfigBuilder, error) {
+const DefaultKubecfgAdminLifetime = 18 * time.Hour


Could you explain the 18 hours default?
It is a pretty big change from previous versions.

Ten years is an unreasonably long time to have an unrevokable credential.

18 hours lets an admin go a working day without having to obtain a new credential, yet makes it likely that their next day starts off with an expired credential. It is more convenient to have to reacquire a credential at the start of a task than mid-way through it.

Seems like a good explanation for the purpose of strong security.

IMHO, this is a step in the right direction, but may be a bit too extreme. I don't think there was a strong request to reduce the lifetime of the credentials so much. It may be a bit excessive for people with multiple clusters to do this every day, or for every test cluster. I would suggest to change the default to something like 7-30 days that would at least not require new exports for short lived clusters.

Also, an env var may be desirable to change the default to something more suitable for each operator.

7-30 days is too long for a default lifetime. Fixed-lifetime credentials that aren't themselves protected by other credentials typically have a lifetime no longer than a day. For example, temporary IAM credentials have a default lifetime of 12 hours.

Do we have a precedent/pattern for env vars providing defaults for flags? The env vars I can think of don't have corresponding flags. Do we want to adopt something like viper?

Specifically on the viper point, we do have limited precedent for --state KOPS_STATE_STORE and we even have a config file. However, I couldn't figure out how to get viper to just do this automatically - the state store configuration is a non-trivial amount of repetition. I'm not sure if we're "holding it wrong" but it wasn't very clear.

Multiple clusters (as hakman points out) is indeed going to be more annoying. I'm wondering if we should think about a helper process for login (that's essentially how GKE gets around this problem), or even nudging users to using a different authenticator, reserving the admin kubeconfig for "break glass".

I'm wondering if we could use a helper process to generate this on the fly, effectively chaining off your AWS credentials, so we'd inherit the same validity period.

A kubectl credential plugin is an interesting idea.

For production clusters it would be better to nudge users to use a different authenticator.

justinsb · 2020-07-26T21:13:51Z

cmd/kops/export_kubecfg.go

@@ -76,7 +77,8 @@ func NewCmdExportKubecfg(f *util.Factory, out io.Writer) *cobra.Command {

 	cmd.Flags().StringVar(&options.KubeConfigPath, "kubeconfig", options.KubeConfigPath, "the location of the kubeconfig file to create.")
 	cmd.Flags().BoolVar(&options.all, "all", options.all, "export all clusters from the kops state store")
-	cmd.Flags().BoolVar(&options.admin, "admin", options.admin, "export the cluster admin user and add it to the context")
+	cmd.Flags().DurationVar(&options.admin, "admin", options.admin, "export a cluster admin user credential with the given lifetime and add it to the cluster context")
+	cmd.Flags().Lookup("admin").NoOptDefVal = kubeconfig.DefaultKubecfgAdminLifetime.String()


OK, now that is cool.

justinsb · 2020-07-26T21:15:02Z

cmd/kops/export_kubecfg.go

@@ -92,7 +94,7 @@ func RunExportKubecfg(ctx context.Context, f *util.Factory, out io.Writer, optio
 			return fmt.Errorf("cannot use both --all flag and positional arguments")
 		}
 	}
-	if options.admin && options.user != "" {
+	if options.admin != 0 && options.user != "" {
 		return fmt.Errorf("cannot use both --admin and --user")


Should we allow setting the duration when --user is specified? (--user is not an option I use myself, so I don't really know)

--user doesn't generate credentials, so a duration isn't relevant.

justinsb · 2020-07-26T21:23:59Z

pkg/kubeconfig/create_kubecfg.go

 	"k8s.io/kops/upup/pkg/fi"
 )

-func BuildKubecfg(cluster *kops.Cluster, keyStore fi.Keystore, secretStore fi.SecretStore, status kops.StatusStore, configAccess clientcmd.ConfigAccess, admin bool, user string) (*KubeconfigBuilder, error) {
+const DefaultKubecfgAdminLifetime = 18 * time.Hour


Specifically on the viper point, we do have limited precedent for --state KOPS_STATE_STORE and we even have a config file. However, I couldn't figure out how to get viper to just do this automatically - the state store configuration is a non-trivial amount of repetition. I'm not sure if we're "holding it wrong" but it wasn't very clear.

Multiple clusters (as hakman points out) is indeed going to be more annoying. I'm wondering if we should think about a helper process for login (that's essentially how GKE gets around this problem), or even nudging users to using a different authenticator, reserving the admin kubeconfig for "break glass".

I'm wondering if we could use a helper process to generate this on the fly, effectively chaining off your AWS credentials, so we'd inherit the same validity period.

justinsb · 2020-07-26T21:25:47Z

pkg/kubeconfig/create_kubecfg.go

+			Signer: fi.CertificateIDCA,
+			Type:   "client",
+			Subject: pkix.Name{
+				CommonName:   "kubecfg",


Yeah, that's not a bad change to make at the same time. Or maybe "admin-kubecfg-${USER}", just in case of usage inside a container where I expect most of these will end up as "root" or "jenkins" which might be confusing.

justinsb · 2020-07-26T21:28:02Z

pkg/kubeconfig/create_kubecfg.go

-func BuildKubecfg(cluster *kops.Cluster, keyStore fi.Keystore, secretStore fi.SecretStore, status kops.StatusStore, configAccess clientcmd.ConfigAccess, admin bool, user string) (*KubeconfigBuilder, error) {
+const DefaultKubecfgAdminLifetime = 18 * time.Hour
+
+func BuildKubecfg(cluster *kops.Cluster, keyStore fi.Keystore, secretStore fi.SecretStore, status kops.StatusStore, configAccess clientcmd.ConfigAccess, admin time.Duration, user string) (*KubeconfigBuilder, error) {


Nit: I might consider splitting admin into buildAdmin and duration; keep this function logical and not too tightly bound to the flags. Although admin=false/0 is weird... it looks like the kubeconfig won't work?

A duration without a buildAdmin wouldn't make sense. Built credentails with duration 0 would not be useful. buildAdmin without duration would be pushing the defaulting logic down, making it less visible to the command line.

justinsb · 2020-07-26T21:30:47Z

Thanks for driving this discussion @johngmyers ... I am going to think more about whether a helper authn process will help us out here, using something like https://kubernetes.io/docs/reference/access-authn-authz/authentication/#input-and-output-formats

olemarkus · 2020-08-03T09:49:33Z

FWIW I think that either way we should merge this PR so that those who provision the static certs really know what they are doing if they are producing long-lived ones.

johngmyers · 2020-08-14T20:47:46Z

@justinsb can we land this now there's a kubectl plugin in the works?

hakman · 2020-08-15T01:37:51Z

/lgtm

johngmyers · 2020-08-15T01:47:53Z

/approve

k8s-ci-robot · 2020-08-15T01:48:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

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:

~~OWNERS~~ [johngmyers]

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

Reduce the lifetime of exported kubecfg credentials

a45b07c

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 18, 2020

k8s-ci-robot requested review from gjtempleton and rdrgmnzs July 18, 2020 06:10

k8s-ci-robot added area/documentation approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 18, 2020

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2020

johngmyers commented Jul 18, 2020

View reviewed changes

hakman reviewed Jul 22, 2020

View reviewed changes

cmd/kops/export_kubecfg.go Show resolved Hide resolved

hakman reviewed Jul 22, 2020

View reviewed changes

justinsb reviewed Jul 26, 2020

View reviewed changes

justinsb mentioned this pull request Aug 2, 2020

Support authentication helper for kubectl #9667

Merged

johngmyers mentioned this pull request Aug 8, 2020

Certificate issuance cleanup and hardening #9110

Closed

k8s-ci-robot assigned hakman Aug 15, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 15, 2020

k8s-ci-robot merged commit ec8b47d into kubernetes:master Aug 15, 2020

k8s-ci-robot added this to the v1.19 milestone Aug 15, 2020

johngmyers deleted the kubectl-lifetime branch August 15, 2020 02:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the lifetime of exported kubecfg credentials #9593

Reduce the lifetime of exported kubecfg credentials #9593

johngmyers commented Jul 18, 2020

johngmyers commented Jul 18, 2020

johngmyers Jul 18, 2020 •

edited

Loading

justinsb Jul 26, 2020

hakman Jul 22, 2020

johngmyers Jul 22, 2020

hakman Jul 22, 2020

johngmyers Jul 23, 2020

justinsb Jul 26, 2020

johngmyers Jul 26, 2020

justinsb Jul 26, 2020

justinsb Jul 26, 2020

johngmyers Jul 26, 2020

justinsb Jul 26, 2020

justinsb Jul 26, 2020

justinsb Jul 26, 2020

johngmyers Jul 26, 2020

justinsb commented Jul 26, 2020

olemarkus commented Aug 3, 2020

johngmyers commented Aug 14, 2020

hakman commented Aug 15, 2020

johngmyers commented Aug 15, 2020

k8s-ci-robot commented Aug 15, 2020

Reduce the lifetime of exported kubecfg credentials #9593

Reduce the lifetime of exported kubecfg credentials #9593

Conversation

johngmyers commented Jul 18, 2020

johngmyers commented Jul 18, 2020

johngmyers Jul 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Jul 26, 2020

olemarkus commented Aug 3, 2020

johngmyers commented Aug 14, 2020

hakman commented Aug 15, 2020

johngmyers commented Aug 15, 2020

k8s-ci-robot commented Aug 15, 2020

johngmyers Jul 18, 2020 •

edited

Loading