Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add proper handling for golang ssh #1136

Merged
merged 1 commit into from
Sep 26, 2022
Merged

add proper handling for golang ssh #1136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,9 @@ type Destination struct {

// Identity file with ssh key, optional
Identity string `toml:"identity,omitempty"`

// isMachine describes if the remote destination is a machine.
IsMachine bool `toml:"is_machine,omitempty"`
}

// NewConfig creates a new Config. It starts with an empty config and, if
Expand Down Expand Up @@ -1235,32 +1238,32 @@ func Reload() (*Config, error) {
return defConfig()
}

func (c *Config) ActiveDestination() (uri, identity string, err error) {
func (c *Config) ActiveDestination() (uri, identity string, machine bool, err error) {
cdoern marked this conversation as resolved.
Show resolved Hide resolved
if uri, found := os.LookupEnv("CONTAINER_HOST"); found {
if v, found := os.LookupEnv("CONTAINER_SSHKEY"); found {
identity = v
}
return uri, identity, nil
return uri, identity, false, nil
}
connEnv := os.Getenv("CONTAINER_CONNECTION")
switch {
case connEnv != "":
d, found := c.Engine.ServiceDestinations[connEnv]
if !found {
return "", "", fmt.Errorf("environment variable CONTAINER_CONNECTION=%q service destination not found", connEnv)
return "", "", false, fmt.Errorf("environment variable CONTAINER_CONNECTION=%q service destination not found", connEnv)
}
return d.URI, d.Identity, nil
return d.URI, d.Identity, d.IsMachine, nil

case c.Engine.ActiveService != "":
d, found := c.Engine.ServiceDestinations[c.Engine.ActiveService]
if !found {
return "", "", fmt.Errorf("%q service destination not found", c.Engine.ActiveService)
return "", "", false, fmt.Errorf("%q service destination not found", c.Engine.ActiveService)
}
return d.URI, d.Identity, nil
return d.URI, d.Identity, d.IsMachine, nil
case c.Engine.RemoteURI != "":
return c.Engine.RemoteURI, c.Engine.RemoteIdentity, nil
return c.Engine.RemoteURI, c.Engine.RemoteIdentity, false, nil
}
return "", "", errors.New("no service destination configured")
return "", "", false, errors.New("no service destination configured")
}

var (
Expand Down
12 changes: 8 additions & 4 deletions pkg/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,9 +588,10 @@ image_copy_tmp_dir="storage"`
cfg, err = ReadCustomConfig()
gomega.Expect(err).ShouldNot(gomega.HaveOccurred())

u, i, err := cfg.ActiveDestination()
u, i, m, err := cfg.ActiveDestination()
gomega.Expect(err).ShouldNot(gomega.HaveOccurred())

gomega.Expect(m).To(gomega.Equal(false))
cdoern marked this conversation as resolved.
Show resolved Hide resolved
gomega.Expect(u).To(gomega.Equal("https://qa/run/podman/podman.sock"))
gomega.Expect(i).To(gomega.Equal("/.ssh/id_rsa"))
})
Expand Down Expand Up @@ -620,14 +621,15 @@ image_copy_tmp_dir="storage"`
oldContainerConnection, hostEnvSet := os.LookupEnv("CONTAINER_CONNECTION")
os.Setenv("CONTAINER_CONNECTION", "QB")

u, i, err := cfg.ActiveDestination()
u, i, m, err := cfg.ActiveDestination()
// Undo that
if hostEnvSet {
os.Setenv("CONTAINER_CONNECTION", oldContainerConnection)
} else {
os.Unsetenv("CONTAINER_CONNECTION")
}
gomega.Expect(err).ShouldNot(gomega.HaveOccurred())
gomega.Expect(m).To(gomega.Equal(false))

gomega.Expect(u).To(gomega.Equal("https://qb/run/podman/podman.sock"))
gomega.Expect(i).To(gomega.Equal("/.ssh/qb_id_rsa"))
Expand Down Expand Up @@ -660,7 +662,8 @@ image_copy_tmp_dir="storage"`
os.Setenv("CONTAINER_HOST", "foo.bar")
os.Setenv("CONTAINER_SSHKEY", "/.ssh/newid_rsa")

u, i, err := cfg.ActiveDestination()
u, i, m, err := cfg.ActiveDestination()

// Undo that
if hostEnvSet {
os.Setenv("CONTAINER_HOST", oldContainerHost)
Expand All @@ -675,6 +678,7 @@ image_copy_tmp_dir="storage"`
}

gomega.Expect(err).ShouldNot(gomega.HaveOccurred())
gomega.Expect(m).To(gomega.Equal(false))

gomega.Expect(u).To(gomega.Equal("foo.bar"))
gomega.Expect(i).To(gomega.Equal("/.ssh/newid_rsa"))
Expand All @@ -684,7 +688,7 @@ image_copy_tmp_dir="storage"`
cfg, err := ReadCustomConfig()
gomega.Expect(err).ShouldNot(gomega.HaveOccurred())

_, _, err = cfg.ActiveDestination()
_, _, _, err = cfg.ActiveDestination()
gomega.Expect(err).Should(gomega.HaveOccurred())
})

Expand Down
83 changes: 73 additions & 10 deletions pkg/ssh/connection_golang.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package ssh
import (
"bytes"
"encoding/json"
"errors"
"fmt"
"io"
"net"
Expand Down Expand Up @@ -70,7 +71,7 @@ func golangConnectionDial(options ConnectionDialOptions) (*ConnectionDialReport,
if err != nil {
return nil, err
}
cfg, err := ValidateAndConfigure(uri, options.Identity)
cfg, err := ValidateAndConfigure(uri, options.Identity, options.InsecureIsMachineConnection)
cdoern marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}
Expand All @@ -84,12 +85,15 @@ func golangConnectionDial(options ConnectionDialOptions) (*ConnectionDialReport,
}

func golangConnectionExec(options ConnectionExecOptions) (*ConnectionExecReport, error) {
if !strings.HasPrefix(options.Host, "ssh://") {
options.Host = "ssh://" + options.Host
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throughout, in several instances of these changes: Why does it make sense to set a “host” field to something URI-like?

IMHO fields/variables, should almost always clear and unambiguous semantics — the obvious semantics. If there are two possible formats, have two separate variables/fields.

It would at least be somewhat manageable if the ConnectionExecOptions.Host field were clearly documented that way, but… it isn’t. How is someone in the future (3 years into the future? or maybe just next week) going to update this undocumented code, without exhaustively enumerating every single caller to understand what formats are acceptable? Or perhaps even validating many past versions of Podman to see what they were writing into persistent config files?

If you currently know these things, please write them down.

If the user’s input is always supposed to be a host name and not an URI, but Validate uses an URI parser for some reason, I’d expect Validate to do this internally (or, maybe, not to use an URI parser at all).

If the user’s input can either be a URI or a non-URI, do we have to have that ambiguity? Maybe that ambiguity should only exist at some top CLI level (possibly as a helper provided by this package) but not here inside the fairly low levels of the API?

If the user’s input can be a URI, what happens if the input uses a different scheme than ssh?

}
_, uri, err := Validate(options.User, options.Host, options.Port, options.Identity)
if err != nil {
return nil, err
}

cfg, err := ValidateAndConfigure(uri, options.Identity)
cfg, err := ValidateAndConfigure(uri, options.Identity, false)
if err != nil {
return nil, err
}
Expand All @@ -111,11 +115,15 @@ func golangConnectionScp(options ConnectionScpOptions) (*ConnectionScpReport, er
return nil, err
}

// removed for parsing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t understand what this comment is trying to say.

if !strings.HasPrefix(host, "ssh://") {
host = "ssh://" + host
}
_, uri, err := Validate(options.User, host, options.Port, options.Identity)
if err != nil {
return nil, err
}
cfg, err := ValidateAndConfigure(uri, options.Identity)
cfg, err := ValidateAndConfigure(uri, options.Identity, false)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -209,7 +217,7 @@ func GetUserInfo(uri *url.URL) (*url.Userinfo, error) {
// ValidateAndConfigure will take a ssh url and an identity key (rsa and the like) and ensure the information given is valid
// iden iden can be blank to mean no identity key
// once the function validates the information it creates and returns an ssh.ClientConfig.
func ValidateAndConfigure(uri *url.URL, iden string) (*ssh.ClientConfig, error) {
func ValidateAndConfigure(uri *url.URL, iden string, insecureIsMachineConnection bool) (*ssh.ClientConfig, error) {
var signers []ssh.Signer
passwd, passwdSet := uri.User.Password()
if iden != "" { // iden might be blank if coming from image scp or if no validation is needed
Expand Down Expand Up @@ -272,23 +280,61 @@ func ValidateAndConfigure(uri *url.URL, iden string) (*ssh.ClientConfig, error)
if err != nil {
return nil, err
}
keyFilePath := filepath.Join(homedir.Get(), ".ssh", "known_hosts")
known, err := knownhosts.New(keyFilePath)
if err != nil {
return nil, fmt.Errorf("creating host key callback function for %s: %w", keyFilePath, err)

var callback ssh.HostKeyCallback
if insecureIsMachineConnection {
callback = ssh.InsecureIgnoreHostKey()
} else {
callback = ssh.HostKeyCallback(func(host string, remote net.Addr, pubKey ssh.PublicKey) error {
keyFilePath := filepath.Join(homedir.Get(), ".ssh", "known_hosts")
known, err := knownhosts.New(keyFilePath)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
logrus.Warn("please create a known_hosts file. The next time this host is connected to, podman will add it to known_hosts")
return nil
}
return err
}
// we need to check if there is an error from reading known hosts for this public key and if there is an error, what is it, and why is it happening?
// if it is a key mismatch we want to error since we know the host using another key
// however, if it is a general error not because of a known key, we want to add our key to the known_hosts file
hErr := known(host, remote, pubKey)
var keyErr *knownhosts.KeyError
// if keyErr.Want is not empty, we are receiving a different key meaning the host is known but we are using the wrong key
as := errors.As(hErr, &keyErr)
switch {
case as && len(keyErr.Want) > 0:
logrus.Warnf("ssh host key mismatch for host %s, got key %s of type %s", host, ssh.FingerprintSHA256(pubKey), pubKey.Type())
return keyErr
// if keyErr.Want is empty that just means we do not know this host yet, add it.
case as && len(keyErr.Want) == 0:
// write to known_hosts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, this is an improvement over the status quo but I’ve been talking about the need to interactively get user’s approval first, for some time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I will prompt

Copy link
Contributor Author

@cdoern cdoern Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtrmac this will actually break the machine tests since there is no way to force through the prompt. suggestions?

and most podman-remote tests actually

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For machine, maybe that’s consistent with the isMachine bool in general. But that rather depends on the specifics of the semantics of that field.

For “most podman-remote tests”, how does that fail? (It might very well be possible but just a lot of work.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would a simple command to a new "machine" in the e2e tests work in the testing suite if we need to prompt for y/n to continue?

I could do a timeout default yes here, where if the user starts a machine or executes podman-remote and doesn't answer y/n we default to yes after 15 seconds.

Other than that, this is a huge hindrance to regular usage and people who use this in applied scenarios... a very breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would a simple command to a new "machine" in the e2e tests work in the testing suite if we need to prompt for y/n to continue?

Whatever the current equivalent of expect(1) is. I assume (without checking) that we have some tests of podman run -it, that’s the same issue.

I could do a timeout default yes here, where if the user starts a machine or executes podman-remote and doesn't answer y/n we default to yes after 15 seconds.

No thank you. That’s not an explicit expression of intent.

Other than that, this is a huge hindrance to regular usage and people who use this in applied scenarios... a very breaking change.

How is prompting on podman connection add, and never again, a huge hindrance?

I’ve been talking about the need to interactively prompt since the very start of the existence of this package, and bringing up the need to walk though the UX to ensure it is practical.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree a prompt is best but that is not something that can be introduced in 4.3 I do not think. if you exec podman-remote or any podman machine command and it idles, we will break a lot of existing builds.

err := addKnownHostsEntry(host, pubKey)
if err != nil {
if os.IsNotExist(err) {
logrus.Warn("podman will soon require a known_hosts file to function properly.")
Copy link
Contributor

@mtrmac mtrmac Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this code is confident enough to in the public key to add an entry, why would it be reluctant to create a new file with a single line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought our desired path was to warn people for a few podman versions and then maybe talk about creating a file? I think this is exiting the scope of this PR.

This fix is needed for 4.3 next week

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @mtrmac. The stock behavior for podman machine is that a file does not need to be created. Granted, it was because it wasn't in use, but from a UX perspective, we should avoid having podman machine instances break after the upgrade. This is also the stock behavior for stock ssh (silent file creation). Change-wise should be safe and small (just adding O_CREATE to flags)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@n1hility This path is not really a podman machine concern in this sense. If the file is entirely missing, we get into the other path with the insecureIsMachineConnection check.

Even this path does not break machines, it warns and continues.

It’s the non-machine case where the behavior between existing file with no match / no file is hard for me to understand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, my original comment is way off base.

If we get to the path that start with a known() call at all, we know that the file did exist when creating known. So the only way we can get here is if someone removed known_hosts, or a parent directory, between the read and this write.

In that case I don’t think we really need any special handling for os.IsNotExist (though using O_CREATE would, I think, be closer to the right thing to do [with the absolutely right thing being worrying about locking the known_hosts writers across disparate implementations??? ugh]).

But also, the current warning text seems very unrelated to the situation at hand: it is warning about a known_hosts file that did exist just a few milliseconds ago.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtrmac @cdoern aha sorry! I just skimmed the shown snippets without looking at the full code. Ok, that alleviates my primary concern.

That said I agree it would make sense to auto-create in the non-machine case as well, and this could simplify the logic a bit.

_ need any special handling for os.IsNotExist (though using O_CREATE would, I think, be closer to the right thing to do [with the absolutely right thing being worrying about locking the known_hosts writers across disparate implementations??? ugh]).

I think its ok relying on O_APPENDs atomicity as long as its always one write for the record. I just double checked the openssh source and they dont use cooperative file locking on known hosts, so its effect would be limited to just concurrent usage of podman

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am mocking up a version where we create a known_hosts file which removes the need for insecureIsMachineConnection

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its ok relying on O_APPENDs atomicity as long as its always one write for the record. I just double checked the openssh source and they dont use cooperative file locking on known hosts

Thanks for taking the time to actually check. Yes, looking at add_host_to_hostfile in https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/hostfile.c?rev=1.93&content-type=text/x-cvsweb-markup , we don’t need to do any better than that:

  • Just write text+\n, don’t worry about files that don’t end with a newline.
  • Use O_CREATE | O_APPEND, don’t do anything special about concurrency (but potentially create ~/.ssh, and in that case it is fairly important to use mode 0700).

I guess that’s for a future version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtrmac do you think we should merge this as is for 4.3? I modified it locally to create ~/.ssh/known_hosts, I can push that if needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cdoern I apologize, for some reason I thought that our 4.2 baseline is #1094 , and that this PR is a net decrease in security. I have only today actually checked, and 4.2 has the pre-c/common ssh implementation if I’m not mistaken.

Assuming that’s the case, any version of this is an improvement over 4.2. So, at this point I think the priorities are, in this order:

  • Make sure this + the corresponding Podman PRs is merged into 4.3, so that we have a working Podman machine in 4.3.
  • Get the implementation of the feature set as is right now correct and usable. (I think the PR as is is fine in isolation, but can we rely on tests for all relevant features? I.e. I think that rather than add more last-minute features, it would be more useful to do a manual walk-through of all the relevant use cases [and to capture that for posterity], and ensure that we haven’t overlooked any corner cases / interactions. (E.g. is there really no intersection between ssh_native and the IsMachineConnection cases?)

And for 4.4, let’s first document what we want to do, then see what pieces are missing, and finally finish this.

return nil
}
return err
}
cdoern marked this conversation as resolved.
Show resolved Hide resolved
case hErr != nil:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: Simplifying this to default: return hErr would make it a tiny bit clearer that we never return nil without being intentional about it.

return hErr
}
cdoern marked this conversation as resolved.
Show resolved Hide resolved
return nil
cdoern marked this conversation as resolved.
Show resolved Hide resolved
})
}

cfg := &ssh.ClientConfig{
User: uri.User.Username(),
Auth: authMethods,
HostKeyCallback: known,
HostKeyCallback: callback,
Timeout: tick,
}
return cfg, nil
}

func getUDS(uri *url.URL, iden string) (string, error) {
cfg, err := ValidateAndConfigure(uri, iden)
cfg, err := ValidateAndConfigure(uri, iden, false)
if err != nil {
return "", fmt.Errorf("failed to validate: %w", err)
}
Expand Down Expand Up @@ -324,3 +370,20 @@ func getUDS(uri *url.URL, iden string) (string, error) {
}
return info.Host.RemoteSocket.Path, nil
}

// addKnownHostsEntry adds (host, pubKey) to user’s known_hosts.
func addKnownHostsEntry(host string, pubKey ssh.PublicKey) error {
hd := homedir.Get()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely non-blocking: The caller already computed this, so it could pass a path instead of this function recomputing.

(Right now this makes ~no difference, but it will eventually help if we start maintaining per-machine known_hosts files in non-default locations.)

known := filepath.Join(hd, ".ssh", "known_hosts")
f, err := os.OpenFile(known, os.O_APPEND|os.O_WRONLY, 0o600)
if err != nil {
return err
}
defer f.Close()
l := knownhosts.Line([]string{host}, pubKey)
if _, err = f.WriteString("\n" + l + "\n"); err != nil {
return err
}
logrus.Infof("key %s added to %s", ssh.FingerprintSHA256(pubKey), known)
return nil
}
13 changes: 7 additions & 6 deletions pkg/ssh/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,13 @@ type ConnectionCreateOptions struct {
}

type ConnectionDialOptions struct {
Host string
Identity string
User *url.Userinfo
Port int
Auth []string
Timeout time.Duration
Host string
Identity string
User *url.Userinfo
Port int
Auth []string
Timeout time.Duration
InsecureIsMachineConnection bool
}

type ConnectionDialReport struct {
Expand Down
5 changes: 3 additions & 2 deletions pkg/ssh/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ func Validate(user *url.Userinfo, path string, port int, identity string) (*conf
if strings.Contains(path, "/run") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking, about all of Validate:

This kind of parsing/transformation function, overall, seems to me like a pretty good candidate for writing fairly exhaustive unit tests: it is

  • non—trivial, with quite a few code paths and, apparently, corner cases (assuming they can’t be eliminated in the first place)
  • very isolated, so cheap to test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: BTW naming a transformative parser like this “validate” is a bit misleading.

Non-blocking: And why are native callers passing options.Identity to this function, and immediately afterwards to ValidateAndConfigure? It seems to me that this could be simplified, e.g. by this function returning a struct (resolvedConnectionInfo[1]?), and callers can then pass around that single struct and access dest.URI and dest.Identity.

[1] I guess that struct can’t be *config.Destination, because the on-disk format and the normalized internal data seem to differ.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Regardless of the name, AFAICS Validate does not need to be public at all; so, with the recent plans to make the c/common API stable, it should start private. We can easily make things public later, but we can’t take back public API nearly as easily. This applies to other functions in this package as well.)

sock = strings.Split(path, "/run")[1]
}
Comment on lines 21 to 23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this supposed to do? Consider things like ssh://run.my.machines.

  • If we have that complex URL parser, with various escaping formats and the like, why is it preceded by a naive Contains condition? My best guess (which is a bad guess, there is no documentation) is that sock is supposed to be set to something like uri.Path; if there is some special reason not to use exactly that, it is not documented.

Applies similarly to various other instances of "/run" in this subpackage.

// url.Parse NEEDS ssh://, if this ever fails or returns some nonsense, that is why.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See elsewhere, why do the callers need to deal with that?

Why is a “path” parameter being parsed as an URI? What is the actually intended semantics of that value? Please name parameters exactly correctly, or if that’s impossible (preferably restructure code to make that possible, or at least) clearly, and exhaustively, document the expected formats, in the function’s/field’s doc string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the “sometimes we are not going to have a path” comment below?

  • What’s an example? I guessed ssh://foo and ssh://user@foo, but that didn’t match the code. If there is a a corner case (ssh:foo??), it should be documented clearly — perhaps in a comment, but ideally actually documented also by a unit test that ensures the behavior is not accidentally broken.
  • How exactly does “not going to have a path” relate to the presence, or not, of a user designation? I can‘t see the relationship.

uri, err := url.Parse(path)
if err != nil {
return nil, nil, err
Expand All @@ -33,9 +34,9 @@ func Validate(user *url.Userinfo, path string, port int, identity string) (*conf

if uri.Port() == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if both uri.Port() and port is set?

Looking at various Podman callers, so many (but not all of them) call url.Parse themselves already. Why do that twice, and then need to worry about these port/port conflicts that might not be actually possible in practice?

Maybe there should be a single ssh.Destination type, containing all relevant information in a native support optimized for further use (whether that’s an url.URL, individual values, or some combination), and helpers vaguely like DestinationFromConfigDestination(*config.Destination), DestinationFromURI, DestinationFromCLIString, DestinationFromPodmanConnectionAddComponents — each with appropriate heuristics, without several round-trips back and forth.

if port != 0 {
uri.Host = net.JoinHostPort(uri.Hostname(), strconv.Itoa(port))
uri.Host = net.JoinHostPort(uri.Host, strconv.Itoa(port))
} else {
uri.Host = net.JoinHostPort(uri.Hostname(), "22")
uri.Host = net.JoinHostPort(uri.Host, "22")
}
}

Expand Down