From f34b85c34a0b760155a76e33475a25f872dd87c5 Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Thu, 1 Dec 2022 14:30:37 -0500 Subject: [PATCH 1/2] Fix bug where updates to config would fail is password isn't provided --- plugin/backend_test.go | 41 ++++++++++++++++++++++++++++++++++ plugin/checkout_handler.go | 10 ++++----- plugin/client/fieldregistry.go | 9 ++++---- plugin/client/time.go | 10 ++++----- plugin/path_config.go | 20 ++++++++++++++++- plugin/path_config_test.go | 3 ++- 6 files changed, 76 insertions(+), 17 deletions(-) diff --git a/plugin/backend_test.go b/plugin/backend_test.go index cd3c0ec1..2a505141 100644 --- a/plugin/backend_test.go +++ b/plugin/backend_test.go @@ -34,6 +34,7 @@ func TestBackend(t *testing.T) { t.Run("write config", WriteConfig) t.Run("read config", ReadConfig) t.Run("delete config", DeleteConfig) + t.Run("update config", UpdateConfig) // Plant a config for further testing. t.Run("plant config", PlantConfig) @@ -82,6 +83,46 @@ func WriteConfig(t *testing.T) { } } +func UpdateConfig(t *testing.T) { + req := &logical.Request{ + Operation: logical.UpdateOperation, + Path: configPath, + Storage: testStorage, + Data: map[string]interface{}{ + "binddn": "tester", + "password": "pa$$w0rd", + "url": "ldap://138.91.247.105", + "certificate": validCertificate, + "userdn": "dc=example,dc=com", + "formatter": "mycustom{{PASSWORD}}", + "last_rotation_tolerance": 10, + }, + } + resp, err := testBackend.HandleRequest(testCtx, req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatal(err) + } + if resp != nil { + t.Fatal("expected no response because Vault generally doesn't return it for posts") + } + + req = &logical.Request{ + Operation: logical.UpdateOperation, + Path: configPath, + Storage: testStorage, + Data: map[string]interface{}{ + "certificate": validCertificate, + }, + } + resp, err = testBackend.HandleRequest(testCtx, req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatal(err) + } + if resp != nil { + t.Fatal("expected no response because Vault generally doesn't return it for posts") + } +} + func ReadConfig(t *testing.T) { req := &logical.Request{ Operation: logical.ReadOperation, diff --git a/plugin/checkout_handler.go b/plugin/checkout_handler.go index 61d02144..96ea7364 100644 --- a/plugin/checkout_handler.go +++ b/plugin/checkout_handler.go @@ -127,8 +127,8 @@ func (h *checkOutHandler) CheckIn(ctx context.Context, storage logical.Storage, } // LoadCheckOut returns either: -// - A *CheckOut and nil error if the serviceAccountName is currently managed by this engine. -// - A nil *Checkout and errNotFound if the serviceAccountName is not currently managed by this engine. +// - A *CheckOut and nil error if the serviceAccountName is currently managed by this engine. +// - A nil *Checkout and errNotFound if the serviceAccountName is not currently managed by this engine. func (h *checkOutHandler) LoadCheckOut(ctx context.Context, storage logical.Storage, serviceAccountName string) (*CheckOut, error) { if ctx == nil { return nil, errors.New("ctx must be provided") @@ -174,9 +174,9 @@ func (h *checkOutHandler) Delete(ctx context.Context, storage logical.Storage, s // retrievePassword is a utility function for grabbing a service account's password from storage. // retrievePassword will return: -// - "password", nil if it was successfully able to retrieve the password. -// - errNotFound if there's no password presently. -// - Some other err if it was unable to complete successfully. +// - "password", nil if it was successfully able to retrieve the password. +// - errNotFound if there's no password presently. +// - Some other err if it was unable to complete successfully. func retrievePassword(ctx context.Context, storage logical.Storage, serviceAccountName string) (string, error) { entry, err := storage.Get(ctx, passwordStoragePrefix+serviceAccountName) if err != nil { diff --git a/plugin/client/fieldregistry.go b/plugin/client/fieldregistry.go index 4baf1e0a..792888b7 100644 --- a/plugin/client/fieldregistry.go +++ b/plugin/client/fieldregistry.go @@ -9,14 +9,13 @@ import ( // // Example: Accessing constants // -// FieldRegistry.AccountExpires -// FieldRegistry.BadPasswordCount +// FieldRegistry.AccountExpires +// FieldRegistry.BadPasswordCount // // Example: Utility methods // -// FieldRegistry.List() -// FieldRegistry.Parse("givenName") -// +// FieldRegistry.List() +// FieldRegistry.Parse("givenName") var FieldRegistry = newFieldRegistry() // newFieldRegistry iterates through all the fields in the registry, diff --git a/plugin/client/time.go b/plugin/client/time.go index c451ba47..a0da47d4 100644 --- a/plugin/client/time.go +++ b/plugin/client/time.go @@ -27,11 +27,11 @@ func ParseTicks(ticks string) (time.Time, error) { // TicksToTime converts an ActiveDirectory time in ticks to a time. // This algorithm is summarized as: // -// Many dates are saved in Active Directory as Large Integer values. -// These attributes represent dates as the number of 100-nanosecond intervals since 12:00 AM January 1, 1601. -// 100-nanosecond intervals, equal to 0.0000001 seconds, are also called ticks. -// Dates in Active Directory are always saved in Coordinated Universal Time, or UTC. -// More: https://social.technet.microsoft.com/wiki/contents/articles/31135.active-directory-large-integer-attributes.aspx +// Many dates are saved in Active Directory as Large Integer values. +// These attributes represent dates as the number of 100-nanosecond intervals since 12:00 AM January 1, 1601. +// 100-nanosecond intervals, equal to 0.0000001 seconds, are also called ticks. +// Dates in Active Directory are always saved in Coordinated Universal Time, or UTC. +// More: https://social.technet.microsoft.com/wiki/contents/articles/31135.active-directory-large-integer-attributes.aspx // // If we directly follow the above algorithm we encounter time.Duration limits of 290 years and int overflow issues. // Thus below, we carefully sidestep those. diff --git a/plugin/path_config.go b/plugin/path_config.go index 7a20f894..4d840387 100644 --- a/plugin/path_config.go +++ b/plugin/path_config.go @@ -100,11 +100,29 @@ func (b *backend) configFields() map[string]*framework.FieldSchema { } func (b *backend) configUpdateOperation(ctx context.Context, req *logical.Request, fieldData *framework.FieldData) (*logical.Response, error) { + + conf, err := readConfig(ctx, req.Storage) + if err != nil { + return nil, err + } + + if conf == nil { + conf = new(configuration) + conf.ADConf = new(client.ADConf) + } + + // Use the existing ldap client config if it is set + var existing *ldaputil.ConfigEntry + if conf.ADConf != nil && conf.ADConf.ConfigEntry != nil { + existing = conf.ADConf.ConfigEntry + } + // Build and validate the ldap conf. - activeDirectoryConf, err := ldaputil.NewConfigEntry(nil, fieldData) + activeDirectoryConf, err := ldaputil.NewConfigEntry(existing, fieldData) if err != nil { return nil, err } + if err := activeDirectoryConf.Validate(); err != nil { return nil, err } diff --git a/plugin/path_config_test.go b/plugin/path_config_test.go index 0991df8d..44d7e8ab 100644 --- a/plugin/path_config_test.go +++ b/plugin/path_config_test.go @@ -2,9 +2,10 @@ package plugin import ( "context" + "testing" + "github.com/mitchellh/mapstructure" "github.com/stretchr/testify/assert" - "testing" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/logical" From 8bf214ec01054c2477911943ebe71bf7365d1b77 Mon Sep 17 00:00:00 2001 From: Jason O'Donnell <2160810+jasonodonnell@users.noreply.github.com> Date: Thu, 1 Dec 2022 14:35:02 -0500 Subject: [PATCH 2/2] Make test clearer --- plugin/backend_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/plugin/backend_test.go b/plugin/backend_test.go index 2a505141..b40e0fb9 100644 --- a/plugin/backend_test.go +++ b/plugin/backend_test.go @@ -92,7 +92,6 @@ func UpdateConfig(t *testing.T) { "binddn": "tester", "password": "pa$$w0rd", "url": "ldap://138.91.247.105", - "certificate": validCertificate, "userdn": "dc=example,dc=com", "formatter": "mycustom{{PASSWORD}}", "last_rotation_tolerance": 10,