From f8ea8c9744aae4c39016ccbcff4a5e9f5d6f78d6 Mon Sep 17 00:00:00 2001 From: Jim Date: Thu, 27 Jul 2023 15:41:37 -0400 Subject: [PATCH] fix (ldap): support only updating the bind-dn and/or bind-password (#3511) * fix (ldap): support only updating the bind-dn and/or bind-password * chore: changelog entry for PR #3511 --- CHANGELOG.md | 4 + internal/auth/ldap/auth_method_test.go | 10 -- internal/auth/ldap/options.go | 4 - internal/auth/ldap/options_test.go | 12 +- .../ldap/repository_auth_method_update.go | 2 +- .../repository_auth_method_update_test.go | 56 +++++++ .../authmethods/authmethod_service_test.go | 14 +- .../controller/handlers/authmethods/ldap.go | 6 - .../handlers/authmethods/ldap_test.go | 144 ++++++++++++++++-- 9 files changed, 211 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40ebfae00d..2cdb7f9804 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. ## Next +### Bug Fixes + +* LDAP auth methods: allow bind-dn and bind-password to be updated independently. ([PR](https://github.com/hashicorp/boundary/pull/3511)) + ## 0.13.1 (2023/07/10) ### New and Improved diff --git a/internal/auth/ldap/auth_method_test.go b/internal/auth/ldap/auth_method_test.go index 128e3dc78f..38cabda3ad 100644 --- a/internal/auth/ldap/auth_method_test.go +++ b/internal/auth/ldap/auth_method_test.go @@ -127,16 +127,6 @@ func TestNewAuthMethod(t *testing.T) { wantErrCode: errors.InvalidParameter, wantErrContains: `https scheme in url "https://alice.com" is not either ldap or ldaps`, }, - { - name: "opt-error", - ctx: testCtx, - scopeId: "global", - urls: TestConvertToUrls(t, "ldaps://alice.com"), - opts: []Option{WithBindCredential(testCtx, "dn", "")}, - wantErr: true, - wantErrCode: errors.InvalidParameter, - wantErrContains: "ldap.WithBindCredential: missing password", - }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/auth/ldap/options.go b/internal/auth/ldap/options.go index 78e8ad4de5..3c78dd001a 100644 --- a/internal/auth/ldap/options.go +++ b/internal/auth/ldap/options.go @@ -250,10 +250,6 @@ func WithBindCredential(ctx context.Context, dn, password string) Option { switch { case dn == "" && password == "": return errors.New(ctx, errors.InvalidParameter, op, "missing both dn and password") - case dn != "" && password == "": - return errors.New(ctx, errors.InvalidParameter, op, "missing password") - case dn == "" && password != "": - return errors.New(ctx, errors.InvalidParameter, op, "missing dn") } o.withBindDn = dn o.withBindPassword = password diff --git a/internal/auth/ldap/options_test.go b/internal/auth/ldap/options_test.go index acf132b06e..331c2d9588 100644 --- a/internal/auth/ldap/options_test.go +++ b/internal/auth/ldap/options_test.go @@ -184,18 +184,18 @@ func Test_getOpts(t *testing.T) { testOpts.withBindPassword = "password" assert.Equal(opts, testOpts) }) - t.Run("WithBindCredential-missing-dn", func(t *testing.T) { + t.Run("WithBindCredential-only-password", func(t *testing.T) { assert := assert.New(t) opts, err := getOpts(WithBindCredential(testCtx, "", "password")) - require.Error(t, err) + require.NoError(t, err) assert.Empty(opts.withBindDn) - assert.Empty(opts.withBindPassword) + assert.NotEmpty(opts.withBindPassword) }) - t.Run("WithBindCredential-missing-password", func(t *testing.T) { + t.Run("WithBindCredential-only-dn", func(t *testing.T) { assert := assert.New(t) opts, err := getOpts(WithBindCredential(testCtx, "dn", "")) - require.Error(t, err) - assert.Empty(opts.withBindDn) + require.NoError(t, err) + assert.NotEmpty(opts.withBindDn) assert.Empty(opts.withBindPassword) }) t.Run("WithBindCredential-missing-dn-and-password", func(t *testing.T) { diff --git a/internal/auth/ldap/repository_auth_method_update.go b/internal/auth/ldap/repository_auth_method_update.go index b3ba337024..5598eebb0b 100644 --- a/internal/auth/ldap/repository_auth_method_update.go +++ b/internal/auth/ldap/repository_auth_method_update.go @@ -297,7 +297,7 @@ func (r *Repository) UpdateAuthMethod(ctx context.Context, am *AuthMethod, versi return nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) } if err := bc.encrypt(ctx, dbWrapper); err != nil { - return nil, db.NoRowsAffected, errors.Wrap(ctx, err, op) + return nil, db.NoRowsAffected, errors.Wrap(ctx, err, op, errors.WithMsg("error wrapping updated bind credential")) } addBindCred = bc } diff --git a/internal/auth/ldap/repository_auth_method_update_test.go b/internal/auth/ldap/repository_auth_method_update_test.go index ed3d6d2547..b547483d5f 100644 --- a/internal/auth/ldap/repository_auth_method_update_test.go +++ b/internal/auth/ldap/repository_auth_method_update_test.go @@ -494,6 +494,62 @@ func TestRepository_UpdateAuthMethod(t *testing.T) { return am }, }, + { + name: "update-just-binddn", + ctx: testCtx, + repo: testRepo, + setup: func() *AuthMethod { + return TestAuthMethod(t, + testConn, databaseWrapper, + org.PublicId, + []string{"ldaps://ldap1", "ldap://ldap2"}, + WithBindCredential(testCtx, "orig-bind-dn", "orig-bind-password"), + ) + }, + updateWith: func(orig *AuthMethod) *AuthMethod { + am := AllocAuthMethod() + am.PublicId = orig.PublicId + am.BindDn = "bind-dn" + return &am + }, + fieldMasks: []string{ + BindDnField, + }, + version: 1, + want: func(orig, updateWith *AuthMethod) *AuthMethod { + am := orig.clone() + am.BindDn = updateWith.BindDn + return am + }, + }, + { + name: "update-just-bind-password", + ctx: testCtx, + repo: testRepo, + setup: func() *AuthMethod { + return TestAuthMethod(t, + testConn, databaseWrapper, + org.PublicId, + []string{"ldaps://ldap1", "ldap://ldap2"}, + WithBindCredential(testCtx, "orig-bind-dn", "orig-bind-password"), + ) + }, + updateWith: func(orig *AuthMethod) *AuthMethod { + am := AllocAuthMethod() + am.PublicId = orig.PublicId + am.BindPassword = "bind-password" + return &am + }, + fieldMasks: []string{ + BindPasswordField, + }, + version: 1, + want: func(orig, updateWith *AuthMethod) *AuthMethod { + am := orig.clone() + am.BindPassword = updateWith.BindPassword + return am + }, + }, { name: "missing-auth-method", ctx: testCtx, diff --git a/internal/daemon/controller/handlers/authmethods/authmethod_service_test.go b/internal/daemon/controller/handlers/authmethods/authmethod_service_test.go index 17a3570472..fc70c39caa 100644 --- a/internal/daemon/controller/handlers/authmethods/authmethod_service_test.go +++ b/internal/daemon/controller/handlers/authmethods/authmethod_service_test.go @@ -1232,8 +1232,7 @@ func TestCreate(t *testing.T) { }, }, }}, - err: handlers.ApiErrorWithCode(codes.InvalidArgument), - errContains: "attributes.bind_password is missing required attributes.bind_dn field", + errContains: "missing dn", }, { name: "ldap-auth-method-missing-bind-password", @@ -1247,8 +1246,7 @@ func TestCreate(t *testing.T) { }, }, }}, - err: handlers.ApiErrorWithCode(codes.InvalidArgument), - errContains: "attributes.bind_dn is missing required attributes.bind_password field", + errContains: "missing password", }, { name: "ldap-auth-method-invalid-client-cert", @@ -1352,14 +1350,20 @@ func TestCreate(t *testing.T) { require.NoError(err, "Error when getting new auth_method service.") got, gErr := s.CreateAuthMethod(requestauth.DisabledAuthTestContext(iamRepoFn, tc.req.GetItem().GetScopeId()), tc.req) - if tc.err != nil { + switch { + case tc.err != nil: require.Error(gErr) assert.True(errors.Is(gErr, tc.err), "CreateAuthMethod(%+v) got error %v, wanted %v", tc.req, gErr, tc.err) if tc.errContains != "" { assert.Contains(gErr.Error(), tc.errContains) } return + case tc.errContains != "": + require.Error(gErr) + assert.Contains(gErr.Error(), tc.errContains) + return } + require.NoError(gErr) if tc.res == nil { require.Nil(got) diff --git a/internal/daemon/controller/handlers/authmethods/ldap.go b/internal/daemon/controller/handlers/authmethods/ldap.go index c51b877842..22e7ceaf28 100644 --- a/internal/daemon/controller/handlers/authmethods/ldap.go +++ b/internal/daemon/controller/handlers/authmethods/ldap.go @@ -299,12 +299,6 @@ func validateLdapAttributes(ctx context.Context, attrs *pb.LdapAuthMethodAttribu badFields[certificatesField] = fmt.Sprintf("invalid %s: %s", certificatesField, err.Error()) } } - if attrs.GetBindDn().GetValue() != "" && attrs.GetBindPassword().GetValue() == "" { - badFields[bindPasswordField] = fmt.Sprintf("%s is missing required %s field", bindDnField, bindPasswordField) - } - if attrs.GetBindPassword().GetValue() != "" && attrs.GetBindDn().GetValue() == "" { - badFields[bindDnField] = fmt.Sprintf("%s is missing required %s field", bindPasswordField, bindDnField) - } if attrs.GetClientCertificate().GetValue() != "" && attrs.GetClientCertificateKey().GetValue() == "" { badFields[clientCertificateKeyField] = fmt.Sprintf("%s is missing required %s field", clientCertificateField, clientCertificateKeyField) } diff --git a/internal/daemon/controller/handlers/authmethods/ldap_test.go b/internal/daemon/controller/handlers/authmethods/ldap_test.go index 8fa1911228..946d2ec967 100644 --- a/internal/daemon/controller/handlers/authmethods/ldap_test.go +++ b/internal/daemon/controller/handlers/authmethods/ldap_test.go @@ -81,14 +81,17 @@ func Test_UpdateLdap(t *testing.T) { }, } - freshAuthMethod := func(t *testing.T) (*pb.AuthMethod, func()) { + freshAuthMethod := func(t *testing.T, attrs *pb.AuthMethod_LdapAuthMethodsAttributes) (*pb.AuthMethod, func()) { + if attrs == nil { + attrs = defaultAttributes + } ctx := auth.DisabledAuthTestContext(iamRepoFn, o.GetPublicId()) am, err := tested.CreateAuthMethod(ctx, &pbs.CreateAuthMethodRequest{Item: &pb.AuthMethod{ ScopeId: o.GetPublicId(), Name: wrapperspb.String("default"), Description: wrapperspb.String("default"), Type: ldap.Subtype.String(), - Attrs: defaultAttributes, + Attrs: attrs, }}) require.NoError(t, err) @@ -103,12 +106,13 @@ func Test_UpdateLdap(t *testing.T) { _, testEncodedCert := ldap.TestGenerateCA(t, "localhost") tests := []struct { - name string - req *pbs.UpdateAuthMethodRequest - res *pbs.UpdateAuthMethodResponse - err error - errContains string - wantErr bool + name string + newAttrsOverride *pb.AuthMethod_LdapAuthMethodsAttributes + req *pbs.UpdateAuthMethodRequest + res *pbs.UpdateAuthMethodResponse + err error + errContains string + wantErr bool }{ { name: "update-an-existing-auth-method", @@ -349,6 +353,128 @@ func Test_UpdateLdap(t *testing.T) { }, }, }, + { + name: "update-only-bind-dn", + newAttrsOverride: &pb.AuthMethod_LdapAuthMethodsAttributes{ + LdapAuthMethodsAttributes: &pb.LdapAuthMethodAttributes{ + Urls: []string{"ldaps://ldap1"}, + State: "active-private", + BindDn: &wrapperspb.StringValue{Value: "bind-dn"}, + BindPassword: &wrapperspb.StringValue{Value: "bind-password"}, + }, + }, + req: &pbs.UpdateAuthMethodRequest{ + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"attributes.bind_dn"}, + }, + Item: &pb.AuthMethod{ + Attrs: &pb.AuthMethod_LdapAuthMethodsAttributes{ + LdapAuthMethodsAttributes: &pb.LdapAuthMethodAttributes{ + BindDn: &wrapperspb.StringValue{Value: "updated"}, + }, + }, + }, + }, + res: &pbs.UpdateAuthMethodResponse{ + Item: &pb.AuthMethod{ + ScopeId: o.GetPublicId(), + Version: 2, + Name: &wrapperspb.StringValue{Value: "default"}, + Description: &wrapperspb.StringValue{Value: "default"}, + Type: ldap.Subtype.String(), + Attrs: &pb.AuthMethod_LdapAuthMethodsAttributes{ + LdapAuthMethodsAttributes: &pb.LdapAuthMethodAttributes{ + Urls: []string{"ldaps://ldap1"}, + State: "active-private", + BindDn: &wrapperspb.StringValue{Value: "updated"}, + // note: BindPassword is never returned (an HMAC'd + // value is returned in a separate attribute) + }, + }, + Scope: defaultScopeInfo, + AuthorizedActions: ldapAuthorizedActions, + AuthorizedCollectionActions: authorizedCollectionActions, + }, + }, + }, + { + name: "err-update-only-bind-dn-with-no-orig-bind-password", + req: &pbs.UpdateAuthMethodRequest{ + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"attributes.bind_dn"}, + }, + Item: &pb.AuthMethod{ + Attrs: &pb.AuthMethod_LdapAuthMethodsAttributes{ + LdapAuthMethodsAttributes: &pb.LdapAuthMethodAttributes{ + BindDn: &wrapperspb.StringValue{Value: "updated"}, + }, + }, + }, + }, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), + errContains: "missing password", + }, + { + name: "update-only-bind-password", + newAttrsOverride: &pb.AuthMethod_LdapAuthMethodsAttributes{ + LdapAuthMethodsAttributes: &pb.LdapAuthMethodAttributes{ + Urls: []string{"ldaps://ldap1"}, + State: "active-private", + BindDn: &wrapperspb.StringValue{Value: "bind-dn"}, + BindPassword: &wrapperspb.StringValue{Value: "bind-password"}, + }, + }, + req: &pbs.UpdateAuthMethodRequest{ + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"attributes.bind_password"}, + }, + Item: &pb.AuthMethod{ + Attrs: &pb.AuthMethod_LdapAuthMethodsAttributes{ + LdapAuthMethodsAttributes: &pb.LdapAuthMethodAttributes{ + BindPassword: &wrapperspb.StringValue{Value: "updated"}, + }, + }, + }, + }, + res: &pbs.UpdateAuthMethodResponse{ + Item: &pb.AuthMethod{ + ScopeId: o.GetPublicId(), + Version: 2, + Name: &wrapperspb.StringValue{Value: "default"}, + Description: &wrapperspb.StringValue{Value: "default"}, + Type: ldap.Subtype.String(), + Attrs: &pb.AuthMethod_LdapAuthMethodsAttributes{ + LdapAuthMethodsAttributes: &pb.LdapAuthMethodAttributes{ + Urls: []string{"ldaps://ldap1"}, + State: "active-private", + BindDn: &wrapperspb.StringValue{Value: "bind-dn"}, + // note: BindPassword is never returned (an HMAC'd + // value is returned in a separate attribute) + }, + }, + Scope: defaultScopeInfo, + AuthorizedActions: ldapAuthorizedActions, + AuthorizedCollectionActions: authorizedCollectionActions, + }, + }, + }, + { + name: "err-update-only-bind-password-with-no-orig-bind-dn", + req: &pbs.UpdateAuthMethodRequest{ + UpdateMask: &field_mask.FieldMask{ + Paths: []string{"attributes.bind_password"}, + }, + Item: &pb.AuthMethod{ + Attrs: &pb.AuthMethod_LdapAuthMethodsAttributes{ + LdapAuthMethodsAttributes: &pb.LdapAuthMethodAttributes{ + BindPassword: &wrapperspb.StringValue{Value: "updated"}, + }, + }, + }, + }, + err: handlers.ApiErrorWithCode(codes.InvalidArgument), + errContains: "missing dn", + }, { name: "update-a-non-existent-auth-method", req: &pbs.UpdateAuthMethodRequest{ @@ -689,7 +815,7 @@ func Test_UpdateLdap(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - am, cleanup := freshAuthMethod(t) + am, cleanup := freshAuthMethod(t, tc.newAttrsOverride) defer cleanup() tc.req.Item.Version = am.GetVersion()