Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Oct 8, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions pkg/ccl/ldapccl/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ go_library(
"authentication_ldap.go",
"authorization_ldap.go",
"ldap_manager.go",
"ldap_test_util.go",
"ldap_util.go",
"settings.go",
],
Expand Down Expand Up @@ -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",
],
Expand All @@ -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",
Expand All @@ -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",
],
)
27 changes: 0 additions & 27 deletions pkg/ccl/ldapccl/authentication_ldap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down
40 changes: 1 addition & 39 deletions pkg/ccl/ldapccl/authentication_ldap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
19 changes: 0 additions & 19 deletions pkg/ccl/ldapccl/authorization_ldap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down
26 changes: 1 addition & 25 deletions pkg/ccl/ldapccl/authorization_ldap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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",
Expand Down
Loading
Loading