Add support for ssh_config for connection #23847

afbjorklund · 2024-09-03T18:39:14Z

Previously had to use podman system connection

Normal ssh://<alias> would fail to work properly

e.g. "default", podman --host ssh://default --ssh native ps

Host default
  HostName 127.0.0.1
  User vagrant
  Port 2222
  UserKnownHostsFile /dev/null
  StrictHostKeyChecking no
  PasswordAuthentication no
  IdentityFile $PWD/.vagrant/machines/default/virtualbox/private_key
  IdentitiesOnly yes
  LogLevel FATAL

Does this PR introduce a user-facing change?

ssh connection URL now supports regular ssh_config

Also adds support for default ssh user and path.

Tested manually with Lima. No unit tests found.

$ limactl start --name podman template://podman
$ ssh -F ~/.lima/podman/ssh.config lima-podman podman

$ limactl show-ssh --format=config podman >>~/.ssh/config
$ CONTAINER_HOST=ssh://lima-podman podman-remote version

Uses https://github.com/kevinburke/ssh_config

Closes #23831
Fixes #17452

packit-as-a-service · 2024-09-03T18:42:48Z

Ephemeral COPR build failed. @containers/packit-build please check.

afbjorklund · 2024-09-03T19:26:58Z

Added required files to the "vendor" directory (by using go1.22.0 mod vendor)

bloat approved

***************************************************************************
* bin/podman grew by 110536 bytes; max allowed is 51200.
*
* Please investigate, and fix if possible.
*
* A repo admin can override by setting the bloat_approved label
***************************************************************************

no new tests

pr-should-include-tests: PR does not include changes in the 'tests' directory

Please write a regression test for what you're fixing. Even if it
seems trivial or obvious, try to add a test that will prevent
regressions.

If your change is minor, feel free to piggyback on already-written
tests, possibly just adding a small step to a similar existing test.
Every second counts in CI.

If your commit really, truly does not need tests, you can proceed
by asking a repo maintainer to set the 'No New Tests' github label.
This will only be done when there's no reasonable alternative.

BTW; There is no "tests" directory, but there is a "test" directory?

podman ssh work, using new c/common interface #15094

The "secure" parameter doesn't seem to do anything anymore?

ssh://<user>@<host>[:port]/run/podman/podman.sock?secure=True

afbjorklund · 2024-09-03T20:08:45Z

This will fix trying to nslookup the alias, but it will not add support for ProxyJump

In order to support that, Podman needs to use more of containers/common SSHMode

Like it said in the original PR #15094 (that added --ssh=native):

Overall, this is a v1 of a long process of offering native ssh

     ProxyCommand
             Specifies the command to use to connect to the server.  The com‐
             mand string extends to the end of the line, and is executed using
             the user's shell ‘exec’ directive to avoid a lingering shell
             process.
             ...

     ProxyJump
             Specifies one or more jump proxies as either [user@]host[:port]
             or an ssh URI.  Multiple proxies may be separated by comma char‐
             acters and will be visited sequentially.  Setting this option
             will cause ssh(1) to connect to the target host by first making a
             ssh(1) connection to the specified ProxyJump host and then estab‐
             lishing a TCP forwarding to the ultimate target from there.
             ...

podman tries to lookup hostname which is used for ssh connection. #21113

The current workaround is to tunnel a temporary unix: socket with a ssh -L command

mheon · 2024-09-05T18:23:20Z

* bin/podman grew by 110536 bytes; max allowed is 51200.

110kb from a 1700 line diffstat, ouch. Bloat approved label added still.

pkg/bindings/connection.go

mheon · 2024-09-05T18:27:27Z

Few nits, looks fine overall. @baude PTAL when you get back.

afbjorklund · 2024-09-05T18:32:16Z

* bin/podman grew by 110536 bytes; max allowed is 51200.

110kb from a 1700 line diffstat, ouch. Bloat approved label added still.

Not sure if there was any obvious culprit, it seemed to be evenly spread...

11714	runtime
1323	os
806	fmt
698	regexp
521	strings
501	strconv
437	unicode
430	bytes
429	sync
366	io
243	os/user
214	path/filepath
73	errors

EDIT: I take that back, the use of "runtime" seems suspicious and bloated

https://github.com/kevinburke/ssh_config/blob/v1.2.0/config.go#L312

                if r := recover(); r != nil {
                        if _, ok := r.(runtime.Error); ok {
                                panic(r)
                        }
                        if e, ok := r.(error); ok && e == ErrDepthExceeded {
                                err = e
                                return
                        }
                        err = errors.New(r.(string))
                }

https://go.dev/wiki/CodeReviewComments#dont-panic
https://go.dev/blog/defer-panic-and-recover

So maybe the bloat checker was onto something anyway?

pkg/bindings/connection.go

Luap99 · 2024-09-06T09:30:09Z

pkg/bindings/connection.go

+		cfg := ssh_config.DefaultUserSettings
+		if val := cfg.Get(alias, "User"); val != "" {
+			userinfo = url.User(val)
+			logrus.Debugf("User: %s", val)


These debug statements are not clear, they need to mention ssh config at least

They could be removed altogether, they were mostly used during implementing...

There is likely value in having them to see what the lib parsed in case of weird user configs

A compromise might be to log the new URL (once), once all the substitution (if any) is done.

Yes logging the final url once sounds good to me.

Wasn't so easy to log the information since _url is not the only source of information used...

But changed it so that it only outputs something, if an alias was actually found in ssh_config.

Luap99 · 2024-09-06T09:38:55Z

pkg/bindings/connection.go

+			if err := session.Run(
+				"podman info --format '{{.Host.RemoteSocket.Path}}'"); err != nil {
+				return nil, err
+			}


This is not a good idea for performance reasons, podman info is slow...
Also podman info has no actual knowledge of the actual API socket path and just guess the /run/podman/podman.sock or /run/user/xxx/podman/podman.sock as rootless so there is no reason to do that as you might as well guess on the client. Of course you would need to get the uid on the server to do this.

I mean alone the extra ssh connection is already slow but I think there is no need to avoid that if we need the uid on the server so maybe the podman info at this point is not to bad either?
Given that is is properly not a good idea to recommend leaving it the socket path if users care about performance.

I think it was more about compatibility/simplicity, and less about performance?

There is already the "system connection", that should be the preferred option...

I don't think it can hardcode the /run/user/$UID either, need to query for $XDG_RUNTIME_DIR.

Most systems with systemd will use the default, but other systems might use another directory...

Yeah I looked at this was like podman info isn't really great for this but thinking about this again given there is no way around opening an extra ssh connection for it so doing the podman info call should not be that big of a deal either.

Eventually it might use the containers/common functionality for this (the "SSHMode").

https://github.com/containers/common/tree/main/pkg/ssh (the "golang" vs "native")

afbjorklund · 2024-09-06T10:03:41Z

Note that if providing a fully qualified URL, this will be a no-op (assuming an empty/small ~/.ssh/config)

export CONTAINER_HOST=ssh://[email protected]:2222/run/user/1000/podman/podman.sock
export CONTAINER_SSHKEY=$PWD/.vagrant/machines/default/virtualbox/private_key

It will only kick in, if there is an "alias" defined or if there are any parameters missing (like user or path)

afbjorklund · 2024-09-06T10:52:18Z

There is more talk about performance in the issue, beyond using a legacy --host=ssh://

Podman over SSH in native mode does not use custom config ~/.ssh/config #23831 (comment)

How to reuse the same ssh connection (and path), and how to choose a faster encryption...

For lima there is a "host agent" that sets up a persistent tunnel, and provides a unix socket

Luap99

Sorry for the delay, I have not forgotten this.

pkg/bindings/connection.go

Luap99 · 2024-09-24T16:29:09Z

Also can you squash your commits please.

packit-as-a-service · 2024-09-27T08:00:30Z

Cockpit tests failed for commit b174663c2f1354c5b3f32c234f780303b177a2ac. @martinpitt, @jelly, @mvollmer please check.

Luap99

LGTM, sorry for the late reply this time it fell of my list.
Can you rebase this please? If you don't have time I can also do that.

@mheon PTAL so we can merge once rebased.

openshift-ci · 2024-10-30T13:08:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Anders F Björklund <[email protected]>

mheon · 2024-10-30T14:30:22Z

/lgtm

openshift-ci bot added the release-note label Sep 3, 2024

afbjorklund force-pushed the ssh-config-main branch from da20322 to 096a372 Compare September 3, 2024 18:42

mheon added bloat_approved Approve a PR in which binary file size grows by over 50k No New Tests Allow PR to proceed without adding regression tests labels Sep 5, 2024

mheon reviewed Sep 5, 2024

View reviewed changes

pkg/bindings/connection.go Outdated Show resolved Hide resolved

mheon reviewed Sep 5, 2024

View reviewed changes

pkg/bindings/connection.go Outdated Show resolved Hide resolved

afbjorklund force-pushed the ssh-config-main branch from 8bea341 to 46bbf98 Compare September 6, 2024 06:11

Luap99 reviewed Sep 6, 2024

View reviewed changes

afbjorklund mentioned this pull request Sep 7, 2024

refactor: add sshClient function #23893

Merged

afbjorklund force-pushed the ssh-config-main branch 2 times, most recently from 186b0ab to fe5df7e Compare September 11, 2024 16:50

afbjorklund requested review from mheon and Luap99 September 11, 2024 16:50

Luap99 reviewed Sep 24, 2024

View reviewed changes

pkg/bindings/connection.go Outdated Show resolved Hide resolved

pkg/bindings/connection.go Outdated Show resolved Hide resolved

afbjorklund force-pushed the ssh-config-main branch from fe5df7e to b174663 Compare September 27, 2024 07:31

afbjorklund requested a review from Luap99 September 27, 2024 07:37

afbjorklund force-pushed the ssh-config-main branch from b174663 to d529d42 Compare September 27, 2024 07:47

Luap99 approved these changes Oct 30, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2024

afbjorklund added 3 commits October 30, 2024 14:44

Add support for ssh_config for connection

e523734

Signed-off-by: Anders F Björklund <[email protected]>

Use current user if no user specified

48a8a9c

Signed-off-by: Anders F Björklund <[email protected]>

Add default remote socket path if empty

b455f94

Signed-off-by: Anders F Björklund <[email protected]>

afbjorklund force-pushed the ssh-config-main branch from d529d42 to b455f94 Compare October 30, 2024 13:45

openshift-ci bot assigned mheon Oct 30, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2024

openshift-merge-bot bot merged commit 5751154 into containers:main Oct 30, 2024
85 checks passed

afbjorklund mentioned this pull request Nov 17, 2024

Users may need to ignore 127.0.0.1 in SSH config to use Podman 5.3 #24567

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ssh_config for connection #23847

Add support for ssh_config for connection #23847

afbjorklund commented Sep 3, 2024 •

edited

Loading

packit-as-a-service bot commented Sep 3, 2024

afbjorklund commented Sep 3, 2024 •

edited

Loading

afbjorklund commented Sep 3, 2024 •

edited

Loading

mheon commented Sep 5, 2024

mheon commented Sep 5, 2024

afbjorklund commented Sep 5, 2024 •

edited

Loading

Luap99 Sep 6, 2024

afbjorklund Sep 6, 2024

Luap99 Sep 6, 2024

afbjorklund Sep 6, 2024

Luap99 Sep 6, 2024

afbjorklund Sep 11, 2024

Luap99 Sep 6, 2024

afbjorklund Sep 6, 2024

afbjorklund Sep 6, 2024

Luap99 Sep 6, 2024

afbjorklund Sep 6, 2024 •

edited

Loading

afbjorklund Sep 6, 2024 •

edited

Loading

afbjorklund commented Sep 6, 2024 •

edited

Loading

afbjorklund commented Sep 6, 2024

Luap99 left a comment

Luap99 commented Sep 24, 2024

packit-as-a-service bot commented Sep 27, 2024

Luap99 left a comment

openshift-ci bot commented Oct 30, 2024

mheon commented Oct 30, 2024

Add support for ssh_config for connection #23847

Add support for ssh_config for connection #23847

Conversation

afbjorklund commented Sep 3, 2024 • edited Loading

Does this PR introduce a user-facing change?

packit-as-a-service bot commented Sep 3, 2024

afbjorklund commented Sep 3, 2024 • edited Loading

afbjorklund commented Sep 3, 2024 • edited Loading

mheon commented Sep 5, 2024

mheon commented Sep 5, 2024

afbjorklund commented Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afbjorklund Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

afbjorklund Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

afbjorklund commented Sep 6, 2024 • edited Loading

afbjorklund commented Sep 6, 2024

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 commented Sep 24, 2024

packit-as-a-service bot commented Sep 27, 2024

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 30, 2024

mheon commented Oct 30, 2024

afbjorklund commented Sep 3, 2024 •

edited

Loading

afbjorklund commented Sep 3, 2024 •

edited

Loading

afbjorklund commented Sep 3, 2024 •

edited

Loading

afbjorklund commented Sep 5, 2024 •

edited

Loading

afbjorklund Sep 6, 2024 •

edited

Loading

afbjorklund Sep 6, 2024 •

edited

Loading

afbjorklund commented Sep 6, 2024 •

edited

Loading