Skip to content

Commit

Permalink
Merge pull request #132748 from souravcrl/backport24.2-132086
Browse files Browse the repository at this point in the history 
release-24.2: ldapccl,sql: validate ldap options provided in HBA config entry
  • Loading branch information
@souravcrl
souravcrl authored Oct 17, 2024
2 parents 0b09a40 + f6cfdb9 commit 9a4eedc
Show file tree
Hide file tree
Showing 8 changed files with 419 additions and 176 deletions.
4 changes: 2 additions & 2 deletions pkg/ccl/ldapccl/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ go_library(
name = "ldapccl",
srcs = [
"authentication_ldap.go",
"ldap_test_util.go",
"ldap_util.go",
"settings.go",
],
Expand All @@ -13,6 +14,7 @@ go_library(
"//pkg/ccl/utilccl",
"//pkg/clusterversion",
"//pkg/security",
"//pkg/security/distinguishedname",
"//pkg/security/username",
"//pkg/server/telemetry",
"//pkg/settings",
Expand Down Expand Up @@ -49,7 +51,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 @@ -58,7 +59,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",
],
)
92 changes: 60 additions & 32 deletions pkg/ccl/ldapccl/authentication_ldap.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,15 @@ package ldapccl

import (
"context"
"net/url"
"regexp"

"github.com/cockroachdb/cockroach/pkg/ccl/utilccl"
"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/security/distinguishedname"
"github.com/cockroachdb/cockroach/pkg/security/username"
"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"
Expand All @@ -36,6 +40,15 @@ var (
beginAuthUseCounter = telemetry.GetCounterOnce(beginAuthCounterName)
loginSuccessUseCounter = telemetry.GetCounterOnce(loginSuccessCounterName)
enableUseCounter = telemetry.GetCounterOnce(enableCounterName)
// ldapSearchRe performs a regex match for ldap search options provided in HBA
// configuration. This generally adheres to the format "(key=value)" with
// interleaved spaces but could be more flexible as value field could be
// provided as a regex string(mail=*@example.com), a key-value distinguished
// name(memberOf="CN=test") or a combination of both(memberOf="CN=Sh*").
//
// The regex string is kept generic as search options could also contain
// multiple search entries like "(key1=value1)(key2=value2)".
ldapSearchRe = regexp.MustCompile(`\(\s*\S+\s*=\s*\S+.*\)`)
)

// ldapAuthenticator is an object that is used to enable ldap connection
Expand Down Expand Up @@ -139,33 +152,6 @@ func (authenticator *ldapAuthenticator) setLDAPConfigOptions(entry *hba.Entry) e
return nil
}

// validateLDAPOptions checks the ldap authenticator config values for validity.
func (authenticator *ldapAuthenticator) validateLDAPOptions() error {
const ldapOptionsErrorMsg = "ldap params in HBA conf missing"
if authenticator.mu.conf.ldapServer == "" {
return errors.New(ldapOptionsErrorMsg + " ldap server")
}
if authenticator.mu.conf.ldapPort == "" {
return errors.New(ldapOptionsErrorMsg + " ldap port")
}
if authenticator.mu.conf.ldapBaseDN == "" {
return errors.New(ldapOptionsErrorMsg + " base DN")
}
if authenticator.mu.conf.ldapBindDN == "" {
return errors.New(ldapOptionsErrorMsg + " bind DN")
}
if authenticator.mu.conf.ldapBindPassword == "" {
return errors.New(ldapOptionsErrorMsg + " bind password")
}
if authenticator.mu.conf.ldapSearchFilter == "" {
return errors.New(ldapOptionsErrorMsg + " search filter")
}
if authenticator.mu.conf.ldapSearchAttribute == "" {
return errors.New(ldapOptionsErrorMsg + " search attribute")
}
return nil
}

// ValidateLDAPLogin validates an attempt to bind to an LDAP server.
// In particular, it checks that:
// * The cluster has an enterprise license.
Expand Down Expand Up @@ -216,11 +202,6 @@ func (authenticator *ldapAuthenticator) ValidateLDAPLogin(
errors.Newf("LDAP authentication: unable to parse hba conf options")
}

if err := authenticator.validateLDAPOptions(); err != nil {
return redact.Sprintf("error validating hba conf options for LDAP: %v", err),
errors.Newf("LDAP authentication: unable to validate authenticator options")
}

// Establish a LDAPs connection with the set LDAP server and port
err := authenticator.mu.util.InitLDAPsConn(ctx, authenticator.mu.conf)
if err != nil {
Expand Down Expand Up @@ -253,6 +234,52 @@ func (authenticator *ldapAuthenticator) ValidateLDAPLogin(
return "", nil
}

// 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
}
// 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
}

// ConfigureLDAPAuth initializes and returns a ldapAuthenticator. It also sets up listeners so
// that the ldapAuthenticator's config is updated when the cluster settings values change.
var ConfigureLDAPAuth = func(
Expand All @@ -278,4 +305,5 @@ var ConfigureLDAPAuth = func(

func init() {
pgwire.ConfigureLDAPAuth = ConfigureLDAPAuth
pgwire.RegisterAuthMethod("ldap", pgwire.AuthLDAP, hba.ConnAny, checkHBAEntryLDAP)
}
127 changes: 0 additions & 127 deletions pkg/ccl/ldapccl/authentication_ldap_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,118 +9,19 @@ import (
"context"
"crypto/tls"
"fmt"
"strings"
"testing"

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/security/username"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/hba"
"github.com/cockroachdb/cockroach/pkg/testutils"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/errors"
"github.com/cockroachdb/redact"
"github.com/go-ldap/ldap/v3"
"github.com/stretchr/testify/require"
)

const (
emptyParam = "empty"
invalidParam = "invalid"
)

type mockLDAPUtil struct {
conn *ldap.Conn
tlsConfig *tls.Config
}

// InitLDAPsConn implements the ILDAPUtil interface.
func (lu *mockLDAPUtil) InitLDAPsConn(ctx context.Context, conf ldapAuthenticatorConf) error {
if strings.Contains(conf.ldapServer, invalidParam) {
return errors.Newf(ldapsFailureMessage + ": invalid ldap server provided")
} else if strings.Contains(conf.ldapPort, invalidParam) {
return errors.Newf(ldapsFailureMessage + ": invalid ldap port provided")
}
lu.conn = &ldap.Conn{}
return nil
}

// Bind implements the ILDAPUtil interface.
func (lu *mockLDAPUtil) Bind(ctx context.Context, userDN string, ldapPwd string) error {
if strings.Contains(userDN, invalidParam) {
return errors.Newf(bindFailureMessage + ": invalid username provided")
} else if strings.Contains(ldapPwd, invalidParam) {
return errors.Newf(bindFailureMessage + ": invalid password provided")
}

return nil
}

// Search implements the ILDAPUtil interface.
func (lu *mockLDAPUtil) Search(
ctx context.Context, conf ldapAuthenticatorConf, username string,
) (userDN string, err error) {
if err := lu.Bind(ctx, conf.ldapBindDN, conf.ldapBindPassword); err != nil {
return "", errors.Wrap(err, searchFailureMessage)
}
if strings.Contains(conf.ldapBaseDN, invalidParam) {
return "", errors.Newf(searchFailureMessage+": invalid base DN %q provided", conf.ldapBaseDN)
}
if strings.Contains(conf.ldapSearchFilter, invalidParam) {
return "", errors.Newf(searchFailureMessage+": invalid search filter %q provided", conf.ldapSearchFilter)
}
if strings.Contains(conf.ldapSearchAttribute, invalidParam) {
return "", errors.Newf(searchFailureMessage+": invalid search attribute %q provided", conf.ldapSearchAttribute)
}
if strings.Contains(username, invalidParam) {
return "", errors.Newf(searchFailureMessage+": invalid search value %q provided", username)
}
distinguishedNames := strings.Split(username, ",")
switch {
case len(username) == 0:
return "", errors.Newf(searchFailureMessage+": user %q does not exist", username)
case len(distinguishedNames) > 1:
return "", errors.Newf(searchFailureMessage+": too many matching entries returned for user %q", username)
}
return distinguishedNames[0], nil
}

var _ ILDAPUtil = &mockLDAPUtil{}

func constructHBAEntry(
t *testing.T,
hbaEntryBase string,
hbaConfLDAPDefaultOpts map[string]string,
hbaConfLDAPOpts map[string]string,
) hba.Entry {
hbaEntryLDAP := hbaEntryBase
// add options from default and override default options when provided with one
for opt, value := range hbaConfLDAPDefaultOpts {
setValue := value
if hbaConfLDAPOpts[opt] == emptyParam {
continue
} else if hbaConfLDAPOpts[opt] != "" {
setValue = hbaConfLDAPOpts[opt]
}
hbaEntryLDAP += fmt.Sprintf("\"%s=%s\" ", opt, setValue)
}
// add non default options
for additionalOpt, additionalOptValue := range hbaConfLDAPOpts {
if _, ok := hbaConfLDAPDefaultOpts[additionalOpt]; !ok {
hbaEntryLDAP += fmt.Sprintf("\"%s=%s\" ", additionalOpt, additionalOptValue)
}
}
hbaConf, err := hba.ParseAndNormalize(hbaEntryLDAP)
if err != nil {
t.Fatalf("error parsing hba conf: %v", err)
}
if len(hbaConf.Entries) != 1 {
t.Fatalf("hba conf value invalid: should contain only 1 entry")
}
return hbaConf.Entries[0]
}

func TestLDAPAuthentication(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
Expand Down Expand Up @@ -162,62 +63,34 @@ 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 authenticator options",
expectedDetailedErrMsg: "error validating 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 authenticator options",
expectedDetailedErrMsg: "error validating 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",
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", pwd: "bar", ldapAuthSuccess: false,
expectedErr: "LDAP authentication: unable to validate authenticator options",
expectedDetailedErrMsg: "error validating 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", pwd: "bar", ldapAuthSuccess: 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", pwd: "bar", ldapAuthSuccess: false,
expectedErr: "LDAP authentication: unable to validate authenticator options",
expectedDetailedErrMsg: "error validating 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", pwd: "bar", ldapAuthSuccess: 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", pwd: "bar", ldapAuthSuccess: false,
expectedErr: "LDAP authentication: unable to validate authenticator options",
expectedDetailedErrMsg: "error validating 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", pwd: "bar", ldapAuthSuccess: 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", pwd: "bar", ldapAuthSuccess: false,
expectedErr: "LDAP authentication: unable to validate authenticator options",
expectedDetailedErrMsg: "error validating hba conf options for LDAP: ldap params in HBA conf missing search attribute"},
{testName: "invalid search attribute",
hbaConfLDAPOpts: map[string]string{"ldapsearchattribute": invalidParam}, user: "foo", pwd: "bar", ldapAuthSuccess: 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", pwd: "bar", ldapAuthSuccess: false,
expectedErr: "LDAP authentication: unable to validate authenticator options",
expectedDetailedErrMsg: "error validating hba conf options for LDAP: ldap params in HBA conf missing search filter"},
{testName: "invalid search filter",
hbaConfLDAPOpts: map[string]string{"ldapsearchfilter": invalidParam}, user: "foo", pwd: "bar", ldapAuthSuccess: false,
expectedErr: "LDAP authentication: unable to find LDAP user distinguished name",
Expand Down
Loading

0 comments on commit 9a4eedc

Please sign in to comment.