Reconsider password-cache policy

Reconsider password-cache policy

[akater]

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

Consider the following snippet from =tramp.el=:

#+begin_example elisp
;; Try the password cache.
(progn
  (setq auth-passwd (password-read pw-prompt key)
	tramp-password-save-function
	(lambda () (password-cache-add key auth-passwd)))
  auth-passwd)
#+end_example

I think that if the intent of the progn form is to “try the password
cache” then such trial should fail whenever password caching is turned
off, i.e. when ~password-cache~ variable set to nil.

This suggestion will likely be questioned but in any case, there seems
to be no way at all to prevent TRAMP from caching passwords.
Cf. ~auth-source-do-cache~ variable; there does not seem to be any
equivalent in TRAMP.  So there should be ~tramp-do-password-cache~
variable at least.

However, I have no doubts that if ~password-cache~ is set to nil then
~password-data~ must be kept empty.  Authors of third party libraries
should not attempt to use password cache when ~password-cache~ is nil.
Thus, ~password-cache-add~ should always check the value of
~password-cache~ before doing anything and if yes, it should emit a
warning that there was an attempt to add a password to cache but
~password-cache~ is nil.

So, at the very least in ~tramp-read-passwd~ there should be
#+begin_example elisp
;; Try the password cache.
(when tramp-do-password-cache
  (setq auth-passwd (password-read pw-prompt key)
	tramp-password-save-function
	(lambda () (password-cache-add key auth-passwd)))
  auth-passwd)
#+end_example

with newly defined ~tramp-do-password-cache~ variable.

but I certainly suggest to go further, explicitly warn third party
library authors against using password cache when ~password-cache~ is
nil, and alter (at least) the definitions of

- ~tramp-read-passwd~:

#+begin_example elisp
;; Try the password cache.
(when password-cache
  (setq auth-passwd (password-read pw-prompt key)
	tramp-password-save-function
	(lambda () (password-cache-add key auth-passwd)))
  auth-passwd)
#+end_example

- ~password-cache-add~:

#+begin_example elisp
(defun password-cache-add (key password)
  "Add password to cache.
The password is removed by a timer after `password-cache-expiry' seconds."
  (if (not password-cache)
      (warn
       (format
        "There was an attempt to add a password to cache while `%s' is nil"
        'password-cache))
    (when (and password-cache-expiry
               (eq (gethash key password-data 'password-cache-no-data)
                   'password-cache-no-data))
      (run-at-time password-cache-expiry nil
		   #'password-cache-remove
		   key))
    (puthash key password password-data))
  nil)
#+end_example

- ~auth-source-search~:

#+begin_example elisp
(when password-cache
  (auth-source-remember spec found))
#+end_example

and alias ~auth-source-do-cache~ variable to ~password-cache~, marking
it as obsolete.

There is no point in allowing one library to use the cache but
disallowing another to do it.  It does not help with security as any
Elisp code can access that data anyway, any time, while added complexity
is always bad for security.  In contrast, there certainly must be a
clear way to turn caching off once and for all.  Given the current
policy, it can not possibly exist.  Multiplying ~..-do-cache~ variables
across elisp libraries will not do users any good.

I also think that password caching should be turned off by default.

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

Re: Reconsider password-cache policy

[Michael Albinus]

akater <nuclearspace@gmail.com> writes:

Hi,

> There is no point in allowing one library to use the cache but
> disallowing another to do it.  It does not help with security as any
> Elisp code can access that data anyway, any time, while added complexity
> is always bad for security.  In contrast, there certainly must be a
> clear way to turn caching off once and for all.  Given the current
> policy, it can not possibly exist.  Multiplying ~..-do-cache~ variables
> across elisp libraries will not do users any good.

Setting password-cache-expiry to 0 should do this?

> I also think that password caching should be turned off by default.

I disagree. There are cases you cannot work properly w/o password
caching. See for example the recent discussion about Tramp's sudoedit
method in bug#49724.

Best regards, Michael.

Re: Reconsider password-cache policy

[akater]

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

Michael Albinus <michael.albinus@gmx.de> writes:

> Setting password-cache-expiry to 0 should do this?

You're right, that works.  Better have a more explicit way to do this
but it's not that important.

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

Re: Reconsider password-cache policy

[Michael Albinus]

akater <nuclearspace@gmail.com> writes:

>> Setting password-cache-expiry to 0 should do this?
>
> You're right, that works.

Thanks for the feedback.

> Better have a more explicit way to do this but it's not that
> important.

That's the way the password cache is implemented. I must confess it is
poorly documented in the Emacs manual; Tramp at least speaks about in
(info "(tramp) Password handling")

Best regards, Michael.