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

Change to allow override of CA CERT for LDAP over TLS #4913

Merged
merged 2 commits into from
May 2, 2024

Conversation

mmoore2012
Copy link
Contributor

I struggled to make LDAPS work with the docker app. I did get this to work by injecting the certificate into the alpine container, but this is not great as the changes will get removed on an update to the container.

The changes I propose allow using the env LDAP_TLS_CACERTFILE to set a file to use to override the CA CERT used to verify LDAPS connections. This is to make this process easier for docker use.

Using the env LDAP_TLS_CACERTFILE to set a file to use to override
the CA CERT used to verify LDAPS connections. This is to make this
process easier for docker use.
@ssddanbrown
Copy link
Member

ssddanbrown commented Mar 26, 2024

Thanks for offering this @mmoore2012!
I have a feeling this can already be achieved via openldap config on the BookStack host system, but maybe we should add this to provide a direct config option for the main scenario, with anything more complex deferred to playing with openldap direct.
Related to #4075.

  • Could you just confirm that you've tested these changes against a live LDAP system?
  • Could you also add the option to the .env.example.complete file?
  • If possible, could you add a test method to the LDAP tests to cover this (at least to check the LDAP option is set when this option is present)?
    • Some further info about our PHP testing here.
    • If needed, I can do this if you have trouble with this, or find it too daunting/unfamiliar.

Doc Updates (reference for release process)

@mmoore2012
Copy link
Contributor Author

Thank you for reviewing this. I can confirm I have tested these changes against a live LDAP system (Windows 2019 Active Directory Domain) .

I will try and have a look at sorting a test out, but I have not had much experience with this.

@mmoore2012
Copy link
Contributor Author

I am struggling with the test. PHP has a function to set the LDAP option value:
ldap_set_option(?LDAP\Connection $ldap, int $option, array|string|int|bool $value): bool
https://www.php.net/manual/en/function.ldap-set-option.php
You can use 'null' for the first parameter to set the option globally. Unfortunately the ldap_get_option function only accepts a valid ldap connection so is useless for returning globally set options.
https://www.php.net/manual/en/function.ldap-get-option.php

I will probably need your assistance with this as I am unsure how to verify this has actually been set within PHP.

@ssddanbrown
Copy link
Member

@mmoore2012 No worries, thanks for attempting and for updating the .env.example.complete, I'll look to add tests when I come to review & merge this.

ssddanbrown added a commit that referenced this pull request May 2, 2024
Review of #4913
Added testing to cover option.
Updated option so it can be used for a CA directory, or a CA file.
Updated option name to be somewhat abstracted from original underling
PHP option.

Tested against Jumpcloud.
Testing took hours due to instability which was due to these settings
sticking and being unstable on change until php process restart.
Also due to little documentation for these options.
X_TLS_CACERTDIR option needs cert files to be named via specific hashes
which can be achieved via c_rehash utility.

This also adds detail on STARTTLS failure, which took a long time to
discover due to little detail out there for deeper PHP LDAP debugging.
ssddanbrown added a commit that referenced this pull request May 2, 2024
Review of #4913
Added testing to cover option.
Updated option so it can be used for a CA directory, or a CA file.
Updated option name to be somewhat abstracted from original underling
PHP option.

Tested against Jumpcloud.
Testing took hours due to instability which was due to these settings
sticking and being unstable on change until php process restart.
Also due to little documentation for these options.
X_TLS_CACERTDIR option needs cert files to be named via specific hashes
which can be achieved via c_rehash utility.

This also adds detail on STARTTLS failure, which took a long time to
discover due to little detail out there for deeper PHP LDAP debugging.
@ssddanbrown ssddanbrown merged commit 18269f2 into BookStackApp:development May 2, 2024
1 check failed
@ssddanbrown ssddanbrown changed the title Change to allow override of CA CERT for LDAPS Change to allow override of CA CERT for LDAP over TLS May 2, 2024
@ssddanbrown
Copy link
Member

ssddanbrown commented May 2, 2024

Thanks again @mmoore2012, this has now been merged for next feature release.
I made some further changes on top of this as part of #4985, mainly to allow the option to point to a cert or a directory of certs.

I also tweaked the title of this to prevent potential confusion for folks searching in the future, since I'm assuming you were using LDAP over TLS, rather than LDAPS. As far as I've been able to tell, ldaps is SSL only and I'm not sure such options would support such connections compared to LDAP over TLS via STARTTLS.
Let me know if you're confident I've got that wrong though.

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

Successfully merging this pull request may close these issues.

2 participants