[Spice-devel] [spice-common 2/3] ssl: Log an error when peer certificate verification failed

[Christophe Fergeau]

Hey,

On Sun, Sep 22, 2013 at 02:07:23PM +0300, Uri Lublin wrote:
> On 09/20/2013 06:07 PM, Christophe Fergeau wrote:
> >We currently log an error when openssl_verify() is called with
> >preverify_ok set to 0 for all certificates in the certificate chain
> >except for the peer certificate (when 'depth' is 0).
> >This commit logs an error in the latter case as well.
> >---
> >  common/ssl_verify.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> >diff --git a/common/ssl_verify.c b/common/ssl_verify.c
> >index d4b89f0..7af78bc 100644
> >--- a/common/ssl_verify.c
> >+++ b/common/ssl_verify.c
> >@@ -456,8 +456,16 @@ static int openssl_verify(int preverify_ok, X509_STORE_CTX *ctx)
> >              failed_verifications |= SPICE_SSL_VERIFY_OP_PUBKEY;
> >      }
> >-    if (!v->all_preverify_ok || !preverify_ok)
> >+    if (!preverify_ok) {
> >+        err = X509_STORE_CTX_get_error(ctx);
> >+        depth = X509_STORE_CTX_get_error_depth(ctx);
> >+        spice_warning("Error in server certificate verification: %s (num=%d:depth%d:%s)",
> >+                      X509_verify_cert_error_string(err), err, depth, buf);
> >          return 0;
> >+    }
> >+    if (!v->all_preverify_ok) {
> >+        return 0;
> >+    }
> 
> Hi Christophe,
> 
> Nitpick1: if !all_preverfiy_ok then something has already failed and
> reported. Maybe it's better to
> switch those new ifs (to not report error twice).

I think the error that would be reported in the !preverify_ok case would be
a different failure than the one that has been reported when
all_preverify_ok was set to FALSE, so it's probably better to report it as
well.

> Nitpick2: err and depth are already set in the beginning of the function.

Indeed, I even c&p'ed these 2 lines :-/ I'll remove them.

> Nitpick3: Maybe it's better to move this code above the check for
> verify_pubkey

It would be much better, but it seems we have older deployments which
expects that if a public key is set during migration, then certificates are
blindly accepted regardless of their validity :-/ So it's better to keep it
this way, especially as Marc-André had to fix that once already, see
e3f69418

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20130923/51c9f635/attachment.pgp>