feh does encounter certificate errors with valid certificates

Done

Details

4 participants

Ludovic Courtès
Marius Bakke
ng0
Ricardo Wurmus

Owner: unassigned

Submitted by: ng0

Severity: normal

ng0 wrote on 22 Oct 2017 22:33

Recipients:(address . bug-guix@gnu.org)

Message-ID:20171022203339.qomgp4xm2rqh4zwe@abyayala

feh https://i.imgur.com/263enxT.jpg
feh opens image

Problem:
user@abyayala ~/src/guix/guix$ feh https://i.imgur.com/263enxT.jpg
feh WARNING: open url: server certificate verification failed. CAfile: none CRLfile: none
feh WARNING: https://i.imgur.com/263enxT.jpg- File does not exist
feh: No loadable images specified.
See 'man feh' for detailed usage information

nss etc are in my profile, no problem with other curl based applications.
-- 
ng0
GnuPG: A88C8ADD129828D7EAC02E52E22F9BBFEE348588
GnuPG: https://dist.ng0.infotropique.org/dist/keys/
https://www.infotropique.orghttps://ng0.infotropique.org

Marius Bakke wrote on 29 Oct 2017 13:27

Re: bug#28948: feh does encounter certificate errors with valid certificates

Recipients:

Message-ID:871slm5eby.fsf@fastmail.com

ng0 <ng0@infotropique.org> writes:

Toggle quote (10 lines)> feh https://i.imgur.com/263enxT.jpg
> feh opens image
>
> Problem:
> user@abyayala ~/src/guix/guix$ feh https://i.imgur.com/263enxT.jpg
> feh WARNING: open url: server certificate verification failed. CAfile: none CRLfile: none
> feh WARNING: https://i.imgur.com/263enxT.jpg - File does not exist
> feh: No loadable images specified.
> See 'man feh' for detailed usage information

This is the same issue with libcurl as has been discussed many times in
the past.  Since it won't be fixed upstream any time soon (support for
CURL_CA_BUNDLE has been removed also for Windows), I suggest we "bite
the bullet" this time and add a hard-coded default.

I've verified that this patch works (on GuixSD):

From 2ae03883c2526965f1a93cf5c691c41f02dc14b4 Mon Sep 17 00:00:00 2001
From: Marius Bakke <mbakke@fastmail.com>
Date: Fri, 9 Jun 2017 16:45:38 +0200
Subject: [PATCH] gnu: curl: Look up SSL certificates in /etc/ssl/certs by
 default.

* gnu/packages/curl.scm (curl)[arguments]<#:configure-flags>: Add '--with-ca-path'.
<#:phases>: Delete test that tries to use it.
---
 gnu/packages/curl.scm | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Toggle diff (33 lines)diff --git a/gnu/packages/curl.scm b/gnu/packages/curl.scm
index 2e4a48d1e..7248a6d40 100644
--- a/gnu/packages/curl.scm
+++ b/gnu/packages/curl.scm
@@ -67,7 +67,14 @@
        ("pkg-config" ,pkg-config)
        ("python" ,python-2)))
    (arguments
-    `(#:configure-flags '("--with-gnutls" "--with-gssapi")
+    `(#:configure-flags '("--with-gnutls" "--with-gssapi"
+                          ;; Hard-code a default CA certificate path so that
+                          ;; most things work "out of the box", at least on
+                          ;; GuixSD and Debian-based distributions.
+                          ;; libcurl does not support overriding this at runtime
+                          ;; except through the API, and it's impractical to
+                          ;; patch every application to respect CURL_CA_BUNDLE.
+                          "--with-ca-bundle=/etc/ssl/certs/ca-certificates.crt")
       ;; Add a phase to patch '/bin/sh' occurances in tests/runtests.pl
       #:phases
       (modify-phases %standard-phases
@@ -87,6 +94,10 @@
            (substitute* "tests/runtests.pl"
              (("/bin/sh") (which "sh")))
 
+           ;; XXX: This test fails because the default CA bundle path
+           ;; does not exist in the build environment.
+           (delete-file "tests/data/test324")
+
            ;; XXX FIXME: Test #1510 seems to work on some machines and not
            ;; others, possibly based on the kernel version.  It works on GuixSD
            ;; on x86_64 with linux-libre-4.1, but fails on Hydra for both i686
-- 
2.14.3

Ricardo Wurmus wrote on 29 Oct 2017 22:35

Re: bug#28948: feh does encounter certificate errors with valid certificates

Recipients:(name . Marius Bakke)(address . mbakke@fastmail.com)

Message-ID:87k1zdljro.fsf@elephly.net

Marius Bakke <mbakke@fastmail.com> writes:

Toggle quote (17 lines)> ng0 <ng0@infotropique.org> writes:
>
>> feh https://i.imgur.com/263enxT.jpg
>> feh opens image
>>
>> Problem:
>> user@abyayala ~/src/guix/guix$ feh https://i.imgur.com/263enxT.jpg
>> feh WARNING: open url: server certificate verification failed. CAfile: none CRLfile: none
>> feh WARNING: https://i.imgur.com/263enxT.jpg - File does not exist
>> feh: No loadable images specified.
>> See 'man feh' for detailed usage information
>
> This is the same issue with libcurl as has been discussed many times in
> the past.  Since it won't be fixed upstream any time soon (support for
> CURL_CA_BUNDLE has been removed also for Windows), I suggest we "bite
> the bullet" this time and add a hard-coded default.

This would mean that individual users no longer have control over what

certificate authorities they want to trust.

Does anything speak against patching in support for the CURL_CA_BUNDLE

environment variable?

Ricardo

GPG: BCA6 89B6 3655 3801 C3C6 2150 197A 5888 235F ACAC

https://elephly.net

Marius Bakke wrote on 29 Oct 2017 23:00

Recipients:(name . Ricardo Wurmus)(address . rekado@elephly.net)

Message-ID:87tvyh4ntj.fsf@fastmail.com

Ricardo Wurmus <rekado@elephly.net> writes:

Toggle quote (22 lines)> Marius Bakke <mbakke@fastmail.com> writes:
>
>> ng0 <ng0@infotropique.org> writes:
>>
>>> feh https://i.imgur.com/263enxT.jpg
>>> feh opens image
>>>
>>> Problem:
>>> user@abyayala ~/src/guix/guix$ feh https://i.imgur.com/263enxT.jpg
>>> feh WARNING: open url: server certificate verification failed. CAfile: none CRLfile: none
>>> feh WARNING: https://i.imgur.com/263enxT.jpg - File does not exist
>>> feh: No loadable images specified.
>>> See 'man feh' for detailed usage information
>>
>> This is the same issue with libcurl as has been discussed many times in
>> the past.  Since it won't be fixed upstream any time soon (support for
>> CURL_CA_BUNDLE has been removed also for Windows), I suggest we "bite
>> the bullet" this time and add a hard-coded default.
>
> This would mean that individual users no longer have control over what
> certificate authorities they want to trust.

Check and mate. I never considered this, but that makes this patch a

non-starter.

Toggle quote (3 lines)

> Does anything speak against patching in support for the CURL_CA_BUNDLE

> environment variable?

No, it looks like the only option. Should set a good precedent. :-)

Marius Bakke wrote on 30 Oct 2017 00:47

Re: bug#28948: feh does encounter certificate errors with valid certificates

Recipients:

Message-ID:87r2tl4iuz.fsf@fastmail.com

ng0 <ng0@infotropique.org> writes:

Toggle quote (12 lines)> feh https://i.imgur.com/263enxT.jpg
> feh opens image
>
> Problem:
> user@abyayala ~/src/guix/guix$ feh https://i.imgur.com/263enxT.jpg
> feh WARNING: open url: server certificate verification failed. CAfile: none CRLfile: none
> feh WARNING: https://i.imgur.com/263enxT.jpg - File does not exist
> feh: No loadable images specified.
> See 'man feh' for detailed usage information
>
> nss etc are in my profile, no problem with other curl based applications.

The attached patch should fix the problem. Can you try it?

From cadea693c636affd0d4cc5749eb88b5408aac07f Mon Sep 17 00:00:00 2001
From: Marius Bakke <mbakke@fastmail.com>
Date: Mon, 30 Oct 2017 00:18:03 +0100
Subject: [PATCH] gnu: feh: Respect $CURL_CA_BUNDLE.

* gnu/packages/patches/feh-respect-CURL_CA_BUNDLE.patch: New file.
* gnu/local.mk (dist_patch_DATA): Register it.
* gnu/packages/image-viewers.scm (feh)[source]: Use it.
[native-search-paths]: New field.
---
 gnu/local.mk                                          |  1 +
 gnu/packages/image-viewers.scm                        |  8 ++++++++
 gnu/packages/patches/feh-respect-CURL_CA_BUNDLE.patch | 18 ++++++++++++++++++
 3 files changed, 27 insertions(+)
 create mode 100644 gnu/packages/patches/feh-respect-CURL_CA_BUNDLE.patch

Toggle diff (64 lines)diff --git a/gnu/local.mk b/gnu/local.mk
index 90dc7aec1..7a74501aa 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -616,6 +616,7 @@ dist_patch_DATA =						\
   %D%/packages/patches/fasthenry-spFactor.patch			\
   %D%/packages/patches/fcgi-2.4.0-gcc44-fixes.patch		\
   %D%/packages/patches/fcgi-2.4.0-poll.patch			\
+  %D%/packages/patches/feh-respect-CURL_CA_BUNDLE.patch		\
   %D%/packages/patches/file-CVE-2017-1000249.patch		\
   %D%/packages/patches/findutils-localstatedir.patch		\
   %D%/packages/patches/findutils-gnulib-multi-core.patch	\
diff --git a/gnu/packages/image-viewers.scm b/gnu/packages/image-viewers.scm
index 9e93a97a9..98193063e 100644
--- a/gnu/packages/image-viewers.scm
+++ b/gnu/packages/image-viewers.scm
@@ -61,6 +61,7 @@
               (method url-fetch)
               (uri (string-append home-page
                                   name "-" version ".tar.bz2"))
+              (patches (search-patches "feh-respect-CURL_CA_BUNDLE.patch"))
               (sha256
                (base32
                 "0azgpr4al2pi4858z4xh4lfz84cvzxw3n426fn7rz6cdj34q212j"))))
@@ -79,6 +80,13 @@
               ("libxt" ,libxt)
               ("libx11" ,libx11)
               ("libxinerama" ,libxinerama)))
+    (native-search-paths
+    ;; Respect the same options as the `curl` command-line client.
+    (list (search-path-specification
+           (variable "CURL_CA_BUNDLE")
+           (file-type 'regular)
+           (separator #f)                         ;single entry
+           (files '("etc/ssl/certs/ca-certificates.crt")))))
     (synopsis "Fast and light imlib2-based image viewer")
     (description
       "feh is an X11 image viewer aimed mostly at console users.
diff --git a/gnu/packages/patches/feh-respect-CURL_CA_BUNDLE.patch b/gnu/packages/patches/feh-respect-CURL_CA_BUNDLE.patch
new file mode 100644
index 000000000..cbe2fa16d
--- /dev/null
+++ b/gnu/packages/patches/feh-respect-CURL_CA_BUNDLE.patch
@@ -0,0 +1,18 @@
+Make feh respect CURL_CA_BUNDLE similar to the `curl` tool.
+
+diff --git a/src/imlib.c b/src/imlib.c
+index dfb79aa..82a9865 100644
+--- a/src/imlib.c
++++ b/src/imlib.c
+@@ -429,6 +429,10 @@ static char *feh_http_load_image(char *url)
+ 			if (opt.insecure_ssl) {
+ 				curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0);
+ 				curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 0);
++			} else {
++				// Allow the user to specify custom CA certificates.
++				curl_easy_setopt(curl, CURLOPT_CAINFO,
++						getenv("CURL_CA_BUNDLE"));
+ 			}
+ 
+ 			res = curl_easy_perform(curl);
+
-- 
2.14.3

Ricardo Wurmus wrote on 30 Oct 2017 09:02

Re: bug#28948: feh does encounter certificate errors with valid certificates

Recipients:(name . Marius Bakke)(address . mbakke@fastmail.com)

Message-ID:87fua1kqqu.fsf@elephly.net

Marius Bakke <mbakke@fastmail.com> writes:

Toggle quote (16 lines)> ng0 <ng0@infotropique.org> writes:
>
>> feh https://i.imgur.com/263enxT.jpg
>> feh opens image
>>
>> Problem:
>> user@abyayala ~/src/guix/guix$ feh https://i.imgur.com/263enxT.jpg
>> feh WARNING: open url: server certificate verification failed. CAfile: none CRLfile: none
>> feh WARNING: https://i.imgur.com/263enxT.jpg - File does not exist
>> feh: No loadable images specified.
>> See 'man feh' for detailed usage information
>>
>> nss etc are in my profile, no problem with other curl based applications.
>
> The attached patch should fix the problem.  Can you try it?

We’ve done something similar in r-curl IIRC. I wonder if we should just

patch libcurl, so that all users of libcurl would benefit from this change.

Toggle quote (14 lines)> +diff --git a/src/imlib.c b/src/imlib.c
> +index dfb79aa..82a9865 100644
> +--- a/src/imlib.c
> ++++ b/src/imlib.c
> +@@ -429,6 +429,10 @@ static char *feh_http_load_image(char *url)
> + 			if (opt.insecure_ssl) {
> + 				curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0);
> + 				curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 0);
> ++			} else {
> ++				// Allow the user to specify custom CA certificates.
> ++				curl_easy_setopt(curl, CURLOPT_CAINFO,
> ++						getenv("CURL_CA_BUNDLE"));
> + 			}

Is it safe to pass the empty string to curl_easy_setopt, in case

CURL_CA_BUNDLE is unset? Do we need to check the value first or can we

pass it without checking?

Ricardo

GPG: BCA6 89B6 3655 3801 C3C6 2150 197A 5888 235F ACAC

https://elephly.net

ng0 wrote on 30 Oct 2017 15:06

Recipients:(name . Ricardo Wurmus)(address . rekado@elephly.net)

Message-ID:20171030140649.dt6n2v6i7im4rrx4@abyayala

Ricardo Wurmus transcribed 1.6K bytes:

Toggle quote (19 lines)> 
> Marius Bakke <mbakke@fastmail.com> writes:
> 
> > ng0 <ng0@infotropique.org> writes:
> >
> >> feh https://i.imgur.com/263enxT.jpg
> >> feh opens image
> >>
> >> Problem:
> >> user@abyayala ~/src/guix/guix$ feh https://i.imgur.com/263enxT.jpg
> >> feh WARNING: open url: server certificate verification failed. CAfile: none CRLfile: none
> >> feh WARNING: https://i.imgur.com/263enxT.jpg - File does not exist
> >> feh: No loadable images specified.
> >> See 'man feh' for detailed usage information
> >>
> >> nss etc are in my profile, no problem with other curl based applications.
> >
> > The attached patch should fix the problem.  Can you try it?

Thanks! I'll test it in the next couple of days.

Toggle quote (3 lines)

> We’ve done something similar in r-curl IIRC. I wonder if we should just

> patch libcurl, so that all users of libcurl would benefit from this change.

In my opinion that would be preferable.

Toggle quote (27 lines)> > +diff --git a/src/imlib.c b/src/imlib.c
> > +index dfb79aa..82a9865 100644
> > +--- a/src/imlib.c
> > ++++ b/src/imlib.c
> > +@@ -429,6 +429,10 @@ static char *feh_http_load_image(char *url)
> > + 			if (opt.insecure_ssl) {
> > + 				curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0);
> > + 				curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 0);
> > ++			} else {
> > ++				// Allow the user to specify custom CA certificates.
> > ++				curl_easy_setopt(curl, CURLOPT_CAINFO,
> > ++						getenv("CURL_CA_BUNDLE"));
> > + 			}
> 
> Is it safe to pass the empty string to curl_easy_setopt, in case
> CURL_CA_BUNDLE is unset?  Do we need to check the value first or can we
> pass it without checking?
> 
> --
> Ricardo
> 
> GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
> https://elephly.net
> 
> 
> 

-- 
ng0
GnuPG: A88C8ADD129828D7EAC02E52E22F9BBFEE348588
GnuPG: https://dist.ng0.infotropique.org/dist/keys/
https://www.infotropique.orghttps://ng0.infotropique.org

Marius Bakke wrote on 1 Nov 2017 21:55

Recipients:(name . Ricardo Wurmus)(address . rekado@elephly.net)

Message-ID:87d1514t3u.fsf@fastmail.com

Ricardo Wurmus <rekado@elephly.net> writes:

Toggle quote (21 lines)> Marius Bakke <mbakke@fastmail.com> writes:
>
>> ng0 <ng0@infotropique.org> writes:
>>
>>> feh https://i.imgur.com/263enxT.jpg
>>> feh opens image
>>>
>>> Problem:
>>> user@abyayala ~/src/guix/guix$ feh https://i.imgur.com/263enxT.jpg
>>> feh WARNING: open url: server certificate verification failed. CAfile: none CRLfile: none
>>> feh WARNING: https://i.imgur.com/263enxT.jpg - File does not exist
>>> feh: No loadable images specified.
>>> See 'man feh' for detailed usage information
>>>
>>> nss etc are in my profile, no problem with other curl based applications.
>>
>> The attached patch should fix the problem.  Can you try it?
>
> We’ve done something similar in r-curl IIRC.  I wonder if we should just
> patch libcurl, so that all users of libcurl would benefit from this change.

IIRC the reason it's not supported in libcurl is because getenv() is not

thread-safe, whereas libcurl is designed to be.

Toggle quote (19 lines)>
>> +diff --git a/src/imlib.c b/src/imlib.c
>> +index dfb79aa..82a9865 100644
>> +--- a/src/imlib.c
>> ++++ b/src/imlib.c
>> +@@ -429,6 +429,10 @@ static char *feh_http_load_image(char *url)
>> + 			if (opt.insecure_ssl) {
>> + 				curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 0);
>> + 				curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 0);
>> ++			} else {
>> ++				// Allow the user to specify custom CA certificates.
>> ++				curl_easy_setopt(curl, CURLOPT_CAINFO,
>> ++						getenv("CURL_CA_BUNDLE"));
>> + 			}
>
> Is it safe to pass the empty string to curl_easy_setopt, in case
> CURL_CA_BUNDLE is unset?  Do we need to check the value first or can we
> pass it without checking?

getenv() returns NULL if the variable is unset. I'm not sure if it

would reset the default on other distros, but it makes no difference for

Guix since libcurl does not have a default CA bundle and handles NULL

here gracefully.

I submitted it upstream in hope of getting feedback/testing there, but

it was simply merged as-is: https://github.com/derf/feh/pull/340

I do agree that it's rather crude, will try to improve it a bit.

Marius Bakke wrote on 5 Nov 2017 16:21

Recipients:(name . Ricardo Wurmus)(address . rekado@elephly.net)

Message-ID:87lgjkhhuc.fsf@fastmail.com

Marius Bakke <mbakke@fastmail.com> writes:

Toggle quote (5 lines)

> I submitted it upstream in hope of getting feedback/testing there, but

> it was simply merged as-is: <https://github.com/derf/feh/pull/340>

> I do agree that it's rather crude, will try to improve it a bit.

Feh 2.22 has been released with this patch, so I pushed the

native-search-path update with it.

I think we should add the CURL_CA_BUNDLE search path to the "curl"

package too so that we can control it on foreign distros (it seems to

opportunistically search /etc/ssl/certs), and make libcurl users that

implement it inherit from curl using (package-native-search-paths ...).

I'll do that on 'core-updates' in a few days if no further comments.

Closed

Ludovic Courtès wrote on 5 Nov 2017 17:14

Recipients:(address . 28948@debbugs.gnu.org)

Message-ID:87tvy8pusw.fsf@gnu.org

Marius Bakke <mbakke@fastmail.com> skribis:

Toggle quote (10 lines)> Marius Bakke <mbakke@fastmail.com> writes:
>
>> I submitted it upstream in hope of getting feedback/testing there, but
>> it was simply merged as-is: <https://github.com/derf/feh/pull/340>
>>
>> I do agree that it's rather crude, will try to improve it a bit.
>
> Feh 2.22 has been released with this patch, so I pushed the
> native-search-path update with it.

Neat.

Toggle quote (7 lines)

> I think we should add the CURL_CA_BUNDLE search path to the "curl"

> package too so that we can control it on foreign distros (it seems to

> opportunistically search /etc/ssl/certs), and make libcurl users that

> implement it inherit from curl using (package-native-search-paths ...).

> I'll do that on 'core-updates' in a few days if no further comments.

Sounds good!

Not entirely sure about duplicating the ‘native-search-paths’ in all the

users of libcurl: it’s inelegant, but OTOH it solves the problem, so

it’s definitely an improvement.

Thank you,

Ludo’.

Your comment

This issue is archived.

To comment on this conversation send an email to 28948@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it

mumi current 28948

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date