Support for disabling basic auth / client cert

[coryodaniel]

Closes #37

• [Added] enable_basic_auth variable. defaults to false¹
• [Added] basic_auth_username variable. defaults to ""
• [Added] basic_auth_password variable. defaults to ""
• [Added] issue_client_certificate variable. defaults to
  true²

Notes:

1. This will cause a plan change for existing users. Enabling it will
   require them to set a username and password.

2. This is enabled by default, despite being a poor security practice
   because changing this value is destructive to the cluster and we decided
   to err on not trigger destroy plan changes to existing users.

[morgante]

Could you add some minor tests to cover this?

[Jberlinsky]

In addition to the comments inline, can we add something to the README (and changelog) explicitly discussing this?

This will cause a plan change for existing users. Enabling it will require them to set a username and password.

[aaron-lane]

Tiny changes needed.

With respect to the plan change, this will require a major version bump.

[morgante]

@coryodaniel Please fix merge conflicts and requested changes.

[coryodaniel]

I believe I got all requested changes in. I'll resolve the merge conflicts first thing tomorrow AM.

[morgante]

@coryodaniel Please investigate why tests are failing and fix. /cc @Jberlinsky

[adrienthebo]

@coryodaniel we're getting failures in CI on this branch, but they're unrelated to your work on this pull request. I've opened #74 to fix this issue.

[coryodaniel]

Interesting. Should I wait and rebase off master once it’s in or do you want to point the PR at this branch? Thanks, Cory O’Daniel

…

On Jan 28, 2019, at 4:33 PM, Adrien Thebo ***@***.***> wrote: @coryodaniel we're getting failures in CI on this branch, but they're unrelated to your work on this pull request. I've opened #74 to fix this issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[adrienthebo]

@coryodaniel I've seen other pull requests have their reviews dismissed upon rebase, which might be good in some cases but may not be ideal here. Let's get that code into master so that we're merging into a clean build, merge master into this branch, and when CI goes green we can merge into master.

[coryodaniel]

Agreed! I’m out of town til the 9th. I’ll rebase and make the changes as soon as I’m off vacay! 🌴 Thanks, Cory O’Daniel

…

On Mar 26, 2019, at 11:34 AM, Olivier Cervello ***@***.***> wrote: @ocervell commented on this pull request. In main.tf: > @@ -143,6 +143,9 @@ locals { cluster_http_load_balancing_enabled = "${local.cluster_type_output_http_load_balancing_enabled[local.cluster_type] ? false : true}" cluster_horizontal_pod_autoscaling_enabled = "${local.cluster_type_output_horizontal_pod_autoscaling_enabled[local.cluster_type] ? false : true}" cluster_kubernetes_dashboard_enabled = "${local.cluster_type_output_kubernetes_dashboard_enabled[local.cluster_type] ? false : true}" + + cluster_basic_auth_username = "${var.enable_basic_auth ? var.basic_auth_username : ""}" + cluster_basic_auth_password = "${var.enable_basic_auth ? var.basic_auth_password : ""}" } Based on those two lines, if enable_basic_auth is true and basic_auth_username and basic_auth_password are not specified by the user, based on the google_container_resource behaviour, Basic Auth will be disabled (counter-intuitive): maybe in that case we can generate default credentials ? locals { basic_auth_username = "${var.basic_auth_username == "" ? "admin" : var.basic_auth_username}" basic_auth_password = "${var.basic_auth_password == "" ? resource.random_string.password.result : var.basic_auth_password}" } resource "random_string" "password" { length = 16 special = true } and change the above to: cluster_basic_auth_username = "${var.basic_auth_enabled ? local.basic_auth_username : ""}" cluster_basic_auth_password = "${var.basic_auth_enabled ? local.basic_auth_password : ""}" In autogen/main.tf: > @@ -143,6 +143,9 @@ locals { cluster_http_load_balancing_enabled = "${local.cluster_type_output_http_load_balancing_enabled[local.cluster_type] ? false : true}" cluster_horizontal_pod_autoscaling_enabled = "${local.cluster_type_output_horizontal_pod_autoscaling_enabled[local.cluster_type] ? false : true}" cluster_kubernetes_dashboard_enabled = "${local.cluster_type_output_kubernetes_dashboard_enabled[local.cluster_type] ? false : true}" + + cluster_basic_auth_username = "${var.enable_basic_auth ? var.basic_auth_username : ""}" + cluster_basic_auth_password = "${var.enable_basic_auth ? var.basic_auth_password : ""}" Same comment as below. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[coryodaniel]

Some notes on functionality I wanted to revisit since its been a while since this was originally created:

• setting the username, but leaving the password blank will autogenerate a password on the back end
• setting the password, but leaving the username blank causes an error in terraform apply, but not plan ¹
• setting basic auth credentials on an existing cluster does update the cluster, not recreate 👍
• removing basic auth credentials does update the cluster, not recreate 👍

Auth credential changes do seem to disrupt access to the k8s API via kubectl and the GCP console

@aaron-lane Should we default the username to admin when username is blank, but the password is present, so that users don't get false expectations out of a plan?

Satisfied request to autogenerate content.

[aaron-lane]

@coryodaniel: thanks for the review!

With respect to only basic_auth_password being set, my preference is to allow Terraform to fail on apply rather than conditionally overriding the default value for basic_auth_username. Using Basic Authentication is not recommended, so we should try to avoid extra complexity in supporting it.

We could investigate if the provider can be enhanced to identify this invalid case while planning.

[coryodaniel]

It's a pretty straightforward presence rule. Not sure if provider validation supports conditional presence.

[morgante]

Did we not release v2.0.0 yet?