Use Terraform to bcrypt the admin password #1771

edevenport · 2017-08-25T04:50:31Z

Frontend and backend have been adjusted to use Terraform bcrypt interpolation for the adminPassword per issue #643. I have tested locally with make apply and through the UI with local terraform versions 0.9.11 and 0.10.2.

Password has been tested with the following special characters:

`-=\][';/.,~!@#$%^&*()_+|}{":?><

Backslashes and double quotes will need to be escaped when setting the password in terraform.tfvars manually (template descriptions updated accordingly), but all above special characters handled successfully through the UI.

I am unable to validate tests through Jenkins, but TF_VAR_tectonic_admin_password_hash is now TF_VAR_tectonic_admin_password and requires a plain text password.

edevenport · 2017-08-31T07:50:53Z

@squat Do you have some time to review the code and test results?

squat · 2017-08-31T15:36:33Z

Thanks @edevenport, I'll TAL now.

sym3tri · 2017-09-01T12:50:48Z

Jenkins & GitHub are complaining about the same merge conflicts.

edevenport · 2017-09-05T07:11:19Z

ok to test

edevenport · 2017-09-05T07:13:46Z

@sym3tri @squat I merged the latest updates from master and corrected the merge conflicts.

cpanato · 2017-09-05T07:20:21Z

@edevenport there are two small issues in the lint for the frontend.

/home/ubuntu/workspace/tectonic-installer_PR-1771-MDYVNSYM5ANLWQBB6KSQTDFNNEI4ELV7WSD4XPYFG4CEHLHS2QWQ/installer/frontend/cluster-config.js
  196:37  error  'opts' is assigned a value but never used  no-unused-vars
  297:43  error  'opts' is assigned a value but never used  no-unused-vars

✖ 2 problems (2 errors, 0 warnings)

edevenport · 2017-09-05T19:05:28Z

ok to test

edevenport · 2017-09-05T19:38:23Z

Thank you @cpanato - the lint errors should be resolved now.

squat · 2017-09-05T20:28:47Z

@edevenport looking better! now the frontend tests are failing with:

    Difference:
    
    - Expected
    + Received
    
    @@ -9,11 +9,11 @@
       "platform": "aws",
       "pullSecret": "<TECTONIC_PULL_SECRET>",
       "retry": false,
       "variables": Object {
         "tectonic_admin_email": "[email protected]",
    -    "tectonic_admin_password": "PASSWORD",
    +    "tectonic_admin_password": "pjazbahvw!Singon9cwh^ygsi9hoblxW",
         "tectonic_aws_extra_tags": Object {
           "test_tag": "testing",
         },
         "tectonic_aws_master_custom_subnets": Object {
           "us-west-1a": "10.0.0.0/19",

To fix this we need to update the *.progress files in installer/frontend/__tests__/examples/ and replace https://github.com/coreos/tectonic-installer/blob/master/installer/frontend/__tests__/examples/tectonic-aws-vpc.progress#L49 with the new example password you specified in the *.json files. Alternatively, the json files could all use PASSWORD as the test password.

edevenport · 2017-09-05T23:05:17Z

ok to test

squat · 2017-09-06T00:12:45Z

Documentation/install/azure/azure-terraform.md

@@ -0,0 +1,277 @@
+# Install Tectonic on Azure with Terraform


@edevenport why was this included in this PR? could we remove this doc?

@squat Sorry about that. It was updated in my original PR before it was removed from master and seems to have gotten restored during my pull. I rebased and removed the file.

edevenport · 2017-09-06T00:39:28Z

Thank you @squat - looks like the tests are passing now.

squat · 2017-09-06T01:46:33Z

@edevenport the tests look good. The one main outstanding issue is the azure doc. Did that sneak in accidentally?

edevenport · 2017-09-06T17:56:51Z

ok to test

squat

Looks great! Two small nits concerning wording.

squat · 2017-09-06T18:05:55Z

config.tf

  type = "string"

  description = <<EOF
-The bcrypt hash of admin user password to login to the Tectonic Console.
-Use the bcrypt-hash tool (https://github.com/coreos/bcrypt-tool/releases/tag/v1.0.0) to generate it.
+The admin user password to login to the Tectonic Console. Backslashes and double quotes should


s/should/must/ to convey that it is a requirement.

Done. I also moved the special character requirement to the Note line for better division between parameter description and requirements.

squat · 2017-09-06T18:07:29Z

modules/tectonic/variables.tf

-variable "admin_password_hash" {
-  description = "Hashed password used to by the cluster admin to login to the Tectonic Console. Generate with the bcrypt-hash tool (https://github.com/coreos/bcrypt-tool/releases/tag/v1.0.0)."
+variable "admin_password" {
+  description = "Password used to by the cluster admin to login to the Tectonic Console. Backslashes and double quotes should be escaped."


s/should/must/ to convey that it is a requirement.

squat · 2017-09-06T19:00:51Z

Documentation/variables/config.md

@@ -7,7 +7,7 @@ This document gives an overview of variables used in all platforms of the Tecton
 | Name | Description | Type | Default |
 |------|-------------|:----:|:-----:|
 | tectonic_admin_email | The e-mail address used to: 1. login as the admin user to the Tectonic Console. 2. generate DNS zones for some providers.<br><br>Note: This field MUST be in all lower-case e-mail address format and set manually prior to creating the cluster. | string | - |
-| tectonic_admin_password_hash | The bcrypt hash of admin user password to login to the Tectonic Console. Use the bcrypt-hash tool (https://github.com/coreos/bcrypt-tool/releases/tag/v1.0.0) to generate it.<br><br>Note: This field MUST be set manually prior to creating the cluster. | string | - |
+| tectonic_admin_password | The admin user password to login to the Tectonic Console. Backslashes and double quotes should be escaped.<br><br>Note: This field MUST be set manually prior to creating the cluster. | string | - |


Thanks for making that change. Looks like we need to make docs && make examples again now

squat · 2017-09-06T21:58:43Z

The tests are failing with the following error:

1 error(s) occurred:

* module.tectonic.template_dir.tectonic: 1 error(s) occurred:

* module.tectonic.template_dir.tectonic: 1:3: unknown function called: bcrypt in:

${bcrypt(var.admin_password, 12)}
Makefile:44: recipe for target 'apply' failed

This is because the smoke tests use Terraform 0.9.x, which does not have support for bcrypt. We'll need to bump that or wait for #1841.

cpanato · 2017-09-07T14:17:33Z

@edevenport do you mind to rebase this PR? Thanks!

edevenport · 2017-09-14T09:10:39Z

A merge conflict was introduced recently, so I rebased the branch and corrected the issue.

edevenport · 2017-09-21T08:51:54Z

@squat We are ready for a review (and hopefully merger) again.

s-urbaniak

This LGTM modulo the hard-coded password values.

s-urbaniak · 2017-09-21T08:54:08Z

tests/smoke/bare-metal/vars/metal.tfvars

 tectonic_admin_email = "[email protected]"
-tectonic_admin_password_hash = "$2a$12$T8hTe.NlOPDP0SS3DxNeDuVhHSFbdGXZEhGps/W.BG4QC7.1/nDaG"
+tectonic_admin_password = "password"


Let's get rid of this here and inject it via an env var in CI, @cpanato do you mind to give directions here?

@cpanato I have no visibility into Jenkins. Would you be able to assist me here?

s-urbaniak · 2017-09-21T08:55:31Z

modules/tectonic/variables.tf

  type        = "string"
-  default     = "2a$12$k9wa31uE/4uD9aVtT/vNtOZwxXyEJ/9DwXXEYB/eUpb9fvEPsH/kO"
+  default     = "password"


I strongly suggest to remove this default value here.

s-urbaniak · 2017-09-21T08:56:08Z

installer/frontend/__tests__/examples/tectonic-baremetal.progress

@@ -37,7 +37,7 @@
    "tectonicLicense": "<TECTONIC_LICENSE>",
    "pullSecret": "<TECTONIC_PULL_SECRET>",
    "adminEmail": "[email protected]",
-    "adminPassword": "PASSWORD",
+    "adminPassword": "password",


those variables should be dependency-injected via CI, /cc @cpanato

@cpanato Same issue here with no visibility into Jenkins.

alexsomesan

Looks good, but I second @s-urbaniak 's suggestion of removing the hardcoded password value from the test configs. We will inject that from CI.

I'm asking myself if we should rather also keep the old variable there for a while (and fall back to using it) to provide a migration path. This is a rather sensitive change since it alters the confidentiality level of the tfvars file. I'd rather users have some time to adjust their workflows around it.

s-urbaniak · 2017-09-26T08:09:59Z

ping @edevenport: do you mind to rebase+address the review comments?

edevenport · 2017-09-26T21:53:30Z

@s-urbaniak The rebase is completed. I'll see if @cpanato can assist with the Jenkins piece.

alexsomesan · 2017-09-27T13:03:41Z

retest this please

cpanato · 2017-09-27T16:40:35Z

Jenkinsfile

+    $class: 'UsernamePasswordMultiBinding',
+    credentialsId: 'tectonic-console-login',
+    passwordVariable: 'TF_VAR_tectonic_admin_email',
+    usernameVariable: 'TF_VAR_tectonic_admin_password'


We dont need to set the admin_email/password because in the rspec tests we randon generate those.

can you please remove this change?

Fixed trailed comma in frontend test. Changed test password to be lowercase due to failing test.

cpanato · 2017-09-28T08:47:13Z

@edevenport @alexsomesan @s-urbaniak @squat all green now :)

s-urbaniak

LGTM, thanks!

bcrypt support landed in coreos#1771 but the change was not promoted to GCP. This fixes it.

* modules/tectonic: add kube_dns_service_ip variable This fixes a regression introduced in #2014. * platforms/gcp: replace admin_password_hash with admin_password bcrypt support landed in #1771 but the change was not promoted to GCP. This fixes it. * modules/tectonic: use literals for values of data keys ConfigMaps in k8s are key-value pairs in a data section. The values need to be literal YAML strings otherwise the nested YAML will be interpreted causing a failure in deploying the ConfigMap. This fixes another regression introduced in #2014

…assword

sym3tri requested a review from squat August 25, 2017 12:18

edevenport changed the title ~~WIP: Use Terraform to bcrypt the admin password~~ [WIP]: Use Terraform to bcrypt the admin password Aug 31, 2017

sym3tri changed the title ~~[WIP]: Use Terraform to bcrypt the admin password~~ Use Terraform to bcrypt the admin password Sep 1, 2017

sym3tri added the needs review label Sep 1, 2017

squat suggested changes Sep 6, 2017

View reviewed changes

edevenport force-pushed the terraform_bcrypt_643 branch from 56b7ed2 to 3d630f0 Compare September 6, 2017 17:45

squat reviewed Sep 6, 2017

View reviewed changes

edevenport force-pushed the terraform_bcrypt_643 branch 2 times, most recently from c728dca to 367441d Compare September 6, 2017 19:07

squat previously approved these changes Sep 6, 2017

View reviewed changes

squat added run-smoke-tests and removed needs review labels Sep 6, 2017

squat closed this Sep 6, 2017

squat reopened this Sep 6, 2017

edevenport dismissed squat’s stale review via 55c25d5 September 14, 2017 08:50

edevenport force-pushed the terraform_bcrypt_643 branch from 367441d to 55c25d5 Compare September 14, 2017 08:50

sym3tri assigned alexsomesan Sep 20, 2017

sym3tri added the needs review label Sep 20, 2017

edevenport force-pushed the terraform_bcrypt_643 branch from 55c25d5 to 44b5e6a Compare September 20, 2017 15:44

s-urbaniak suggested changes Sep 21, 2017

View reviewed changes

alexsomesan reviewed Sep 21, 2017

View reviewed changes

jasmingacic force-pushed the terraform_bcrypt_643 branch from 09fd997 to b130bc8 Compare September 26, 2017 10:58

edevenport force-pushed the terraform_bcrypt_643 branch from 4e2b8fa to cebf485 Compare September 26, 2017 21:41

cpanato reviewed Sep 27, 2017

View reviewed changes

edevenport and others added 4 commits September 28, 2017 10:05

Use Terraform to bcrypt the adminPassword.

dac0fe2

Fixed trailed comma in frontend test. Changed test password to be lowercase due to failing test.

Removed default password value.

aec0d14

Removed username/password binding.

66fa6c8

tests: update rspec to use tectonic_admin_password instead hash

6dbacb3

cpanato force-pushed the terraform_bcrypt_643 branch from d476dea to 6dbacb3 Compare September 28, 2017 08:05

s-urbaniak approved these changes Sep 28, 2017

View reviewed changes

alexsomesan approved these changes Sep 28, 2017

View reviewed changes

alexsomesan merged commit 2101f86 into master Sep 28, 2017

s-urbaniak pushed a commit to s-urbaniak/tectonic-installer that referenced this pull request Oct 4, 2017

platforms/gcp: replace admin_password_hash with admin_password

25920b1

bcrypt support landed in coreos#1771 but the change was not promoted to GCP. This fixes it.

s-urbaniak mentioned this pull request Oct 4, 2017

modules/tectonic: add kube_dns_service_ip variable #2037

Merged

nreisbeck pushed a commit to nreisbeck/tectonic-installer that referenced this pull request Oct 19, 2017

align DO branch with coreos#1771; Use Terraform to bcrypt the admin p…

5676dd4

…assword

joshix mentioned this pull request Nov 9, 2017

Terraform vars: Make secrets internal #2327

Merged

squat deleted the terraform_bcrypt_643 branch November 17, 2017 15:47

Use Terraform to bcrypt the admin password #1771

Use Terraform to bcrypt the admin password #1771

Conversation

edevenport commented Aug 25, 2017

edevenport commented Aug 31, 2017

squat commented Aug 31, 2017

sym3tri commented Sep 1, 2017

edevenport commented Sep 5, 2017

edevenport commented Sep 5, 2017

cpanato commented Sep 5, 2017

edevenport commented Sep 5, 2017

edevenport commented Sep 5, 2017

squat commented Sep 5, 2017

edevenport commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edevenport commented Sep 6, 2017

squat commented Sep 6, 2017

edevenport commented Sep 6, 2017

squat left a 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

squat commented Sep 6, 2017

cpanato commented Sep 7, 2017

edevenport commented Sep 14, 2017

edevenport commented Sep 21, 2017

s-urbaniak left a 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

alexsomesan left a comment

Choose a reason for hiding this comment

s-urbaniak commented Sep 26, 2017

edevenport commented Sep 26, 2017 • edited Loading

alexsomesan commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpanato commented Sep 28, 2017 • edited Loading

s-urbaniak left a comment

Choose a reason for hiding this comment

edevenport commented Sep 26, 2017 •

edited

Loading

cpanato commented Sep 28, 2017 •

edited

Loading