[PATCH] Fix random selection of keyserver

[PATCH] Fix random selection of keyserver

[Philip Kaludercic]

* epa-ks.el (epa-keyserver-list): Add new variable
(epa-keyserver): Use epa-keyserver-list to generate type
(epa-ks--fetch-key): Check if epa-keyserver is 'random
(epa-search-keys): Check if epa-keyserver is 'random
---
 lisp/epa-ks.el | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/lisp/epa-ks.el b/lisp/epa-ks.el
index a33025b112..dcdbefc57b 100644
--- a/lisp/epa-ks.el
+++ b/lisp/epa-ks.el
@@ -38,18 +38,23 @@ epa-ks
   :version "28.1"
   :group 'epa)
 
+(defvar epa-keyserver-list
+  '("keyring.debian.org"
+    "keys.gnupg.net"
+    "keyserver.ubuntu.com"
+    "pgp.mit.edu"
+    "pool.sks-keyservers.net"
+    "zimmermann.mayfirst.org")
+  "List of default keyservers.")
+
 (defcustom epa-keyserver "pgp.mit.edu"
   "Domain of keyserver.
 
 This is used by `epa-ks-lookup-key', for looking up public keys."
-  :type '(choice :tag "Keyserver"
+  :type `(choice :tag "Keyserver"
                  (const random)
-                 (const "keyring.debian.org")
-                 (const "keys.gnupg.net")
-                 (const "keyserver.ubuntu.com")
-                 (const "pgp.mit.edu")
-                 (const "pool.sks-keyservers.net")
-                 (const "zimmermann.mayfirst.org")
+                 ,@(mapcar (lambda (server) `(const ,server))
+                           epa-keyserver-list)
                  (string :tag "Custom keyserver"))
   :version "28.1")
 
@@ -145,7 +150,10 @@ epa-ks--fetch-key
   "Send request to import key with specified ID."
   (url-retrieve
    (format "https://%s/pks/lookup?%s"
-           epa-keyserver
+           (if (eq epa-keyserver 'random)
+               (nth (random (length epa-keyserver-list))
+                    epa-keyserver-list)
+               epa-keyserver)
            (url-build-query-string
             `(("search" ,(concat "0x" (url-hexify-string id)))
               ("options" "mr")
@@ -225,7 +233,10 @@ epa-search-keys
       (epa-ks-search-mode))
     (url-retrieve
      (format "https://%s/pks/lookup?%s"
-             epa-keyserver
+             (if (eq epa-keyserver 'random)
+               (nth (random (length epa-keyserver-list))
+                    epa-keyserver-list)
+               epa-keyserver)
              (url-build-query-string
               (append `(("search" ,query)
                         ("options" "mr")
-- 
2.30.2

Re: [PATCH] Fix random selection of keyserver

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sun, 30 May 2021 18:36:29 +0000
> 
> * epa-ks.el (epa-keyserver-list): Add new variable
> (epa-keyserver): Use epa-keyserver-list to generate type
> (epa-ks--fetch-key): Check if epa-keyserver is 'random
> (epa-search-keys): Check if epa-keyserver is 'random

I think it's better to have a :set function for the defcustom, which
would create a list from what the user customized, and then you can
randomly select from what the user defined, not from the standard list
out of user's control.  Right?

Re: [PATCH] Fix random selection of keyserver

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Sun, 30 May 2021 18:36:29 +0000
>> 
>> * epa-ks.el (epa-keyserver-list): Add new variable
>> (epa-keyserver): Use epa-keyserver-list to generate type
>> (epa-ks--fetch-key): Check if epa-keyserver is 'random
>> (epa-search-keys): Check if epa-keyserver is 'random
>
> I think it's better to have a :set function for the defcustom, which
> would create a list from what the user customized, and then you can
> randomly select from what the user defined, not from the standard list
> out of user's control.  Right?

Perhaps, the intent of this patch was just to fix the currently broken
behaviour suggested by epa-keyserver's type.

I am not sure if a :set function could be used to randomize the
keyserver for every request. Then again, is might not even really be
necessary to provide such a feature.

-- 
	Philip K.

Re: [PATCH] Fix random selection of keyserver

[Lars Ingebrigtsen]

Philip Kaludercic <philipk@posteo.net> writes:

> Perhaps, the intent of this patch was just to fix the currently broken
> behaviour suggested by epa-keyserver's type.
>
> I am not sure if a :set function could be used to randomize the
> keyserver for every request. Then again, is might not even really be
> necessary to provide such a feature.

I wonder whether it might make sense instead to allow `epa-keyserver' to
be a list, and if so, then choose a random server from that list.
That'd give users control over what servers to use when randomising. 

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Fix random selection of keyserver

[Philip Kaludercic]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Perhaps, the intent of this patch was just to fix the currently broken
>> behaviour suggested by epa-keyserver's type.
>>
>> I am not sure if a :set function could be used to randomize the
>> keyserver for every request. Then again, is might not even really be
>> necessary to provide such a feature.
>
> I wonder whether it might make sense instead to allow `epa-keyserver' to
> be a list, and if so, then choose a random server from that list.
> That'd give users control over what servers to use when randomising. 

That does seem to be a better approach, how does the patch below look
like?

-- 
	Philip K.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-random-selection-of-keyservers.patch --]
[-- Type: text/x-diff, Size: 3278 bytes --]

From fdbb17539a465c49aad93cef4e0b8b4b76699561 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sun, 30 May 2021 20:34:54 +0200
Subject: [PATCH] Improve random selection of keyservers

* epa-ks.el (epa-keyserver): Interpret a list as a pool
(epa-ks--query-url): Add new auxiliary function
(epa-ks--fetch-key): Use epa-ks--query-url
(epa-search-keys): Use epa-ks--query-url
---
 lisp/epa-ks.el | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/lisp/epa-ks.el b/lisp/epa-ks.el
index a33025b112..1d17c1399c 100644
--- a/lisp/epa-ks.el
+++ b/lisp/epa-ks.el
@@ -42,8 +42,9 @@ epa-keyserver
   "Domain of keyserver.
 
 This is used by `epa-ks-lookup-key', for looking up public keys."
-  :type '(choice :tag "Keyserver"
-                 (const random)
+  :type `(choice :tag "Keyserver"
+                 (repeat :tag "Random pool"
+                         (string :tag "Keyserver address"))
                  (const "keyring.debian.org")
                  (const "keys.gnupg.net")
                  (const "keyserver.ubuntu.com")
@@ -141,20 +142,33 @@ epa-ks-do-key-to-fetch
         (epa-ks--fetch-key id))))
   (tabulated-list-clear-all-tags))
 
+(defun epa-ks--query-url (query exact)
+  "Return URL for QUERY.
+If EXACT is non-nil, don't accept approximate matches."
+  (format "https://%s/pks/lookup?%s"
+          (cond ((null epa-keyserver)
+                 (user-error "Empty keyserver pool"))
+                ((listp epa-keyserver)
+                 (nth (random (length epa-keyserver))
+                      epa-keyserver))
+                ((stringp epa-keyserver)
+                 epa-keyserver)
+                ((error "Invalid type for `epa-keyserver'")))
+          (url-build-query-string
+           (append `(("search" ,query)
+                     ("options" "mr")
+                     ("op" "index"))
+                   (and exact '(("exact" "on")))))))
+
 (defun epa-ks--fetch-key (id)
   "Send request to import key with specified ID."
   (url-retrieve
-   (format "https://%s/pks/lookup?%s"
-           epa-keyserver
-           (url-build-query-string
-            `(("search" ,(concat "0x" (url-hexify-string id)))
-              ("options" "mr")
-              ("op" "get"))))
+   (epa-ks--query-url (concat "0x" (url-hexify-string id)) t)
    (lambda (status)
      (when (plist-get status :error)
        (error "Request failed: %s"
-           (caddr (assq (caddr (plist-get status :error))
-                        url-http-codes))))
+              (caddr (assq (caddr (plist-get status :error))
+                           url-http-codes))))
      (forward-paragraph)
      (save-excursion
        (goto-char (point-max))
@@ -224,13 +238,7 @@ epa-search-keys
         (erase-buffer))
       (epa-ks-search-mode))
     (url-retrieve
-     (format "https://%s/pks/lookup?%s"
-             epa-keyserver
-             (url-build-query-string
-              (append `(("search" ,query)
-                        ("options" "mr")
-                        ("op" "index"))
-                      (and exact '(("exact" "on"))))))
+     (epa-ks--query-url query exact)
      (lambda (status)
        (when (plist-get status :error)
          (when buf
-- 
2.30.2

Re: [PATCH] Fix random selection of keyserver

[Andreas Schwab]

On Mai 31 2021, Philip Kaludercic wrote:

> diff --git a/lisp/epa-ks.el b/lisp/epa-ks.el
> index a33025b112..1d17c1399c 100644
> --- a/lisp/epa-ks.el
> +++ b/lisp/epa-ks.el
> @@ -42,8 +42,9 @@ epa-keyserver
>    "Domain of keyserver.
>  
>  This is used by `epa-ks-lookup-key', for looking up public keys."
> -  :type '(choice :tag "Keyserver"
> -                 (const random)
> +  :type `(choice :tag "Keyserver"
> +                 (repeat :tag "Random pool"
> +                         (string :tag "Keyserver address"))

The backquote is no longer needed.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] Fix random selection of keyserver

[Philip Kaludercic]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Mai 31 2021, Philip Kaludercic wrote:
>
>> diff --git a/lisp/epa-ks.el b/lisp/epa-ks.el
>> index a33025b112..1d17c1399c 100644
>> --- a/lisp/epa-ks.el
>> +++ b/lisp/epa-ks.el
>> @@ -42,8 +42,9 @@ epa-keyserver
>>    "Domain of keyserver.
>>  
>>  This is used by `epa-ks-lookup-key', for looking up public keys."
>> -  :type '(choice :tag "Keyserver"
>> -                 (const random)
>> +  :type `(choice :tag "Keyserver"
>> +                 (repeat :tag "Random pool"
>> +                         (string :tag "Keyserver address"))
>
> The backquote is no longer needed.

Yes, of course. Should I resubmit the patch, or can it be corrected
before applying it?

> Andreas.

-- 
	Philip K.

Re: [PATCH] Fix random selection of keyserver

[Lars Ingebrigtsen]

Philip Kaludercic <philipk@posteo.net> writes:

> That does seem to be a better approach, how does the patch below look
> like?

Looks good to me.  Pushed to Emacs 28 (with the change mentioned by
Andreas).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Fix random selection of keyserver

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Mon, 31 May 2021 11:00:47 +0200
> 
> Philip Kaludercic <philipk@posteo.net> writes:
> 
> > Perhaps, the intent of this patch was just to fix the currently broken
> > behaviour suggested by epa-keyserver's type.
> >
> > I am not sure if a :set function could be used to randomize the
> > keyserver for every request. Then again, is might not even really be
> > necessary to provide such a feature.
> 
> I wonder whether it might make sense instead to allow `epa-keyserver' to
> be a list, and if so, then choose a random server from that list.
> That'd give users control over what servers to use when randomising. 

Yes, that's what I meant: make the :set function generate the list
from which we then randomly select.  Sorry for unclear wording.