diff --git a/pkg/ccl/ldapccl/BUILD.bazel b/pkg/ccl/ldapccl/BUILD.bazel index 4cbfab87ddc5..e3ea28d182bc 100644 --- a/pkg/ccl/ldapccl/BUILD.bazel +++ b/pkg/ccl/ldapccl/BUILD.bazel @@ -4,6 +4,7 @@ go_library( name = "ldapccl", srcs = [ "authentication_ldap.go", + "ldap_test_util.go", "ldap_util.go", "settings.go", ], @@ -13,6 +14,7 @@ go_library( "//pkg/ccl/utilccl", "//pkg/clusterversion", "//pkg/security", + "//pkg/security/distinguishedname", "//pkg/security/username", "//pkg/server/telemetry", "//pkg/settings", @@ -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", @@ -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", ], ) diff --git a/pkg/ccl/ldapccl/authentication_ldap.go b/pkg/ccl/ldapccl/authentication_ldap.go index 0741c2783daf..babfed997299 100644 --- a/pkg/ccl/ldapccl/authentication_ldap.go +++ b/pkg/ccl/ldapccl/authentication_ldap.go @@ -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" @@ -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 @@ -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. @@ -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 { @@ -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( @@ -278,4 +305,5 @@ var ConfigureLDAPAuth = func( func init() { pgwire.ConfigureLDAPAuth = ConfigureLDAPAuth + pgwire.RegisterAuthMethod("ldap", pgwire.AuthLDAP, hba.ConnAny, checkHBAEntryLDAP) } diff --git a/pkg/ccl/ldapccl/authentication_ldap_test.go b/pkg/ccl/ldapccl/authentication_ldap_test.go index e39db993274f..d363dcc502b4 100644 --- a/pkg/ccl/ldapccl/authentication_ldap_test.go +++ b/pkg/ccl/ldapccl/authentication_ldap_test.go @@ -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) @@ -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", diff --git a/pkg/ccl/ldapccl/ldap_test_util.go b/pkg/ccl/ldapccl/ldap_test_util.go new file mode 100644 index 000000000000..1231dac8e6c3 --- /dev/null +++ b/pkg/ccl/ldapccl/ldap_test_util.go @@ -0,0 +1,121 @@ +// Copyright 2024 The Cockroach Authors. +// +// Use of this software is governed by the CockroachDB Software License +// included in the /LICENSE file. + +package ldapccl + +import ( + "context" + "crypto/tls" + "fmt" + "strings" + "testing" + + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/hba" + "github.com/cockroachdb/errors" + "github.com/go-ldap/ldap/v3" +) + +const ( + emptyParam = "empty" + invalidParam = "invalid" +) + +type mockLDAPUtil struct { + conn *ldap.Conn + tlsConfig *tls.Config +} + +var _ ILDAPUtil = &mockLDAPUtil{} + +var NewMockLDAPUtil func(context.Context, ldapAuthenticatorConf) (ILDAPUtil, error) = func( + ctx context.Context, + conf ldapAuthenticatorConf, +) (ILDAPUtil, error) { + return &mockLDAPUtil{}, nil +} + +// 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 +} + +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] +} diff --git a/pkg/ccl/testccl/authccl/BUILD.bazel b/pkg/ccl/testccl/authccl/BUILD.bazel index 9ddf1bd98b71..cbed9fced75d 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 2122a4d02a08..e1f4e6511ee8 100644 --- a/pkg/ccl/testccl/authccl/auth_test.go +++ b/pkg/ccl/testccl/authccl/auth_test.go @@ -9,6 +9,7 @@ import ( "context" gosql "database/sql" "fmt" + "io" "math" "net" "net/url" @@ -23,6 +24,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/sql/pgwire" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" @@ -106,7 +108,7 @@ func TestAuthenticationAndHBARules(t *testing.T) { skip.UnderRace(t, "takes >1min under race") testutils.RunTrueAndFalse(t, "insecure", func(t *testing.T, insecure bool) { - jwtRunTest(t, insecure) + authCCLRunTest(t, insecure) }) } @@ -141,8 +143,7 @@ func makeSocketFile(t *testing.T) (socketDir, socketFile string, cleanupFn func( func() { _ = os.RemoveAll(tempDir) } } -func jwtRunTest(t *testing.T, insecure bool) { - +func authCCLRunTest(t *testing.T, insecure bool) { datadriven.Walk(t, datapathutils.TestDataPath(t), func(t *testing.T, path string) { defer leaktest.AfterTest(t)() @@ -186,13 +187,28 @@ 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 := s.AdminURL().WithPath("/debug/hba_conf").String() sv := &s.ClusterSettings().SV - if _, err := conn.ExecContext(context.Background(), fmt.Sprintf(`CREATE USER %s`, username.TestUser)); err != nil { t.Fatal(err) } + // Intercept the call to NewLDAPUtil and return the mocked NewLDAPUtil function + if strings.Contains(path, "ldap") { + 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 { @@ -373,7 +389,46 @@ func jwtRunTest(t *testing.T, insecure bool) { return "", err } return "ok " + dbName, 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/ccl/testccl/authccl/testdata/ldap b/pkg/ccl/testccl/authccl/testdata/ldap new file mode 100644 index 000000000000..de44085ed671 --- /dev/null +++ b/pkg/ccl/testccl/authccl/testdata/ldap @@ -0,0 +1,167 @@ +# Verify LDAP HBA entry and authentication/authorization works. + +config secure +---- + +sql +CREATE USER ldap_user; +CREATE USER test; +CREATE USER test2; +---- +ok + +subtest missing_ldap_hba_param + +set_hba +host all ldap_user 127.0.0.1/32 ldap +---- +ERROR: ldap option not found in hba entry: "ldapserver" + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost +---- +ERROR: ldap option not found in hba entry: "ldapport" + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 +---- +ERROR: ldap option not found in hba entry: "ldapbasedn" + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" +---- +ERROR: ldap option not found in hba entry: "ldapbinddn" + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" +---- +ERROR: ldap option not found in hba entry: "ldapbindpasswd" + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd +---- +ERROR: ldap option not found in hba entry: "ldapsearchattribute" + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName +---- +ERROR: ldap option not found in hba entry: "ldapsearchfilter" + +subtest end + +subtest incorrect_ldap_hba_param + +set_hba +host all ldap_user 127.0.0.1/32 ldap "ldapserver=:invalid" ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" +---- +ERROR: LDAP option "ldapserver" is set to invalid value: ":invalid": parse ":invalid": missing protocol scheme + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=007 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" +---- +ERROR: LDAP option "ldapport" is set to invalid value: "007": "ldapport" is not set to either 389 or 636 + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=baseDN" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" +---- +ERROR: LDAP option "ldapbasedn" is set to invalid value: "baseDN": failed to parse distinguished name baseDN: DN ended with incomplete type, value pair + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=bindDN" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" +---- +ERROR: LDAP option "ldapbinddn" is set to invalid value: "bindDN": failed to parse distinguished name bindDN: DN ended with incomplete type, value pair + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" "ldapbindpasswd=" ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" +---- +ERROR: LDAP option "ldapbindpasswd" is set to invalid value: "": "ldapbindpasswd" is set to empty + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd "ldapsearchattribute=" "ldapsearchfilter=(memberOf=*)" +---- +ERROR: LDAP option "ldapsearchattribute" is set to invalid value: "": "ldapsearchattribute" is set to empty + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(*)" +---- +ERROR: LDAP option "ldapsearchfilter" is set to invalid value: "(*)": "ldapsearchfilter" is not of the format "(key = value)" + +subtest end + +subtest invalid_ldap_password + +set_hba +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" +---- +# Active authentication configuration on this node: +# Original configuration: +# loopback all all all trust # built-in CockroachDB default +# host all root all cert-password # CockroachDB mandatory rule +# host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" +# +# Interpreted configuration: +# TYPE DATABASE USER ADDRESS METHOD OPTIONS +loopback all all all trust +host all root all cert-password +host all ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" + +connect user=ldap_user password="invalid" +---- +ERROR: LDAP authentication: unable to bind as LDAP user (SQLSTATE 28000) +DETAIL: credentials invalid for LDAP server user ldap_user + +subtest end + +subtest correct_ldap_password + +connect user=ldap_user password="valid" +---- +ok defaultdb + +subtest end + +subtest unknown_ldap_user + +set_hba +host all invalid_ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" +---- +# Active authentication configuration on this node: +# Original configuration: +# loopback all all all trust # built-in CockroachDB default +# host all root all cert-password # CockroachDB mandatory rule +# host all invalid_ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" +# +# Interpreted configuration: +# TYPE DATABASE USER ADDRESS METHOD OPTIONS +loopback all all all trust +host all root all cert-password +host all invalid_ldap_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" + +connect user=invalid_ldap_user password="valid" +---- +ERROR: password authentication failed for user invalid_ldap_user (SQLSTATE 28P01) + +subtest end + +subtest unknown_sql_user + +set_hba +host all unknown_sql_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" +---- +# Active authentication configuration on this node: +# Original configuration: +# loopback all all all trust # built-in CockroachDB default +# host all root all cert-password # CockroachDB mandatory rule +# host all unknown_sql_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" +# +# Interpreted configuration: +# TYPE DATABASE USER ADDRESS METHOD OPTIONS +loopback all all all trust +host all root all cert-password +host all unknown_sql_user 127.0.0.1/32 ldap ldapserver=localhost ldapport=636 "ldapbasedn=O=security org,DC=localhost" "ldapbinddn=CN=service_account,O=security org,DC=localhost" ldapbindpasswd=ldap_pwd ldapsearchattribute=sAMAccountName "ldapsearchfilter=(memberOf=*)" + +connect user=unknown_sql_user password="valid" +---- +ERROR: password authentication failed for user unknown_sql_user (SQLSTATE 28P01) + +subtest end diff --git a/pkg/sql/pgwire/auth_methods.go b/pkg/sql/pgwire/auth_methods.go index 749343c3df47..94ecad0b3817 100644 --- a/pkg/sql/pgwire/auth_methods.go +++ b/pkg/sql/pgwire/auth_methods.go @@ -80,13 +80,6 @@ func loadDefaultMethods() { // The "trust" method accepts any connection attempt that matches // the current rule. 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 - // - // 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) } // AuthMethod is a top-level factory for composing the various @@ -109,7 +102,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 @@ -842,9 +835,14 @@ var ConfigureLDAPAuth = func( return &noLDAPConfigured{} } -// authLDAP is the AuthMethod constructor for the CRDB-specific -// ldap auth mechanism. -func authLDAP( +// AuthLDAP is the AuthMethod constructor for the CRDB-specific ldap auth +// mechanism. 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. +// +// Care should be taken by administrators to only accept this auth method over +// secure connections, e.g. those encrypted using SSL. +func AuthLDAP( sCtx context.Context, c AuthConn, _ tls.ConnectionState,