Avoid exception when parsing AD path for port number #109977
Conversation
|
@steveharter no we do not have any test around this code. |
There was a problem hiding this comment.
Since the original PR regressed I wanted to give a bit more scrutiny. @kumarravik78c or @jay98014 Can you please have a look at my comments and let me know what you think? We need you expertise here. Thanks!
|
|
||
| string dnsDomainName = ""; | ||
|
|
||
| using (DirectoryEntry rootDse = new DirectoryEntry("LDAP://" + this.dnsHostName + ":" + port + "/rootDse", "", "", AuthenticationTypes.Anonymous)) | ||
| using (DirectoryEntry rootDse = new DirectoryEntry($"LDAP://{this.dnsHostName}:{port}/rootDse", "", "", AuthenticationTypes.Anonymous)) |
There was a problem hiding this comment.
I know it wasn't a reported problem, but double checking we are OK to add the default port values here when not specified by the user? Was that an important part of the original fix?
There was a problem hiding this comment.
Since this code only interacts with a dc it is safe to use default ports, ie., 389 for regular LDAP and 636 for secure version of it.
| { | ||
| port = ldapUri.Port; | ||
| } | ||
| else if (string.Equals(ldapUri.Scheme, "LDAPS", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Another issue from the original PR - it seems odd to me that we ignore this scheme and instead use LDAP below with a port. Do you know if that's intentional?
There was a problem hiding this comment.
i.e. should we use "LDAPS://" below when ldapUri.Scheme == "LDAPS"? Would the connection bel secure \ encrypted just because it's on the LDAPS port even though "LDAP://" is used?
There was a problem hiding this comment.
It has been like this from day one. After digging a little deeper and talking with my team, I found out that LDAPS is not valid provider name supported. Hence it uses LDAP as the supported provider name. This allows the user to call the api with LDAPS and the api in turn uses the ssl port(636) along with the supported provider name which LDAP. This is a less strict way to call the api and allow ssl port. Other way would not allow LDAPS at all which might break a lot of applications.
|
PTAL @JasonDebug |
Fixes #109858
This will need to be ported to v9
We don't have automated tests for this; can @kumarravik78c or @jay98014 verify.
Regression in v9 from #89787