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

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

[ng0]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

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.org https://ng0.infotropique.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Marius Bakke]

[-- Attachment #1.1: Type: text/plain, Size: 736 bytes --]

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.

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-gnu-curl-Look-up-SSL-certificates-in-etc-ssl-certs-b.patch --]
[-- Type: text/x-patch, Size: 2082 bytes --]

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(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Ricardo Wurmus]

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.

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

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

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]

Ricardo Wurmus <rekado@elephly.net> writes:

> 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.

> 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.  :-)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Marius Bakke]

[-- Attachment #1.1: Type: text/plain, Size: 559 bytes --]

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?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-gnu-feh-Respect-CURL_CA_BUNDLE.patch --]
[-- Type: text/x-patch, Size: 3370 bytes --]

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

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Ricardo Wurmus]

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.

> +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

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

[ng0]

[-- Attachment #1: Type: text/plain, Size: 1981 bytes --]

Ricardo Wurmus transcribed 1.6K bytes:
> 
> 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.

> 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.

> > +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.org https://ng0.infotropique.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 2140 bytes --]

Ricardo Wurmus <rekado@elephly.net> writes:

> 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.

>
>> +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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

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.

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Ludovic Courtès]

Marius Bakke <mbakke@fastmail.com> skribis:

> 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.

> 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’.