[SRU] PooledLDAPHandler.result3 does not release pool connection back when an exception is raised

[Mustafa Kemal Gilor]

[Impact]

This SRU is a backport of https://review.opendev.org/c/openstack/keystone/+/866723 to the respective Ubuntu and UCA releases. The patch is merged to the all respective upstream branches (master & stable/[u,v,w,x,y,z]).

This SRU intends to fix a denial-of-service bug that happens when keystone uses pooled ldap connections. In pooled ldap connection mode, keystone borrows a connection from the pool, do the LDAP operation and release it back to the pool. But, if an exception or error happens while the LDAP connection is still borrowed, Keystone fails to release the connection back to the pool, hogging it forever. If this happens for all the pooled connections, the connection pool will be exhausted and Keystone will no longer be able to perform LDAP operations.

The fix corrects this behavior by allowing the connection to release back to the pool even if an exception/error happens during the LDAP operation.

[Test Case]

- Deploy an LDAP server of your choice
- Fill it with many data so the search takes more than `pool_connection_timeout` seconds
- Define a keystone domain with the LDAP driver with following options:

[ldap]
use_pool = True
page_size = 100
pool_connection_timeout = 3
pool_retry_max = 3
pool_size = 10

- Point the domain to the LDAP server
- Try to login to the OpenStack dashboard, or try to do anything that uses the LDAP user
- Observe the /var/log/apache2/keystone_error.log, it should contain ldap.TIMEOUT() stack traces followed by `ldappool.MaxConnectionReachedError` stack traces

To confirm the fix, repeat the scenario and observe that the "/var/log/apache2/keystone_error.log" does not contain `ldappool.MaxConnectionReachedError` stack traces and LDAP operation in motion is successful (e.g. OpenStack Dashboard login)

[Regression Potential]
The patch is quite trivial and should not affect any deployment in a negative way. The LDAP pool functionality can be disabled by setting "use_pool=False" in case of any regression.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/keystone/+/866723

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/keystone/+/866723
Committed: https://opendev.org/openstack/keystone/commit/ff632a81fb09e6d9f3298e494d53eb6df50269cf
Submitter: "Zuul (22348)"
Branch: master

commit ff632a81fb09e6d9f3298e494d53eb6df50269cf
Author: Mustafa Kemal Gilor <email address hidden>
Date: Mon Dec 5 17:33:47 2022 +0300

    [PooledLDAPHandler] Ensure result3() invokes message.clean()

    result3 does not invoke message.clean() when an exception is thrown
    by `message.connection.result3()` call, causing pool connection
    associated with the message to be marked active forever. This causes
    a denial-of-service on ldappool.

    The fix ensures message.clean() is invoked by wrapping the offending
    call in try-except-finally and putting the message.clean() in finally
    block.

    Closes-Bug: #1998789

    Change-Id: I59ebf0fa77391d49b2349e918fc55f96318c42a6
    Signed-off-by: Mustafa Kemal Gilor <email address hidden>

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/zed
Review: https://review.opendev.org/c/openstack/keystone/+/874841

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/yoga
Review: https://review.opendev.org/c/openstack/keystone/+/874842

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/xena
Review: https://review.opendev.org/c/openstack/keystone/+/874843

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/wallaby
Review: https://review.opendev.org/c/openstack/keystone/+/874844

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/train
Review: https://review.opendev.org/c/openstack/keystone/+/874845

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/ussuri
Review: https://review.opendev.org/c/openstack/keystone/+/874846

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/victoria
Review: https://review.opendev.org/c/openstack/keystone/+/874847

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/keystone 23.0.0.0rc1 release candidate.

[OpenStack Infra (hudson-openstack)]

Change abandoned by "Mustafa Kemal Gilor <email address hidden>" on branch: stable/train
Review: https://review.opendev.org/c/openstack/keystone/+/874845

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/keystone/+/874841
Committed: https://opendev.org/openstack/keystone/commit/7c30c9e000b58055b61d8cf58e52493f2b5aba8a
Submitter: "Zuul (22348)"
Branch: stable/zed

commit 7c30c9e000b58055b61d8cf58e52493f2b5aba8a
Author: Mustafa Kemal Gilor <email address hidden>
Date: Mon Dec 5 17:33:47 2022 +0300

    [PooledLDAPHandler] Ensure result3() invokes message.clean()

    result3 does not invoke message.clean() when an exception is thrown
    by `message.connection.result3()` call, causing pool connection
    associated with the message to be marked active forever. This causes
    a denial-of-service on ldappool.

    The fix ensures message.clean() is invoked by wrapping the offending
    call in try-except-finally and putting the message.clean() in finally
    block.

    Closes-Bug: #1998789

    Change-Id: I59ebf0fa77391d49b2349e918fc55f96318c42a6
    Signed-off-by: Mustafa Kemal Gilor <email address hidden>
    (cherry picked from commit ff632a81fb09e6d9f3298e494d53eb6df50269cf)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/keystone/+/874842
Committed: https://opendev.org/openstack/keystone/commit/7c96280555d1de5ef5e7e3b12362439669427e4e
Submitter: "Zuul (22348)"
Branch: stable/yoga

commit 7c96280555d1de5ef5e7e3b12362439669427e4e
Author: Mustafa Kemal Gilor <email address hidden>
Date: Mon Dec 5 17:33:47 2022 +0300

    [PooledLDAPHandler] Ensure result3() invokes message.clean()

    result3 does not invoke message.clean() when an exception is thrown
    by `message.connection.result3()` call, causing pool connection
    associated with the message to be marked active forever. This causes
    a denial-of-service on ldappool.

    The fix ensures message.clean() is invoked by wrapping the offending
    call in try-except-finally and putting the message.clean() in finally
    block.

    Closes-Bug: #1998789

    Change-Id: I59ebf0fa77391d49b2349e918fc55f96318c42a6
    Signed-off-by: Mustafa Kemal Gilor <email address hidden>
    (cherry picked from commit ff632a81fb09e6d9f3298e494d53eb6df50269cf)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/keystone/+/874843
Committed: https://opendev.org/openstack/keystone/commit/3e5805e40e26b51ce5c4a69aa31d81226d38ccf9
Submitter: "Zuul (22348)"
Branch: stable/xena

commit 3e5805e40e26b51ce5c4a69aa31d81226d38ccf9
Author: Mustafa Kemal Gilor <email address hidden>
Date: Mon Dec 5 17:33:47 2022 +0300

    [PooledLDAPHandler] Ensure result3() invokes message.clean()

    result3 does not invoke message.clean() when an exception is thrown
    by `message.connection.result3()` call, causing pool connection
    associated with the message to be marked active forever. This causes
    a denial-of-service on ldappool.

    The fix ensures message.clean() is invoked by wrapping the offending
    call in try-except-finally and putting the message.clean() in finally
    block.

    Closes-Bug: #1998789

    Change-Id: I59ebf0fa77391d49b2349e918fc55f96318c42a6
    Signed-off-by: Mustafa Kemal Gilor <email address hidden>
    (cherry picked from commit ff632a81fb09e6d9f3298e494d53eb6df50269cf)

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "focal.debdiff" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/keystone 21.0.1 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/keystone 22.0.1 release.

[Corey Bryant (corey.bryant)]

Thanks Mustafa. For the SRU, this will be included for victoria+ in https://bugs.launchpad.net/ubuntu/+source/keystone/+bug/2039176. Let's target this bug for focal/ussuri.

[Corey Bryant (corey.bryant)]

Mustafa, this has been uploaded to the focal unapproved queue: https://launchpad.net/ubuntu/focal/+queue?queue_state=1&queue_text=keystone

[Lukas Märdian (slyon)]

Removing ~ubuntu-sponsors, as this got sponsored already for Focal and is included in bug #2039176 for the other series.

[Edward Hope-Morley (hopem)]

this is now fix released down to victoria as per [1]

[1] https://bugs.launchpad.net/ubuntu/+source/keystone/+bug/2039176

[Andreas Hasenack (ahasenack)]

> "This SRU intends to fix a denial-of-service bug"

This sounds like it should go into the focal-security pocket, as well as updates, no?

Subscribing security for their evaluation.

[Andreas Hasenack (ahasenack)]

Regarding the test plan, wouldn't an iptables DROP rule to the ldap server also trigger an exception, and the bug? If yes, it's probably much easier and more reliable to trigger than a search timeout.

[Andreas Hasenack (ahasenack)]

The rest looks good, I think this is ready to accept, just pending the security question from comment #30

[Mustafa Kemal Gilor (mustafakemalgilor)]

Hi Andreas,

Thanks for the comments!

"This sounds like it should go into the focal-security pocket, as well as updates, no?"

IMO, because the consumers of the LDAP can be external-facing (e.g. Horizon, a.k.a OpenStack Dashboard), it can be considered a security bug. Still, I'll leave the real assessments to the security experts.

"Regarding the test plan, wouldn't an iptables DROP rule to the LDAP server also trigger an exception, and the bug?"

Yes, an iptables DROP rule should do the trick. Probably need to put it into place after the connection is established, though.

[Andreas Hasenack (ahasenack)]

This SRU is still blocked pending a decision from the Security Team whether it should go into the security or update pockets.

[Marc Deslauriers (mdeslaur)]

This looks like a low-priority security issue, please handle it as a SRU for now, and it will get put in the security pocket once we fix more important security issues in the future.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/keystone ussuri-eol release.

[Robie Basak (racb)]

Hello Mustafa, or anyone else affected,

Accepted keystone into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/keystone/2:17.0.1-0ubuntu2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Mustafa Kemal Gilor (mustafakemalgilor)]

Verification is done for Focal. Tested the -proposed package version 17.0.1-0ubuntu2 on Focal with the test procedure, the problem has now disappeared.

[Launchpad Janitor (janitor)]

This bug was fixed in the package keystone - 2:17.0.1-0ubuntu2

---------------
keystone (2:17.0.1-0ubuntu2) focal; urgency=medium

  * d/p/lp1998789-pooledldap-conn-release-on-exception.patch: Release
    pooled ldap connection back to pool when an exception is thrown
    by ldap methods (LP: #1998789).

 -- Mustafa Kemal GILOR <email address hidden> Wed, 04 Oct 2023 12:29:32 +0300

[Chris Halse Rogers (raof)]

The verification of the Stable Release Update for keystone has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[James Page (james-page)]

Hello Mustafa, or anyone else affected,

Accepted keystone into ussuri-proposed. The package will build now and be available in the Ubuntu Cloud Archive in a few hours, and then in the -proposed repository.

Please help us by testing this new package. To enable the -proposed repository:

  sudo add-apt-repository cloud-archive:ussuri-proposed
  sudo apt-get update

Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-ussuri-needed to verification-ussuri-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-ussuri-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Mustafa Kemal Gilor (mustafakemalgilor)]

Verification is done for Ussuri. Tested the -proposed package with the test plan and confirmed that the problem has disappeared.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/keystone victoria-eom release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/keystone wallaby-eom release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/keystone xena-eom release.