Skip to content

ensure TLS string options are properly inherited (regression) #18547

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

Closed
wants to merge 1 commit into from

Conversation

remicollet
Copy link
Member

@remicollet remicollet commented May 13, 2025

See #18529 regression in 8.3.21 and 8.4.7

This also fix the tests issue

  • ldaps_basic.phpt was failing (XFAIL) because of ldap.conf (TLS_CACERT) set in setup-slapd.sh
  • ldap_start_tls_basic.phpt was passing because global options not properly inherited

Using LDAPNOINIT=1 ensure ldap.conf is not used, so the local cert cannot be verified.

@remicollet remicollet changed the base branch from master to PHP-8.3 May 13, 2025 14:09
@remicollet remicollet marked this pull request as ready for review May 14, 2025 08:28
@remicollet remicollet added the Bug label May 14, 2025
@remicollet remicollet changed the title ensure TLS string options are properly inherited ensure TLS string options are properly inherited (regression) May 14, 2025
Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

I think this is an improvement. I don't have time to properly verify but based on what was described, it makes sense to me.

Would be probably good to wait @rainerjung to confirm that it fixes issue for him too.

@rainerjung
Copy link

Wow, thanks for taking the hard way. I will rebuild 8.4.7 with this PR on top and run the tests plus our local tests that showed the original problem. I should be able to report back tomorrow.

@rainerjung
Copy link

The patch has a minor bug, which makes it disfunctional. Instead of

ldap_set_option(ld, opts[i], &path);

it has to be

ldap_set_option(ld, opts[i], path);

Then my simple tests are successful.

@remicollet
Copy link
Member Author

@rainerjung Thanks

PR updated (and squashed)

…ap_start_tls()

Regresion introduced in fix for phpGH-17776

- ensure TLS string options are properly inherited
  workaround to openldap issue https://bugs.openldap.org/show_bug.cgi?id=10337

- fix ldaps/start_tls tests using LDAPNOINIT in ldaps/tls tests
@remicollet
Copy link
Member Author

Merged in 8.3+ as 2760a3e + 8da9530

@remicollet remicollet closed this May 15, 2025
@remicollet remicollet deleted the issue-ldapctx branch May 15, 2025 07:24
@remicollet
Copy link
Member Author

Notice: test coverage is not good, as we can't easily verify this case

I start PR #18561 which add tests for this case, but in master only, as it requires for "skipif" to be able to check some global option, which is not allowed by ldap_get_option for now)

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

Successfully merging this pull request may close these issues.

4 participants