feat(ldapsearch):add custom PKI/internal CA support in TLS connections#1954
Open
CharlesLR-sekoia wants to merge 3 commits intodevelopfrom
Open
feat(ldapsearch):add custom PKI/internal CA support in TLS connections#1954CharlesLR-sekoia wants to merge 3 commits intodevelopfrom
CharlesLR-sekoia wants to merge 3 commits intodevelopfrom
Conversation
Contributor
Reviewer's GuideImplements TLS client configuration for Microsoft Active Directory actions with optional custom CA support, adds configuration/plumbing for the new ca_certificate option, and covers the behavior with new tests and documentation updates. Sequence diagram for TLS client creation with optional custom CA certificatesequenceDiagram
participant Caller
participant MicrosoftADAction
participant MicrosoftADModule
participant MicrosoftADConfiguration
participant Tempfile
participant Tls
participant Server
participant Connection
Caller->>MicrosoftADAction: access client
MicrosoftADAction->>MicrosoftADModule: get configuration
MicrosoftADModule->>MicrosoftADConfiguration: return configuration
MicrosoftADAction->>MicrosoftADConfiguration: read ca_certificate
alt ca_certificate is present
MicrosoftADAction->>Tempfile: create pem file and write ca_certificate
Tempfile-->>MicrosoftADAction: return ca_file path
MicrosoftADAction->>Tls: create validate CERT_REQUIRED, ca_certs_file ca_file
else ca_certificate is absent
MicrosoftADAction->>Tls: create validate CERT_NONE
end
MicrosoftADAction->>Server: create host, port 636, use_ssl true, tls tls_config
MicrosoftADAction->>Connection: create with server, credentials, auto_bind
Connection-->>MicrosoftADAction: return bound connection
MicrosoftADAction-->>Caller: return client (connection)
Class diagram for updated Microsoft Active Directory configuration and action TLS setupclassDiagram
class MicrosoftADConfiguration {
+str servername
+str admin_username
+str admin_password
+str ca_certificate
}
class MicrosoftADModule {
+MicrosoftADConfiguration configuration
}
class MicrosoftADAction {
+MicrosoftADModule module
+client
}
class Tls {
+int validate
+str ca_certs_file
}
class Server {
+str host
+int port
+bool use_ssl
+Tls tls
}
class Connection {
+Server server
+str user
+str password
+bool auto_bind
}
MicrosoftADModule --> MicrosoftADConfiguration : has
MicrosoftADAction --> MicrosoftADModule : uses
MicrosoftADAction --> Connection : creates
Connection --> Server : uses
Server --> Tls : uses
MicrosoftADAction --> Tls : configures
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
CharlesLR-sekoia
requested review from
TOUFIKIzakarya and
squioc
and removed request for
squioc
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
clientproperty writes the CA certificate to a named temporary file withdelete=Falsebut never cleans it up, which can leak files over time; consider using a context-specific cleanup strategy (e.g., tracking and deleting on process shutdown) or an in-memory mechanism if the TLS library supports it. - When deciding whether to enable
CERT_REQUIRED, you currently check only for a truthyca_certificatestring; you may want to guard against empty/whitespace-only values or clearly validate that the content looks like a PEM certificate before writing and using it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `client` property writes the CA certificate to a named temporary file with `delete=False` but never cleans it up, which can leak files over time; consider using a context-specific cleanup strategy (e.g., tracking and deleting on process shutdown) or an in-memory mechanism if the TLS library supports it.
- When deciding whether to enable `CERT_REQUIRED`, you currently check only for a truthy `ca_certificate` string; you may want to guard against empty/whitespace-only values or clearly validate that the content looks like a PEM certificate before writing and using it.
## Individual Comments
### Comment 1
<location> `MicrosoftActiveDirectory/microsoft_ad/actions_base.py:20-24` </location>
<code_context>
def client(self):
+ tls_config = None
+ ca_cert = self.module.configuration.ca_certificate
+ if ca_cert:
+ with tempfile.NamedTemporaryFile(mode="w", suffix=".pem", delete=False) as f:
+ f.write(ca_cert)
+ ca_file = f.name
+ tls_config = Tls(validate=ssl.CERT_REQUIRED, ca_certs_file=ca_file)
+ else:
+ tls_config = Tls(validate=ssl.CERT_NONE)
</code_context>
<issue_to_address>
**🚨 issue (security):** Temporary CA cert file is never cleaned up, which can lead to file-system clutter and potential leakage of sensitive material.
Because `NamedTemporaryFile(..., delete=False)` is used, these PEM files will remain on disk indefinitely, causing clutter and potentially exposing CA material. Please either rely on `delete=True` (if `ldap3` can use the open file handle) or explicitly remove the temp file once the LDAP connection is established (e.g., with a `try/finally` around connection creation or a helper that manages the file lifecycle).
</issue_to_address>
### Comment 2
<location> `MicrosoftActiveDirectory/microsoft_ad/actions_base.py:25-26` </location>
<code_context>
+ f.write(ca_cert)
+ ca_file = f.name
+ tls_config = Tls(validate=ssl.CERT_REQUIRED, ca_certs_file=ca_file)
+ else:
+ tls_config = Tls(validate=ssl.CERT_NONE)
+
server = Server(
</code_context>
<issue_to_address>
**🚨 issue (security):** Falling back to `CERT_NONE` fully disables TLS verification, which is a notable security regression compared to typical defaults.
When `ca_certificate` is not provided, this path now unconditionally sets `validate=ssl.CERT_NONE`, disabling certificate verification. Prefer either relying on the library’s default trust store by omitting `tls_config` in the `else` branch, or introducing an explicit option (e.g. `skip_tls_verification`) so that turning off verification is an intentional, opt‑in choice rather than the default behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request adds support for custom PKI/internal CA certificates in TLS connections for the Microsoft Active Directory integration. It allows users to optionally provide a custom CA certificate for environments using internal certificate authorities while maintaining backward compatibility.
Changes:
- Added an optional
ca_certificateconfiguration parameter to support custom CA certificates for TLS verification - Updated TLS client implementation to write CA certificates to temporary files and configure certificate validation accordingly
- Added comprehensive test coverage for TLS configuration scenarios including custom CA certificates and SSL/port configuration
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| manifest.json | Added ca_certificate field to configuration schema and bumped version to 1.5.0 |
| microsoft_ad/models/common_models.py | Added optional ca_certificate field to MicrosoftADConfiguration model |
| microsoft_ad/actions_base.py | Implemented TLS configuration with custom CA support, writing certificates to temporary files |
| tests/test_base.py | Added comprehensive test suite for TLS configuration covering multiple scenarios |
| CHANGELOG.md | Documented the new feature in version 1.5.0 release notes |
CharlesLR-sekoia
changed the title
…earch_in_base_exception
mchupeau-sk
approved these changes
Feb 18, 2026
Contributor
mchupeau-sk
left a comment
mchupeau-sk
left a comment
There was a problem hiding this comment.
Ok for me, you can merge if it's finish
Contributor
|
@CharlesLR-sekoia there is just a conflict for the version |
Contributor
Author
|
thanks @mchupeau-sk not yes tested in production environment asprivate app. I will come back to you ASAP |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add configurable TLS client behavior for Microsoft Active Directory connections, including support for an optional custom CA certificate and corresponding configuration schema updates.
New Features:
Enhancements:
Documentation:
Tests:
Chores: