SSH: sss_ssh_knownhosts must accept port numbers #7583

aplopez · 2024-09-11T13:19:30Z

sss_ssh_knownhosts was only accepting a hostname or IP address, but no port number. Because token %H of ssh(1) could pass a port number, it must be accepted.

The %H token can provide the hostname and port number in the following formats:

hostname
canonical.host.name
IP-address
[hostname]:port
[canonical.host.name]:port
[IP-address]:port

The port is specified only when a non-default port is used.

Identifiers without the brackets are also recognized in case a user invokes the tool manually.

When the retrieved keys do not include the hostname, the hostname received as parameter will be added before the keytype to output a correctly formatted line. The hostname will be added unmodified or just the hostname (no port number), depending on whether the new -o,--only-host-name option was provided. This was done to handle the situation when a non-default port number is used and the keys don't include the hostname and port number.

alexey-tikhonov · 2024-09-16T15:37:37Z

https://issues.redhat.com/browse/RHEL-56701

src/util/sss_ssh.c

src/sss_client/ssh/sss_ssh_knownhosts.c

alexey-tikhonov · 2024-09-18T10:46:34Z

Shall not sss_ssh_get_ent() get ssh_host instead of host as an arg?

src/sss_client/ssh/sss_ssh_knownhosts.c

aplopez · 2024-09-18T12:06:36Z

Shall not sss_ssh_get_ent() get ssh_host instead of host as an arg?

This is uncertain, because we are trying to do something that will work with an IPA feature that doesn't exist nowadays.

However, my understanding is that IPA will continue identifying the hosts by their canonicalized hostname as it currently does, and all the keys (with and without a port) for that host will be returned. ssh will filter out the key that don't apply to its request.

What is returned today as "the key" is the keytype and the key itself, while in the future the result will also include the hostname and port number (if it is not the default). But we still need to identify the host object in IPA by the host name without the port number.

alexey-tikhonov · 2024-09-18T12:23:41Z

Shall not sss_ssh_get_ent() get ssh_host instead of host as an arg?

This is uncertain, because we are trying to do something that will work with an IPA feature that doesn't exist nowadays.

However, my understanding is that IPA will continue identifying the hosts by their canonicalized hostname as it currently does, and all the keys (with and without a port) for that host will be returned. ssh will filter out the key that don't apply to its request.

What is returned today as "the key" is the keytype and the key itself, while in the future the result will also include the hostname and port number (if it is not the default). But we still need to identify the host object in IPA by the host name without the port number.

How is 'port' used then?
Only to get canonicalized name?

aplopez · 2024-09-18T12:46:32Z

How is 'port' used then? Only to get canonicalized name?

For the canonicalized name and to add it to the keys that don't include the host list (if -ois not provided).

Depending on how users start using these features in the future, we could also want to filter the retrieved keys to return only those that match the host and port that ssh requested. Doing this currently is overkill because there are not too many keys and ssh can perfectly filter them.

src/util/sss_ssh.c

aplopez · 2024-09-20T09:37:44Z

I found an error in the PR. I will "block" it until it is fixed.

aplopez · 2024-09-20T10:58:14Z

@alexey-tikhonov
When a user provided hostname:port (without the brackets, which is not expected but probably we want to support this case), and the key had the hostname in the proper format (with the brackets), the presence of the hostname would not be identified. So I added an extra parameter to the sss_ssh_print_pubkey() function used just for the presence detection, and kept the old one with the hostname to be added to the key.
(I had to push several times because some files were left outside of the commit and some unwanted ones were included).

alexey-tikhonov · 2024-09-20T15:42:17Z

src/util/sss_ssh.c

-    if (keyhost == NULL) {
+    /* Check if the host name part is included with the key.
+     * OpenSSH expects a linebreak after each key. */
+    if (keyhost == NULL || sss_ssh_key_has_host_name(repr, needlehost)) {


(Not currently possible but) there is no guarantee that needlehost != NULL here.

Wouldn't it be possible to deduce needlehost from keyhost instead of new arg?

Of course it can be done, but that means we need to call parse_ssh_host() again from this function, when it was already called from the caller function.

I think it is overkill to call this function here when we already have the values.

If you feel uncomfortable about needlehost being NULL, I can add a check. Or just document it must not be NULL if keyhost is not NULL.

I documented the behavior and added a simple check.

sss_ssh_knownhosts was only accepting a hostname or IP address, but no port number. Because token %H of ssh(1) could pass a port number, it must be accepted. The %H token can provide the hostname and port number in the following format: hostname canonical.host.name IP-address [hostname]:port [canonical.host.name]:port [IP-address]:port The port is specified only when a non-default port is used. Identifiers without the brackets are also recognized in case a user invokes the tool directly.

src/sss_client/ssh/sss_ssh_knownhosts.c

thalman

Thanks for the fix

alexey-tikhonov · 2024-09-25T13:15:51Z

Pushed PR: #7583

master
- 823d787 - SSH: sss_ssh_knownhosts must accept port numbers

aplopez added the no-backport This should go to target branch only. label Sep 11, 2024

aplopez force-pushed the known-hosts branch from 8d4019c to 7899645 Compare September 16, 2024 15:20

alexey-tikhonov added the Bugzilla label Sep 16, 2024

aplopez added the Waiting for review label Sep 17, 2024

aplopez marked this pull request as ready for review September 17, 2024 09:43

andreboscatto assigned thalman and alexey-tikhonov Sep 17, 2024

andreboscatto requested review from thalman and alexey-tikhonov September 17, 2024 12:36