Skip to content

Conversation

@daedric
Copy link
Contributor

@daedric daedric commented Feb 11, 2025

In case of a local LDAP server, certificates do not always match the address.
In this case, we cannot connect to the LDAP server.

This PR allows one to configure a policy for certificate checking.

PHP is not my forte, please do not hesitate if anything is not idiomatic nor good :)

@tchapi
Copy link
Owner

tchapi commented Feb 13, 2025

Hi @daedric and thanks for your contribution!

I'm not a LDAP expert, but it seems overall correct

  • I would be very intentional in the naming => LDAPCertificateCheckingStrategy and LDAP_TLS_CERT_CHECK_STRATEGY probably for instance
  • Also, you need to update the .env file (here) to add the default value, and some comment on what it does
  • Same for the README 🙏🏼

Thank you again!

@tchapi tchapi self-requested a review February 13, 2025 11:25
@tchapi tchapi self-assigned this Feb 13, 2025
@tchapi tchapi added the enhancement New feature or request label Feb 13, 2025
@daedric
Copy link
Contributor Author

daedric commented Feb 13, 2025

Hi @daedric and thanks for your contribution!

I'm not a LDAP expert, but it seems overall correct
Me neither :d

* I would be very intentional in the naming => `LDAPCertificateCheckingStrategy` and `LDAP_TLS_CERT_CHECK_STRATEGY` probably for instance

I don't mind, the reason for this naming that the usual config/env variable for that is ldap_tls_reqcert or tls_reqcert.

I really don't mind changing it, as I find it clearer too. Please let me know.

* Also, you need to update the `.env` file ([here](https://github.com/tchapi/davis/blob/main/.env)) to add the default value, and some comment on what it does

* Same for the README 🙏🏼

Will do!

Thank you again!

My pleasure!

@tchapi
Copy link
Owner

tchapi commented Feb 13, 2025

I really don't mind changing it, as I find it clearer too. Please let me know.

Yes, I'd change it personally 🙏🏼 Thanks

@daedric daedric force-pushed the ts/chore/tls-check branch 3 times, most recently from dead85c to 41c427c Compare February 14, 2025 13:49
@daedric
Copy link
Contributor Author

daedric commented Feb 18, 2025

FYI I've done the requested changed :)

@tchapi tchapi merged commit 0291178 into tchapi:main Feb 19, 2025
3 checks passed
@tchapi
Copy link
Owner

tchapi commented Feb 19, 2025

FYI I've done the requested changed :)

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants