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

Option in admin config to disable hostname verification within guzzle client would be nice #376

Closed
troglodyne opened this issue Mar 2, 2022 · 2 comments

Comments

@troglodyne
Copy link

Howdy there! First off let me thank you for putting in so much work for this plugin. I recently did work to upgrade the version of this plugin shipped with cPanel (along with roundcube of course) to the latest release. Many of the bugs which I did not like with the 3.x series appear to be fixed in the 4.x releases, so I am rather pleased with the outcome.

I did wind up patching the constructor of https://github.com/mstilkerich/carddavclient/blob/master/src/HttpClientAdapterGuzzle.php to add verify => false to the $guzzleOptions, as there's no guarantee that an SSL certificate associated with a carddav server on localhost will pass hostname verification.

Would this be something you'd consider worth adding a configuration option about (at the very least in the admin config pathway)? If so, I'd be happy to look at writing the relevant changes to this repo and the carddavclient repo, but I also realize that might not be something you think is worth doing.

Feel free to close this if you don't think it is worth doing, and keep up the good work!

@mstilkerich
Copy link
Owner

Hi,

thanks, happy to hear you are a keen rcmcarddav user :-)

Actually I have not included the option to disable certificate verification on purpose, since I don't want to facilitate the improper use of TLS.

Concerning your described setup:

  • Option 1: Do not use TLS if the carddav server lives on local host, since it is pointless if you talk to the server through the loopback interface.
  • Option 2: If the server is externally reachable and has a valid certificate for its public hostname, just connect to it using the public hostname instead of using localhost.

If there is a use case where it actually makes sense to disable certificate verification, I would gladly add the option, but so far I cannot think of one.

@troglodyne
Copy link
Author

Agreed. Personally I'd prefer for TLS to be properly provisioned on all the customer machines which could possibly be using the autoconfigured carddav, but despite doing a lot of work elsewhere to ensure this is so (automatic provisioning, etc.), we still wind up getting customer support requests about "why is this broken" with customers not understanding how the two could possibly be related. In this case, you only really figure out that something is wrong by reading log messages anyways, so it is understandable why someone might not immediately understand why the error would be related.

After looking a bit more at some of the other issues in the tracker again, I think that implementing #137 might be sufficient to give people the proper guidance when problems like this exist. As such I will close this case and begin looking a bit deeper at what's needed to implement that reporting to the UI.

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

No branches or pull requests

2 participants