Prevent panic on random_string and random_password when character…

… set is empty (#551) * resource/random_password+random_string: Prevent panic when special, upper, lower, numeric/number are set to false (#549) Previously, a panic would be generated if the following configuration was used: ``` resource "random_string" "random" { length = 16 special = false upper = false lower = false numeric = false } ``` ``` ╷ │ Error: Plugin did not respond │ │ with random_string.random, │ on resource.tf line 1, in resource "random_string" "random": │ 1: resource "random_string" "random" { │ │ The plugin encountered an error, and failed to respond to the plugin.(*GRPCProvider).ApplyResourceChange call. The plugin logs may contain more details. ╵ Stack trace from the terraform-provider-random plugin: panic: crypto/rand: argument to Int is <= 0 ``` This change will cause validation to fail if special, upper, lower, and numeric/number are all set to false Output from acceptance testing: ```console TF_ACC=1 go test -count=1 -run='TestAccResourceString_NumericFalse' -timeout=10m -v ./internal/provider === RUN TestAccResourceString_NumericFalse === PAUSE TestAccResourceString_NumericFalse === CONT TestAccResourceString_NumericFalse --- PASS: TestAccResourceString_NumericFalse (0.15s) ``` * Adding changelog entries and copyright header (#549) * Linting * Updating description for number and numeric attributes on resource_password and resource_string
hashicorp · Apr 15, 2024 · 9192f92 · 9192f92
1 parent 9eccb5c
commit 9192f92
Show file tree

Hide file tree

Showing 10 changed files with 340 additions and 10 deletions.
diff --git a/.changes/unreleased/BUG FIXES-20240410-175423.yaml b/.changes/unreleased/BUG FIXES-20240410-175423.yaml
@@ -0,0 +1,6 @@
+kind: BUG FIXES
+body: 'provider/random_password: Fix bug which causes panic when special, upper, lower
+  and number/numeric are all false'
+time: 2024-04-10T17:54:23.219223+01:00
+custom:
+  Issue: "551"
diff --git a/.changes/unreleased/BUG FIXES-20240410-175536.yaml b/.changes/unreleased/BUG FIXES-20240410-175536.yaml
@@ -0,0 +1,6 @@
+kind: BUG FIXES
+body: 'provider/random_string: Fix bug which causes panic when special, upper, lower
+  and number/numeric are all false'
+time: 2024-04-10T17:55:36.979987+01:00
+custom:
+  Issue: "551"
diff --git a/docs/resources/password.md b/docs/resources/password.md
@@ -45,8 +45,8 @@ resource "aws_db_instance" "example" {
 - `min_numeric` (Number) Minimum number of numeric characters in the result. Default value is `0`.
 - `min_special` (Number) Minimum number of special characters in the result. Default value is `0`.
 - `min_upper` (Number) Minimum number of uppercase alphabet characters in the result. Default value is `0`.
-- `number` (Boolean, Deprecated) Include numeric characters in the result. Default value is `true`. **NOTE**: This is deprecated, use `numeric` instead.
-- `numeric` (Boolean) Include numeric characters in the result. Default value is `true`.
+- `number` (Boolean, Deprecated) Include numeric characters in the result. Default value is `true`. If `number`, `upper`, `lower`, and `special` are all configured, at least one of them must be set to `true`. **NOTE**: This is deprecated, use `numeric` instead.
+- `numeric` (Boolean) Include numeric characters in the result. Default value is `true`. If `numeric`, `upper`, `lower`, and `special` are all configured, at least one of them must be set to `true`.
 - `override_special` (String) Supply your own list of special characters to use for string generation.  This overrides the default character list in the special argument.  The `special` argument must still be set to true for any overwritten characters to be used in generation.
 - `special` (Boolean) Include special characters in the result. These are `!@#$%&*()-_=+[]{}<>:?`. Default value is `true`.
 - `upper` (Boolean) Include uppercase alphabet characters in the result. Default value is `true`.

diff --git a/docs/resources/string.md b/docs/resources/string.md
@@ -40,8 +40,8 @@ resource "random_string" "random" {
 - `min_numeric` (Number) Minimum number of numeric characters in the result. Default value is `0`.
 - `min_special` (Number) Minimum number of special characters in the result. Default value is `0`.
 - `min_upper` (Number) Minimum number of uppercase alphabet characters in the result. Default value is `0`.
-- `number` (Boolean, Deprecated) Include numeric characters in the result. Default value is `true`. **NOTE**: This is deprecated, use `numeric` instead.
-- `numeric` (Boolean) Include numeric characters in the result. Default value is `true`.
+- `number` (Boolean, Deprecated) Include numeric characters in the result. Default value is `true`. If `number`, `upper`, `lower`, and `special` are all configured, at least one of them must be set to `true`. **NOTE**: This is deprecated, use `numeric` instead.
+- `numeric` (Boolean) Include numeric characters in the result. Default value is `true`. If `numeric`, `upper`, `lower`, and `special` are all configured, at least one of them must be set to `true`.
 - `override_special` (String) Supply your own list of special characters to use for string generation.  This overrides the default character list in the special argument.  The `special` argument must still be set to true for any overwritten characters to be used in generation.
 - `special` (Boolean) Include special characters in the result. These are `!@#$%&*()-_=+[]{}<>:?`. Default value is `true`.
 - `upper` (Boolean) Include uppercase alphabet characters in the result. Default value is `true`.

diff --git a/internal/provider/resource_password.go b/internal/provider/resource_password.go
@@ -26,6 +26,7 @@ import (
 	mapplanmodifiers "github.com/terraform-providers/terraform-provider-random/internal/planmodifiers/map"
 	stringplanmodifiers "github.com/terraform-providers/terraform-provider-random/internal/planmodifiers/string"
 	"github.com/terraform-providers/terraform-provider-random/internal/random"
+	"github.com/terraform-providers/terraform-provider-random/internal/validators"
 )
 
 var (
@@ -622,6 +623,8 @@ func passwordSchemaV3() schema.Schema {
 
 			"number": schema.BoolAttribute{
 				Description: "Include numeric characters in the result. Default value is `true`. " +
+					"If `number`, `upper`, `lower`, and `special` are all configured, at least one " +
+					"of them must be set to `true`. " +
 					"**NOTE**: This is deprecated, use `numeric` instead.",
 				Optional: true,
 				Computed: true,
@@ -630,16 +633,32 @@ func passwordSchemaV3() schema.Schema {
 					boolplanmodifier.RequiresReplace(),
 				},
 				DeprecationMessage: "**NOTE**: This is deprecated, use `numeric` instead.",
+				Validators: []validator.Bool{
+					validators.AtLeastOneOfTrue(
+						path.MatchRoot("special"),
+						path.MatchRoot("upper"),
+						path.MatchRoot("lower"),
+					),
+				},
 			},
 
 			"numeric": schema.BoolAttribute{
-				Description: "Include numeric characters in the result. Default value is `true`.",
-				Optional:    true,
-				Computed:    true,
+				Description: "Include numeric characters in the result. Default value is `true`. " +
+					"If `numeric`, `upper`, `lower`, and `special` are all configured, at least one " +
+					"of them must be set to `true`.",
+				Optional: true,
+				Computed: true,
 				PlanModifiers: []planmodifier.Bool{
 					boolplanmodifiers.NumberNumericAttributePlanModifier(),
 					boolplanmodifier.RequiresReplace(),
 				},
+				Validators: []validator.Bool{
+					validators.AtLeastOneOfTrue(
+						path.MatchRoot("special"),
+						path.MatchRoot("upper"),
+						path.MatchRoot("lower"),
+					),
+				},
 			},
 
 			"min_numeric": schema.Int64Attribute{

diff --git a/internal/provider/resource_password_test.go b/internal/provider/resource_password_test.go
@@ -67,6 +67,47 @@ func TestGenerateHash(t *testing.T) {
 	}
 }
 
+func TestCreateString(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]struct {
+		input         random.StringParams
+		expectedError error
+	}{
+		"input-false": {
+			input: random.StringParams{
+				Length:  16, // Required
+				Lower:   false,
+				Numeric: false,
+				Special: false,
+				Upper:   false,
+			},
+			expectedError: errors.New("the character set specified is empty"),
+		},
+	}
+
+	equateErrorMessage := cmp.Comparer(func(x, y error) bool {
+		if x == nil || y == nil {
+			return x == nil && y == nil
+		}
+		return x.Error() == y.Error()
+	})
+
+	for name, testCase := range testCases {
+		name, testCase := name, testCase
+
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			_, err := random.CreateString(testCase.input)
+
+			if diff := cmp.Diff(testCase.expectedError, err, equateErrorMessage); diff != "" {
+				t.Errorf("unexpected difference: %s", diff)
+			}
+		})
+	}
+}
+
 func TestAccResourcePassword_Import(t *testing.T) {
 	resource.UnitTest(t, resource.TestCase{
 		ProtoV5ProviderFactories: protoV5ProviderFactories(),
@@ -2718,6 +2759,61 @@ func TestAccResourcePassword_Keepers_FrameworkMigration_NullMapValueToValue(t *t
 	})
 }
 
+func TestAccResourcePassword_NumericFalse(t *testing.T) {
+	resource.ParallelTest(t, resource.TestCase{
+		Steps: []resource.TestStep{
+			{
+				ProtoV5ProviderFactories: protoV5ProviderFactories(),
+				Config: `resource "random_password" "test" {
+					length = 12
+					special = false
+					upper = false
+					lower = false
+					numeric = false
+				}`,
+				ExpectError: regexp.MustCompile(`At least one attribute out of \[special,upper,lower,numeric\] must be specified`),
+			},
+		},
+	})
+}
+
+func TestAccResourcePassword_NumberFalse(t *testing.T) {
+	resource.ParallelTest(t, resource.TestCase{
+		Steps: []resource.TestStep{
+			{
+				ProtoV5ProviderFactories: protoV5ProviderFactories(),
+				Config: `resource "random_password" "test" {
+					length = 12
+					special = false
+					upper = false
+					lower = false
+					number = false
+				}`,
+				ExpectError: regexp.MustCompile(`At least one attribute out of \[special,upper,lower,number\] must be specified`),
+			},
+		},
+	})
+}
+
+func TestAccResourcePassword_NumericNumberFalse(t *testing.T) {
+	resource.ParallelTest(t, resource.TestCase{
+		Steps: []resource.TestStep{
+			{
+				ProtoV5ProviderFactories: protoV5ProviderFactories(),
+				Config: `resource "random_password" "test" {
+					length = 12
+					special = false
+					upper = false
+					lower = false
+					numeric = false
+					number = false
+				}`,
+				ExpectError: regexp.MustCompile(`At least one attribute out of \[special,upper,lower,numeric\] must be specified((.|\n)*)At least one attribute out of \[special,upper,lower,number\] must be specified`),
+			},
+		},
+	})
+}
+
 func testBcryptHashInvalid(hash *string, password *string) resource.TestCheckFunc {
 	return func(_ *terraform.State) error {
 		if hash == nil || *hash == "" {

diff --git a/internal/provider/resource_string.go b/internal/provider/resource_string.go
@@ -24,6 +24,7 @@ import (
 	mapplanmodifiers "github.com/terraform-providers/terraform-provider-random/internal/planmodifiers/map"
 	stringplanmodifiers "github.com/terraform-providers/terraform-provider-random/internal/planmodifiers/string"
 	"github.com/terraform-providers/terraform-provider-random/internal/random"
+	"github.com/terraform-providers/terraform-provider-random/internal/validators"
 )
 
 var (
@@ -424,6 +425,8 @@ func stringSchemaV3() schema.Schema {
 
 			"number": schema.BoolAttribute{
 				Description: "Include numeric characters in the result. Default value is `true`. " +
+					"If `number`, `upper`, `lower`, and `special` are all configured, at least one " +
+					"of them must be set to `true`. " +
 					"**NOTE**: This is deprecated, use `numeric` instead.",
 				Optional: true,
 				Computed: true,
@@ -432,16 +435,32 @@ func stringSchemaV3() schema.Schema {
 					boolplanmodifier.RequiresReplace(),
 				},
 				DeprecationMessage: "**NOTE**: This is deprecated, use `numeric` instead.",
+				Validators: []validator.Bool{
+					validators.AtLeastOneOfTrue(
+						path.MatchRoot("special"),
+						path.MatchRoot("upper"),
+						path.MatchRoot("lower"),
+					),
+				},
 			},
 
 			"numeric": schema.BoolAttribute{
-				Description: "Include numeric characters in the result. Default value is `true`.",
-				Optional:    true,
-				Computed:    true,
+				Description: "Include numeric characters in the result. Default value is `true`. " +
+					"If `numeric`, `upper`, `lower`, and `special` are all configured, at least one " +
+					"of them must be set to `true`.",
+				Optional: true,
+				Computed: true,
 				PlanModifiers: []planmodifier.Bool{
 					boolplanmodifiers.NumberNumericAttributePlanModifier(),
 					boolplanmodifier.RequiresReplace(),
 				},
+				Validators: []validator.Bool{
+					validators.AtLeastOneOfTrue(
+						path.MatchRoot("special"),
+						path.MatchRoot("upper"),
+						path.MatchRoot("lower"),
+					),
+				},
 			},
 
 			"min_numeric": schema.Int64Attribute{

diff --git a/internal/provider/resource_string_test.go b/internal/provider/resource_string_test.go
@@ -1758,6 +1758,61 @@ func TestAccResourceString_Import_FromVersion3_4_2(t *testing.T) {
 	})
 }
 
+func TestAccResourceString_NumericFalse(t *testing.T) {
+	resource.ParallelTest(t, resource.TestCase{
+		Steps: []resource.TestStep{
+			{
+				ProtoV5ProviderFactories: protoV5ProviderFactories(),
+				Config: `resource "random_string" "test" {
+					length = 12
+					special = false
+					upper = false
+					lower = false
+					numeric = false
+				}`,
+				ExpectError: regexp.MustCompile(`At least one attribute out of \[special,upper,lower,numeric\] must be specified`),
+			},
+		},
+	})
+}
+
+func TestAccResourceString_NumberFalse(t *testing.T) {
+	resource.ParallelTest(t, resource.TestCase{
+		Steps: []resource.TestStep{
+			{
+				ProtoV5ProviderFactories: protoV5ProviderFactories(),
+				Config: `resource "random_string" "test" {
+					length = 12
+					special = false
+					upper = false
+					lower = false
+					number = false
+				}`,
+				ExpectError: regexp.MustCompile(`At least one attribute out of \[special,upper,lower,number\] must be specified`),
+			},
+		},
+	})
+}
+
+func TestAccResourceString_NumericNumberFalse(t *testing.T) {
+	resource.ParallelTest(t, resource.TestCase{
+		Steps: []resource.TestStep{
+			{
+				ProtoV5ProviderFactories: protoV5ProviderFactories(),
+				Config: `resource "random_string" "test" {
+					length = 12
+					special = false
+					upper = false
+					lower = false
+					numeric = false
+					number = false
+				}`,
+				ExpectError: regexp.MustCompile(`At least one attribute out of \[special,upper,lower,numeric\] must be specified((.|\n)*)At least one attribute out of \[special,upper,lower,number\] must be specified`),
+			},
+		},
+	})
+}
+
 func testCheckLen(expectedLen int) func(input string) error {
 	return func(input string) error {
 		if len(input) != expectedLen {

diff --git a/internal/random/string.go b/internal/random/string.go
@@ -5,6 +5,7 @@ package random
 
 import (
 	"crypto/rand"
+	"errors"
 	"math/big"
 	"sort"
 )
@@ -47,6 +48,10 @@ func CreateString(input StringParams) ([]byte, error) {
 		chars += specialChars
 	}
 
+	if chars == "" {
+		return nil, errors.New("the character set specified is empty")
+	}
+
 	minMapping := map[string]int64{
 		numChars:     input.MinNumeric,
 		lowerChars:   input.MinLower,
@@ -84,6 +89,14 @@ func CreateString(input StringParams) ([]byte, error) {
 }
 
 func generateRandomBytes(charSet *string, length int64) ([]byte, error) {
+	if charSet == nil {
+		return nil, errors.New("charSet is nil")
+	}
+
+	if *charSet == "" && length > 0 {
+		return nil, errors.New("charSet is empty")
+	}
+
 	bytes := make([]byte, length)
 	setLen := big.NewInt(int64(len(*charSet)))
 	for i := range bytes {