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.

Sign up for GitHub

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

Jump to bottom

OpenLDAP - Adding support for Active Directory Auth with Start TLS #20124

Merged
merged 59 commits into from
Sep 8, 2022
Merged

OpenLDAP - Adding support for Active Directory Auth with Start TLS #20124

merged 59 commits into from
Sep 8, 2022

Conversation

ShacharKidor
Copy link
Contributor

@ShacharKidor ShacharKidor commented Jul 19, 2022 •
edited
Loading

The Pull Request will be reviewed only after the contribution registration form is filled.

Status

  • In Progress
  • Ready
  • In Hold - (Reason for hold)

Related Issues

fixes: CIAC-2478.

Description

Support Active Directory Auth with STARTTLS as part of OpenLDAP auth integration:

  1. Added support for AD Auth (you can now login to XSOAR using active directory users).

  2. Added support for Start TLS connection.

  3. Enlarged the 'Allowed ciphers list' for SSL/TLS - In python 3.10 they hardened the SSL protocol by reducing the list of ciphers, which caused an LDAP server that did not have the ciphers in the reduced list to fail when establishing the connection. In this PR I returned the list of ciphers to the original list (before the hardening) if the user chose Trust any certificate = True (as done in this PR for Active Directory Query V2 Integration).

  4. Added a new TPB - LDAP Authentication - Test:
    OpenLDAP_-_Test_Tue_Aug_30_2022 (1)

A recommendation to use this integration for Active directory with STARTTLS was added to the detailed description of the 'Active Directory Authentication' (golang) integration - in this MR.

Minimum version of Cortex XSOAR

  • 6.0.0
  • 6.1.0
  • 6.2.0
  • 6.5.0

Does it break backward compatibility?

  • Yes
    • Further details: The new 'server type' parameter is required, but the default value is 'OpenLDAP' - so I think that users should be able to upgrade to this version without reconfiguring their existing instances. Not sure if it's considered a BC break.
  • No

Must have

  • Tests
  • Documentation

@guardrails
Copy link

guardrails bot commented Jul 24, 2022 •
edited
Loading

All previously detected findings have been fixed. Good job! 👍🎉

We will keep this comment up-to-date as you go along and notify you of any security issues that we identify.

👉 Go to the dashboard for detailed results.

📥 Happy? Share your feedback with us.

@ShacharKidor ShacharKidor changed the title Adding support for Active Directory Auth. OpenLDAP - Adding support for Active Directory Auth with TLS Jul 24, 2022
@ShacharKidor
- Changed the TLS to Start TLS.
93d97f9 
- Added the use of Auto Bind parameter.
@ShacharKidor ShacharKidor changed the title OpenLDAP - Adding support for Active Directory Auth with TLS OpenLDAP - Adding support for Active Directory Auth with Start TLS Jul 25, 2022
@ShacharKidor ShacharKidor marked this pull request as ready for review July 28, 2022 15:51
@ShirleyDenkberg
Copy link
Contributor

@JudahSchwartz Doc review completed.

JudahSchwartz
JudahSchwartz approved these changes Aug 1, 2022
View reviewed changes
Copy link
Contributor

@JudahSchwartz JudahSchwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing

Packs/OpenLDAP/Integrations/OpenLDAP/OpenLDAP.py Outdated Show resolved Hide resolved
Packs/OpenLDAP/Integrations/OpenLDAP/OpenLDAP.yml Outdated Show resolved Hide resolved
Packs/OpenLDAP/Integrations/OpenLDAP/OpenLDAP.yml Outdated Show resolved Hide resolved
Packs/OpenLDAP/Integrations/OpenLDAP/OpenLDAP.py Show resolved Hide resolved
Packs/OpenLDAP/Integrations/OpenLDAP/OpenLDAP.py Outdated Show resolved Hide resolved
Packs/OpenLDAP/Integrations/OpenLDAP/OpenLDAP.py Outdated Show resolved Hide resolved
Packs/OpenLDAP/pack_metadata.json Outdated Show resolved Hide resolved
@ShacharKidor ShacharKidor mentioned this pull request Aug 31, 2022
Merged
8 tasks
@ShacharKidor ShacharKidor removed the request for review from ilaner August 31, 2022 15:02
@ShacharKidor
Copy link
Contributor Author

ShacharKidor commented Sep 1, 2022 •
edited
Loading

A link to a successful build that used the commit merge of a fix in the SDK - 3578632.
The related SDK PR with the fix - 2308.

@xsoar-bot
Copy link
Contributor

Link to the unit tests coverage report:
https://output.circle-artifacts.com/output/job/e7d20427-6ee6-497b-972c-e6a93ccc9df7/artifacts/0/artifacts/coverage_report/html/index.html

@ShacharKidor
Copy link
Contributor Author

@ShahafBenYakir - Please force merge the PR, the reasons are:

  1. Validate failed on a new required parameter - should be required.
  2. Lint failed on low coverage - I added unit test only for Active directory new methods due to this open issue - CRTX-59131.

@ShacharKidor ShacharKidor added the ForceMerge Forcing the merge of the PR despite the build status label Sep 8, 2022
@ShahafBenYakir ShahafBenYakir merged commit a34269f into master Sep 8, 2022
@ShahafBenYakir ShahafBenYakir deleted the unify_openldap_and_ad branch September 8, 2022 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JudahSchwartz JudahSchwartz JudahSchwartz approved these changes

Assignees

@ShirleyDenkberg ShirleyDenkberg

@jrosenpanw jrosenpanw

Labels
docs-approved ForceMerge Forcing the merge of the PR despite the build status
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ShacharKidor @ShirleyDenkberg @xsoar-bot @JudahSchwartz @ShahafBenYakir @jrosenpanw @dorschw