Re: svn commit: r1863018 - /subversion/trunk/subversion/libsvn_subr/win32_crypto.c

Branko Čibej <brane_at_apache.org> writes:

> Uh. I don't think so.
>
> First of all, the reference to Chromium source is a red herring ... that
> code disables CRL/OCSP checks *if some caller required it to*. You'll
> find that browsers do, indeed, check CRL or OCSP info, if it's available.

I went through an additional round of fact-checking to ensure that Chromium
browsers have never been doing online revocation checks by default.

Back in 2014, this could be controlled by a user preference (disabled by
default), but since then the UI option was removed and the current state
is that such checks only operate from cache:

  https://codereview.chromium.org/250583004/diff/20001/chrome/browser/net/ssl_config_service_manager_pref.cc
  https://chromium.googlesource.com/chromium/src/+/refs/tags/77.0.3852.2/chrome/browser/ssl/ssl_config_service_manager_pref.cc#243

On Windows, this can be additionally verified by examining the CryptoAPI
log where I can see the certificate chains being constructed with the
CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY flag.

> Your change disables all online verification, regardless, and that seems
> quite wrong.

This change affects the Win32 callback used to *override specific* certificate
validation failures, namely SVN_AUTH_SSL_UNKNOWNCA. So in the current
design, the revocation checks are only supplemental, but not authoritative.
As an example, if the Serf/OpenSSL layer thinks that the certificate is OK
by itself (without performing a revocation check), this callback is not going
to be called at all.

Since all infrastructure-related failures, stalls and timeouts during online
revocation checks appear to be fairly common, combined, this means that we
are putting the users through all these potential problems, but in the end
the whole checking is still non-exhaustive. Perhaps, the actual result of
that is just adding to the perception that Subversion is slow in general.

With that in mind, I tend to think that it would be much better to just stick
to the locally available data for this particular callback, that is currently
used to only resolve a subset of certificate validation failures.

> So ... -1, please revert

Will certainly do, unless this new data alters your opinion on the topic.

Thanks,
Evgeny Kotkov
Received on 2019-07-14 17:46:24 CEST