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

BUG: Remote HTTP arguments not honored by get_elasticsearch_version when attempting to use plugin hooks within a theme #2466

Closed
maiorano84 opened this issue Nov 18, 2021 · 3 comments
Labels
bug Something isn't working

Comments

@maiorano84
Copy link

maiorano84 commented Nov 18, 2021

Describe the bug

When attempting to override Wordpress remote HTTP requests from within a theme, the ElasticPress lifecycle executes too early for any of the theme hooks to be able to adjust these settings.

Specifically, the register_indexable_posts function occurs during the plugins_loaded Wordpress hook, which then kicks off various checks involving the get_elasticsearch_version method within the ElasticPress\Indexable\Post\SyncManager class setup.

By executing this early, any attempts to intercept the remote request either via ep_do_intercept_request, ep_pre_request_args, or even http_request_args will seemingly do nothing on the initial version checks, and ElasticPress will output the There is a problem with connecting to your Elasticsearch host. ElasticPress can try your host again, or you may need to change your settings. message within the admin panel.

Use Case

In our specific case, we have a private ElasticSearch cluster running on Kubernetes. This private cluster utilizes a private certificate signed by an Amazon ACM PCA Root CA to facilitate secure communications between any Wordpress services that need to access this Elasticsearch installation.

The Cluster was configured both with xpack.security.enabled and xpack.security.http.ssl.enabled options set to true within the corresponding Helm Chart using the Security Example Chart as a base.

Wordpress uses its own certificate bundle under wp-includes/certificates/ca-bundle.crt. All remote requests use this bundle by default in order for cURL requests to be validated properly.

When using self-signed or unidentified Root CAs that are not defined within this bundle, cURL requests will fail validation unless one of three approaches are taken:

  1. (Recommended) Use ep_pre_request_args or http_request_args to modify the sslcertificates setting to point to a file on the server containing the custom root CA.
  2. Append the Root CA Certificate to wp-includes/certificates/ca-bundle.crt so that Wordpress can understand it by default. All verifications will pass this way, but this method will not hold up when updating Wordpress unless automatic processes are in place to ensure that the modified bundle remains in place after an update.
  3. (Not Recommended) Use http_request_args or https_ssl_verify to change the sslverify setting to false. This is NOT recommended for anybody who understands or cares about how SSL Certificates work, though it is recognized as the most common "solution" for the average layman.

When the server is configured correctly and when attempting to use any of the hook methods outline in options 1 or 3, ElasticPress will recognize the 200 OK response, but fail on the is_wp_error check on Line 1170 because Wordpress will come back with an unable to get local issuer certificate error.

Steps to Reproduce

  1. Set up a blank theme containing only the necessary index.php and style.css files.
  2. Add a functions.php file with the following contents:
add_filter('ep_pre_request_args', function($args){
    $args['sslcertificates'] = '/path/to/ca-certificate.crt';
    return $args;
});
  1. Activate the theme
  2. Add the following line to L1169: echo '<pre>';var_dump($args);die('</pre>');
  3. (If the Admin Screen doesn't dump any output) Click the "Save Changes" button from within ElasticPress->Settings
  4. Observe the var_dump output and that no sslcertificates setting is part of the provided arguments, despite executing well after L1097 where the first call in the lifecycle happens.

Expected behavior

The theme should be able to modify these arguments through the provided hooks as needed.

Environment information

  • Device: All devices
  • OS: All Operating Systems (OSX, Windows, Linux)
  • WordPress version: Tested on 5.8.0-5.8.2
  • ElasticPress version: 3.6.4
  • Elasticsearch version: 7.x (all versions affected)
  • Where do you host your Elasticsearch server? Self-Hosted on Kubernetes

Current Workarounds

Only two workarounds are known:

  1. Appending the CA Root Certificate to wp-includes/certificates/ca-bundle.crt works, but will fail after upgrading Wordpress and need to be reapplied manually or through custom automation.
  2. Creating a Must Use Plugin with the filter logic outlined above will succeed without issue. MU Plugins are loaded first, and execute the earliest in the Wordpress lifecycle. At the moment, this is the only approach that appears to be the cleanest and most maintainable solution without altering any Wordpress or Plugin files directly.

While Workaround 2 is the current solution we are employing at the moment, it may not be ideal or apparent to the average user who may need to modify the underlying requests for their own purposes.

Possible Solutions

Consider changing the registration plugin hook to something later like init or after_setup_theme:

https://codex.wordpress.org/Plugin_API/Action_Reference

This will allow theme developers to modify any remote http settings they need without having to chase down why the get_elasticsearch_version remote calls aren't honoring their environment settings.

I don't know if there are any far-reaching consequences to this approach, but that appears to be the simplest.

@maiorano84 maiorano84 added the bug Something isn't working label Nov 18, 2021
@felipeelia
Copy link
Member

HI @maiorano84, thanks for the detailed explanation. This topic was covered in this other issue and, currently, we consider this to be plugin territory.
In that issue, we were trying to find out any cases where it could not be handled by a plugin, so if you have one, please let us know and we can reconsider that. Thanks

@maiorano84
Copy link
Author

@felipeelia Thanks for getting back so quickly!

Our use-case is pretty specific, so I don't think there's anything wrong with us rolling our own MU Plugin for the overrides we need.

I think the general concern is that if you're providing hooks for developers, the expectation for those hooks to be leveraged will typically happen in a theme first before a developer considers rolling their own plugin.

Even when not using EP's own hooks and instead attempting to hook into the underlying WP Request hooks (ie: http_request_args), I think most developers would expect their own theme callbacks to have some kind of an effect over those concerns.

That said, in our case, overriding the default certificate for certain calls to ElasticPress is a system concern, not a theme concern, so it definitely seems more appropriate to have this functionality in place as an MU Plugin for us. I have no problem with maintaining that approach.

If this is this something that you do not feel is an appropriate issue for ElasticPress to address as a code solution, do you feel that this can be touched upon in your documentation?

https://10up.github.io/ElasticPress/ep_pre_request_args.html

I realize that this particular hook is more recent, but I think most devs are going to think at first blush that this hook will affect ALL remote HTTP calls, when get_elasticsearch_version is definitely an exception here.

@felipeelia
Copy link
Member

You have a good point @maiorano84. To make things clearer and to address some other concerns and questions we receive more often, we put together this article. We'll be linking to it in our documentation, so people setting their own hosting solution will have more to work with.
Although I'm going to close the issue, feel free to add any feedback you have. Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants