Re: [Bug-wget] Windows cert store support

[Eli Zaretskii]

> Date: Thu, 10 Dec 2015 01:12:37 +0100
> From: Ángel González 
> Cc: bug-wget 
> 
> On 09/12/15 03:06, Random Coder wrote:
> > I'm not sure if the wget maintainers would be interested, but I've
> > been carrying this patch around in my private builds of wget for a
> > while.  It allows wget to load SSL certs from the default Windows cert
> > store.
> >
> > The patch itself is fairly straightforward, but as it changes the
> > default SSL behavior, and no care was taken to follow coding convents
> > when I wrote it, so it's probably not ready for inclusion in the
> > codebase.  Still, if it's useful, feel free to use it for ideas.
> Wow, supporting the OS store would certainly be very cool.
> 
> I would probably move it to windows.c and attempt to make it also work 
> in gnutls, but in general it looks good.

Wget compiled with GnuTLS already supports this feature: it calls
gnutls_certificate_set_x509_system_trust when the GnuTLS library
supports that.  gnutls_certificate_set_x509_system_trust does
internally what the proposed patch does.

So I think this code should indeed go only to openssl.c, as gnutls.c
already has its equivalent.

One other comment I have about the patch is that it's inconsistent
with what gnutls.c does:

  if (!opt.ca_directory)
ncerts = gnutls_certificate_set_x509_system_trust (credentials);
  /* If GnuTLS version is too old or CA loading failed, fallback to old 
behaviour.
   * Also use old behaviour if the CA directory is user-provided.  */
  if (ncerts <= 0)
{

IOW, condition the attempt to load the system certs on
opt.ca_directory, and fall back to the certs from files if that fails.

Thanks.

Re: [Bug-wget] Windows cert store support

[Petr Pisar]

On Fri, Dec 11, 2015 at 01:22:48PM +0200, Eli Zaretskii wrote:
> > Date: Thu, 10 Dec 2015 01:12:37 +0100
> > From: Ángel González 
> > Cc: bug-wget 
> > 
> > On 09/12/15 03:06, Random Coder wrote:
> > > I'm not sure if the wget maintainers would be interested, but I've
> > > been carrying this patch around in my private builds of wget for a
> > > while.  It allows wget to load SSL certs from the default Windows cert
> > > store.
> > >
> > > The patch itself is fairly straightforward, but as it changes the
> > > default SSL behavior, and no care was taken to follow coding convents
> > > when I wrote it, so it's probably not ready for inclusion in the
> > > codebase.  Still, if it's useful, feel free to use it for ideas.
> > Wow, supporting the OS store would certainly be very cool.
> > 
> > I would probably move it to windows.c and attempt to make it also work 
> > in gnutls, but in general it looks good.
> 
> Wget compiled with GnuTLS already supports this feature: it calls
> gnutls_certificate_set_x509_system_trust when the GnuTLS library
> supports that.  gnutls_certificate_set_x509_system_trust does
> internally what the proposed patch does.
> 
> So I think this code should indeed go only to openssl.c, as gnutls.c
> already has its equivalent.
> 
AFAIK OpenSSL source contains crypto engine that delegates all operations
to Windows native cryptographical subsystem. It's only matter of default
configuration.

-- Petr


signature.asc
Description: PGP signature

Re: [Bug-wget] Windows cert store support

[Gisle Vanem]

Random Coder wrote:

> I'm not sure if the wget maintainers would be interested, but I've
> been carrying this patch around in my private builds of wget for a
> while.  It allows wget to load SSL certs from the default Windows cert
> store.

I've applied your patch. It seems to work fine. Nice!

But in a message like:
  X509 certificate successfully verified and matches host
  www.ssllabs.com

it would be nice to know if it succeeded because of WinCrypt or
OpenSSL.

> +  /* Loop through all the certs in the Windows cert store */
> +  for ( pCertCtx = Local_CertEnumCertificatesInStore(hStore, NULL);
> +  pCertCtx != NULL;
> +  pCertCtx = Local_CertEnumCertificatesInStore(hStore, pCertCtx) )
> +  {
> +if (!((pCertCtx->dwCertEncodingType & PKCS_7_ASN_ENCODING) == 
> PKCS_7_ASN_ENCODING))
> +{
> +  /* Add all certs we find to OpenSSL's store */

How does this prevent an expired Cert to be used?
I see in the 'CERT_INFO' structure a 'NotAfter' member. But this
struct seems to support for WINAPI_PARTITION_APP only :-(
I assume this could be used to check expired certificates.

-- 
--gv

Re: [Bug-wget] Windows cert store support

[Ángel González]

On 09/12/15 03:06, Random Coder wrote:

I'm not sure if the wget maintainers would be interested, but I've
been carrying this patch around in my private builds of wget for a
while.  It allows wget to load SSL certs from the default Windows cert
store.

The patch itself is fairly straightforward, but as it changes the
default SSL behavior, and no care was taken to follow coding convents
when I wrote it, so it's probably not ready for inclusion in the
codebase.  Still, if it's useful, feel free to use it for ideas.

Wow, supporting the OS store would certainly be very cool.

I would probably move it to windows.c and attempt to make it also work 
in gnutls, but in general it looks good.


Thanks!

Re: [Bug-wget] Windows cert store support

[Random Coder]

On Wed, Dec 9, 2015 at 4:12 PM, Ángel González  wrote:
> I would probably move it to windows.c and attempt to make it also work in
> gnutls, but in general it looks good.

Fair enough.  I'll fix up the patch in the coming weeks.  If anyone
else wants a stab before me, feel free!

[Bug-wget] Windows cert store support

[Random Coder]

I'm not sure if the wget maintainers would be interested, but I've
been carrying this patch around in my private builds of wget for a
while.  It allows wget to load SSL certs from the default Windows cert
store.

The patch itself is fairly straightforward, but as it changes the
default SSL behavior, and no care was taken to follow coding convents
when I wrote it, so it's probably not ready for inclusion in the
codebase.  Still, if it's useful, feel free to use it for ideas.


openssl.patch
Description: Binary data