ldapccl,sql: validate ldap options provided in HBA config entry

fixes CRDB-41624 Epic: CRDB-33829 Currently, we validate ldap configuration provided as HBA entry options at the time an auth request comes in for ldap. This prevents us from disallowing invalid/incomplete list of ldap options in HBA. This PR fixes the issue. Release note(security, ops): HBA config entry for LDAP will be evaluated with validations for proper ldap config parameter values and any invalid/incomplete options list will be disallowed to amend the HBA setting. We will validate all fields provided as ldap auth method options in HBA entry.
cockroachdb · Oct 7, 2024 · 948be68 · 948be68
1 parent dcce4ca
commit 948be68
Show file tree

Hide file tree

Showing 10 changed files with 136 additions and 145 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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",