tkf / emacs-request

Request.el -- Easy HTTP request for Emacs Lisp
http://tkf.github.com/emacs-request/
GNU General Public License v3.0
629 stars 93 forks source link

Support authentication with the curl backend #176

Closed ndw closed 4 years ago

ndw commented 4 years ago

Hello,

This relates to issue #32 from a while back. It adds support (in the curl backend only) for authentication with a new :auth key.

(request
 "https://example.com/path/"
 :auth '(:user "ndw" :password "sekrit" :type "digest")
 :complete (cl-function
           (lambda (&key response &allow-other-keys)
             (message "Done: %s" (request-response-status-code response)))))

If the password is not provided, attempt to find it with auth-source. If, for example, you have a ~/.authinfo file that contains:

machine example.com login ndw port 443 password sekrit

Then the password can be elided from the request:

(request
 "https://example.com/path/"
 :auth '(:user "ndw")
 :complete (cl-function
           (lambda (&key response &allow-other-keys)
             (message "Done: %s" (request-response-status-code response)))))

The :type key can be left out as well because the default is --anyauth.

There is one thing I cannot work out: with the authentication arguments, request--curl-absolutify-redirects gets passed (nil) as its redirects and that causes an error. The second commit is an ugly hack that works around this problem, but a better solution would be to work out why nil gets passed in the first place.

The problem seems to be caused by the presence of the "USER:PASS" value that occurs after --user, but there are other options with values and I don't see any mapping to handle them.

Suggestions most welcome.

alphapapa commented 4 years ago

This looks like a good feature, but I think you should carefully review what happens behind the scenes. I discovered some time ago that request was leaving some temporary files on-disk which are passed to curl that could contain sensitive data, as well as potentially exposing sensitive data via command-line arguments (which are visible to all users on the system): https://github.com/alphapapa/matrix-client.el/issues/25. I don't know if that's still happening, but it would not be good if secrets, especially ones the user expects to be protected by auth-source sources, were exposed in plain-text in /tmp files or command-line arguments. Until this issue is carefully investigated, I'm not sure that request should be used for anything sensitive that requires authentication.

Edit: It appears that the issue does remain, since the code hasn't been changed: https://github.com/tkf/emacs-request/blob/4be823a89b2d53b97609d41a5f2caaae9355437e/request.el#L709

But even if the cleanup code is made more reliable, it's still bad to write sensitive data to plain-text files, regardless of filesystem permissions.

I don't know how much can be done--curl has to receive the data one way or another--but writing passwords and access tokens to disk in plain-text, where they could be recovered out-of-band by attackers, is insecure by design. At the very least, it should be prominently documented as a risk.

ndw commented 4 years ago

The ~/.authinfo file that I referred to is a long standing feature and can be encrypted. (It's ~/authinfo.gpg on my system.) The authentication code I added doesn't store any of the credentials in temporary files. I'm not sure what sensitive data you're thinking of.

It is true that credentials passed on the command line can be sniffed. That's no more or less true of my patch than any other use of curl from the command line. It would be worth mentioning that in the README, I suppose.

There's code in uri-auth.el in the Emacs distribution that appears to support digest authentication. That's actually what rekindled my interest in trying to fix this bug. I assume that this could all be done without curl, but that's considerably more effort than I have time (or skill, at the moment) to undertake.

dickmao commented 4 years ago

Please see #177