[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501
From: |
Tim Ruehsen |
Subject: |
Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501 |
Date: |
Wed, 12 Nov 2014 15:44:52 +0100 |
User-agent: |
KMail/4.14.2 (Linux/3.16.0-4-amd64; KDE/4.14.2; x86_64; ; ) |
On Wednesday 12 November 2014 19:44:30 Darshit Shah wrote:
> On 11/11, Tim Rühsen wrote:
> >On Tuesday 11 November 2014 11:58:26 Giuseppe Scrivano wrote:
> >> Tim Ruehsen <address@hidden> writes:
> >> > On Saturday 08 November 2014 13:00:13 Giuseppe Scrivano wrote:
> >> >> Tim Ruehsen <address@hidden> writes:
> >> >> > On Friday 07 November 2014 09:26:58 Giuseppe Scrivano wrote:
> >> >> >> Tim Ruehsen <address@hidden> writes:
> >> >> >> > Here is a first patch (GnuTLS only) for review and comments (and
> >> >> >> > playing
> >> >> >> > around).
> >> >> >>
> >> >> >> I think we should fail and avoid any connection instead of printing
> >> >> >> just
> >> >> >> a warning as it seems from the code now. Have you tested it with
> >> >> >> some
> >> >> >> crl file? Would be good to add some automatic tests for this new
> >> >> >> feature.
> >> >> >>
> >> >> >> > - Should we support complete directories ?
> >> >> >> > - Should we allow more than one --crl-file option ?
> >> >> >>
> >> >> >> We can add this later, but we need to ensure that wget fails now if
> >> >> >> more
> >> >> >> --crl-file are passed so that the user knows it is not supported
> >> >> >> now.
> >> >> >
> >> >> > Amended patch.
> >> >>
> >> >> thanks, the patch looks fine to me.
> >> >
> >> > I just moved a block of code (loading of --ca-certificate) to the right
> >> > place and added output on failure and success.
> >> >
> >> > To made up a test, I had to recreate testenv/certs. The former CN
> >> > component
> >> > did not have the correct name, which would allow us to generate a CRL
> >> > file.
> >> > This also allows us to use the CA cert (--ca-certificate=) and remove
> >> > the
> >> > very general --no-check-certificate from the Wget command line within
> >> > Test-- https.py.
> >> >
> >> > The testenv/certs directory now seems somehow cleaner and better to
> >> > understand (to me). I documented the cert/key/crl creation steps (using
> >> > certtool) in testenv/certs/README.
> >> >
> >> > Review and comments appreciated.
> >>
> >> great work, it looks fine to me. Feel free to push it.
> >
> >It has been pushed.
> >
> >Tim
>
> Hi,
>
> I just looked at the patch.
>
> diff --git a/src/openssl.c b/src/openssl.c
> index 6685ee7..371913c 100644
> --- a/src/openssl.c
> +++ b/src/openssl.c
> @@ -254,6 +254,22 @@ ssl_init (void)
> SSL_CTX_set_default_verify_paths (ssl_ctx);
> SSL_CTX_load_verify_locations (ssl_ctx, opt.ca_cert, opt.ca_directory);
>
> + if (opt.crl_file)
> + {
> + X509_STORE *store = SSL_CTX_get_cert_store (ssl_ctx);
> + X509_LOOKUP *lookup;
> + int rc;
> Variable rc is declared here...
>
> +
> + if (!(lookup = X509_STORE_add_lookup (store, X509_LOOKUP_file ()))
> + || (!(rc = X509_load_crl_file (lookup, opt.crl_file,
> X509_FILETYPE_PEM)))) rc is initialized **only** if the first condition is
> false.
> + {
> + logprintf (LOG_NOTQUIET, _("ERROR: Failed to load CRL file '%s':
> (%d)\n"), opt.crl_file, rc); rc is used uninitialized in case the first
> condition is true.
>
> Based on the error being printed, I'm going to assume that rc needs to be
> initialized in both cases. Or the error message needs to be changed.
Thanks Darshit, good catch.
Also, rc is always 0 when X509_load_crl_file() fails. So no need for it.
Second, instead of a return false we should jump to 'error:', to print the
OpenSSL error and to free ssl_ctx.
I just pushed the code fix (f**k...and forgot to mention you in Reported-by,
sorry).
Tim
signature.asc
Description: This is a digitally signed message part.
- Re: [Bug-wget] certificate revocation lists (CRLs) #43501, (continued)
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501, Tim Ruehsen, 2014/11/06
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501, Giuseppe Scrivano, 2014/11/07
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501, Tim Ruehsen, 2014/11/07
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501, Tim Ruehsen, 2014/11/07
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501, Giuseppe Scrivano, 2014/11/08
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501, Tim Ruehsen, 2014/11/10
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501, Giuseppe Scrivano, 2014/11/11
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501, Tim Ruehsen, 2014/11/11
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501, Darshit Shah, 2014/11/12
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501,
Tim Ruehsen <=
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501, Tim Ruehsen, 2014/11/11
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501, Tim Rühsen, 2014/11/11
- Re: [Bug-wget] [PATCH] certificate revocation lists (CRLs) #43501, Giuseppe Scrivano, 2014/11/12