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
Attachment:
pgpBvyDoCLtED.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel