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.

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

ldap: support partial ldap groups sync/roles grant #133779

Closed
souravcrl opened this issue Oct 30, 2024 · 1 comment · Fixed by #135552
Closed

ldap: support partial ldap groups sync/roles grant #133779

souravcrl opened this issue Oct 30, 2024 · 1 comment · Fixed by #135552
Assignees
Labels
backport-24.3.x Flags PRs that need to be backported to 24.3 branch-master Failures and bugs on the master branch. branch-release-24.3 Used to mark GA and release blockers, technical advisories, and bugs for 24.3 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-product-security

Comments

@souravcrl
Copy link
Contributor

souravcrl commented Oct 30, 2024

Currently an LDAP user may be configured to be a member of multiple LDAP groups, and when we retrieve the group roles for the user during LDAP authZ we try to find corresponding roles for all the groups being synced and fail the roles grant operation if any if the group roles do not exist. This is a problem as not all groups are desired to have corresponding CRDB roles and in such a case a partial roles grant should take place.

link to conversation: https://cockroachlabs.slack.com/archives/C06S9FMQKK9/p1727820768305069

Proposal: support partial roles grant for ldap synced group roles and ensure appropriate erroring for undesired behavior.

Epic CRDB-33829

Jira issue: CRDB-43744

Epic CRDB-33829

@souravcrl souravcrl added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) branch-master Failures and bugs on the master branch. T-product-security backport-24.3.x Flags PRs that need to be backported to 24.3 labels Oct 30, 2024
@souravcrl souravcrl assigned rafiss and souravcrl and unassigned rafiss Oct 30, 2024
souravcrl added a commit to souravcrl/cockroach that referenced this issue Nov 18, 2024
fixes cockroachdb#133779
Epic CRDB-33829

Currently, an LDAP user may be configured to be a member of multiple LDAP
groups, and when we retrieve the group roles for the user during LDAP authZ we
try to find corresponding roles for all the groups being synced and fail the
roles grant operation if any of the group roles do not exist on CRDB. This is a
problem as not all groups are desired to have corresponding CRDB roles and in
such a case a partial roles grant should take place.

Release note(security):  This fix will add support for partial roles from ldap
synced group to be mapped to crdb roles and ensure appropriate erroring for
undesired behavior.
souravcrl added a commit to souravcrl/cockroach that referenced this issue Nov 18, 2024
fixes cockroachdb#133779
Epic CRDB-33829

Currently, an LDAP user may be configured to be a member of multiple LDAP
groups, and when we retrieve the group roles for the user during LDAP authZ we
try to find corresponding roles for all the groups being synced and fail the
roles grant operation if any of the group roles do not exist on CRDB. This is a
problem as not all groups are desired to have corresponding CRDB roles and in
such a case a partial roles grant should take place.

Release note(security):  This fix will add support for partial roles from ldap
synced group to be mapped to crdb roles and ensure appropriate erroring for
undesired behavior.
craig bot pushed a commit that referenced this issue Nov 18, 2024
126587: kvpb: removed disused Path field from ExportResp r=dt a=dt

Release note: none.
Epic: none.

134228: rpc: reduce certificate-related allocations during authentication r=mgartner a=mgartner

#### rpc: add authenication benchmark

Release note: None

#### rpc: reduce certificate-related allocations during authentication

This commit avoids certificate-related allocations in the happy path of
authenticating network requests.

Release note: None

#### security: inline getCertificatePrincipals

The `getCertificatePrincipals` function has been inlined into its one
callsite to avoid allocating a slice of the results.

Fixes #133317

Release note: None

135291: logictest: enable local-mixed-24.3 config r=RaduBerinde a=RaduBerinde

#### bootstrap: add 24.3 bootstrap data

Obtained by running (on release-24.3 branch):
```
./dev build sql-bootstrap-data && bin/sql-bootstrap-data
```

Epic: REL-1322
Release note: None

#### logictest: enable local-mixed-24.3 config

Includes a minor fix to the schema changer (thanks Rafi and Annie!).

Fixes #135358

Epic: REL-1322
Release note: None


135337: cli: skip TestTenantZip under deadlock r=dhartunian a=dhartunian

We had some CPU profile failures under deadlock, skipping under that scenario since that's not relevant for this test.

Resolves: #134187

Release note: None

135552: ldapccl: support partial ldap groups mapping on authz r=souravcrl a=souravcrl

fixes #133779
Epic CRDB-33829

Currently, an LDAP user may be configured to be a member of multiple LDAP groups, and when we retrieve the group roles for the user during LDAP authZ we try to find corresponding roles for all the groups being synced and fail the roles grant operation if any of the group roles do not exist on CRDB. This is a problem as not all groups are desired to have corresponding CRDB roles and in such a case a partial roles grant should take place.

Release note(security):  This fix will add support for partial roles from ldap synced group to be mapped to crdb roles and ensure appropriate erroring for undesired behavior.

135565: gcs: remove test that asserts buckets produce not found error r=jeffswenson a=jeffswenson

The behavior of GCS appears to have changed. If a bucket does not exist,
it will return a 403 forbidden. So we can't disambiguate a bucket that
does not exist from a bucket we can't access.

Overall, this is annoying but fine. The main value of not found as a
sentinel error the program can check is it indicates the name is free
and can be used. This property doesn't apply if the bucket does not
exist because CRDB will never create a bucket.

Release Justification: Test only change
Release Note: None
Fixes: #135307
Fixes: #135348
Fixes: #135349
Fixes: #135350
Fixes: #135351
Fixes: #135352

Co-authored-by: David Taylor <tinystatemachine@gmail.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Co-authored-by: David Hartunian <davidh@cockroachlabs.com>
Co-authored-by: souravcrl <sourav.sarangi@cockroachlabs.com>
Co-authored-by: Jeff Swenson <jeffswenson@betterthannull.com>
@craig craig bot closed this as completed in 35f723e Nov 18, 2024
Copy link

blathers-crl bot commented Nov 18, 2024

Based on the specified backports for linked PR #135552, I applied the following new label(s) to this issue: branch-release-24.3. Please adjust the labels as needed to match the branches actually affected by this issue, including adding any known older branches.

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

@blathers-crl blathers-crl bot added the branch-release-24.3 Used to mark GA and release blockers, technical advisories, and bugs for 24.3 label Nov 18, 2024
blathers-crl bot pushed a commit that referenced this issue Nov 18, 2024
fixes #133779
Epic CRDB-33829

Currently, an LDAP user may be configured to be a member of multiple LDAP
groups, and when we retrieve the group roles for the user during LDAP authZ we
try to find corresponding roles for all the groups being synced and fail the
roles grant operation if any of the group roles do not exist on CRDB. This is a
problem as not all groups are desired to have corresponding CRDB roles and in
such a case a partial roles grant should take place.

Release note(security):  This fix will add support for partial roles from ldap
synced group to be mapped to crdb roles and ensure appropriate erroring for
undesired behavior.
craig bot pushed a commit that referenced this issue Nov 19, 2024
135634: sql: fix statement generation when ensuring roles r=souravcrl a=rafiss

Since we started skipping over non-existent roles in 35f723e, we need to make sure we only add commas at the appropriate point.

The test update demonstrates that there was a bug before this patch, as it would fail with:
```
expected:
ok defaultdb

found:
ERROR: LDAP authorization: error assigning roles to user ldap_user: EnsureUserOnlyBelongsToRoles-grant: at or near ",": syntax error (SQLSTATE 42601)
HINT: try \h GRANT
DETAIL: source SQL:
GRANT , "ldap-parent-synced" TO ldap_user
```

informs #133779
Release note: None

Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-24.3.x Flags PRs that need to be backported to 24.3 branch-master Failures and bugs on the master branch. branch-release-24.3 Used to mark GA and release blockers, technical advisories, and bugs for 24.3 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-product-security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants