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

Jump to bottom

Use skeema/knownhosts, not x/crypto/ssh/knownhosts #2212

Merged
merged 1 commit into from
Nov 5, 2024
Merged

Use skeema/knownhosts, not x/crypto/ssh/knownhosts #2212

merged 1 commit into from
Nov 5, 2024

Conversation

l0rd
Copy link
Member

@l0rd l0rd commented Oct 22, 2024 •
edited
Loading

TL;DR This PR addresses this issue. In this containers/podman PR, I have added some specific tests.

image

The package golang.org/x/crypto/ssh/knownhosts has an issue when an SSH server has many public keys (i.e., supports multiple crypto algorithms).

For instance, if the local known_hosts file entries don't match the first SSH server key but match other SSH server keys, the handshake fails with a key mismatch error.

See golang/go#29286 and containers/podman#23575.

Package github.com/skeema/knownhosts is a wrapper of x/crypto/ssh/knownhosts that addresses this issue.

This commit replaces the usage of x/crypto/ssh/knownhosts in containers/common with github.com/skeema/knownhosts.

l0rd added a commit to l0rd/podman that referenced this pull request Oct 22, 2024
@l0rd
New system connection add test
ff087db 
The e2e test introduced by this PR verifies that
the command `system connection add` against an
SSH server parses and updates correctly the local
`known_hosts` file.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
@l0rd l0rd mentioned this pull request Oct 22, 2024
Merged
@l0rd l0rd force-pushed the skeema-knownhosts branch 2 times, most recently from 064f89f to 6599177 Compare October 23, 2024 06:38
l0rd added a commit to l0rd/podman that referenced this pull request Oct 23, 2024
@l0rd
New system connection add test
9e1d288 
The e2e test introduced by this PR verifies that
the command `system connection add` against an
SSH server parses and updates correctly the local
`known_hosts` file.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 24, 2024
@l0rd
New test for system connection add
9b56dcc 
The e2e test introduced by this PR verifies that
the command `system connection add`, run against an
SSH server, parses and updates correctly the local
`known_hosts` file.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 24, 2024
@l0rd
New test for system connection add
56813c3 
The e2e test introduced by this PR verifies that
the command `system connection add`, run against an
SSH server, parses and updates correctly the local
`known_hosts` file.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 24, 2024
@l0rd
New test for system connection add
8313a0e 
The e2e test introduced by this PR verifies that
the command `system connection add`, run against an
SSH server, parses and updates correctly the local
`known_hosts` file.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 24, 2024
@l0rd
New test for system connection add
2c43969 
The e2e test introduced by this PR verifies that
the command `system connection add`, run against an
SSH server, parses and updates correctly the local
`known_hosts` file.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 24, 2024
@l0rd
New test for system connection add
a52d965 
The e2e test introduced by this PR verifies that
the command `system connection add`, run against an
SSH server, parses and updates correctly the local
`known_hosts` file.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 25, 2024
@l0rd
Vendor update to get the fix in containers/common#2212
bde95dd 
Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 25, 2024
@l0rd
New system connection add tests
f830c64 
These tests verify that podman successfully adds (or
fails to add) a connection to an SSH server based on
the entries in the `~/.ssh/known_hosts` file.

In particular `system connection add` should succeed if:
- there is no `know_hosts` file
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry for another SSH server, not for the target server

It should fail if the `known_host` file has an entry for
the target server that matches the protocol but not the key.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 25, 2024
@l0rd
Vendor update to get the fix in containers/common#2212
e2a3c57 
Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 25, 2024
@l0rd
New system connection add tests
13c9d3d 
These tests verify that podman successfully adds (or
fails to add) a connection to an SSH server based on
the entries in the `~/.ssh/known_hosts` file.

In particular `system connection add` should succeed if:
- there is no `know_hosts` file
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry for another SSH server, not for the target server

It should fail if the `known_host` file has an entry for
the target server that matches the protocol but not the key.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
@l0rd l0rd force-pushed the skeema-knownhosts branch from 6599177 to f5a4be3 Compare October 25, 2024 15:43
l0rd added a commit to l0rd/podman that referenced this pull request Oct 25, 2024
@l0rd
Vendor update to get containers/common#2212
60f41e2 
Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 25, 2024
@l0rd
New system connection add tests
b8d92be 
These tests verify that podman successfully adds (or
fails to add) a connection to an SSH server based on
the entries in the `~/.ssh/known_hosts` file.

In particular `system connection add` should succeed if:
- there is no `know_hosts` file
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry for another SSH server, not for the target server

It should fail if the `known_host` file has an entry for
the target server that matches the protocol but not the key.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
@l0rd
Use skeema/knownhosts, not x/crypto/ssh/knownhosts
2564c8d 
Package [golang.org/x/crypto/ssh/knownhosts](https://pkg.go.dev/golang.org/x/crypto/ssh/knownhosts)
has an issue when an SSH server has many public keys (i.e. supports multiple crypto algorithms).

For instance, if the local `known_hosts` file entries don't match the first SSH server key but match
other keys of the SSH server, the handshake fails with a key mismatch error.

See golang/go#29286 and containers/podman#23575.

Package [github.com/skeema/knownhosts](https://github.com/skeema/knownhosts) is a wrapper of
`x/crypto/ssh/knownhosts` that addresses this issue.

This commit replaces the usage of `x/crypto/ssh/knownhosts` in containers/common with
`github.com/skeema/knownhosts`.

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
@l0rd l0rd force-pushed the skeema-knownhosts branch from f5a4be3 to 2564c8d Compare October 28, 2024 12:04
l0rd added a commit to l0rd/podman that referenced this pull request Oct 28, 2024
@l0rd
New system connection add tests
d5b123f 
These tests verify that podman successfully adds (or
fails to add) a connection to an SSH server based on
the entries in the `~/.ssh/known_hosts` file.

In particular `system connection add` should succeed if:
- there is no `know_hosts` file
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry for another SSH server, not for the target server

It should fail if the `known_host` file has an entry for
the target server that matches the protocol but not the key.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 28, 2024
@l0rd
Vendor update to get the fix in containers/common#2212
c7fe864 
Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 28, 2024
@l0rd
Vendor update to get the fix in containers/common#2212
e4f79eb 
Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 28, 2024
@l0rd
New system connection add tests
361aed9 
These tests verify that podman successfully adds (or
fails to add) a connection to an SSH server based on
the entries in the `~/.ssh/known_hosts` file.

In particular `system connection add` should succeed if:
- there is no `know_hosts` file
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry for another SSH server, not for the target server

It should fail if the `known_host` file has an entry for
the target server that matches the protocol but not the key.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 28, 2024
@l0rd
Vendor update to get the fix in containers/common#2212
ff709b9 
Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Oct 28, 2024
@l0rd
New system connection add tests
bbf8ffc 
These tests verify that podman successfully adds (or
fails to add) a connection to an SSH server based on
the entries in the `~/.ssh/known_hosts` file.

In particular `system connection add` should succeed if:
- there is no `know_hosts` file
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry for another SSH server, not for the target server

It should fail if the `known_host` file has an entry for
the target server that matches the protocol but not the key.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
@rhatdan
Copy link
Member

rhatdan commented Nov 2, 2024

LGTM

l0rd added a commit to l0rd/podman that referenced this pull request Nov 4, 2024
@l0rd
Vendor update to get the fix in containers/common#2212
df9ca6e 
Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Nov 4, 2024
@l0rd
New system connection add tests
1708812 
These tests verify that podman successfully adds (or
fails to add) a connection to an SSH server based on
the entries in the `~/.ssh/known_hosts` file.

In particular `system connection add` should succeed if:
- there is no `know_hosts` file
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry for another SSH server, not for the target server

It should fail if the `known_host` file has an entry for
the target server that matches the protocol but not the key.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
@l0rd l0rd mentioned this pull request Nov 4, 2024
Closed
l0rd added a commit to l0rd/podman that referenced this pull request Nov 4, 2024
@l0rd
New system connection add tests
1745ea4 
These tests verify that podman successfully adds (or
fails to add) a connection to an SSH server based on
the entries in the `~/.ssh/known_hosts` file.

In particular `system connection add` should succeed if:
- there is no `know_hosts` file
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry for another SSH server, not for the target server

It should fail if the `known_host` file has an entry for
the target server that matches the protocol but not the key.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Nov 4, 2024
@l0rd
New system connection add tests
ea0f53d 
These tests verify that podman successfully adds (or
fails to add) a connection to an SSH server based on
the entries in the `~/.ssh/known_hosts` file.

In particular `system connection add` should succeed if:
- there is no `know_hosts` file
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry for another SSH server, not for the target server

It should fail if the `known_host` file has an entry for
the target server that matches the protocol but not the key.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
@l0rd
Copy link
Member Author

l0rd commented Nov 4, 2024

@containers/podman-maintainers PTAL, I have added some new podman tests that are passing with the changes included in this PR.

Luap99
Luap99 approved these changes Nov 5, 2024
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

small nit but I guess that is pre existing so I can live without it

I have not looked into the detail why this is needed but I trust your investigation enough that there is no way to do this with the golang library directly so LGTM

pkg/ssh/connection_golang.go
return nil, err
}
keyDir := path.Dir(keyFilePath)
if err := fileutils.Exists(keyDir); errors.Is(err, os.ErrNotExist) {
Copy link
Member

Choose a reason for hiding this comment

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

this seems pointless, why check if the dir already exists. We could also just directly call mkdir and ignore ErrExist there

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I just left as it was as it's not doing any harm (except that's hard to read with all those nested conditions...).

@openshift-ci openshift-ci bot added the approved label Nov 5, 2024
Luap99
Luap99 approved these changes Nov 5, 2024
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 5, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: l0rd, 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:

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

@openshift-merge-bot openshift-merge-bot bot merged commit ea5e3bd into containers:main Nov 5, 2024
16 checks passed
l0rd added a commit to l0rd/podman that referenced this pull request Nov 7, 2024
@l0rd
New system connection add tests
1f10a61 
These tests verify that podman successfully adds (or
fails to add) a connection to an SSH server based on
the entries in the `~/.ssh/known_hosts` file.

In particular `system connection add` should succeed if:
- there is no `know_hosts` file
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry for another SSH server, not for the target server

It should fail if the `known_host` file has an entry for
the target server that matches the protocol but not the key.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd added a commit to l0rd/podman that referenced this pull request Nov 7, 2024
@l0rd
New system connection add tests
b20960b 
These tests verify that podman successfully adds (or
fails to add) a connection to an SSH server based on
the entries in the `~/.ssh/known_hosts` file.

In particular `system connection add` should succeed if:
- there is no `know_hosts` file
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry that matches the first protocol/key returned
  by the SSH server
- `known_hosts` has an entry for another SSH server, not for the target server

It should fail if the `known_host` file has an entry for
the target server that matches the protocol but not the key.

Depends on containers/common#2212
Fixes containers#23575

Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Luap99 Luap99 Luap99 approved these changes

Assignees

@Luap99 Luap99

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@l0rd @rhatdan @Luap99