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

Add a hash to random_password with StateUpgrader #254

Merged
merged 10 commits into from
May 18, 2022
1 change: 1 addition & 0 deletions docs/resources/password.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ resource "aws_db_instance" "example" {

### Read-Only

- `bcrypt_hash` (String, Sensitive) A bcrypt hash of the generated random string.
- `id` (String) A static value used internally by Terraform, this should not be referenced in configurations.
- `result` (String, Sensitive) The generated random string.

Expand Down
93 changes: 90 additions & 3 deletions internal/provider/resource_password.go
Original file line number Diff line number Diff line change
@@ -1,22 +1,109 @@
package provider

import (
"context"
"fmt"

"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"golang.org/x/crypto/bcrypt"
)

func resourcePassword() *schema.Resource {
create := func(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Why is this function defined inside inside the other function (lamba)?

I can't spot anything that would require this approach ❓

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have removed.

diags := createStringFunc(true)(ctx, d, meta)
if diags.HasError() {
return diags
}

hash, err := generateHash(d.Get("result").(string))
if err != nil {
diags = append(diags, diag.Errorf("err: %s", err)...)
return diags
}

if err := d.Set("bcrypt_hash", hash); err != nil {
diags = append(diags, diag.Errorf("err: %s", err)...)
return diags
}

return nil
}

return &schema.Resource{
Description: "Identical to [random_string](string.html) with the exception that the result is " +
"treated as sensitive and, thus, _not_ displayed in console output. Read more about sensitive " +
"data handling in the [Terraform documentation](https://www.terraform.io/docs/language/state/sensitive-data.html).\n" +
"\n" +
"This resource *does* use a cryptographic random number generator.",
CreateContext: createStringFunc(true),
CreateContext: create,
ReadContext: readNil,
DeleteContext: RemoveResourceFromState,
Schema: stringSchemaV1(true),
Schema: passwordSchemaV1(),
Importer: &schema.ResourceImporter{
StateContext: importStringFunc(true),
StateContext: importPasswordFunc(),
},
SchemaVersion: 1,
StateUpgraders: []schema.StateUpgrader{
{
Version: 0,
Type: resourcePasswordV0().CoreConfigSchema().ImpliedType(),
Upgrade: resourcePasswordStateUpgradeV0,
},
},
}
}

func importPasswordFunc() schema.StateContextFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need a function that returns a function here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have removed.

return func(ctx context.Context, d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
val := d.Id()
d.SetId("none")

if err := d.Set("result", val); err != nil {
return nil, fmt.Errorf("resource password import failed, error setting result: %w", err)
}

hash, err := generateHash(val)
if err != nil {
return nil, fmt.Errorf("resource password import failed, generate hash error: %w", err)
}

if err := d.Set("bcrypt_hash", hash); err != nil {
return nil, fmt.Errorf("resource password import failed, error setting bcrypt_hash: %w", err)
}

return []*schema.ResourceData{d}, nil
}
}

func resourcePasswordV0() *schema.Resource {
return &schema.Resource{
Schema: passwordSchemaV0(),
}
}

func resourcePasswordStateUpgradeV0(_ context.Context, rawState map[string]interface{}, _ interface{}) (map[string]interface{}, error) {
if rawState == nil {
return nil, fmt.Errorf("resource password state upgrade failed, state is nil")
}

result, ok := rawState["result"].(string)
bflad marked this conversation as resolved.
Show resolved Hide resolved
if !ok {
return nil, fmt.Errorf("resource password state upgrade failed, result could not be asserted as string: %T", rawState["result"])
}

hash, err := generateHash(result)
if err != nil {
return nil, fmt.Errorf("resource password state upgrade failed, generate hash error: %w", err)
}

rawState["bcrypt_hash"] = hash

return rawState, nil
}

func generateHash(toHash string) (string, error) {
hash, err := bcrypt.GenerateFromPassword([]byte(toHash), bcrypt.DefaultCost)

return string(hash), err
}
95 changes: 67 additions & 28 deletions internal/provider/resource_pasword_test.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package provider

import (
"context"
"fmt"
"regexp"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
)
Expand All @@ -15,7 +17,9 @@ func TestAccResourcePasswordBasic(t *testing.T) {
ProviderFactories: testAccProviders,
Steps: []resource.TestStep{
{
Config: testAccResourcePasswordBasic,
Config: `resource "random_password" "basic" {
length = 12
}`,
Check: resource.ComposeTestCheckFunc(
testAccResourceStringCheck("random_password.basic", &customLens{
customLen: 12,
Expand All @@ -41,7 +45,7 @@ func TestAccResourcePasswordBasic(t *testing.T) {
},
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"length", "lower", "number", "special", "upper", "min_lower", "min_numeric", "min_special", "min_upper", "override_special"},
ImportStateVerifyIgnore: []string{"bcrypt_hash", "length", "lower", "number", "special", "upper", "min_lower", "min_numeric", "min_special", "min_upper", "override_special"},
},
},
})
Expand All @@ -53,7 +57,13 @@ func TestAccResourcePasswordOverride(t *testing.T) {
ProviderFactories: testAccProviders,
Steps: []resource.TestStep{
{
Config: testAccResourcePasswordOverride,
Config: `resource "random_password" "override" {
length = 4
override_special = "!"
lower = false
upper = false
number = false
}`,
Check: resource.ComposeTestCheckFunc(
testAccResourceStringCheck("random_password.override", &customLens{
customLen: 4,
Expand All @@ -71,7 +81,14 @@ func TestAccResourcePasswordMin(t *testing.T) {
ProviderFactories: testAccProviders,
Steps: []resource.TestStep{
{
Config: testAccResourcePasswordMin,
Config: `resource "random_password" "min" {
length = 12
override_special = "!#@"
min_lower = 2
min_upper = 3
min_special = 1
min_numeric = 4
}`,
Check: resource.ComposeTestCheckFunc(
testAccResourceStringCheck("random_password.min", &customLens{
customLen: 12,
Expand All @@ -86,29 +103,51 @@ func TestAccResourcePasswordMin(t *testing.T) {
})
}

const (
testAccResourcePasswordBasic = `
resource "random_password" "basic" {
length = 12
}`
func TestResourcePasswordStateUpgradeV0(t *testing.T) {
cases := []struct {
name string
stateV0 map[string]interface{}
shouldError bool
errMsg string
expectedStateV1 map[string]interface{}
}{
{
name: "result is not string",
stateV0: map[string]interface{}{"result": 0},
shouldError: true,
errMsg: "resource password state upgrade failed, result could not be asserted as string: int",
},
{
name: "success",
stateV0: map[string]interface{}{"result": "abc123"},
shouldError: false,
expectedStateV1: map[string]interface{}{"result": "abc123", "bcrypt_hash": "123"},
},
}

testAccResourcePasswordOverride = `
resource "random_password" "override" {
length = 4
override_special = "!"
lower = false
upper = false
number = false
}
`
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
actualStateV1, err := resourcePasswordStateUpgradeV0(context.Background(), c.stateV0, nil)

testAccResourcePasswordMin = `
resource "random_password" "min" {
length = 12
override_special = "!#@"
min_lower = 2
min_upper = 3
min_special = 1
min_numeric = 4
}`
)
if c.shouldError {
if !cmp.Equal(c.errMsg, err.Error()) {
t.Errorf("expected: %q, got: %q", c.errMsg, err)
}
if !cmp.Equal(c.expectedStateV1, actualStateV1) {
t.Errorf("expected: %+v, got: %+v", c.expectedStateV1, err)
}
} else {
if err != nil {
t.Errorf("err should be nil, actual: %v", err)
}

for k := range c.expectedStateV1 {
_, ok := actualStateV1[k]
if !ok {
t.Errorf("expected key: %s is missing from state", k)
}
}
}
})
}
}
19 changes: 17 additions & 2 deletions internal/provider/resource_string.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package provider

import (
"context"
"fmt"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
)

Expand All @@ -21,9 +24,21 @@ func resourceString() *schema.Resource {
// [SDK documentation](https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/resource.go#L91).
MigrateState: resourceRandomStringMigrateState,
SchemaVersion: 1,
Schema: stringSchemaV1(false),
Schema: stringSchemaV1(),
Importer: &schema.ResourceImporter{
StateContext: importStringFunc(false),
StateContext: importStringFunc(),
},
}
}

func importStringFunc() schema.StateContextFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above: having this function returning another function seem not necessary.

But I might be missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have removed.

return func(ctx context.Context, d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
val := d.Id()

if err := d.Set("result", val); err != nil {
return nil, fmt.Errorf("error setting result: %w", err)
}

return []*schema.ResourceData{d}, nil
}
}
68 changes: 40 additions & 28 deletions internal/provider/string.go
Original file line number Diff line number Diff line change
@@ -1,26 +1,56 @@
// This file provides shared functionality between `resource_string` and `resource_password`.
// There is no intent to permanently couple their implementations
// Over time they could diverge, or one becomes deprecated
// Package provider string.go provides shared functionality between `resource_string` and `resource_password`.
// There is no intent to permanently couple their implementations.
// Over time, they could diverge, or one becomes deprecated.
package provider

import (
"context"
"crypto/rand"
"fmt"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
"math/big"
"sort"

"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
)

func stringSchemaV1(sensitive bool) map[string]*schema.Schema {
idDesc := "The generated random string."
if sensitive {
idDesc = "A static value used internally by Terraform, this should not be referenced in configurations."
// passwordSchemaV1 uses passwordSchemaV0 to obtain the V0 version of the Schema key-value entries but requires that
// the bcrypt_hash entry be configured.
func passwordSchemaV1() map[string]*schema.Schema {
passwordSchema := passwordSchemaV0()
passwordSchema["bcrypt_hash"] = &schema.Schema{
Description: "A bcrypt hash of the generated random string.",
Type: schema.TypeString,
Computed: true,
Sensitive: true,
}

return passwordSchema
}

// passwordSchemaV0 uses passwordStringSchema to obtain the default Schema key-value entries but requires that the id
// description, result sensitive and bcrypt_hash entries be configured.
func passwordSchemaV0() map[string]*schema.Schema {
passwordSchema := passwordStringSchema()
passwordSchema["id"].Description = "A static value used internally by Terraform, this should not be referenced in configurations."
passwordSchema["result"].Sensitive = true

return passwordSchema
}

// stringSchemaV1 uses passwordStringSchema to obtain the default Schema key-value entries but requires that the id
// description be configured.
func stringSchemaV1() map[string]*schema.Schema {
stringSchema := passwordStringSchema()
stringSchema["id"].Description = "The generated random string."

return stringSchema
}

// passwordStringSchema returns map[string]*schema.Schema with all keys and values that are common to both the
// password and string resources.
Comment on lines +18 to +52
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that all the schemas are now next to each other.

func passwordStringSchema() map[string]*schema.Schema {
return map[string]*schema.Schema{
"keepers": {
Description: "Arbitrary map of values that, when changed, will trigger recreation of " +
Expand Down Expand Up @@ -116,13 +146,11 @@ func stringSchemaV1(sensitive bool) map[string]*schema.Schema {
Description: "The generated random string.",
Type: schema.TypeString,
Computed: true,
Sensitive: sensitive,
},

"id": {
Description: idDesc,
Computed: true,
Type: schema.TypeString,
Computed: true,
Type: schema.TypeString,
},
}
}
Expand Down Expand Up @@ -229,19 +257,3 @@ func generateRandomBytes(charSet *string, length int) ([]byte, error) {
func readNil(_ context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
return nil
}

func importStringFunc(sensitive bool) schema.StateContextFunc {
return func(ctx context.Context, d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
val := d.Id()

if sensitive {
d.SetId("none")
}

if err := d.Set("result", val); err != nil {
return nil, fmt.Errorf("error setting result: %w", err)
}

return []*schema.ResourceData{d}, nil
}
}