diff --git a/pkg/ccl/ldapccl/BUILD.bazel b/pkg/ccl/ldapccl/BUILD.bazel index 2aa4ad86c266..ca6cc522db9f 100644 --- a/pkg/ccl/ldapccl/BUILD.bazel +++ b/pkg/ccl/ldapccl/BUILD.bazel @@ -6,6 +6,7 @@ go_library( "authentication_ldap.go", "authorization_ldap.go", "ldap_manager.go", + "ldap_test_util.go", "ldap_util.go", "settings.go", ], @@ -41,7 +42,6 @@ go_test( srcs = [ "authentication_ldap_test.go", "authorization_ldap_test.go", - "ldap_test_util_test.go", "main_test.go", "settings_test.go", ], @@ -56,7 +56,6 @@ go_test( "//pkg/security/securitytest", "//pkg/security/username", "//pkg/server", - "//pkg/sql/pgwire/hba", "//pkg/testutils", "//pkg/testutils/serverutils", "//pkg/testutils/testcluster", @@ -65,7 +64,6 @@ go_test( "//pkg/util/randutil", "@com_github_cockroachdb_errors//:errors", "@com_github_cockroachdb_redact//:redact", - "@com_github_go_ldap_ldap_v3//:ldap", "@com_github_stretchr_testify//require", ], ) diff --git a/pkg/ccl/ldapccl/authentication_ldap.go b/pkg/ccl/ldapccl/authentication_ldap.go index 5940d1e8c079..8fd79a7f89ac 100644 --- a/pkg/ccl/ldapccl/authentication_ldap.go +++ b/pkg/ccl/ldapccl/authentication_ldap.go @@ -80,16 +80,6 @@ func (authManager *ldapAuthManager) FetchLDAPUserDN( errors.Newf("LDAP authentication: unable to parse hba conf options") } - if err := authManager.validateLDAPBaseOptions(); err != nil { - return nil, redact.Sprintf("error validating base hba conf options for LDAP: %v", err), - errors.Newf("LDAP authentication: unable to validate authManager base options") - } - - if err := authManager.validateLDAPUserFetchOptions(); err != nil { - return nil, redact.Sprintf("error validating authentication hba conf options for LDAP: %v", err), - errors.Newf("LDAP authentication: unable to validate authManager authentication options") - } - // Establish a LDAPs connection with the set LDAP server and port err := authManager.mu.util.MaybeInitLDAPsConn(ctx, authManager.mu.conf) if err != nil { @@ -118,18 +108,6 @@ func (authManager *ldapAuthManager) FetchLDAPUserDN( return retrievedUserDN, "", nil } -// validateLDAPUserFetchOptions checks the ldap user search config values. -func (authManager *ldapAuthManager) validateLDAPUserFetchOptions() error { - const ldapOptionsErrorMsg = "ldap authentication params in HBA conf missing" - if authManager.mu.conf.ldapSearchFilter == "" { - return errors.New(ldapOptionsErrorMsg + " search filter") - } - if authManager.mu.conf.ldapSearchAttribute == "" { - return errors.New(ldapOptionsErrorMsg + " search attribute") - } - return nil -} - // ValidateLDAPLogin validates an attempt to bind provided user DN to configured LDAP server. // In particular, it checks that: // * The cluster has an enterprise license. @@ -173,11 +151,6 @@ func (authManager *ldapAuthManager) ValidateLDAPLogin( errors.Newf("LDAP authentication: unable to parse hba conf options") } - if err := authManager.validateLDAPBaseOptions(); err != nil { - return redact.Sprintf("error validating base hba conf options for LDAP: %v", err), - errors.Newf("LDAP authentication: unable to validate authManager base options") - } - // Establish a LDAPs connection with the set LDAP server and port err := authManager.mu.util.MaybeInitLDAPsConn(ctx, authManager.mu.conf) if err != nil { diff --git a/pkg/ccl/ldapccl/authentication_ldap_test.go b/pkg/ccl/ldapccl/authentication_ldap_test.go index 38f671c510e1..22d93b25b310 100644 --- a/pkg/ccl/ldapccl/authentication_ldap_test.go +++ b/pkg/ccl/ldapccl/authentication_ldap_test.go @@ -29,9 +29,7 @@ func TestLDAPFetchUser(t *testing.T) { // Intercept the call to NewLDAPUtil and return the mocked NewLDAPUtil function defer testutils.TestingHook( &NewLDAPUtil, - func(ctx context.Context, conf ldapConfig) (ILDAPUtil, error) { - return &mockLDAPUtil{tlsConfig: &tls.Config{}}, nil - })() + NewMockLDAPUtil)() ctx := context.Background() s := serverutils.StartServerOnly(t, base.TestServerArgs{}) defer s.Stopper().Stop(ctx) @@ -63,62 +61,34 @@ func TestLDAPFetchUser(t *testing.T) { hbaConfLDAPOpts: map[string]string{"invalidOpt": "invalidVal"}, user: "foo", fetchUserSuccess: false, expectedErr: "LDAP authentication: unable to parse hba conf options", expectedDetailedErrMsg: `error parsing hba conf options for LDAP: invalid LDAP option provided in hba conf: ‹invalidOpt›`}, - {testName: "empty server", - hbaConfLDAPOpts: map[string]string{"ldapserver": emptyParam}, user: "foo", fetchUserSuccess: false, - expectedErr: "LDAP authentication: unable to validate authManager base options", - expectedDetailedErrMsg: "error validating base hba conf options for LDAP: ldap params in HBA conf missing ldap server"}, {testName: "invalid server", hbaConfLDAPOpts: map[string]string{"ldapserver": invalidParam}, user: "foo", fetchUserSuccess: false, expectedErr: "LDAP authentication: unable to establish LDAP connection", expectedDetailedErrMsg: "error when trying to create LDAP connection: LDAPs connection failed: invalid ldap server provided"}, - {testName: "empty port", - hbaConfLDAPOpts: map[string]string{"ldapport": emptyParam}, user: "foo", fetchUserSuccess: false, - expectedErr: "LDAP authentication: unable to validate authManager base options", - expectedDetailedErrMsg: "error validating base hba conf options for LDAP: ldap params in HBA conf missing ldap port"}, {testName: "invalid port", hbaConfLDAPOpts: map[string]string{"ldapport": invalidParam}, user: "foo", fetchUserSuccess: false, expectedErr: "LDAP authentication: unable to establish LDAP connection", expectedDetailedErrMsg: "error when trying to create LDAP connection: LDAPs connection failed: invalid ldap port provided"}, - {testName: "empty base dn", - hbaConfLDAPOpts: map[string]string{"ldapbasedn": emptyParam}, user: "foo", fetchUserSuccess: false, - expectedErr: "LDAP authentication: unable to validate authManager base options", - expectedDetailedErrMsg: "error validating base hba conf options for LDAP: ldap params in HBA conf missing base DN"}, {testName: "invalid base dn", hbaConfLDAPOpts: map[string]string{"ldapbasedn": invalidParam}, user: "foo", fetchUserSuccess: false, expectedErr: "LDAP authentication: unable to find LDAP user distinguished name", expectedErrDetails: "cannot find provided user foo on LDAP server", expectedDetailedErrMsg: `error when searching for user in LDAP server: LDAP search failed: invalid base DN ‹"invalid"› provided`}, - {testName: "empty bind dn", - hbaConfLDAPOpts: map[string]string{"ldapbinddn": emptyParam}, user: "foo", fetchUserSuccess: false, - expectedErr: "LDAP authentication: unable to validate authManager base options", - expectedDetailedErrMsg: "error validating base hba conf options for LDAP: ldap params in HBA conf missing bind DN"}, {testName: "invalid bind dn", hbaConfLDAPOpts: map[string]string{"ldapbinddn": invalidParam}, user: "foo", fetchUserSuccess: false, expectedErr: "LDAP authentication: unable to find LDAP user distinguished name", expectedErrDetails: "cannot find provided user foo on LDAP server", expectedDetailedErrMsg: "error when searching for user in LDAP server: LDAP search failed: LDAP bind failed: invalid username provided"}, - {testName: "empty bind pwd", - hbaConfLDAPOpts: map[string]string{"ldapbindpasswd": emptyParam}, user: "foo", fetchUserSuccess: false, - expectedErr: "LDAP authentication: unable to validate authManager base options", - expectedDetailedErrMsg: "error validating base hba conf options for LDAP: ldap params in HBA conf missing bind password"}, {testName: "invalid bind pwd", hbaConfLDAPOpts: map[string]string{"ldapbindpasswd": invalidParam}, user: "foo", fetchUserSuccess: false, expectedErr: "LDAP authentication: unable to find LDAP user distinguished name", expectedErrDetails: "cannot find provided user foo on LDAP server", expectedDetailedErrMsg: "error when searching for user in LDAP server: LDAP search failed: LDAP bind failed: invalid password provided"}, - {testName: "empty search attribute", - hbaConfLDAPOpts: map[string]string{"ldapsearchattribute": emptyParam}, user: "foo", fetchUserSuccess: false, - expectedErr: "LDAP authentication: unable to validate authManager authentication options", - expectedDetailedErrMsg: "error validating authentication hba conf options for LDAP: ldap authentication params in HBA conf missing search attribute"}, {testName: "invalid search attribute", hbaConfLDAPOpts: map[string]string{"ldapsearchattribute": invalidParam}, user: "foo", fetchUserSuccess: false, expectedErr: "LDAP authentication: unable to find LDAP user distinguished name", expectedErrDetails: "cannot find provided user foo on LDAP server", expectedDetailedErrMsg: `error when searching for user in LDAP server: LDAP search failed: invalid search attribute ‹"invalid"› provided`}, - {testName: "empty search filter", - hbaConfLDAPOpts: map[string]string{"ldapsearchfilter": emptyParam}, user: "foo", fetchUserSuccess: false, - expectedErr: "LDAP authentication: unable to validate authManager authentication options", - expectedDetailedErrMsg: "error validating authentication hba conf options for LDAP: ldap authentication params in HBA conf missing search filter"}, {testName: "invalid search filter", hbaConfLDAPOpts: map[string]string{"ldapsearchfilter": invalidParam}, user: "foo", fetchUserSuccess: false, expectedErr: "LDAP authentication: unable to find LDAP user distinguished name", @@ -189,18 +159,10 @@ func TestLDAPAuthentication(t *testing.T) { hbaConfLDAPOpts: map[string]string{"invalidOpt": "invalidVal"}, user: "foo", pwd: "bar", ldapAuthSuccess: false, expectedErr: "LDAP authentication: unable to parse hba conf options", expectedDetailedErrMsg: `error parsing hba conf options for LDAP: invalid LDAP option provided in hba conf: ‹invalidOpt›`}, - {testName: "empty server", - hbaConfLDAPOpts: map[string]string{"ldapserver": emptyParam}, user: "foo", pwd: "bar", ldapAuthSuccess: false, - expectedErr: "LDAP authentication: unable to validate authManager base options", - expectedDetailedErrMsg: "error validating base hba conf options for LDAP: ldap params in HBA conf missing ldap server"}, {testName: "invalid server", hbaConfLDAPOpts: map[string]string{"ldapserver": invalidParam}, user: "foo", pwd: "bar", ldapAuthSuccess: false, expectedErr: "LDAP authentication: unable to establish LDAP connection", expectedDetailedErrMsg: "error when trying to create LDAP connection: LDAPs connection failed: invalid ldap server provided"}, - {testName: "empty port", - hbaConfLDAPOpts: map[string]string{"ldapport": emptyParam}, user: "foo", pwd: "bar", ldapAuthSuccess: false, - expectedErr: "LDAP authentication: unable to validate authManager base options", - expectedDetailedErrMsg: "error validating base hba conf options for LDAP: ldap params in HBA conf missing ldap port"}, {testName: "invalid port", hbaConfLDAPOpts: map[string]string{"ldapport": invalidParam}, user: "foo", pwd: "bar", ldapAuthSuccess: false, expectedErr: "LDAP authentication: unable to establish LDAP connection", diff --git a/pkg/ccl/ldapccl/authorization_ldap.go b/pkg/ccl/ldapccl/authorization_ldap.go index c5b3a9e66595..f3de821d6ae3 100644 --- a/pkg/ccl/ldapccl/authorization_ldap.go +++ b/pkg/ccl/ldapccl/authorization_ldap.go @@ -33,15 +33,6 @@ var ( authZSuccessCounter = telemetry.GetCounterOnce(authZSuccessCounterName) ) -// validateLDAPAuthZOptions checks the ldap authorization config values. -func (authManager *ldapAuthManager) validateLDAPAuthZOptions() error { - const ldapOptionsErrorMsg = "ldap authorization params in HBA conf missing" - if authManager.mu.conf.ldapGroupListFilter == "" { - return errors.New(ldapOptionsErrorMsg + " group list attribute") - } - return nil -} - // FetchLDAPGroups retrieves ldap groups for supplied ldap user DN. // In particular, it checks that: // * The cluster has an enterprise license. @@ -86,16 +77,6 @@ func (authManager *ldapAuthManager) FetchLDAPGroups( errors.Newf("LDAP authorization: unable to parse hba conf options") } - if err := authManager.validateLDAPBaseOptions(); err != nil { - return nil, redact.Sprintf("error validating base hba conf options for LDAP: %v", err), - errors.Newf("LDAP authorization: unable to validate authManager base options") - } - - if err := authManager.validateLDAPAuthZOptions(); err != nil { - return nil, redact.Sprintf("error validating authorization hba conf options for LDAP: %v", err), - errors.Newf("LDAP authorization: unable to validate authManager authorization options") - } - // Establish a LDAPs connection with the set LDAP server and port err := authManager.mu.util.MaybeInitLDAPsConn(ctx, authManager.mu.conf) if err != nil { diff --git a/pkg/ccl/ldapccl/authorization_ldap_test.go b/pkg/ccl/ldapccl/authorization_ldap_test.go index a33784beffb8..ff3ac2484794 100644 --- a/pkg/ccl/ldapccl/authorization_ldap_test.go +++ b/pkg/ccl/ldapccl/authorization_ldap_test.go @@ -29,7 +29,7 @@ func TestLDAPAuthorization(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) // Intercept the call to NewLDAPUtil and return the mocked NewLDAPUtil function - mockLDAP := &mockLDAPUtil{tlsConfig: &tls.Config{}} + mockLDAP := &mockLDAPUtil{} defer testutils.TestingHook( &NewLDAPUtil, func(ctx context.Context, conf ldapConfig) (ILDAPUtil, error) { @@ -65,53 +65,29 @@ func TestLDAPAuthorization(t *testing.T) { hbaConfLDAPOpts: map[string]string{"invalidOpt": "invalidVal"}, user: "cn=foo", authZSuccess: false, expectedErr: "LDAP authorization: unable to parse hba conf options", expectedDetailedErrMsg: `error parsing hba conf options for LDAP: invalid LDAP option provided in hba conf: ‹invalidOpt›`}, - {testName: "empty server", - hbaConfLDAPOpts: map[string]string{"ldapserver": emptyParam}, user: "cn=foo", authZSuccess: false, - expectedErr: "LDAP authorization: unable to validate authManager base options", - expectedDetailedErrMsg: "error validating base hba conf options for LDAP: ldap params in HBA conf missing ldap server"}, {testName: "invalid server", hbaConfLDAPOpts: map[string]string{"ldapserver": invalidParam}, user: "cn=foo", authZSuccess: false, expectedErr: "LDAP authorization: unable to establish LDAP connection", expectedDetailedErrMsg: "error when trying to create LDAP connection: LDAPs connection failed: invalid ldap server provided"}, - {testName: "empty port", - hbaConfLDAPOpts: map[string]string{"ldapport": emptyParam}, user: "cn=foo", authZSuccess: false, - expectedErr: "LDAP authorization: unable to validate authManager base options", - expectedDetailedErrMsg: "error validating base hba conf options for LDAP: ldap params in HBA conf missing ldap port"}, {testName: "invalid port", hbaConfLDAPOpts: map[string]string{"ldapport": invalidParam}, user: "cn=foo", authZSuccess: false, expectedErr: "LDAP authorization: unable to establish LDAP connection", expectedDetailedErrMsg: "error when trying to create LDAP connection: LDAPs connection failed: invalid ldap port provided"}, - {testName: "empty base dn", - hbaConfLDAPOpts: map[string]string{"ldapbasedn": emptyParam}, user: "cn=foo", authZSuccess: false, - expectedErr: "LDAP authorization: unable to validate authManager base options", - expectedDetailedErrMsg: "error validating base hba conf options for LDAP: ldap params in HBA conf missing base DN"}, {testName: "invalid base dn", hbaConfLDAPOpts: map[string]string{"ldapbasedn": invalidParam}, user: "cn=foo", authZSuccess: false, expectedErr: "LDAP authorization: unable to fetch groups for user", expectedErrDetails: "cannot find groups for which user is a member", expectedDetailedErrMsg: `error when fetching groups for user dn ‹"cn=foo"› in LDAP server: LDAP groups list failed: invalid base DN ‹"invalid"› provided`}, - {testName: "empty bind dn", - hbaConfLDAPOpts: map[string]string{"ldapbinddn": emptyParam}, user: "cn=foo", authZSuccess: false, - expectedErr: "LDAP authorization: unable to validate authManager base options", - expectedDetailedErrMsg: "error validating base hba conf options for LDAP: ldap params in HBA conf missing bind DN"}, {testName: "invalid bind dn", hbaConfLDAPOpts: map[string]string{"ldapbinddn": invalidParam}, user: "cn=foo", authZSuccess: false, expectedErr: "LDAP authorization: unable to fetch groups for user", expectedErrDetails: "cannot find groups for which user is a member", expectedDetailedErrMsg: `error when fetching groups for user dn ‹"cn=foo"› in LDAP server: LDAP groups list failed: LDAP bind failed: invalid username provided`}, - {testName: "empty bind pwd", - hbaConfLDAPOpts: map[string]string{"ldapbindpasswd": emptyParam}, user: "cn=foo", authZSuccess: false, - expectedErr: "LDAP authorization: unable to validate authManager base options", - expectedDetailedErrMsg: "error validating base hba conf options for LDAP: ldap params in HBA conf missing bind password"}, {testName: "invalid bind pwd", hbaConfLDAPOpts: map[string]string{"ldapbindpasswd": invalidParam}, user: "cn=foo", authZSuccess: false, expectedErr: "LDAP authorization: unable to fetch groups for user", expectedErrDetails: "cannot find groups for which user is a member", expectedDetailedErrMsg: `error when fetching groups for user dn ‹"cn=foo"› in LDAP server: LDAP groups list failed: LDAP bind failed: invalid password provided`}, - {testName: "empty group list filter", - hbaConfLDAPOpts: map[string]string{"ldapgrouplistfilter": emptyParam}, user: "cn=foo", authZSuccess: false, - expectedErr: "LDAP authorization: unable to validate authManager authorization options", - expectedDetailedErrMsg: "error validating authorization hba conf options for LDAP: ldap authorization params in HBA conf missing group list attribute"}, {testName: "invalid group list filter", hbaConfLDAPOpts: map[string]string{"ldapgrouplistfilter": invalidParam}, user: "cn=foo", authZSuccess: false, expectedErr: "LDAP authorization: unable to fetch groups for user", diff --git a/pkg/ccl/ldapccl/ldap_manager.go b/pkg/ccl/ldapccl/ldap_manager.go index 499adf8be284..71bb211987f8 100644 --- a/pkg/ccl/ldapccl/ldap_manager.go +++ b/pkg/ccl/ldapccl/ldap_manager.go @@ -7,8 +7,12 @@ package ldapccl import ( "context" + "net/url" + "regexp" + "github.com/cockroachdb/cockroach/pkg/security/distinguishedname" "github.com/cockroachdb/cockroach/pkg/server/telemetry" + "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/pgwire" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/hba" @@ -23,7 +27,10 @@ const ( enableCounterName = counterPrefix + "enable" ) -var enableUseCounter = telemetry.GetCounterOnce(enableCounterName) +var ( + enableUseCounter = telemetry.GetCounterOnce(enableCounterName) + ldapSearchRe = regexp.MustCompile(`\(\s*\S+\s*=\s*\S+.*\)`) +) // ldapAuthManager is an object that is used for both: // 1. enabling ldap connection validation that are used as part of the CRDB @@ -79,11 +86,10 @@ func (authManager *ldapAuthManager) reloadConfig(ctx context.Context, st *cluste // reloadConfig refreshes the values in conf from the cluster settings without locking the mutex. func (authManager *ldapAuthManager) reloadConfigLocked(ctx context.Context, st *cluster.Settings) { - conf := ldapConfig{ - domainCACert: LDAPDomainCACertificate.Get(&st.SV), - clientTLSCert: LDAPClientTLSCertSetting.Get(&st.SV), - clientTLSKey: LDAPClientTLSKeySetting.Get(&st.SV), - } + conf := authManager.mu.conf + conf.domainCACert = LDAPDomainCACertificate.Get(&st.SV) + conf.clientTLSCert = LDAPClientTLSCertSetting.Get(&st.SV) + conf.clientTLSKey = LDAPClientTLSKeySetting.Get(&st.SV) authManager.mu.conf = conf var err error @@ -103,9 +109,7 @@ func (authManager *ldapAuthManager) reloadConfigLocked(ctx context.Context, st * // setLDAPConfigOptions extracts hba conf parameters required for connecting and // querying LDAP server from hba conf entry and sets them for LDAP auth. func (authManager *ldapAuthManager) setLDAPConfigOptions(entry *hba.Entry) error { - conf := ldapConfig{ - domainCACert: authManager.mu.conf.domainCACert, - } + conf := authManager.mu.conf for _, opt := range entry.Options { switch opt[0] { case "ldapserver": @@ -132,23 +136,48 @@ func (authManager *ldapAuthManager) setLDAPConfigOptions(entry *hba.Entry) error return nil } -// validateLDAPBaseOptions checks the mandatory ldap auth config values for validity. -func (authManager *ldapAuthManager) validateLDAPBaseOptions() error { - const ldapOptionsErrorMsg = "ldap params in HBA conf missing" - if authManager.mu.conf.ldapServer == "" { - return errors.New(ldapOptionsErrorMsg + " ldap server") - } - if authManager.mu.conf.ldapPort == "" { - return errors.New(ldapOptionsErrorMsg + " ldap port") - } - if authManager.mu.conf.ldapBaseDN == "" { - return errors.New(ldapOptionsErrorMsg + " base DN") - } - if authManager.mu.conf.ldapBindDN == "" { - return errors.New(ldapOptionsErrorMsg + " bind DN") +// checkHBAEntryLDAP validates that the HBA entry for ldap has all the options +// set to acceptable values and mandatory options are all set. +func checkHBAEntryLDAP(_ *settings.Values, entry hba.Entry) error { + var parseErr error + entryOptions := map[string]bool{} + for _, opt := range entry.Options { + switch opt[0] { + case "ldapserver": + _, parseErr = url.Parse(opt[1]) + case "ldapport": + if opt[1] != "389" && opt[1] != "636" { + parseErr = errors.Newf("%q is not set to either 389 or 636", opt[0]) + } + case "ldapbasedn": + fallthrough + case "ldapbinddn": + _, parseErr = distinguishedname.ParseDN(opt[1]) + case "ldapbindpasswd": + fallthrough + case "ldapsearchattribute": + if opt[1] == "" { + parseErr = errors.Newf("%q is set to empty", opt[0]) + } + case "ldapsearchfilter": + fallthrough + case "ldapgrouplistfilter": + if !ldapSearchRe.MatchString(opt[1]) { + parseErr = errors.Newf("%q is not of the format \"(key = value)\"", opt[0]) + } + default: + return errors.Newf("unknown ldap option provided in hba conf: %q", opt[0]) + } + if parseErr != nil { + return errors.Wrapf(parseErr, "LDAP option %q is set to invalid value: %q", opt[0], opt[1]) + } + entryOptions[opt[0]] = true } - if authManager.mu.conf.ldapBindPassword == "" { - return errors.New(ldapOptionsErrorMsg + " bind password") + // check for missing ldap options + for _, opt := range []string{"ldapserver", "ldapport", "ldapbasedn", "ldapbinddn", "ldapbindpasswd", "ldapsearchattribute", "ldapsearchfilter"} { + if _, ok := entryOptions[opt]; !ok { + return errors.Newf("ldap option not found in hba entry: %q", opt) + } } return nil } @@ -178,4 +207,5 @@ var ConfigureLDAPAuth = func( func init() { pgwire.ConfigureLDAPAuth = ConfigureLDAPAuth + pgwire.RegisterAuthMethod("ldap", pgwire.AuthLDAP, hba.ConnAny, checkHBAEntryLDAP) } diff --git a/pkg/ccl/ldapccl/ldap_test_util_test.go b/pkg/ccl/ldapccl/ldap_test_util.go similarity index 96% rename from pkg/ccl/ldapccl/ldap_test_util_test.go rename to pkg/ccl/ldapccl/ldap_test_util.go index f0a7b466e71c..28fd644d5cea 100644 --- a/pkg/ccl/ldapccl/ldap_test_util_test.go +++ b/pkg/ccl/ldapccl/ldap_test_util.go @@ -28,6 +28,15 @@ type mockLDAPUtil struct { groupDNs []string } +var _ ILDAPUtil = &mockLDAPUtil{} + +var NewMockLDAPUtil func(context.Context, ldapConfig) (ILDAPUtil, error) = func( + ctx context.Context, + conf ldapConfig, +) (ILDAPUtil, error) { + return &mockLDAPUtil{}, nil +} + // MaybeInitLDAPsConn implements the ILDAPUtil interface. func (lu *mockLDAPUtil) MaybeInitLDAPsConn(ctx context.Context, conf ldapConfig) error { if strings.Contains(conf.ldapServer, invalidParam) { @@ -113,8 +122,6 @@ func (lu *mockLDAPUtil) ListGroups( return lu.groupDNs, nil } -var _ ILDAPUtil = &mockLDAPUtil{} - func constructHBAEntry( t *testing.T, hbaEntryBase string, diff --git a/pkg/ccl/testccl/authccl/BUILD.bazel b/pkg/ccl/testccl/authccl/BUILD.bazel index 20e3750e216d..4836684160dd 100644 --- a/pkg/ccl/testccl/authccl/BUILD.bazel +++ b/pkg/ccl/testccl/authccl/BUILD.bazel @@ -11,6 +11,7 @@ go_test( "//pkg/base", "//pkg/ccl", "//pkg/ccl/jwtauthccl", + "//pkg/ccl/ldapccl", "//pkg/security/securityassets", "//pkg/security/securitytest", "//pkg/security/username", diff --git a/pkg/ccl/testccl/authccl/auth_test.go b/pkg/ccl/testccl/authccl/auth_test.go index 72f78163bac0..9da37b5e67f2 100644 --- a/pkg/ccl/testccl/authccl/auth_test.go +++ b/pkg/ccl/testccl/authccl/auth_test.go @@ -10,6 +10,7 @@ import ( "context" gosql "database/sql" "fmt" + "io" "math" "net" "net/http" @@ -25,6 +26,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/ccl/jwtauthccl" + "github.com/cockroachdb/cockroach/pkg/ccl/ldapccl" "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/apiconstants" "github.com/cockroachdb/cockroach/pkg/server/authserver" @@ -146,6 +148,10 @@ func makeSocketFile(t *testing.T) (socketDir, socketFile string, cleanupFn func( } func jwtRunTest(t *testing.T, insecure bool) { + httpScheme := "http://" + if !insecure { + httpScheme = "https://" + } datadriven.Walk(t, datapathutils.TestDataPath(t), func(t *testing.T, path string) { defer leaktest.AfterTest(t)() @@ -190,6 +196,16 @@ func jwtRunTest(t *testing.T, insecure bool) { }) defer srv.Stopper().Stop(context.Background()) s := srv.ApplicationLayer() + pgServer := s.PGServer().(*pgwire.Server) + pgServer.TestingEnableConnLogging() + pgServer.TestingEnableAuthLogging() + s.PGPreServer().(*pgwire.PreServeConnHandler).TestingAcceptSystemIdentityOption(true) + + httpClient, err := s.GetAdminHTTPClient() + if err != nil { + t.Fatal(err) + } + httpHBAUrl := httpScheme + s.HTTPAddr() + "/debug/hba_conf" sv := &s.ClusterSettings().SV @@ -197,6 +213,11 @@ func jwtRunTest(t *testing.T, insecure bool) { t.Fatal(err) } + // Intercept the call to NewLDAPUtil and return the mocked NewLDAPUtil function + defer testutils.TestingHook( + &ldapccl.NewLDAPUtil, + ldapccl.NewMockLDAPUtil)() + datadriven.RunTest(t, path, func(t *testing.T, td *datadriven.TestData) string { resultString, err := func() (string, error) { switch td.Cmd { @@ -420,6 +441,47 @@ func jwtRunTest(t *testing.T, insecure bool) { defer resp.Body.Close() return strconv.Itoa(resp.StatusCode), nil + case "set_hba": + _, err := conn.ExecContext(context.Background(), + `SET CLUSTER SETTING server.host_based_authentication.configuration = $1`, td.Input) + if err != nil { + return "", err + } + + // Wait until the configuration has propagated back to the + // test client. We need to wait because the cluster setting + // change propagates asynchronously. + expConf := pgwire.DefaultHBAConfig + if td.Input != "" { + expConf, err = pgwire.ParseAndNormalize(td.Input) + if err != nil { + // The SET above succeeded so we don't expect a problem here. + t.Fatal(err) + } + } + testutils.SucceedsSoon(t, func() error { + curConf, _ := pgServer.GetAuthenticationConfiguration() + if expConf.String() != curConf.String() { + return errors.Newf( + "HBA config not yet loaded\ngot:\n%s\nexpected:\n%s", + curConf, expConf) + } + return nil + }) + + // Verify the HBA configuration was processed properly by + // reporting the resulting cached configuration. + resp, err := httpClient.Get(httpHBAUrl) + if err != nil { + return "", err + } + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + if err != nil { + return "", err + } + return string(body), nil + default: td.Fatalf(t, "unknown command: %s", td.Cmd) } diff --git a/pkg/sql/pgwire/auth_methods.go b/pkg/sql/pgwire/auth_methods.go index ef400a1a8b26..b1a1b4967029 100644 --- a/pkg/sql/pgwire/auth_methods.go +++ b/pkg/sql/pgwire/auth_methods.go @@ -84,11 +84,12 @@ func loadDefaultMethods() { RegisterAuthMethod("trust", authTrust, hba.ConnAny, NoOptionsAllowed) // The "ldap" method requires a clear text password which will be used to bind // with a LDAP server. The remaining connection parameters are provided in hba - // conf options + // conf options. This auth method is re-registered in ldapccl module with + // proper handler for CheckHBAEntry. // // Care should be taken by administrators to only accept this auth // method over secure connections, e.g. those encrypted using SSL. - RegisterAuthMethod("ldap", authLDAP, hba.ConnAny, nil) + RegisterAuthMethod("ldap", AuthLDAP, hba.ConnAny, nil) } // AuthMethod is a top-level factory for composing the various @@ -112,7 +113,7 @@ var _ AuthMethod = authTrust var _ AuthMethod = authReject var _ AuthMethod = authSessionRevivalToken([]byte{}) var _ AuthMethod = authJwtToken -var _ AuthMethod = authLDAP +var _ AuthMethod = AuthLDAP // authPassword is the AuthMethod constructor for HBA method // "password": authenticate using a cleartext password received from @@ -918,9 +919,9 @@ var ConfigureLDAPAuth = func( return &noLDAPConfigured{} } -// authLDAP is the AuthMethod constructor for the CRDB-specific +// AuthLDAP is the AuthMethod constructor for the CRDB-specific // ldap auth mechanism. -func authLDAP( +func AuthLDAP( sCtx context.Context, c AuthConn, sessionUser username.SQLUsername,