Skip to content

feat(ldap): [OCISDEV-1031] add opt-in LDAP connection pool - #12660

Open
LukasHirt wants to merge 3 commits into
stable-8.0from
feat/ocisdev-1031-ldap-pool
Open

feat(ldap): [OCISDEV-1031] add opt-in LDAP connection pool#12660
LukasHirt wants to merge 3 commits into
stable-8.0from
feat/ocisdev-1031-ldap-pool

Conversation

@LukasHirt

@LukasHirt LukasHirt commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Description

Bumps the vendored reva dependency to pick up the opt-in bounded LDAP connection pool (owncloud/reva#658, owncloud/reva#665), and wires it into oCIS:

  • Adds pool_enabled/pool_size/pool_checkout_timeout config to auth-basic, users, groups and graph.
  • Forwards the new config into the reva config map for auth-basic, users and groups.
  • Unifies the graph service's identity backend onto reva's LDAP client (utils.GetLDAPClientFromConfig) instead of its own hand-rolled reconnecting client, now that reva implements PasswordModify and ModifyWithResult on both LDAP clients (feat: [OCISDEV-1031] implement PasswordModify and ModifyWithResult on LDAP clients reva#665) — removing the second, now-redundant implementation.
  • Adds a dedicated ldap-pool-tests CI job that reruns the apiGraphUser/apiGraph/apiGraphGroup acceptance suites with OCIS_LDAP_POOL_ENABLED=true, in addition to their existing pooling-disabled runs.
  • Bumps github.com/go-chi/chi/v5 and go.opentelemetry.io/otel to fix newly-disclosed, unrelated govulncheck findings on stable-8.0's baseline that were blocking CI on this PR.

Pooling is off by default and fully backwards compatible: with it disabled, behavior is unchanged from the existing single-connection ConnWithReconnect path.

Related Issue

Motivation and Context

OCISDEV-1031 is Story 1 of the OCISDEV-1030 epic (LDAP client hardening for the consolidated central-idm architecture, ~600 connections across 200 clusters). The goal is to replace the single long-lived LDAP connection per backend with a bounded pool, so concurrent requests no longer serialize on one socket.

How Has This Been Tested?

  • test environment: local oCIS build (stable-8.0) against the embedded LDAP idm, with OCIS_LDAP_POOL_ENABLED=true and GRAPH_LDAP_SERVER_USE_PASSWORD_MODIFY_EXOP=true set; plus CI's new ldap-pool-tests job (apiGraphUser, apiGraph, apiGraphGroup with OCIS_LDAP_POOL_ENABLED=true).
  • test case 1: basic auth login and /graph/v1.0/me — succeeds with pooling enabled.
  • test case 2: list users and list groups via the graph API — succeed with pooling enabled.
  • test case 3: password change via PATCH /graph/v1.0/users/{id} with passwordProfile (exercises PasswordModify through reva's client) — old password rejected afterwards, new password authenticates successfully.
  • test case 4: 20 concurrent authenticated requests against the pool — all succeed, no errors logged.
  • test case 5: go build ./..., go vet ./..., and existing unit tests for auth-basic/users/groups/graph all pass unmodified (pooling disabled, default behavior confirmed unchanged).
  • test case 6: acceptance suites apiGraphUser, apiGraph, apiGraphGroup re-run with OCIS_LDAP_POOL_ENABLED=true in CI (ldap-pool-tests job), covering login, user/group CRUD and password modify against the pooled LDAP client end-to-end.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Bump the vendored reva dependency to pick up the opt-in bounded LDAP
connection pool (owncloud/reva#658, #665), and wire it into oCIS: add
pool_enabled/pool_size/pool_checkout_timeout config to auth-basic,
users, groups and graph, forward it into the reva config map for the
first three, and unify graph's identity backend onto reva's LDAP
client (utils.GetLDAPClientFromConfig) instead of its own hand-rolled
reconnecting client, now that reva implements PasswordModify and
ModifyWithResult on both LDAP clients. Pooling is off by default and
fully backwards compatible.

Signed-off-by: Lukas Hirt <info@hirt.cz>
@kw-security

kw-security commented Jul 27, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

… in CI

Add a dedicated ldap-pool-tests job to acceptance-tests.yml that reruns
apiGraphUser/apiGraph/apiGraphGroup with OCIS_LDAP_POOL_ENABLED=true, so
concurrent behat requests exercise checkout/release, eviction and the
pooled PasswordModify/ModifyWithResult path end-to-end, in addition to
the existing pooling-disabled runs of the same suites.

Signed-off-by: Lukas Hirt <info@hirt.cz>
@LukasHirt LukasHirt self-assigned this Jul 27, 2026
@2403905

2403905 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Let's separate the dependency bump and pull request.

@2403905

2403905 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Suggestions

1. Graph service loses the explicit TLS MinVersion: tls.VersionTLS12 floor (minor security regression). The old graph code built its tls.Config with MinVersion: tls.VersionTLS12 for both the insecure and CA paths. The new path delegates to reva's tlsConfigFromLDAPConn, which sets no MinVersion — the insecure branch sets only InsecureSkipVerify: true, and the CA branch sets only RootCAs. So with pooling either on or off, the graph LDAP client now negotiates down to whatever Go's default minimum is. This is a behavior change introduced by this PR for the graph service specifically. Worth confirming it's acceptable (auth-basic/users/groups already used reva's helper, so they're unchanged — only graph regresses here).

2. pool_size / pool_checkout_timeout have no defaults in oCIS DefaultConfig(). Only PoolEnabled: false is set in the four defaultconfig.go files. PoolSize/PoolCheckoutTimeout are left as Go zero values (0 / 0s), relying on reva's NewLDAPPool to substitute 5 / 30s when <= 0. That works, but it means ocis init / the generated yaml will show pool_size: 0, which reads as "zero connections" to an operator rather than "default 5". Consider setting the real defaults (5, 30s) in DefaultConfig() so the documented defaults match what's emitted, matching the pattern used for other tunables.

3. CA-cert error path in graph is now unreachable dead code. In the refactored setIdentityBackends, the if options.Config.Identity.LDAP.CACert != "" block now only calls WaitForCA — the actual cert reading/parsing moved into reva's helper. That's fine, but note WaitForCA returns nil even when the cert never appears (it only warns), so a genuinely missing CA now surfaces later as a dial error from the first checkout rather than at startup. Behavior is arguably acceptable but slightly less eager than before. No action required; just flagging.

@LukasHirt

Copy link
Copy Markdown
Contributor Author

Split the dependency bump into its own PR as requested: #12662 (cc @2403905). Rebased this branch to drop that commit.

…faults

Only PoolEnabled: false was set in the four defaultconfig.go files;
PoolSize/PoolCheckoutTimeout were left as Go zero values (0/0s),
relying on reva's NewLDAPPool substituting 5/30s when <= 0. That
reads as "zero connections" to an operator in the generated yaml
config rather than "default 5". Set the real defaults (5, 30s)
so documented defaults match what's emitted.

Signed-off-by: Lukas Hirt <info@hirt.cz>
@LukasHirt

Copy link
Copy Markdown
Contributor Author

Addressed:

  1. TLS MinVersion regression — this lives in vendored reva's tlsConfigFromLDAPConn (shared by all three GetLDAPClient* constructors), not in oCIS code, so it affects auth-basic/users/groups too, not just graph, once fixed. Opened fix(ldap): restore TLS 1.2 floor for LDAP connections reva#672 to restore MinVersion: tls.VersionTLS12 on both the insecure and CA-cert paths upstream (cc @2403905 for review there). Will bump the vendored reva dep here once that merges.
  2. pool_size/pool_checkout_timeout defaults — fixed in 7c5773b: set the real defaults (5, 30s) in all four DefaultConfig() functions so the generated yaml no longer shows pool_size: 0.
  3. CA-cert error path — agreed no action needed, per your own note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants