-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix LDAP network timeout not being honored on Linux #119210
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: ericstj <[email protected]>
Co-authored-by: ericstj <[email protected]>
Co-authored-by: ericstj <[email protected]>
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
@@ -307,6 +307,39 @@ public void Dispose_MultipleTimes_Nop() | |||
connection.Dispose(); | |||
} | |||
|
|||
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsLinux))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look to me like this needs to be platform specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed platform-specific conditional. The test now works across platforms. Fixed in commit 75f43a4.
connection.Dispose(); | ||
} | ||
|
||
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsLinux))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again this doesn't need to be platform specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed platform-specific conditional. The test is no longer Linux-only. Fixed in commit 75f43a4.
}; | ||
|
||
// Verify the timeout is set correctly | ||
Assert.Equal(TimeSpan.Zero, connection.Timeout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add another test that will cause the timeout to fail the connection? We should make sure that we fail consistently across platforms. Maybe we can use an endpoint that we know won't respond in time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added NetworkTimeout_UnreachableServer_ThrowsTimeoutException test that uses 192.0.2.1 (TEST-NET-1) to cause actual timeout failure and verifies consistent exception handling across platforms. Fixed in commit 75f43a4.
src/libraries/System.DirectoryServices.Protocols/tests/LdapConnectionTests.cs
Outdated
Show resolved
Hide resolved
}; | ||
|
||
// Verify the timeout is set correctly | ||
Assert.Equal(TimeSpan.FromSeconds(10), connection.Timeout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this does is validate that the property getter returns what was set. It would be better to actually check that it was used when establishing the connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced simple property getter/setter tests with NetworkTimeout_UnreachableServer_ThrowsTimeoutException that actually tests timeout behavior during connection attempts. Fixed in commit 75f43a4.
{ | ||
// For debugging: we can add logging here in the future | ||
// The timeout setting failed, but we don't fail the connection | ||
// as network timeout might not be supported on all OpenLDAP versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can you be sure that your change is actually working? Do we need to ignore this error? I see for most other options we use CheckAndSetLdapError.
This comment mentions that timeout might not be supported - what evidence is that concern based on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use ErrorChecking.CheckAndSetLdapError(timeoutResult) instead of ignoring errors, following the same pattern as other LDAP options. The timeout setting should work on modern OpenLDAP versions. Fixed in commit 75f43a4.
@copilot can you address the feedback? |
Co-authored-by: ericstj <[email protected]>
Co-authored-by: ericstj <[email protected]>
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Fixes #62075
Problem
On Linux,
System.DirectoryServices.Protocols.LdapConnection
does not honor theTimeout
property at the network layer, causing network operations (such as connect or bind) to potentially hang indefinitely or much longer than the requested timeout. This occurs because the native LDAP handle does not haveLDAP_OPT_NETWORK_TIMEOUT
set, while on Windows the timeout is properly enforced at the network layer.Solution
This PR implements proper network-level timeout support for Linux by setting
LDAP_OPT_NETWORK_TIMEOUT
vialdap_set_option
on the LDAP handle after initialization and before any bind or search operations.Key Changes
LDAP_OPT_NETWORK_TIMEOUT
constant (0x5005) to theLdapOption
enumldap_set_option_timeval
InternalConnectToServer
inLdapConnection.Linux.cs
to set network timeout using theTimeout
property valueCheckAndSetLdapError
for timeout option settingImplementation Details
The timeout is set in
InternalConnectToServer
after successful URI configuration:The implementation uses standard error handling patterns consistent with other LDAP options in the codebase.
Testing
Impact
This change brings Linux behavior in line with Windows and makes the
Timeout
property work as documented across platforms. While this could change behavior for callers whose connections previously took longer than the default timeout (30s) without overriding it, this is a desirable bug fix that improves reliability and cross-platform consistency.Fixes scenarios where LDAP connections would hang indefinitely on Linux when network connectivity issues occurred, particularly important for applications that rely on predictable timeout behavior.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.