You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ko...@apache.org on 2019/07/13 21:31:25 UTC

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

Author: kotkov
Date: Sat Jul 13 21:31:25 2019
New Revision: 1863018

URL: http://svn.apache.org/viewvc?rev=1863018&view=rev
Log:
Win32: tweak the SSL certificate validation override to avoid hitting the wire
for URL based objects and revocation checks.

The primary purpose of this callback is to resolve SVN_AUTH_SSL_UNKNOWNCA
failures using CryptoAPI and Windows local certificate stores.  To do so, we
should be fine with just using the immediately available data on the local
machine.

Doing the opposite of that appears to be troublesome, as always connecting
to remote CRL and OCSP endpoints can result in spurious errors or significant
(user-reported) network stalls caused by timeouts if the endpoints are
inaccessible or unreliable.

The new approach should also be in par with the default basic behavior of
several major browsers, for example:
  https://chromium.googlesource.com/chromium/src/net/+/3d1dad1c17ae3ff59e7c35841af94b66f4bca1ba/cert/cert_verify_proc_win.cc#919

 * subversion/libsvn_subr/win32_crypto.c
   (windows_validate_certificate): Use the CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL
    and CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY flags when preparing the
    certificate chain.  Ignore errors in obtaining valid revocation information
    when verifying the chain, as they could be induced by the new cache-only
    behavior.

Modified:
    subversion/trunk/subversion/libsvn_subr/win32_crypto.c

Modified: subversion/trunk/subversion/libsvn_subr/win32_crypto.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/win32_crypto.c?rev=1863018&r1=1863017&r2=1863018&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/win32_crypto.c (original)
+++ subversion/trunk/subversion/libsvn_subr/win32_crypto.c Sat Jul 13 21:31:25 2019
@@ -395,16 +395,29 @@ windows_validate_certificate(svn_boolean
       memset(&chain_para, 0, sizeof(chain_para));
       chain_para.cbSize = sizeof(chain_para);
 
+      /* Don't hit the wire for URL based objects and revocation checks, as
+         that may cause stalls, network timeouts or spurious errors in cases
+         such as with the remote OCSP and CRL endpoints being inaccessible or
+         unreliable.
+
+         For this particular case of the SVN_AUTH_SSL_UNKNOWNCA cert failure
+         override we should be okay with just the data that we have immediately
+         available on the local machine.
+       */
       if (CertGetCertificateChain(NULL, cert_context, NULL, NULL, &chain_para,
                                   CERT_CHAIN_CACHE_END_CERT |
-                                  CERT_CHAIN_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT,
+                                  CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL |
+                                  CERT_CHAIN_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT |
+                                  CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY,
                                   NULL, &chain_context))
         {
           CERT_CHAIN_POLICY_PARA policy_para;
           CERT_CHAIN_POLICY_STATUS policy_status;
 
           policy_para.cbSize = sizeof(policy_para);
-          policy_para.dwFlags = 0;
+          /* We only use the local data for revocation checks, so they may
+             fail with errors like CRYPT_E_REVOCATION_OFFLINE; ignore those. */
+          policy_para.dwFlags = CERT_CHAIN_POLICY_IGNORE_ALL_REV_UNKNOWN_FLAGS;
           policy_para.pvExtraPolicyPara = NULL;
 
           policy_status.cbSize = sizeof(policy_status);

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

Posted by Branko Čibej <br...@apache.org>.
On 14.07.2019 17:51, Evgeny Kotkov wrote:
> Branko Čibej <br...@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.


Hmm ... I wasn't aware that this check wasn't always performed. That
changes the impact of you change considerably. Please leave it in for
now ... but let's have that discussion anyway.

-- Brane

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

Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Branko Čibej <br...@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

CRL and OCSP verification [was: Re: svn commit: r1863018 ...]

Posted by Branko Čibej <br...@apache.org>.
On 14.07.2019 00:29, Branko Čibej wrote:
> On 13.07.2019 23:31, kotkov@apache.org wrote:
>> Author: kotkov
>> Date: Sat Jul 13 21:31:25 2019
>> New Revision: 1863018
>>
>> URL: http://svn.apache.org/viewvc?rev=1863018&view=rev
>> Log:
>> Win32: tweak the SSL certificate validation override to avoid hitting the wire
>> for URL based objects and revocation checks.
>>
>> The primary purpose of this callback is to resolve SVN_AUTH_SSL_UNKNOWNCA
>> failures using CryptoAPI and Windows local certificate stores.  To do so, we
>> should be fine with just using the immediately available data on the local
>> machine.
>>
>> Doing the opposite of that appears to be troublesome, as always connecting
>> to remote CRL and OCSP endpoints can result in spurious errors or significant
>> (user-reported) network stalls caused by timeouts if the endpoints are
>> inaccessible or unreliable.
>>
>> The new approach should also be in par with the default basic behavior of
>> several major browsers, for example:
>>   https://chromium.googlesource.com/chromium/src/net/+/3d1dad1c17ae3ff59e7c35841af94b66f4bca1ba/cert/cert_verify_proc_win.cc#919
>
> 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.
>
> Your change disables all online verification, regardless, and that seems
> quite wrong.
>
> If the server certificate contains CRL or OCSP information, the client
> SHOULD verify them. If verification services are flaky, users should
> report that to the server admin (who can then report it to the
> certificate issuer). We don't "fix" that by removing validation
> altogether in our client.
>
> So ... -1, please revert and let's discuss this change on the list
> first. It's far more significant than you seem to think.

To start off this discussion, I'll note that we don't currently use CRLs
or OCSP on other platforms. This is probably because Serf 1.3.9 doesn't
give us any support for that (though it does support OCSP stapling).

However, the as yet unreleased Serf 1.4.0 does have support for OCSP
(I'd have to double-check about CRL).

In any case, given what year it is on the calendar, we SHOULD try to add
OCSP verification to the client. The server can already do that for
client certs, since it's only a matter of httpd configuration.

-- Brane

P.S.: Yes, I realize this means getting Serf 1.4.0 out the door first ...

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

Posted by Branko Čibej <br...@apache.org>.
On 13.07.2019 23:31, kotkov@apache.org wrote:
> Author: kotkov
> Date: Sat Jul 13 21:31:25 2019
> New Revision: 1863018
>
> URL: http://svn.apache.org/viewvc?rev=1863018&view=rev
> Log:
> Win32: tweak the SSL certificate validation override to avoid hitting the wire
> for URL based objects and revocation checks.
>
> The primary purpose of this callback is to resolve SVN_AUTH_SSL_UNKNOWNCA
> failures using CryptoAPI and Windows local certificate stores.  To do so, we
> should be fine with just using the immediately available data on the local
> machine.
>
> Doing the opposite of that appears to be troublesome, as always connecting
> to remote CRL and OCSP endpoints can result in spurious errors or significant
> (user-reported) network stalls caused by timeouts if the endpoints are
> inaccessible or unreliable.
>
> The new approach should also be in par with the default basic behavior of
> several major browsers, for example:
>   https://chromium.googlesource.com/chromium/src/net/+/3d1dad1c17ae3ff59e7c35841af94b66f4bca1ba/cert/cert_verify_proc_win.cc#919


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.

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

If the server certificate contains CRL or OCSP information, the client
SHOULD verify them. If verification services are flaky, users should
report that to the server admin (who can then report it to the
certificate issuer). We don't "fix" that by removing validation
altogether in our client.

So ... -1, please revert and let's discuss this change on the list
first. It's far more significant than you seem to think.

-- Brane