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

Support for disabling basic auth / client cert #40

Merged
merged 18 commits into from
Apr 12, 2019

Conversation

coryodaniel
Copy link
Contributor

@coryodaniel coryodaniel commented Dec 14, 2018

Closes #37

  • [Added] enable_basic_auth variable. defaults to false1
  • [Added] basic_auth_username variable. defaults to ""
  • [Added] basic_auth_password variable. defaults to ""
  • [Added] issue_client_certificate variable. defaults to
    true2

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
Copy link
Contributor

Could you add some minor tests to cover this?

Copy link
Contributor

@Jberlinsky Jberlinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
examples/disable_client_cert/main.tf Outdated Show resolved Hide resolved
examples/disable_client_cert/test_outputs.tf Outdated Show resolved Hide resolved
examples/disable_client_cert/variables.tf Show resolved Hide resolved
test/fixtures/disable_client_cert/example.tf Show resolved Hide resolved
.kitchen.yml Show resolved Hide resolved
@coryodaniel coryodaniel force-pushed the 37-disable-client-cert branch 2 times, most recently from f4f0b46 to 7fa630e Compare January 7, 2019 22:15
Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny changes needed.

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

test/integration/disable_client_cert/controls/gcloud.rb Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
@coryodaniel coryodaniel force-pushed the 37-disable-client-cert branch 2 times, most recently from 028a6f1 to 5c8f0cb Compare January 8, 2019 19:02
@morgante
Copy link
Contributor

@coryodaniel Please fix merge conflicts and requested changes.

@coryodaniel
Copy link
Contributor Author

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

cluster_regional.tf Show resolved Hide resolved
@morgante
Copy link
Contributor

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

aaron-lane
aaron-lane previously approved these changes Jan 21, 2019
@adrienthebo
Copy link
Contributor

@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
Copy link
Contributor Author

coryodaniel commented Jan 29, 2019 via email

@adrienthebo
Copy link
Contributor

@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.

main.tf Outdated Show resolved Hide resolved
autogen/main.tf Outdated Show resolved Hide resolved
@aaron-lane aaron-lane self-assigned this Apr 4, 2019
@aaron-lane aaron-lane changed the base branch from master to aaron-lane-v2.0.0 April 4, 2019 22:51
@coryodaniel
Copy link
Contributor Author

coryodaniel commented Apr 5, 2019 via email

@aaron-lane aaron-lane force-pushed the 37-disable-client-cert branch 2 times, most recently from c0be695 to 2232089 Compare April 5, 2019 15:21
coryodaniel and others added 16 commits April 5, 2019 15:37
* [Added] `enable_basic_auth` variable. defaults to `false`<sup>1</sup>
* [Added] `basic_auth_username` variable. defaults to `""`
* [Added] `basic_auth_password` variable. defaults to `""`
* [Added] `issue_client_certificate` variable. defaults to
`true`<sup>2</sup>

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.
* Replace outputs w/ symlink
* disable color in tests
* adding suffix
* Set basic auth to disabled by default
@coryodaniel
Copy link
Contributor Author

coryodaniel commented Apr 9, 2019

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 1
  • 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?

@aaron-lane aaron-lane dismissed Jberlinsky’s stale review April 9, 2019 18:39

Satisfied request to autogenerate content.

@aaron-lane
Copy link
Contributor

@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
Copy link
Contributor Author

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

@@ -10,10 +10,17 @@ Extending the adopted spec, each change should have a link to its corresponding

## [v2.0.0] - 2019-YY-ZZ
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we not release v2.0.0 yet?

@morgante morgante merged commit 92b342c into aaron-lane-v2.0.0 Apr 12, 2019
@aaron-lane aaron-lane deleted the 37-disable-client-cert branch April 12, 2019 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants