The #:port seems to be ignored #38

graywolf · 2024-04-06T17:09:56Z

Hello,

I am trying to deploy using guix deploy (which under the hood uses guile-ssh), and it seems the #:port argument to (make-session) is ignored. I managed to reproduce it even without guix, the steps follow.

I have this in my ~/.ssh/config:

Host *.foo.bar
  Port 2222

However, on specific host I am deploying to the sshd is still running on 22. I can connect fine using the ssh command (ssh -4 -p22 ...). Using strace I see this line:

connect(3, {sa_family=AF_INET, sin_port=htons(22), sin_addr=inet_addr("xxx.xxx.xxx.xxx")}, 16) = 0

However when I try to do the same using guile-ssh:

$ guix shell guile guile-ssh strace -- strace guile -c '(use-modules (ssh session)) (define session (make-session #:host "pepe.foo.bar" #:user "root" #:log-verbosity '\''functions #:port 22)) (connect! session)' 2>&1 | grep htons

I see this instead:

connect(5, {sa_family=AF_INET, sin_port=htons(2222), sin_addr=inet_addr("xxx.xxx.xxx.xxx")}, 16) = 0

Notice that sin_port is set to the value from ~/.ssh/config instead of what I passed in #:port.

The text was updated successfully, but these errors were encountered:

artyom-poptsov · 2024-04-20T18:28:00Z

Hello!

Thank you for the bug report.

I investigated the problem I came to conclusion that it's because currently the config file is being read after the options are set:

guile-ssh/modules/ssh/session.scm

Line 111 in 7b27597

(%gssh-session-parse-config! session config))

So even though you provide #:port option to make-session and it set port it will be overwritten by the port number from the configuration file.

I think that we must read the config first and then set the provided options.

I'll see what I can do.

-avp

artyom-poptsov · 2024-04-20T18:30:29Z

As a workaround you can try to set the #:config option to #f so the configuration file will not be read:

guile-ssh/modules/ssh/session.scm

Line 105 in 7b27597

(when config

'make-session' would always overwrite the explicitly set options passed by keywords with the values from the SSH configuration file. That lead to unexpected behavior. This patch fixes this error. Reported by graywolf in <#38> * modules/ssh/session.scm (make-session): Bugfix: Set the '#:host' option first, then read the SSH configuration and only after that set the other options.

artyom-poptsov · 2024-05-01T14:50:39Z

The issue is fixed in the latest release.

Thanks!

graywolf · 2024-05-03T18:50:18Z

Hello, thanks for the fix, however I am not sure it works.

I have ran it with guix shell --with-branch=guile-ssh=master guile guile-ssh strace to make sure I have the latest version, and I am still getting the port 2222. I did put together complete reproducer (using guix):

$ printf 'host example.org\nport 2222\n' >/tmp/test-ssh-config
$ guix shell -CN --with-branch=guile-ssh=master --expose=/tmp/test-ssh-config=/home/$USER/.ssh/config guile guile-ssh strace -- strace guile -c '(use-modules (ssh session)) (define session (make-session #:host "example.org" #:user "root" #:log-verbosity '\''functions #:port 22)) (connect! session)' 2>&1 | grep htons
connect(5, {sa_family=AF_INET, sin_port=htons(53), sin_addr=inet_addr("192.168.0.1")}, 16) = 0
recvfrom(5, "\257\316\201\200\0\1\0\1\0\0\0\0\7example\3org\0\0\1\0\1\300\f\0"..., 2048, 0, {sa_family=AF_INET, sin_port=htons(53), sin_addr=inet_addr("192.168.0.1")}, [28 => 16]) = 45
recvfrom(5, "\3717\201\200\0\1\0\1\0\0\0\0\7example\3org\0\0\34\0\1\300\f\0"..., 65536, 0, {sa_family=AF_INET, sin_port=htons(53), sin_addr=inet_addr("192.168.0.1")}, [28 => 16]) = 57
connect(5, {sa_family=AF_INET, sin_port=htons(2222), sin_addr=inet_addr("93.184.215.14")}, 16) = 0
getsockname(5, {sa_family=AF_INET, sin_port=htons(45431), sin_addr=inet_addr("10.11.0.28")}, [28 => 16]) = 0
connect(5, {sa_family=AF_INET6, sin6_port=htons(2222), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "2606:2800:21f:cb07:6820:80da:af6b:8b2c", &sin6_addr), sin6_scope_id=0}, 28) = 0
getsockname(5, {sa_family=AF_INET6, sin6_port=htons(34785), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "fdda:d0d0:cafe:1197::101a", &sin6_addr), sin6_scope_id=0}, [28]) = 0
connect(5, {sa_family=AF_INET, sin_port=htons(2222), sin_addr=inet_addr("93.184.215.14")}, 16) = -1 EINPROGRESS (Operation now in progress)

As you can see, it still tries to connect to 2222.

Also, looking closer at the code, the `config' should be #f anyway, so the whole moved block (when config ...) should not even trigger. And when I check the strace output, I can see the ~/.ssh/config file being opened for reading. So it is probably coming from somewhere else?

The commit is (in my opinion) still desired for situations with config being non-#f, but it does not seem to address this particular issue.

Or I am doing something wrong, also possible.

EDIT: When I pass #:config "/dev/null" into make-session, it starts to use the 22 port.

graywolf · 2024-05-03T19:03:06Z

I think https://github.com/libssh/libssh-mirror/blob/ac6d2fad4a8bf07277127736367e90387646363f/src/options.c#L1472 is the cause why #:config "/dev/null" causes it to work. Looking at https://api.libssh.org/master/group__libssh__session.html#ga7a801b85800baa3f4e16f5b47db0a73d I think #:config #f should imply SSH_OPTIONS_PROCESS_CONFIG being set to 0. Which is currently not possible as far as I can tell.

graywolf · 2024-05-03T19:06:17Z

diff --git a/libguile-ssh/session-func.c b/libguile-ssh/session-func.c
index 7006b62..81fa227 100644
--- a/libguile-ssh/session-func.c
+++ b/libguile-ssh/session-func.c
@@ -55,6 +55,7 @@ static gssh_symbol_t session_options[] = {
   { "knownhosts",         SSH_OPTIONS_KNOWNHOSTS         },
   { "timeout",            SSH_OPTIONS_TIMEOUT            },
   { "timeout-usec",       SSH_OPTIONS_TIMEOUT_USEC       },
+  { "process-config",     SSH_OPTIONS_PROCESS_CONFIG     },
   { "ssh1",               SSH_OPTIONS_SSH1               },
   { "ssh2",               SSH_OPTIONS_SSH2               },
   { "log-verbosity",      SSH_OPTIONS_LOG_VERBOSITY      },
@@ -380,6 +381,7 @@ set_option (SCM scm_session, gssh_session_t* sd, int type, SCM value)
     case SSH_OPTIONS_TIMEOUT_USEC:
       return set_uint64_opt (session, type, value);
 
+    case SSH_OPTIONS_PROCESS_CONFIG:
     case SSH_OPTIONS_SSH1:
     case SSH_OPTIONS_SSH2:
     case SSH_OPTIONS_STRICTHOSTKEYCHECK:
diff --git a/modules/ssh/session.scm b/modules/ssh/session.scm
index 4aaf5d3..f80925b 100644
--- a/modules/ssh/session.scm
+++ b/modules/ssh/session.scm
@@ -82,7 +82,8 @@ Return a new SSH session."
 
     (session-set-if-specified! host)
 
-    (when config
+    (if config
+(begin
       (or host
           (throw 'guile-ssh-error
                  "'config' is specified, but 'host' option is missed."))
@@ -93,6 +94,8 @@ Return a new SSH session."
         (%gssh-session-parse-config! session #f))
        (else
         (throw 'guile-ssh-error "Wrong 'config' value" config))))
+(session-set! session 'process-config #f)
+)
 
     (session-set-if-specified! port)
     (session-set-if-specified! user)

This seems to work? Sorry for the formatting, mg does not have the best scheme support.

artyom-poptsov · 2024-05-05T08:32:18Z

As far as I can tell process-config and #f passed as the value for config option must have different meaning. When config is set to #f it means that the default user configuration file (~/.ssh/config) must be read. On the other hand, setting process-config (SSH_OPTIONS_PROCESS_CONFIG) to #f means that no configuration files should be read at all (including the system config):

SSH_OPTIONS_PROCESS_CONFIG Set it to false to disable automatic processing of per-user and system-wide OpenSSH configuration files. LibSSH automatically uses these configuration files unless you provide it with this option or with different file (bool).

As such, having process-config option in Guile-SSH is desirable indeed, but I think that it does not solve the problem described in this issue.

But your attempt to solve the problem helped me to find another bug: currently there's no way to set config to #f in libssh as when config is false in make-session all the config handling code is skipped. I'll fix it in the next release.

What is interesting with current situation is that when config option is set for make-session the configuration file should be read before setting other options thus options set after config is read should overwrite the options from the config. But according to your observations we don't see this behavior.

graywolf · 2024-05-05T12:26:47Z

As far as I can tell process-config and #f passed as the value for config option must have different meaning. When config is set to #f it means that the default user configuration file (~/.ssh/config) must be read. On the other hand, setting process-config (SSH_OPTIONS_PROCESS_CONFIG) to #f means that no configuration files should be read at all (including the system config):

Albeit that might be the intention, it is not obvious from the documentation:

     ‘config’
          The option specifies whether an SSH config should be parsed or
          not, and optionally the path to a config file.

          Setting the VALUE to ‘#t’ means that the default
          ‘~/.ssh/config’ should be parsed; in turn, setting the option
          to ‘#f’ (the default value) means that the config should not
          be parsed at all.  If the value is a string, then the string
          is expected to be a path to config file.

          The procedure reads the config file after all other specified
          options are set.  When the config file is read, the options
          for SESSION are set, overwriting those that were passed to the
          procedure.

          You _must_ specify at least a host name when using this
          option, otherwise the procedure will fail.

          Optionally you could use ‘session-parse-config!’ procedure
          explicitly to read the config (see below.)

          Expected types of VALUE: Either a string or a boolean value.

My understanding of in turn, setting the option to ‘#f’ (the default value) means that the config should not be parsed at all. is that by default (== #f), no configuration file (not even the default one) will be parsed.

One option I guess would be to change the default value to #t and apply some variation of the patch above.

PS: The documentation snippet above still says

          The procedure reads the config file after all other specified
          options are set.  When the config file is read, the options
          for SESSION are set, overwriting those that were passed to the
          procedure.

should I open another bug? I feel like I am spamming you enough already. :)

What is interesting with current situation is that when config option is set for make-session the configuration file should be read before setting other options thus options set after config is read should overwrite the options from the config. But according to your observations we don't see this behavior.

This seems to be a misunderstanding, as I said above: When I pass #:config "/dev/null" into make-session, it starts to use the 22 port. So passing in any config, even /dev/null is enough to suppress the reading of the default one.

artyom-poptsov · 2024-05-05T16:14:51Z

Albeit that might be the intention, it is not obvious from the documentation:

Okay, that's a good point. The documentation needs to be fixed.

But maybe we can improve the API as follows:

#:process-config #f disables reading of any configuration files altogether as it works in libssh.
#:config "/dev/null" or #:config #f means the same thing -- no user configuration file should be read.
#:config 'default reads the default user configuration file (~/.ssh/config.)
#:config "/path/to/config" specifies a config file to read.

What do you think?

PS: The documentation snippet above still says

Yeah, it seems that was my intention back then when I implemented make-session. But you're right, the options passed to make-session should overwrite the options from the config, it is more logical.

should I open another bug? I feel like I am spamming you enough already. :)

I think it's fine to use this bug to discuss the documentation changes related to make-session as well. Thanks for helping!

BTW, I just fixed (d56fb10) a bug in a test that checks if options passed to make-session overwrite options from the user configuration. The test looks now as follows:

(test-equal "make-session: keywords must overwrite config options"
  22
  (let ((s (make-session #:host   "example"
                         #:port   22
                         ;; Configuration sets port to 2222
                         #:config %config)))
    (session-get s 'port)))

The configuration file contains port number 2222 but #:port overwrites this value with 22 as it should so the test passes fine.

graywolf · 2024-05-06T17:31:47Z

On 2024-05-05 09:15:12 -0700, Artyom V. Poptsov wrote: > Albeit that might be the intention, it is not obvious from the documentation: Okay, that's a good point. The documentation needs to be fixed. But maybe we can improve the API as follows: - `#:process-config #f` disables reading of any configuration files altogether as it works in libssh.

What will be the behavior when called with `#:process-config #f #:config 'default`? I wonder whether we actually need separate keyword (#:process-config) or the #:config would cover everything via the cases below. Naming nitpick: I am pretty new to Guile, but should the name not be `#:process-config?`?

- `#:config "/dev/null"` or `#:config #f` means the same thing -- no user configuration file should be read.

It is not obvious from this proposal whether that is the case, so I will just note that I do not think string "/dev/null" should have a special meaning. It will work to suppress the configuration reading, same like it does now, but should not get special handling.

- `#:config 'default` reads the default user configuration file (`~/.ssh/config`.)

I personally would prefer #t to 'default, it is shorter. But if you prefer the symbol, maybe at least both ('default and #t) could be allowed? I guess for backward compatibility this needs to be default (no pun intended) value of #:config. Do you think someone passes in #f explicitly in their code?

- `#:config "/path/to/config"` specifies a config file to read. What do you think?

I like it ^_^ Just the comments above.

> PS: The documentation snippet above still says Yeah, it seems that was my intention back then when I implemented `make-session`. But you're right, the options passed to `make-session` should overwrite the options from the config, it is more logical. > should I open another bug? I feel like I am spamming you enough already. :) I think it's fine to use this bug to discuss the documentation changes related to `make-session` as well. Thanks for helping!

You are welcome. More like, thank you for writing this library. I was putting together a Guile-based server installer script and guile-ssh was of immense help. Cheers, Tomas

…

-- There are only two hard things in Computer Science: cache invalidation, naming things and off-by-one errors.

artyom-poptsov · 2024-09-15T07:33:25Z

Hello! Sorry for the late answer.

I guess for backward compatibility this needs to be default (no pun intended)
value of #:config. Do you think someone passes in #f explicitly in their code?

Agreed, we should keep backward compatibility where it's possible.

But currently there's a problem in config-handling code in Guile-SSH:

Calling %gssh-session-parse-config! with #f in the place of the config tells libssh to read default configuration files.
In the meantime when config is set to #f in make-session then %gssh-session-parse-config! is not called at all, leading to some default internal libssh behavior. So there's no way to call %gssh-session-parse-config! with #f.

So maybe we should go with the following:

#:config #f means that no user configuration file should be read (as per Guile-SSH documentation.) Internally we'll set process-config? option in libssh to false. In the same time #:config "/dev/null" will be passed to libssh without any special handling as it works now. Thus it would be a bugfix in the code so it will match the description in the documentation.
#:config #t reads the default user configuration file (~/.ssh/config.) We should use it by default for the backward compatibility.
#:config "/path/to/config" specifies a config file to read (no changes in the API.)

That means that instead of

    (when config
      (or host
          (throw 'guile-ssh-error
                 "'config' is specified, but 'host' option is missed."))
      (cond
       ((string? config)
        (%gssh-session-parse-config! session config))
       ((boolean? config)
        (%gssh-session-parse-config! session #f))
       (else
        (throw 'guile-ssh-error "Wrong 'config' value" config))))

we will do something like this (as you proposed):

    (if config
        (begin
          (or host
              (throw 'guile-ssh-error
                     "'config' is specified, but 'host' option is missed."))
          (cond
           ((string? config)
            (%gssh-session-parse-config! session config))
           ((boolean? config)  ; set to #t
            (%gssh-session-parse-config! session #f))
           (else
            (throw 'guile-ssh-error "Wrong 'config' value" config))))
        (session-set! session 'process-config? #f))

Does this sound right?

By the way, I see that you've been quite active in Guile-SSH recently with several useful bug reports. Do you want to participate in Guile-SSH development more directly? You could start with a pull request to one of your issues, and then we'll see -- maybe I could give you the commit access.

Thanks!
-avp

This patch fixes "make-session" procedure to make it handle "#:config" option according to the Guile-SSH documentation. That is, passing "#f" as the "#:config" value disables reading the default SSH documentation. Reported by graywolf in <#38> * modules/ssh/session.scm (make-session): Fix "#:config" handling: disable default configuration reading when set to "#f" as per Guile-SSH documentation. Use "#t" as the default value to keep the backward compatibility. * libguile-ssh/session-func.c (session_options): Add SSH_OPTIONS_PROCESS_CONFIG. (set_option): Handle SSH_OPTIONS_PROCESS_CONFIG. * doc/api-sessions.texi: Update. * NEWS: Update.

artyom-poptsov · 2024-09-15T08:33:07Z

I pushed the proposed fix to the branch 38-the-port-seems-to-be-ignored.

-avp

This patch fixes "make-session" procedure to make it handle "#:config" option according to the Guile-SSH documentation. That is, passing "#f" as the "#:config" value disables reading the default SSH documentation. Reported by graywolf in <#38> * modules/ssh/session.scm (make-session): Fix "#:config" handling: disable default configuration reading when set to "#f" as per Guile-SSH documentation. Use "#t" as the default value to keep the backward compatibility. * libguile-ssh/session-func.c (session_options): Add SSH_OPTIONS_PROCESS_CONFIG. (set_option): Handle SSH_OPTIONS_PROCESS_CONFIG. * doc/api-sessions.texi: Update. * NEWS: Update.

* modules/ssh/session.scm (make-session): Don't try to set "parse-config?" option when libssh version is older than 0.9. Log a message and fallback to the old Guile-SSH behavior instead (see <#38>.)

artyom-poptsov · 2024-11-10T17:17:19Z

This issue should be fixed in the new Guile-SSH release.

artyom-poptsov self-assigned this Apr 6, 2024

artyom-poptsov closed this as completed May 1, 2024

artyom-poptsov added the bug label May 3, 2024

artyom-poptsov reopened this May 4, 2024

artyom-poptsov closed this as completed Nov 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The #:port seems to be ignored #38

The #:port seems to be ignored #38

graywolf commented Apr 6, 2024

artyom-poptsov commented Apr 20, 2024 •

edited

Loading

artyom-poptsov commented Apr 20, 2024

artyom-poptsov commented May 1, 2024

graywolf commented May 3, 2024 •

edited

Loading

graywolf commented May 3, 2024 •

edited

Loading

graywolf commented May 3, 2024 •

edited

Loading

artyom-poptsov commented May 5, 2024

graywolf commented May 5, 2024

artyom-poptsov commented May 5, 2024

graywolf commented May 6, 2024 via email

artyom-poptsov commented Sep 15, 2024

artyom-poptsov commented Sep 15, 2024

artyom-poptsov commented Nov 10, 2024

The #:port seems to be ignored #38

The #:port seems to be ignored #38

Comments

graywolf commented Apr 6, 2024

artyom-poptsov commented Apr 20, 2024 • edited Loading

artyom-poptsov commented Apr 20, 2024

artyom-poptsov commented May 1, 2024

graywolf commented May 3, 2024 • edited Loading

graywolf commented May 3, 2024 • edited Loading

graywolf commented May 3, 2024 • edited Loading

artyom-poptsov commented May 5, 2024

graywolf commented May 5, 2024

artyom-poptsov commented May 5, 2024

graywolf commented May 6, 2024 via email

artyom-poptsov commented Sep 15, 2024

artyom-poptsov commented Sep 15, 2024

artyom-poptsov commented Nov 10, 2024

artyom-poptsov commented Apr 20, 2024 •

edited

Loading

graywolf commented May 3, 2024 •

edited

Loading

graywolf commented May 3, 2024 •

edited

Loading

graywolf commented May 3, 2024 •

edited

Loading