Fix a wrong WebDAV Warning with self-signed-certs

[Niduroki]

Occuring in the admin interface

[scrutinizer-notifier]

The inspection completed: 1 updated code elements

[ghost]

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4241/

[mmattel]

#7045 (
WebDAV / isWebDAVWorking error messages: possible explanation with respect to certificates)

[karlitschek]

Good catch 👍

[DeepDiver1975]

There is no need to extent class OC_DavClient - Sabre_DAV_Client has already a method for this: https://github.com/fruux/sabre-dav/blob/1.7.11/lib/Sabre/DAV/Client.php#L127

[mmattel]

@DeepDiver1975
I agree with the approach of Kondou-ger. The reference you wrote above only handles the parameter CURLOPT_SSL_VERIFYPEER (https://github.com/fruux/sabre-dav/blob/1.7.11/lib/Sabre/DAV/Client.php#L328). But Kondou-ger adds CURLOPT_SSL_VERIFYHOST which is NOT present in the current implementation of Client.php in SabreDav. If you would like to get more details, I please to read #7045 where I documented the difference and the need for a proper CURLOPT_SSL_VERIFYHOST handling.
IMHO it would be great if the developers of SabreDAV would add the host component to Client.php, but up to the point they add it, we have to deal with it.

[DeepDiver1975]

IMHO it would be great if the developers of SabreDAV would add the host component to Client.php, but up to the point they add it, we have to deal with it.

The power of Open Source: submit a pull request! 😉

[DeepDiver1975]

@mmattel Thanks for the clarification - I did not realize the slight difference between the two constants

[Niduroki]

@DeepDiver1975 and wait about a year until this is merged into owncloud's third-party repo and for owncloud8, for this to be released? This is sort of a quickfix.

[DeepDiver1975]

and wait about a year until this is merged into owncloud's third-party repo and for owncloud8, for this to be released? This is sort of a quickfix.

❓ Did I say this will not be merged? Hold your horses! But we need to interact with upstream projects

[Niduroki]

With "this" I mean an updated release of sabredav.

Let's say:

• 3 months for my feature to go into sabredav
• another 3 months for a new sabredav version including my feature
• About 4 months for someone to update sabredav in thirdparty
• owncloud7 is released by this date, so this'd have to wait for owncloud8, about 8 more months

= 1½ year for this single, little fix.

I'll file a PR to sabredav though.

[DeepDiver1975]

I'll file a PR to sabredav though.

Nothing more was requested - THX

[Niduroki]

https://github.com/fruux/sabre-dav/pull/437

[Niduroki]

@DeepDiver1975 this isn't wanted upstream, see fruux/sabre-dav#437.
In a newer version of sabredav this could be a one-liner (two including the comment):
$client->addCurlSetting(CURLOPT_SSL_VERIFYHOST, 0);
But this gives me
Fatal error: Call to undefined method OC_DAVClient::addCurlSetting() in …/testcloud/lib/private/util.php on line 906
probably because our version of sabredav is too old.

So this is the way to do this in OC.

[DeepDiver1975]

probably because our version of sabredav is too old.

Quite expected - right? 😉

[DeepDiver1975]

So this is the way to do this in OC.

👍

[mmattel]

@Kondou-ger
I know that it is a bit late for that but things still pop up, latest at #10069
and that reminded me for following:

I had a chat with @evert a while ago from fruux and he wrote me that the verifyPeer usage will be discouraged and on a long run taken out for sabredav forcing programers to do it on their own.

snip...
verifyPeer just exists for backwards compatibility purposes and will be removed in a future version.
Instead of doing this in such a way that we'd had to add a new method for every curl option, it made a lot more sense to just allow people to set any curl option.
snip...

As you have written the above implementation for CURLOPT_SSL_VERIFYHOST in OC, would you mind extending this for the CURLOPT_SSL_VERIFYPEER parameter setup, then we will not be dependent on future sabredav changes. The current handling of using verifyPeer should be taken out and a curl call like the one you did for verifyHost should be taken in.

[DeepDiver1975]

@mmattel I have to agree to @evert that we should not walk this path. The solution is to NOT use a selfsigned cert.

[evert]

There's nothing wrong with using self-signed certificates. You should provide a facility for people to use them, by actually supporting self-signed certificates ;)

In a controlled environment, using self-signed certificates makes a lot of sense imho. In some ways it's actually more secure, because you don't have to trust SSL root certificate businesses.

[mmattel]

Please let us seperate two things:

1.) let´s implement the CURLOPT_SSL_VERIFYPEER setting making Owncloud futureproof and less dependent on possible changes in sabredav as described above. I really would appreciate the help of Kondou-ger.
2.) let´s fix the annoing and recurring upcoming message about invalid certificates. (Maybe it will finally go away implementing 1)

ad 2. I had a self signed certificate and have now a signed one of CA-Cert (*.domain.com) and still have the problem that this message pop´s up.

[Niduroki]

Well, to not go with this workaround and setVerifyHost() we need to upgrade sabredav (see #8181 (comment)).

@DeepDiver1975 has OCs SabreDav been updated in the last 4 months? I don't think so, because this workaround will not work anymore in newer versions of sabredav, and will result in breakage …

[Niduroki]

And besides that, IIRC there was a PR that moved this test to the client side, instead of the server side, making this entirely superfluous. @PVince81 I think you've worked on that, didn't you?

[PVince81]

Yes, here #7051 (still WIP)