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

[API Proposal]: LdapSessionOptions.CertificateDirectory Property #104260

Closed
onmp opened this issue Jul 1, 2024 · 11 comments · Fixed by #111877
Closed

[API Proposal]: LdapSessionOptions.CertificateDirectory Property #104260

onmp opened this issue Jul 1, 2024 · 11 comments · Fixed by #111877
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.DirectoryServices in-pr There is an active PR which will close this issue when it is merged os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX
Milestone

Comments

@onmp
Copy link

onmp commented Jul 1, 2024

Background and motivation

This is to provide a way for those who may not have the privilege to administer the system CA certificates store or those who do not want to add entries to the system CA stores permanently but still want to verify the server certificate.

The new APIs are only for Linux and MacOS since the LdapSessionOptions.VerifyServerCertificates property is not supported there.

API Proposal

Adds to https://learn.microsoft.com/en-us/dotnet/api/system.directoryservices.protocols.ldapsessionoptions

Assembly: System.DirectoryServices.Protocols.dll

namespace System.DirectoryServices.Protocols;
public class LdapSessionOptions
{
+  // Throws `PlatformNotSupportedException` on non-Windows.
+  public string CertificateDirectory { get; set; }
+  public void StartNewTlsSessionContext();
}

Internally the CertificateDirectory setter calls

LdapPal.SetStringOption(_connection._ldapHandle, LdapOption.LDAPTLS_CACERTDIR \ 0x6003, value);

and simililarily StartNewTLSSessionContext sets

LDAP_OPT_X_TLS_NEWCTX

API Usage

connection.SessionOptions.CertificateDirectory = "~/console/mycerts";
// This is necessary if the connection's context was already created.
connection.SessionOptions.StartNewTLSSessionContext();

Longer example:

// This is an alternative for specifying the directory.
// Environment.SetEnvironmentVariable("LDAPTLS_CACERTDIR", Path.Combine(AppContext.BaseDirectory, "certs"));

LdapDirectoryIdentifier directoryIdentifier = new LdapDirectoryIdentifier("ldap.local", 1636, fullyQualifiedDnsHostName: true, connectionless: false);
NetworkCredential credential = new NetworkCredential("cn=admin,dc=example,dc=org", "password");
using LdapConnection connection = new LdapConnection(directoryIdentifier, credential)
{
    AuthType = AuthType.Basic
};
connection.SessionOptions.ProtocolVersion = 3;
connection.SessionOptions.SecureSocketLayer = true;
Debug.Assert(OperatingSystem.IsLinux());
connection.SessionOptions.CertificateDirectory = "~/console/mycerts";
connection.SessionOptions.StartNewTLSSessionContext();
connection.Bind();
var searchRequest = new SearchRequest("DC=example,DC=org", "(objectClass=*)", SearchScope.Subtree);

_ = (SearchResponse)connection.SendRequest(searchRequest);
Console.WriteLine("Success!");

Backup of original proposal

API Proposal

Namespace:
System.DirectoryServices.Protocols

Assembly:
System.DirectoryServices.Protocols.dll

The property CaCertificate contains a X509CertificateCollection object with one or more CA certificates to use to verify server certificates when an SSL connection is established.

public System.Security.Cryptography.X509Certificates.X509CertificateCollection CaCertificates { set; }

Property value
CaCertficates

CA certificates to verify server certificate.

API Usage

LdapDirectoryIdentifier identifier = new LdapDirectoryIdentifier("192.168.10.100", 3060, false, false);
using (LdapConnection ldapConnection = new LdapConnection(identifier))
            {
                ldapConnection.SessionOptions.ProtocolVersion = 3;
                ldapConnection.AuthType = AuthType.Basic; 
		ldapConnection.Credential = new NetworkCredential("admin", "SomePassword");
		ldapConnection.ClientCertificates.AddRange(myCert);
		ldapConnection.SessionOptions.CaCertificates.AddRange(CaCerts);
		ldapConnection.Bind();
	    }

Alternative Designs

Have LdapSessionOptions.VerifyServerCertificates be functional.

Risks

The risk is minimal because no current application is using this property.

@onmp onmp added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 1, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 1, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-directoryservices, @jay98014
See info in area-owners.md if you want to be subscribed.

@filipnavara
Copy link
Member

I think this is misguided. There's an existing API for certificate validation (LdapSessionOptions.VerifyServerCertificates) that follows established pattern used by other classes like SslStream or HttpClientHandler. The reason it's not implemented on non-Windows system is the lack or corresponding API in the native system LDAP library (*). The API proposed above would still suffer from the same underlying issue which is lack of certificate control at the lower level system API.

(*) See #60972 for details.

@steveharter steveharter added this to the Future milestone Jul 3, 2024
@steveharter steveharter added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Jul 3, 2024
@steveharter
Copy link
Member

Per offline discussion with @buyaa-n, moving to future. Please close if there is a valid alternative.

@buyaa-n
Copy link
Contributor

buyaa-n commented Jul 5, 2024

This is to provide a way for those who may not have the privilege to administer the system CA certificates store or those who do not want to add entries to the system CA stores permanently be able to verify the server certificate.
This also addresses specifically that LdapSessionOptions.VerifyServerCertificates property is not supported on Linux and MAC OS for .NET CORE.

@onmp could you collaborate more on how you imagine this SessionOption.CaCertificates would work further to replace LdapSessionOptions.VerifyServerCertificates on Linux?

@buyaa-n buyaa-n added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Jul 5, 2024
Copy link
Contributor

This issue has been marked needs-author-action and may be missing some important information.

@buyaa-n
Copy link
Contributor

buyaa-n commented Jul 8, 2024

@onmp as a work around, could you set the env variable LDAPTLS_CACERTDIR with the path where your CA certificates are located and run your client app? Please check https://www.openldap.org/doc/admin24/tls.html#TLS%20Configuration for more info about the directory requirements (like might need to run c_rehash utility).

Could try with a ldapsearch commant: env LDAPTLS_CACERTDIR=/path/to/CACerts ldapsearch -H ldaps://ldap.local:1636 -b dc=example,dc=org -x -D cn=admin,dc=example,dc=org -w password

Or run export LDAPTLS_CACERTDIR="/path/to/CACerts" before running you app.

Copy link
Contributor

This issue has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

Copy link
Contributor

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.

@dotnet-policy-service dotnet-policy-service bot removed this from the Future milestone Aug 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2024
@steveharter steveharter reopened this Dec 30, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Dec 30, 2024
@steveharter steveharter self-assigned this Dec 30, 2024
@steveharter steveharter added this to the 10.0.0 milestone Dec 30, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Dec 30, 2024
@steveharter
Copy link
Member

steveharter commented Dec 30, 2024

Re-opened. We have external validation that that adding

namespace System.DirectoryServices.Protocols;
public class LdapSessionOptions
{
+  public string CertificateDirectory { get; set; }
+  public void StartNewTlsSessionContext();
}

addresses this. Internally the CertificateDirectory setter calls

LdapPal.SetStringOption(_connection._ldapHandle, LdapOption.LDAPTLS_CACERTDIR \ 0x6003, value);

This will throw PlatformNotSupportedException on non-Windows.

Usage along the lines of:

Environment.SetEnvironmentVariable("LDAPTLS_CACERTDIR", Path.Combine(AppContext.BaseDirectory, "certs"));
LdapDirectoryIdentifier directoryIdentifier = new LdapDirectoryIdentifier("ldap.local", 1636, fullyQualifiedDnsHostName: true, connectionless: false);
NetworkCredential credential = new NetworkCredential("cn=admin,dc=example,dc=org", "password");
using LdapConnection connection = new LdapConnection(directoryIdentifier, credential)
{
    AuthType = AuthType.Basic
};
connection.SessionOptions.ProtocolVersion = 3;
connection.SessionOptions.SecureSocketLayer = true;
Debug.Assert(OperatingSystem.IsLinux());
connection.SessionOptions.CertificateDirectory = "~/console/mycerts";
connection.SessionOptions.StartNewTLSSessionContext();
connection.Bind();
var searchRequest = new SearchRequest("DC=example,DC=org", "(objectClass=*)", SearchScope.Subtree);

_ = (SearchResponse)connection.SendRequest(searchRequest);
Console.WriteLine("Success!");

@steveharter
Copy link
Member

I will update the original API suggestion and mark it ready for reviewing pending internal discussion.

@steveharter steveharter added the os-linux Linux OS (any supported distro) label Jan 16, 2025
@steveharter steveharter added the os-mac-os-x macOS aka OSX label Jan 16, 2025
@steveharter steveharter added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed needs-author-action An issue or pull request that requires more info or actions from the author. api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 16, 2025
@steveharter steveharter changed the title [API Proposal]: LdapSessionOptions.CaCertificates Property [API Proposal]: LdapSessionOptions.CertificateDirectory Property Jan 17, 2025
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Jan 28, 2025
@bartonjs
Copy link
Member

bartonjs commented Jan 28, 2025

Video

  • The certificate directory property should reflect that it's a directory for defining trust, not just "where to look for certificates"
    • CertificateDirectory => TrustedCertificatesDirectory
  • The approval state includes the suggestion of UnsupportedOSAttributes, but if that's not consistent with the LDAP types, feel free to omit them.
namespace System.DirectoryServices.Protocols;

public partial class LdapSessionOptions
{
    [UnsupportedOS("windows")]
    [UnsupportedOS("android")]
    [UnsupportedOS("browser")]
    [UnsupportedOS("ios")]
    public string? TrustedCertificatesDirectory { get; set; }

    [UnsupportedOS("windows")]
    [UnsupportedOS("android")]
    [UnsupportedOS("browser")]
    [UnsupportedOS("ios")]
    public void StartNewTlsSessionContext();
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.DirectoryServices in-pr There is an active PR which will close this issue when it is merged os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants