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

A dead LDAP upstream can block the authentication and show global variables. #51883

Closed
YangKeao opened this issue Mar 19, 2024 · 2 comments · Fixed by #51927 · May be fixed by #51912
Closed

A dead LDAP upstream can block the authentication and show global variables. #51883

YangKeao opened this issue Mar 19, 2024 · 2 comments · Fixed by #51927 · May be fixed by #51912
Assignees
Labels
affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. sig/sql-infra SIG: SQL Infra type/enhancement The issue or PR belongs to an enhancement.

Comments

@YangKeao
Copy link
Member

YangKeao commented Mar 19, 2024

Enhancement

Some functions about LDAP don't have timeout mechanism (e.g. StartTLS), therefore, the RLock of it cannot be released until the upstream returns.

I'd like to address the following enhancement:

  1. Don't include any network / IO in the RLock. An RLock in golang can block the Lock, and a pending write lock will blocks all other RLock, which will make the things much worse.
  2. Add timeout mechanism for LDAP functions.
  3. Add metrics for them.
@YangKeao YangKeao added the type/enhancement The issue or PR belongs to an enhancement. label Mar 19, 2024
@YangKeao YangKeao self-assigned this Mar 19, 2024
@aki263
Copy link

aki263 commented Mar 19, 2024

@YangKeao
We should also add caching creds for upto a minutes so we dont DDoS LDAP sever.

@YangKeao
Copy link
Member Author

@aki263 Good suggestion.

TiDB has a mechanism to retry to establish connection with the LDAP server (for 10 times), when it fails. I'll add some backoff to avoid retrying too frequently. I think it'll help a lot to limit the request frequency.

Cache on authentication needs more consideration/discussion, because the latency of cache invalidation is not always expected (especially when it's related to security) 🤔 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. sig/sql-infra SIG: SQL Infra type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
3 participants