authserver, ldapccl: enable ldap authorization for db console

[shriramters]

authserver,ldapccl: enable ldap authorization for db console

Previously, when a user authenticated against the DB Console via LDAP,
the system would only perform authentication (verifying the user's
password) but would not perform authorization (synchronizing the
user's roles from their LDAP group memberships).

This was inadequate because it created an inconsistent experience. A
user connecting through a SQL client would have their roles correctly
synced from LDAP, while the same user logging into the DB Console
would not, potentially having different (or no) privileges.

To address this, this patch refactors the LDAP authorization logic
into a single, reusable AuthorizeLDAP function in the pgwire
package. This shared function is now called by both the pgwire path
(for SQL clients) and the DB Console's UserLogin HTTP handler. This
eliminates code duplication and ensures that both connection methods
use the exact same logic for role synchronization, providing a
consistent experience for all users.

Epic: none
Fixes: #149654

Release note (enterprise change): LDAP authentication for the DB
Console now additionally supports role-based access control (RBAC)
through LDAP group membership.
.
To use this feature, an administrator must first create roles in
CockroachDB with names that match the Common Names (CN) of their LDAP
groups. These roles should then be granted the desired privileges for
DB Console access.
.
When a user who is a member of a corresponding LDAP group logs into
the DB Console, they will be automatically granted the role and its
associated privileges, creating consistent behavior with SQL client
connections.

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[souravcrl]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @shriramters)

---

-- commits line 24 at r1:
We need to mention the support for RBAC is complementary to manual authorization, seems appropriate to phrase it:

LDAP authentication for the DB Console now additionally supports role-based access control (RBAC)

---

-- commits line 26 at r1:
The note could be improved by providing context around the database groups being previously created and have been setup with desired privileges for db console access.

---

pkg/server/authserver/authentication.go line 249 at r1 (raw file):

	// If LDAP authentication was successful and the HBA entry includes a
	// group list filter, perform role authorization.
	if ldapAuthSuccess && hbaEntry.GetOption("ldapgrouplistfilter") != "" {

Should we consider using the behaviors.MaybeAuthorize() here as the code feels duplicated? We could use the ReplacementIdentity and cut down further on setup code.

---

pkg/server/authserver/authentication.go line 261 at r1 (raw file):

			// This should ideally not fail as it succeeded moments ago, but handle defensively.
			err := errors.Wrapf(authError, "LDAP authorization: error re-retrieving ldap user DN for authorization")
			log.Warningf(ctx, "%v", err)

Should we wrap this verbosity lvl 1 and also consider logging in the OPS channel, i.e. use log.Ops.VWarningf(ctx, 1, ...) ?

---

pkg/server/authserver/authentication.go line 262 at r1 (raw file):

			err := errors.Wrapf(authError, "LDAP authorization: error re-retrieving ldap user DN for authorization")
			log.Warningf(ctx, "%v", err)
			return nil, srverrors.APIInternalError(ctx, err)

Should we consider only sending the client facing error message here as the error can have sensitive ldap server data?

---

pkg/server/authserver/authentication.go line 273 at r1 (raw file):

				errForLog = errors.Join(errForLog, errors.Newf("%s", detailedErrors))
			}
			log.Warningf(ctx, "%v", errForLog)

Should we wrap this verbosity lvl 1 and also consider logging in the OPS channel, i.e. use log.Ops.VWarningf(ctx, 1, ...) ?

---

pkg/server/authserver/authentication.go line 274 at r1 (raw file):

			}
			log.Warningf(ctx, "%v", errForLog)
			return nil, srverrors.APIInternalError(ctx, errForLog)

Should we consider only sending the client facing error message here as the error can have sensitive ldap server data?

---

pkg/server/authserver/authentication.go line 284 at r1 (raw file):

			if err != nil {
				err := errors.Wrapf(err, "LDAP authorization: error finding matching SQL role for group %s", ldapGroup.String())
				log.Warningf(ctx, "%v", err)

Should we wrap this verbosity lvl 1 and also consider logging in the OPS channel, i.e. use log.Ops.VWarningf(ctx, 1, ...) ?

---

pkg/server/authserver/authentication.go line 285 at r1 (raw file):

				err := errors.Wrapf(err, "LDAP authorization: error finding matching SQL role for group %s", ldapGroup.String())
				log.Warningf(ctx, "%v", err)
				return nil, srverrors.APIInternalError(ctx, err)

Should we consider only sending the client facing error message here as the error can have sensitive ldap server data?

---

pkg/server/authserver/authentication.go line 298 at r1 (raw file):

		if err := sql.EnsureUserOnlyBelongsToRoles(ctx, execCfg, username, sqlRoles); err != nil {
			err = errors.Wrapf(err, "LDAP authorization: error assigning roles to user %s", username)
			log.Warningf(ctx, "%v", err)

Should we wrap this verbosity lvl 1 and also consider logging in the OPS channel, i.e. use log.Ops.VWarningf(ctx, 1, ...) ?

---

pkg/server/authserver/authentication.go line 299 at r1 (raw file):

			err = errors.Wrapf(err, "LDAP authorization: error assigning roles to user %s", username)
			log.Warningf(ctx, "%v", err)
			return nil, srverrors.APIInternalError(ctx, err)

Should we consider only sending the client facing error message here as the error can have sensitive sql role information?

---

pkg/ccl/ldapccl/authentication_ldap_test.go line 276 at r1 (raw file):

	req := &serverpb.UserLoginRequest{
		Username: "alice",
		Password: "password", // The mock will accept any password.

Should we have tests with username = invalid & password = invalid just as an elementary validation which the mock is configured to reject for bind?

---

pkg/ccl/ldapccl/authentication_ldap_test.go line 287 at r1 (raw file):

	rows := tdb.QueryStr(t, `SELECT role FROM system.role_members WHERE member = 'alice'`)
	require.Len(t, rows, 1, "alice should only have one role")
	require.Equal(t, "db_admins", rows[0][0])

Should we perform a revocation with the removal for db_admins for alice?

[shriramters]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @souravcrl)

---

-- commits line 24 at r1:

Previously, souravcrl wrote…

We need to mention the support for RBAC is complementary to manual authorization, seems appropriate to phrase it:

LDAP authentication for the DB Console now additionally supports role-based access control (RBAC)

Done

---

-- commits line 26 at r1:

Previously, souravcrl wrote…

The note could be improved by providing context around the database groups being previously created and have been setup with desired privileges for db console access.

Rewrote it. Please let me know if it looks ok..

---

pkg/server/authserver/authentication.go line 249 at r1 (raw file):

Previously, souravcrl wrote…

Should we consider using the behaviors.MaybeAuthorize() here as the code feels duplicated? We could use the ReplacementIdentity and cut down further on setup code.

I have refactored the Authorization logic into a function in ldapccl which can be reused for DB Console as well.

this also follows the pattern used by FetchLDAPUserDN, FetchLDAPGroups (detailedError and clientError) but we lose two Info logs.

---

pkg/server/authserver/authentication.go line 261 at r1 (raw file):

Previously, souravcrl wrote…

Should we wrap this verbosity lvl 1 and also consider logging in the OPS channel, i.e. use log.Ops.VWarningf(ctx, 1, ...) ?

This code was removed and the auth behaviour code was used

---

pkg/server/authserver/authentication.go line 262 at r1 (raw file):

Previously, souravcrl wrote…

Should we consider only sending the client facing error message here as the error can have sensitive ldap server data?

I removed this piece of code and reused the ldapUserDN which was already being fetched earlier in the function

---

pkg/server/authserver/authentication.go line 273 at r1 (raw file):

Previously, souravcrl wrote…

Should we wrap this verbosity lvl 1 and also consider logging in the OPS channel, i.e. use log.Ops.VWarningf(ctx, 1, ...) ?

This code was removed and the auth behaviour code was used

---

pkg/server/authserver/authentication.go line 274 at r1 (raw file):

Previously, souravcrl wrote…

Should we consider only sending the client facing error message here as the error can have sensitive ldap server data?

This code was removed and the auth behaviour code was used

---

pkg/server/authserver/authentication.go line 284 at r1 (raw file):

Previously, souravcrl wrote…

Should we wrap this verbosity lvl 1 and also consider logging in the OPS channel, i.e. use log.Ops.VWarningf(ctx, 1, ...) ?

This code was removed and the auth behaviour code was used

---

pkg/server/authserver/authentication.go line 285 at r1 (raw file):

Previously, souravcrl wrote…

Should we consider only sending the client facing error message here as the error can have sensitive ldap server data?

This code was removed and the auth behaviour code was used

---

pkg/server/authserver/authentication.go line 298 at r1 (raw file):

Previously, souravcrl wrote…

Should we wrap this verbosity lvl 1 and also consider logging in the OPS channel, i.e. use log.Ops.VWarningf(ctx, 1, ...) ?

This code was removed and the auth behaviour code was used

---

pkg/server/authserver/authentication.go line 299 at r1 (raw file):

Previously, souravcrl wrote…

Should we consider only sending the client facing error message here as the error can have sensitive sql role information?

Done.

---

pkg/ccl/ldapccl/authentication_ldap_test.go line 276 at r1 (raw file):

Previously, souravcrl wrote…

Should we have tests with username = invalid & password = invalid just as an elementary validation which the mock is configured to reject for bind?

Done added.

this test is now moved to authorization_ldap_test.go.

---

pkg/ccl/ldapccl/authentication_ldap_test.go line 287 at r1 (raw file):

Previously, souravcrl wrote…

Should we perform a revocation with the removal for db_admins for alice?

Done. Added a new t.Run for testing whether it revokes

[shriramters]

pkg/server/authserver/authentication.go line 249 at r1 (raw file):

Previously, shriramters (Shriram Ravindranathan) wrote…

I have refactored the Authorization logic into a function in ldapccl which can be reused for DB Console as well.

this also follows the pattern used by FetchLDAPUserDN, FetchLDAPGroups (detailedError and clientError) but we lose two Info logs.

I tried to get the behaviors.MaybeAuthorize() to work here. However, I found that this pgwire AuthBehaviors object is tied to an interactive connection (AuthConn) and cannot be easily created or used within the one-shot HTTP handler in authserver.

Because the DB Console flow doesn't have an interactive connection to perform the back and forth exchanges, we cannot provide the AuthConn object that the AuthLDAP function requires to create the AuthBehaviors object.

As we discussed, we can't have the sql package as a dependency in ldapccl, I have moved the common LDAP Authorization code into a public method inside the pgwire package and reused it in both the DB Console and pgwire flows.

I believe this is the right approach for now without making any huge architectural changes to either the pgwire or authserver components. This solution eliminates the code duplication while respecting the established package dependency policies and the different connection models of the two systems for now.

[Copilot]

Pull Request Overview

This PR enables LDAP role-based authorization for the DB Console by refactoring LDAP authorization logic into a reusable function. Previously, LDAP authentication for the DB Console only verified passwords but didn't synchronize user roles from LDAP group memberships, creating inconsistent behavior compared to SQL client connections.

Key changes include:

• Extracting LDAP authorization logic into a shared AuthorizeLDAP function in the pgwire package
• Integrating LDAP role synchronization into the DB Console's UserLogin handler
• Adding comprehensive test coverage for DB Console LDAP authorization scenarios

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/sql/pgwire/auth_methods.go	Refactors LDAP authorization into reusable AuthorizeLDAP function
pkg/server/authserver/authentication.go	Integrates LDAP authorization into DB Console login flow and fixes import aliasing
pkg/server/authserver/BUILD.bazel	Adds LDAP library dependency
pkg/ccl/ldapccl/authorization_ldap_test.go	Adds comprehensive test coverage for DB Console LDAP authorization
pkg/ccl/ldapccl/BUILD.bazel	Adds test dependencies for new test cases

[souravcrl]

Reviewed 2 of 8 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @shriramters)

---

-- commits line 26 at r1:

Previously, shriramters (Shriram Ravindranathan) wrote…

Rewrote it. Please let me know if it looks ok..

Looks great now!

---

-- commits line 28 at r2:
nit: dangling period not part of a sentence or bullet point

---

-- commits line 33 at r2:
nit: dangling period not part of a sentence or bullet point

---

pkg/sql/pgwire/auth_methods.go line 1109 at r3 (raw file):

// It returns a generic, client-safe error and a detailed error for internal
// logging.
func AuthorizeLDAP(

nit: Naming the method AuthorizeLDAP could be confusing as we already have a behavior.Authorize and this gets invoked from within the authorizer. It would more appropriately to name this as AuthorizeLDAPUtility or AuthorizeLDAPHelper

---

pkg/server/authserver/authentication.go line 268 at r3 (raw file):

				errForLog = errors.Join(errForLog, errors.Newf("%s", detailedErrors))
			}
			log.Warningf(ctx, "%v", errForLog)

should we consider logging to the OPS channel with verbosity set to 1?

[shriramters]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @souravcrl)

---

-- commits line 26 at r1:

Previously, souravcrl wrote…

Looks great now!

Done.

---

-- commits line 28 at r2:

Previously, souravcrl wrote…

nit: dangling period not part of a sentence or bullet point

Sorry, it was out of habit (period indicating newline)

Removed

---

pkg/server/authserver/authentication.go line 268 at r3 (raw file):

Previously, souravcrl wrote…

should we consider logging to the OPS channel with verbosity set to 1?

Done

---

pkg/sql/pgwire/auth_methods.go line 1109 at r3 (raw file):

Previously, souravcrl wrote…

nit: Naming the method AuthorizeLDAP could be confusing as we already have a behavior.Authorize and this gets invoked from within the authorizer. It would more appropriately to name this as AuthorizeLDAPUtility or AuthorizeLDAPHelper

Done. Changed to AuthorizeLDAPHelper