From fda4c47af097b92955585993ecb4aa39fcbd7e91 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Fri, 17 Jun 2022 17:04:14 -0700 Subject: [PATCH 1/4] config: set default length only if password policy is missing --- go.mod | 1 + plugin/path_config.go | 13 ++++- plugin/path_config_test.go | 101 +++++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index d986a556..17af1874 100644 --- a/go.mod +++ b/go.mod @@ -14,5 +14,6 @@ require ( github.com/hashicorp/yamux v0.0.0-20181012175058-2f1d1f20f75d // indirect github.com/mitchellh/mapstructure v1.4.2 github.com/patrickmn/go-cache v2.1.0+incompatible + github.com/stretchr/testify v1.7.0 golang.org/x/text v0.3.7 ) diff --git a/plugin/path_config.go b/plugin/path_config.go index 34382c41..2f42bb92 100644 --- a/plugin/path_config.go +++ b/plugin/path_config.go @@ -114,10 +114,19 @@ func (b *backend) configUpdateOperation(ctx context.Context, req *logical.Reques maxTTL := fieldData.Get("max_ttl").(int) lastRotationTolerance := fieldData.Get("last_rotation_tolerance").(int) - length := fieldData.Get("length").(int) - formatter := fieldData.Get("formatter").(string) passwordPolicy := fieldData.Get("password_policy").(string) + var length int + if lengthRaw, ok := fieldData.GetOk("length"); ok { + length = lengthRaw.(int) + } else if !ok && passwordPolicy == "" { + // If neither the length nor a password policy was provided, fall back + // to the length's field data default value. + length = fieldData.Get("length").(int) + } + + formatter := fieldData.Get("formatter").(string) + if pre111Val, ok := fieldData.GetOk("use_pre111_group_cn_behavior"); ok { activeDirectoryConf.UsePre111GroupCNBehavior = new(bool) *activeDirectoryConf.UsePre111GroupCNBehavior = pre111Val.(bool) diff --git a/plugin/path_config_test.go b/plugin/path_config_test.go index 01de0940..1106857d 100644 --- a/plugin/path_config_test.go +++ b/plugin/path_config_test.go @@ -2,6 +2,8 @@ package plugin import ( "context" + "github.com/mitchellh/mapstructure" + "github.com/stretchr/testify/assert" "testing" "github.com/hashicorp/vault/sdk/framework" @@ -88,3 +90,102 @@ func TestCacheReader(t *testing.T) { t.Fatal("config should be nil") } } + +func TestConfig_PasswordLength(t *testing.T) { + + // we should start with no config + config, err := readConfig(ctx, storage) + if err != nil { + t.Fatal(err) + } + if config != nil { + t.Fatal("config should be nil") + } + + req := &logical.Request{ + Operation: logical.UpdateOperation, + Path: configPath, + Storage: storage, + } + + tests := []struct { + name string + rawFieldData map[string]interface{} + wantErr bool + }{ + { + "length provided", + map[string]interface{}{ + "length": 32, + }, + false, + + }, + { + "password policy provided", + map[string]interface{}{ + "password_policy": "foo", + }, + false, + }, + { + "no length or password policy provided", + nil, + false, + }, + { + "both length and password policy provided", + map[string]interface{}{ + "password_policy": "foo", + "length": 32, + }, + true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // submit a minimal config so we can check that we're using safe defaults + fieldData := &framework.FieldData{ + Schema: testBackend.pathConfig().Fields, + Raw: map[string]interface{}{ + "binddn": "tester", + "password": "pa$$w0rd", + "urls": "ldap://138.91.247.105", + "userdn": "example,com", + }, + } + + for k, v := range tt.rawFieldData { + fieldData.Raw[k] = v + } + + _, err = testBackend.configUpdateOperation(ctx, req, fieldData) + assert.Equal(t, tt.wantErr, err != nil) + + if tt.wantErr && err != nil { + return + } + + config, err := readConfig(ctx, storage) + assert.NoError(t, err) + + var actual map[string]interface{} + + cfg := &mapstructure.DecoderConfig{ + Result: &actual, + TagName: "json", + } + decoder, err := mapstructure.NewDecoder(cfg) + assert.NoError(t, err) + err = decoder.Decode(config.PasswordConf) + assert.NoError(t, err) + + for k, v := range tt.rawFieldData{ + assert.Contains(t, actual, k) + assert.Equal(t, actual[k], v) + } + }) + } +} + From f77de2cddf66db6ab43a2703301fe1f047b96b87 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Fri, 17 Jun 2022 17:13:56 -0700 Subject: [PATCH 2/4] make fmt --- plugin/path_config.go | 2 +- plugin/path_config_test.go | 14 ++++++-------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/plugin/path_config.go b/plugin/path_config.go index 2f42bb92..ad9bbf00 100644 --- a/plugin/path_config.go +++ b/plugin/path_config.go @@ -119,7 +119,7 @@ func (b *backend) configUpdateOperation(ctx context.Context, req *logical.Reques var length int if lengthRaw, ok := fieldData.GetOk("length"); ok { length = lengthRaw.(int) - } else if !ok && passwordPolicy == "" { + } else if !ok && passwordPolicy == "" { // If neither the length nor a password policy was provided, fall back // to the length's field data default value. length = fieldData.Get("length").(int) diff --git a/plugin/path_config_test.go b/plugin/path_config_test.go index 1106857d..60ff6f98 100644 --- a/plugin/path_config_test.go +++ b/plugin/path_config_test.go @@ -109,9 +109,9 @@ func TestConfig_PasswordLength(t *testing.T) { } tests := []struct { - name string + name string rawFieldData map[string]interface{} - wantErr bool + wantErr bool }{ { "length provided", @@ -119,7 +119,6 @@ func TestConfig_PasswordLength(t *testing.T) { "length": 32, }, false, - }, { "password policy provided", @@ -137,7 +136,7 @@ func TestConfig_PasswordLength(t *testing.T) { "both length and password policy provided", map[string]interface{}{ "password_policy": "foo", - "length": 32, + "length": 32, }, true, }, @@ -173,19 +172,18 @@ func TestConfig_PasswordLength(t *testing.T) { var actual map[string]interface{} cfg := &mapstructure.DecoderConfig{ - Result: &actual, - TagName: "json", + Result: &actual, + TagName: "json", } decoder, err := mapstructure.NewDecoder(cfg) assert.NoError(t, err) err = decoder.Decode(config.PasswordConf) assert.NoError(t, err) - for k, v := range tt.rawFieldData{ + for k, v := range tt.rawFieldData { assert.Contains(t, actual, k) assert.Equal(t, actual[k], v) } }) } } - From 8367fbc6188e4999b557bfbd07b9188efbe5429c Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Fri, 17 Jun 2022 17:15:32 -0700 Subject: [PATCH 3/4] update test comment --- plugin/path_config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/path_config_test.go b/plugin/path_config_test.go index 60ff6f98..0991df8d 100644 --- a/plugin/path_config_test.go +++ b/plugin/path_config_test.go @@ -144,7 +144,7 @@ func TestConfig_PasswordLength(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // submit a minimal config so we can check that we're using safe defaults + // Start common config fields and append what we need to test against fieldData := &framework.FieldData{ Schema: testBackend.pathConfig().Fields, Raw: map[string]interface{}{ From 53dc05df3258092d33853c9ca2a744e5b433d838 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang <1883212+calvn@users.noreply.github.com> Date: Tue, 21 Jun 2022 16:50:31 -0700 Subject: [PATCH 4/4] add changelog, remove unneeded ok check --- CHANGELOG.md | 5 +++++ plugin/path_config.go | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 00000000..959489f2 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,5 @@ +## Unreleased + +### IMPROVEMENTS: + +* config: set default length only if password policy is missing [GH-85](https://github.com/hashicorp/vault-plugin-secrets-ad/pull/85) diff --git a/plugin/path_config.go b/plugin/path_config.go index ad9bbf00..7a20f894 100644 --- a/plugin/path_config.go +++ b/plugin/path_config.go @@ -119,7 +119,7 @@ func (b *backend) configUpdateOperation(ctx context.Context, req *logical.Reques var length int if lengthRaw, ok := fieldData.GetOk("length"); ok { length = lengthRaw.(int) - } else if !ok && passwordPolicy == "" { + } else if passwordPolicy == "" { // If neither the length nor a password policy was provided, fall back // to the length's field data default value. length = fieldData.Get("length").(int)