podman ssh work, using new c/common interface

[cdoern]

implement new ssh interface into podman

this completely redesigns the entire functionality of podman image scp,
podman system connection add, and podman --remote. All references to golang.org/x/crypto/ssh
have been moved to common as have native ssh/scp execs and the new usage of the sftp package.

this PR adds a global flag, --ssh to podman which has two valid inputs golang and native where golang is the default.
Users should not notice any difference in their everyday workflows if they continue using the golang option. UNLESS they have been using an improperly verified ssh key, this will now fail. This is because podman was incorrectly using the ssh callback method to IGNORE the ssh known hosts file which is very insecure and golang tells you not yo use this in production.

The native paths allows for immense flexibility, with a new containers.conf field SSH_CONFIG that specifies a specific ssh config file to be used in all operations. Else the users ~/.ssh/config file will be used.
podman --remote currently only uses the golang path, given its deep interconnection with dialing multiple clients and urls.

My goal after this PR is to go back and abstract the idea of podman --remote from golang's dialed clients, as it should not be so intrinsically connected. Overall, this is a v1 of a long process of offering native ssh, and one that covers some good ground with podman system connection add and podman image scp.

Signed-off-by: Charlie Doern [email protected]

Does this PR introduce a user-facing change?

Implement new ssh package, utilizing both native ssh and golang's package, users can access this change by using the --ssh global podman flag setting it to either native or golang

[cdoern]

this is a huge one. I need to add tests, opening this to see what breaks.

@vrothberg @jwhonce @rhatdan @mheon PTAL

[cdoern]

@jwhonce why did any of these tests pass to begin with? They are failing now because the way I made ssh, it tests the connections given. Is that unexpected behavior?

https://api.cirrus-ci.com/v1/artifact/task/5136766632984576/html/int-podman-fedora-36-rootless-host.log.html#t--remove--3

yeah it seems podman main does not actually validate the given connections.... I guess I can add this logic back into my c/common PR

podman/cmd/podman/system/connection/add.go

Lines 111 to 114 in ef6f5a9

	if uri.Path == "" || uri.Path == "/" {
	if uri.Path, err = getUDS(uri, iden); err != nil {
	return err
	}

[cdoern]

fingers crossed all tests pass on this run. If they do, we can merge containers/common#1094

[rhatdan]

We don't care about the bloat test, although you should have dropped all of ssh directory from this repo as well.

[cdoern]

We don't care about the bloat test, although you should have dropped all of ssh directory from this repo as well.

I'll make sure they are all gone. it is not referenced in go.mod but it is in modules.txt

[Luap99]

We don't care about the bloat test, although you should have dropped all of ssh directory from this repo as well.

We care very much about the bloat!

@cdoern Is the use of github.com/pkg/sftp really necessary?
I have not looked closely at the code but I am a bit confused why this drags in so many new deps. Correct if I wrong but as I understand we were already using the internal golang ssh before and now also support the ssh binary. In this case I would expect only some os/exec call to ssh added and not all these new dependencies.

[cdoern]

We don't care about the bloat test, although you should have dropped all of ssh directory from this repo as well.

We care very much about the bloat!

@cdoern Is the use of github.com/pkg/sftp really necessary? I have not looked closely at the code but I am a bit confused why this drags in so many new deps. Correct if I wrong but as I understand we were already using the internal golang ssh before and now also support the ssh binary. In this case I would expect only some os/exec call to ssh added and not all these new dependencies.

@Luap99 sftp replaces the quite janky library:

podman/go.mod

Line 30 in a43cfc1

github.com/dtylman/scp v0.0.0-20181017070807-f3000a34aef4

everything else is vendor bloat, I understand sftp is a big change but it is all in c/common not in here (even though the impacts are seen here).

The ask was for two approaches: one fully golang (ssh +sftp) the other to be fully native (openSSH). This is the simplest way to do that.

[cdoern]

I could continue to use the above library or fork it and make my own version of it. I am wary of the crippling dependency though, thats why I got rid of it. If that repo magically stops working, scp fully breaks.

[Luap99]

I am totally fine if you think this is better I just wanted to bring this up since I would have expected less bloat.
ssh/scp is important from a security POV so if these packages are better maintained this is much better than some saved bytes.

[cdoern]

yeah that was my logic. I wish this was a bit smaller, but it seems like this is the minimum size to get all of the features we want.

[cdoern]

@mtrmac any idea why the sign tests are failing here? Some functions have been moved to containers/common but that should not impact the outcome

edit: nevermind, this is because of #15139

[mtrmac]

(Not really a review, just a skim for the thing we are dealing with in c/common.)

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, rhatdan

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 [rhatdan]

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

[rhatdan]

LGTM
@Luap99 @mheon @vrothberg PTAL

[vrothberg]

/lgtm

[mtrmac]

Just to add a reverse link, #15031 discusses podman machine tests failing, likely because of this PR; those tests did not run in this PR because they IIRC need to be manually opted into.