Closed aagon closed 12 months ago
aagon @.***> writes:
At the moment, when the child emacs process needs to prompt the user for a password or a passphrase (for tramp, for instance), it hangs for ever.
Yes, this is a known issue described here https://github.com/jwiegley/emacs-async#authentication-and-user-interaction
The issue can be fixed by storing the needed password in a .authinfo.gpg file, it is what most people do (me included).
Once you know how to fix this issue, is it worth the effort fixing it?
This PR adds a very simple process filter and attaches it to the child process. I have taken inspiration from packages dired-rsync and mu4e, that also have to deal with child processes. It turns out that a process filter function is the canonical way to deal with prompts in child processes.
The process filter simply looks for a regular expression in the output of the child process. It it matches, the user is prompted in the parent emacs, and the password is then sent to the child, allowing it to terminate correctly.
The regexp has been chosen to match the output of ssh connection prompt with password or with passphrase-protected public key. Feel free to modify it to add more support cases !
This has been tested successfully.
This is a minimal working implementation. Feel free to ask me to improve it, particularly regarding documentation, variable naming, indentation, etc...
Best,
Aymeric Agon-Rambosson
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
You can view, comment on, or merge this pull request online at:
https://github.com/jwiegley/emacs-async/pull/181
Commit Summary
• c36775f Add process filter to prompt for password or passphrase
File Changes
(1 file)
• M dired-async.el (112)
Patch Links:
• https://github.com/jwiegley/emacs-async/pull/181.patch • https://github.com/jwiegley/emacs-async/pull/181.diff
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.*Message ID: @.***>
-- Thierry
The issue can be fixed by storing the needed password in a .authinfo.gpg file, it is what most people do (me included).
My mistake, I was not aware that this mechanism worked for ssh key passphrases. The documentation of auth-source
says that the machine
column in the netrc
file identifies a remote host, either by DNS name or IP. It turns out, I've just tested, that it can also identify hosts aliases as specified in a ~/.ssh/config
file, and use the password specified in the relevant column not as a password to send to the host, but as a passphrase to decrypt the private key with. This behaviour is not trivial to implement, and is not documented, so I was (happily) surprised to see it work.
Once you know how to fix this issue, is it worth the effort fixing it?
Well, it turns out that the process filter mechanism is quite straightforward to use, so the effort was minimal.
Still, I would argue that providing some extra mechanism to authenticate interactively, and independent on the preparation in advance of a ~/.authinfo.gpg
file, wouldn't hurt. It would make the package a little bit more "works-out-of-the-box".
I'm happy either way.
Best,
Aymeric
aagon @.***> writes:
The issue can be fixed by storing the needed password in a .authinfo.gpg file, it is what most people do (me included).
My mistake, I was not aware that this mechanism worked for ssh key passphrases. The documentation of auth-source says that the machine column in the netrc file identifies a remote host, either by DNS name or IP. It turns out, I've just tested, that it can also identify hosts aliases as specified in a ~/.ssh/config file, and use the password specified in the relevant column not as a password to send to the host, but as a passphrase to decrypt the private key with. This behaviour is not trivial to implement, and is not documented, so I was (happily) surprised to see it work.
Once you know how to fix this issue, is it worth the effort fixing it?
Well, it turns out that the process filter mechanism is quite straightforward to use, so the effort was minimal.
Still, I would argue that providing some extra mechanism to authenticate interactively, and independent on the preparation in advance of a ~/.authinfo.gpg file, wouldn't hurt. It would make the package a little bit more "works-out-of-the-box".
Ok, so I will make some comments in your PR and when all is solved we could merge it.
Thanks.
I'm happy either way.
Best,
Aymeric
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.*Message ID: @.***>
-- Thierry
Also, I just tried now and after beeing prompted for password the async process hangs forever (tried with sudo method).
Also, I just tried now and after beeing prompted for password the async process hangs forever (tried with sudo method).
I could not reproduce this with emacs -Q
. Can you give me the content of the buffer *emacs*
when this happens ?
After some attempts to make your patch working, I found that the process hangs after I am asked to save password to authinfo file, so it we prevent auth-source asking for this, it works.
I did this by let binding auth-source-save-behavior
to nil in the child emacs.
+ (setq proc
+ (async-start `(lambda ()
+ (require 'cl-lib) (require 'dired-aux) (require 'dired-x)
+ ,(async-inject-variables dired-async-env-variables-regexp)
+ (advice-add #'files--ask-user-about-large-file
+ :override (lambda (&rest args) nil))
+ (let ((dired-recursive-copies (quote always))
+ (dired-copy-preserve-time
+ ,dired-copy-preserve-time)
+ (dired-create-destination-dirs ',create-dir)
+ auth-source-save-behavior)
[...]
I remember now I am doing the same thing in helm-find-files for the rsync action. https://github.com/emacs-helm/helm/blob/master/helm-files.el#L1415 To prevent having possible Ctrl chars in prompt I use:
(when (string-match tramp-password-prompt-regexp string)
(process-send-string proc (concat (read-passwd (match-string 0 string)) "\n")))
Probably we should do the same.
Also we shoud insert string after the process-mark as described in manual, this to ensure the text is inserted correctly in case it comes in small chunks of text (probably not the case now with emacs process but never know.
(defun dired-async--process-filter (proc string)
"`PROC' Child Emacs process filter.
For now, it simply tries to recognise password/passphrase prompts."
(when (string-match tramp-password-prompt-regexp string)
(process-send-string proc (concat (read-passwd (match-string 0 string)) "\n")))
(with-current-buffer (process-buffer proc)
(let ((moving (= (point) (process-mark proc))))
(save-excursion
;; Insert the text, advancing the process marker.
(goto-char (process-mark proc))
(insert string)
(set-marker (process-mark proc) (point)))
(when moving
(goto-char (process-mark proc))))))
Also can you fix the docstring of dired-async--process-filter? Specially this line: `PROC' Child Emacs process filter.
Thanks.
Something like this?
(defun dired-async--process-filter (proc string)
"Process filter for `dired-async-create-files'.
Insert STRING in PROC buffer while PROC is running.
When STRING match `tramp-password-prompt-regexp' prompt user in the
parent emacs for password."
Also we have to declare tramp-password-prompt-regexp
to shutup byte compiler.
I finally end up with a much simpler patch, can you try it?
From 6e7fc1b8ad65abb1af5781f83522f67a8c3b9a38 Mon Sep 17 00:00:00 2001
From: Thierry Volpiatto <thievol@posteo.net>
Date: Wed, 22 Nov 2023 07:56:39 +0100
Subject: [PATCH] Allow passing tramp password to child emacs Fix issue #181
---
async.el | 4 ++++
dired-async.el | 3 ++-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/async.el b/async.el
index e181773..98b2b74 100644
--- a/async.el
+++ b/async.el
@@ -68,6 +68,8 @@ been setup so any async code can load libraries you expect.")
;; For emacs<29 (only exists in emacs-29+).
(defvar print-symbols-bare)
+(defvar tramp-password-prompt-regexp)
+
(defun async--purecopy (object)
"Remove text properties in OBJECT.
@@ -219,6 +221,8 @@ lasts complete line. Every time we get new input, we try to look
for newline, and if found, process the entire line and bump the
marker position to the end of this next line."
(with-current-buffer (process-buffer proc)
+ (when (string-match tramp-password-prompt-regexp string)
+ (process-send-string proc (concat (read-passwd (match-string 0 string)) "\n")))
(goto-char (point-max))
(save-excursion
(insert string))
diff --git a/dired-async.el b/dired-async.el
index 81ed134..7ae9942 100644
--- a/dired-async.el
+++ b/dired-async.el
@@ -382,7 +382,8 @@ ESC or `q' to not overwrite any of the remaining files,
(let ((dired-recursive-copies (quote always))
(dired-copy-preserve-time
,dired-copy-preserve-time)
- (dired-create-destination-dirs ',create-dir))
+ (dired-create-destination-dirs ',create-dir)
+ auth-source-save-behavior)
(setq overwrite-backup-query nil)
;; Inline `backup-file' as long as it is not
;; available in emacs.
--
2.34.1
I applied all your remarks from yesterday evening. I have tried, everything seems to work.
But careful, I do not end up with the same patch as you just sent me this morning, we must have different commits as reference.
The patch I end up with is the following, to be applied to 3bade0e92e1ee8e716c5db14bc8315b17299f138 (so exactly the content of the PR) :
diff --git a/async.el b/async.el
index e181773..d67cdaf 100644
--- a/async.el
+++ b/async.el
@@ -68,6 +68,8 @@ been setup so any async code can load libraries you expect.")
;; For emacs<29 (only exists in emacs-29+).
(defvar print-symbols-bare)
+(defvar tramp-password-prompt-regexp)
+
(defun async--purecopy (object)
"Remove text properties in OBJECT.
diff --git a/dired-async.el b/dired-async.el
index 81ed134..ec36838 100644
--- a/dired-async.el
+++ b/dired-async.el
@@ -250,6 +250,23 @@ Same as `abort-if-file-too-large' but without user-error."
(files--ask-user-about-large-file
size op-type filename nil)))
+(defun dired-async--process-filter (proc string)
+ "Process filter for `dired-async-create-files'.
+Insert STRING in PROC buffer while PROC is running.
+When STRING match `tramp-password-prompt-regexp' prompt user in the
+parent emacs for password."
+ (when (string-match tramp-password-prompt-regexp string)
+ (process-send-string proc (concat (read-passwd (match-string 0 string)) "\n")))
+ (with-current-buffer (process-buffer proc)
+ (let ((moving (= (point) (process-mark proc))))
+ (save-excursion
+ ;; Insert the text, advancing the process marker.
+ (goto-char (process-mark proc))
+ (insert string)
+ (set-marker (process-mark proc) (point)))
+ (when moving
+ (goto-char (process-mark proc))))))
+
(defvar overwrite-query)
(defun dired-async-create-files (file-creator operation fn-list name-constructor
&optional _marker-char)
@@ -259,7 +276,7 @@ See `dired-create-files' for the behavior of arguments."
(setq overwrite-query nil)
(let ((total (length fn-list))
failures async-fn-list skipped callback
- async-quiet-switch create-dir)
+ async-quiet-switch create-dir proc)
(let (to)
(dolist (from fn-list)
(setq to (funcall name-constructor from))
@@ -373,52 +390,54 @@ ESC or `q' to not overwrite any of the remaining files,
'always))))))
;; Start async process.
(when async-fn-list
- (process-put
- (async-start `(lambda ()
- (require 'cl-lib) (require 'dired-aux) (require 'dired-x)
- ,(async-inject-variables dired-async-env-variables-regexp)
- (advice-add #'files--ask-user-about-large-file
- :override (lambda (&rest args) nil))
- (let ((dired-recursive-copies (quote always))
- (dired-copy-preserve-time
- ,dired-copy-preserve-time)
- (dired-create-destination-dirs ',create-dir))
- (setq overwrite-backup-query nil)
- ;; Inline `backup-file' as long as it is not
- ;; available in emacs.
- (defalias 'backup-file
- ;; Same feature as "cp -f --backup=numbered from to"
- ;; Symlinks are copied as file from source unlike
- ;; `dired-copy-file' which is same as cp -d.
- ;; Directories are omitted.
- (lambda (from to ok)
- (cond ((file-directory-p from) (ignore))
- (t (let ((count 0))
- (while (let ((attrs (file-attributes to)))
- (and attrs (null (nth 0 attrs))))
- (cl-incf count)
- (setq to (concat (file-name-sans-versions to)
- (format ".~%s~" count)))))
- (condition-case err
- (copy-file from to ok dired-copy-preserve-time)
- (file-date-error
- (dired-log "Can't set date on %s:\n%s\n" from err)))))))
- ;; Now run the FILE-CREATOR function on files.
- (cl-loop with fn = (quote ,file-creator)
- for (from . dest) in (quote ,async-fn-list)
- do (condition-case err
- (funcall fn from dest t)
- (file-error
- (dired-log "%s: %s\n" (car err) (cdr err))
- nil)))
- (when (get-buffer dired-log-buffer)
- (dired-log t)
- (with-current-buffer dired-log-buffer
- (write-region (point-min) (point-max)
- ,dired-async-log-file))))
- ,(dired-async-maybe-kill-ftp))
- callback)
- 'dired-async-process t)
+ (setq proc
+ (async-start `(lambda ()
+ (require 'cl-lib) (require 'dired-aux) (require 'dired-x)
+ ,(async-inject-variables dired-async-env-variables-regexp)
+ (advice-add #'files--ask-user-about-large-file
+ :override (lambda (&rest args) nil))
+ (let ((dired-recursive-copies (quote always))
+ (dired-copy-preserve-time
+ ,dired-copy-preserve-time)
+ (dired-create-destination-dirs ',create-dir)
+ auth-source-save-behavior)
+ (setq overwrite-backup-query nil)
+ ;; Inline `backup-file' as long as it is not
+ ;; available in emacs.
+ (defalias 'backup-file
+ ;; Same feature as "cp -f --backup=numbered from to"
+ ;; Symlinks are copied as file from source unlike
+ ;; `dired-copy-file' which is same as cp -d.
+ ;; Directories are omitted.
+ (lambda (from to ok)
+ (cond ((file-directory-p from) (ignore))
+ (t (let ((count 0))
+ (while (let ((attrs (file-attributes to)))
+ (and attrs (null (nth 0 attrs))))
+ (cl-incf count)
+ (setq to (concat (file-name-sans-versions to)
+ (format ".~%s~" count)))))
+ (condition-case err
+ (copy-file from to ok dired-copy-preserve-time)
+ (file-date-error
+ (dired-log "Can't set date on %s:\n%s\n" from err)))))))
+ ;; Now run the FILE-CREATOR function on files.
+ (cl-loop with fn = (quote ,file-creator)
+ for (from . dest) in (quote ,async-fn-list)
+ do (condition-case err
+ (funcall fn from dest t)
+ (file-error
+ (dired-log "%s: %s\n" (car err) (cdr err))
+ nil)))
+ (when (get-buffer dired-log-buffer)
+ (dired-log t)
+ (with-current-buffer dired-log-buffer
+ (write-region (point-min) (point-max)
+ ,dired-async-log-file))))
+ ,(dired-async-maybe-kill-ftp))
+ callback))
+ (process-put proc 'dired-async-process t)
+ (set-process-filter proc 'dired-async--process-filter)
;; Run mode-line notifications while process running.
(dired-async--modeline-mode 1)
(message "%s proceeding asynchronously..." operation))))
aagon @.***> writes:
- ( ) multipart/mixed (*) text/html
I applied all your remarks from yesterday evening. I have tried, everything seems to work.
But careful, I do not end up with the same patch as you just sent me this morning, we must have different commits as reference.
The patch I end up with is the following, to be applied to 3bade0e (so exactly the content of the PR) :
Thanks, can you try the last patch, it does the same like yours but it is 6 lines changes, much simpler, we don't have to define a filter fn as we have one already.
We will have also to check tramp-password-prompt-regexp to avoid failures if tramp is not already loaded.
-- Thierry
@aagon I pushed the last patch to master, have a look at it. Thanks.
Thanks, can you try the last patch, it does the same like yours but it is 6 lines changes, much simpler, we don't have to define a filter fn as we have one already.
Sorry about that, I did not read your patch correctly. I had not seen that a filter was already defined in async.el
.
I pushed the last patch to master, have a look at it.
It prompts correctly and the child is not blocked, so it seems to be working.
We will have also to check tramp-password-prompt-regexp to avoid failures if tramp is not already loaded.
Like this ?
diff --git a/async.el b/async.el
index a741932..f1359c8 100644
--- a/async.el
+++ b/async.el
@@ -220,6 +220,8 @@ lasts complete line. Every time we get new input, we try to look
for newline, and if found, process the entire line and bump the
marker position to the end of this next line."
(with-current-buffer (process-buffer proc)
+ (unless (boundp 'tramp-password-prompt-regexp)
+ (require 'tramp))
(when (string-match tramp-password-prompt-regexp string)
(process-send-string proc (concat (read-passwd (match-string 0 string)) "\n")))
(goto-char (point-max))
But I am not sure that we need this. We have two cases :
async-start-process
has nothing to do with a tramp operation, we require tramp anyway if it is not already loaded, despite the fact that it is not needed.async-start-process
is a tramp file operation, which means the user has entered a tramp path in the minibuffer at some point (or something else to the same effect), which I assume means that tramp has been loaded already by the time the filter is evaluated.If this last assumption is true, then we don't have to add this.
aagon @.***> writes:
But I am not sure that we need this. We have two cases :
• The operation started with async-start-process has nothing to do with a tramp operation, we require tramp anyway if it is not already loaded, despite the fact that it is not needed. • The operation started with async-start-process is a tramp file operation, which means the user has entered a tramp path in the minibuffer at some point (or something else to the same effect), which I assume means that tramp has been loaded already by the time the filter is evaluated.
If this last assumption is true, then we don't have to add this.
You are right, but we have to handle anyway the first case to not fail with an error with string-match, so perhaps not requiring tramp and using:
(when (and (boundp 'tramp-password-prompt-regexp)
(string-match tramp-password-prompt-regexp string))
(process-send-string proc (concat (read-passwd (match-string 0 string)) "\n")))
is the best option.
NOTE: I have already pushed the last patch to master.
-- Thierry
NOTE: I have already pushed the last patch to master.
Perfect, thank you !
Closing now.
Unfortunately, this change is causing problems for me in conjunction with org-wild-notifier. That package expects to be able to output sexps (which contain org-mode headings and so on) from the child Emacs and have them be read by the parent process.
One of my daily org-mode tasks includes the phrase "check shopping list", of which pin
is a substring. Later, there is a timestamp which contains a :
character. As a result, the sexp representing my org-mode agenda tasks matches tramp-password-prompt-regexp
, and Emacs starts randomly prompting me in the minibuffer with some hideously large sexp string once every minute.
Can this password prompt scanning behavior be made configurable instead, perhaps? I acknowledge it is very useful, but it also seems useful to have async functions be able to return arbitrary data unimpeded, and without some awkward escaping or encoding. (It seems that, currently, "messages" are base64-encoded, but "return values" are not.)
Alternately, should the child process instead set up something on minibuffer-setup-hook
and explicitly forward any minibuffer prompts to the parent process, avoiding this regexp-based scanning altogether?
Aaron Zeng @.***> writes:
Can this password prompt scanning behavior be made configurable instead, perhaps? I acknowledge it is very useful, but it also seems useful to have async functions be able to return arbitrary data unimpeded, and without some awkward escaping or encoding. (It seems that, currently, "messages" are base64-encoded, but "return values" are not.)
Done, the var is async-prompt-for-password
.
Alternately, should the child process instead set up something on minibuffer-setup-hook and explicitly forward any minibuffer prompts to the parent process, avoiding this regexp-based scanning altogether?
PR welcome ;-)
Thanks.
-- Thierry
I feel like a user customization is an awkward way to configure this. Shouldn't this be an argument to async-start
, or perhaps a locally bound defvar? Each package may have different needs depending on what it uses async.el
for, and the package author/code writer would best equipped to know whether there are possible password prompts. It feels weird to say "every user of org-wild-notifier.el
should set the customization to nil, but unfortunately it will break X other package".
Aaron Zeng @.***> writes:
I feel like a user customization is an awkward way to configure this. Shouldn't this be an argument to async-start, or perhaps a locally bound defvar? Each package may have different needs depending on what it uses async.el for, and the package author/code writer would best equipped to know whether there are possible password prompts. It feels weird to say "every user of org-wild-notifier.el should set the customization to nil, but unfortunately it will break X other package".
Perhaps you can let-bind tramp-password-prompt-regexp or set it locally? Note: Be sure to update async package otherwise you will have an error if you bind tramp-password-prompt-regexp to nil.
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.*Message ID: @.***>
-- Thierry
Hmm, it's not really effective to use let-binding, since the moment in time we care about the variable's value is when the process filter is running, which is after any local let-binding would have ended.
Setting it buffer-locally somehow in the process's buffer might work, though.
Actually, though, if the whole problem is that the child sends a non-escaped sexp as the return value (from start-func) to the parent, maybe it should just be escaping it like it does for messages?
I submitted #182 just now which makes the suggested workaround—let-binding async-prompt-for-password
to nil at the time of calling async-start
—work correctly.
Aaron Zeng @.***> writes:
I submitted #182 just now which makes the suggested workaround—let-binding async-prompt-for-password to nil at the time of calling async-start—work correctly.
I didn't have the time to look into this, sorry.
However let-binding a local var is generally not recommended:
Making a variable buffer-local within a ‘let’-binding for that
variable does not work reliably, unless the buffer in which you do
this is not current either on entry to or exit from the ‘let’.
This is because ‘let’ does not distinguish between different kinds
of bindings; it knows only which variable the binding was made for.
Don't know if this recommendation apply to your code though.
Thanks.
-- Thierry
Thanks for the pointer. The recommendation does not apply to my code, since the buffer is created inside the let binding (during async-start) and therefore not current upon entry to the let. (It also happens to be not current on exit from the let).
Aaron Zeng @.***> writes:
Thanks for the pointer. The recommendation does not apply to my code, since the buffer is created inside the let binding (during async-start) and therefore not current upon entry to the let. (It also happens to be not current on exit from the let).
Sorry for late reply.
I would like to be sure to understand the issue before installing something. IIUC in your code you let-bind async-prompt-for-password and call async-start like:
(let (async-prompt-for-password)
(async-start [...]))
Is this patch working for you?
diff --git a/async.el b/async.el index a42085e..49f9e46 100644 --- a/async.el +++ b/async.el @@ -215,7 +215,7 @@ It is intended to be used as follows: (process-name proc) (process-exit-status proc)))) (set (make-local-variable 'async-callback-value-set) t))))))
-(defun async-read-from-client (proc string) +(defun async-read-from-client (proc string &optional pwd-prompt) "Process text from client process.
The string chunks usually arrive in maximum of 4096 bytes, so a @@ -227,7 +227,7 @@ lasts complete line. Every time we get new input, we try to look for newline, and if found, process the entire line and bump the marker position to the end of this next line." (with-current-buffer (process-buffer proc)
(set-process-sentinel proc #'async-when-done)
-- Thierry
Yes, I am let-binding the variable like that, and the patch you just posted works for me too. If you prefer to implement it that way and close #182 that is fine with me as well, thanks!
Aaron Zeng @.***> writes:
Yes, I am let-binding the variable like that, and the patch you just posted works for me too. If you prefer to implement it that way and close #182 that is fine with me as well, thanks!
Ok, thanks for testing and explaining, I will close #182 and install this patch. Thanks again!
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.*Message ID: @.***>
-- Thierry
At the moment, when the child emacs process needs to prompt the user for a password or a passphrase (for tramp, for instance), it hangs for ever.
This PR adds a very simple process filter and attaches it to the child process. I have taken inspiration from packages
dired-rsync
andmu4e
, that also have to deal with child processes. It turns out that a process filter function is the canonical way to deal with prompts in child processes.The process filter simply looks for a regular expression in the output of the child process. It it matches, the user is prompted in the parent emacs, and the password is then sent to the child, allowing it to terminate correctly.
The regexp has been chosen to match the output of ssh connection prompt with password or with passphrase-protected public key. Feel free to modify it to add more support cases !
This has been tested successfully.
This is a minimal working implementation. Feel free to ask me to improve it, particularly regarding documentation, variable naming, indentation, etc...
Best,
Aymeric Agon-Rambosson