You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by "frothga (via GitHub)" <gi...@apache.org> on 2023/04/03 18:53:14 UTC
[GitHub] [mina-sshd] frothga opened a new pull request, #353: Follow first-match semantics specified for ssh config file
frothga opened a new pull request, #353:
URL: https://github.com/apache/mina-sshd/pull/353
This change tries to bring config file processing more in line with the semantics indicated in the ssh man page. The most significant changes are that each attribute is drawn from the first matching entry in file order. There are also defaults supplied that are in line with the behavior of the ssh command line tool. In particular, the local user name is used for any host where none is specified, and port 22 is used whenever port is not specified. Even though one might imagine these obvious defaults were in effect before, there were paths through the code where they did not get set.
This change modifies some of the test cases:
testResolvePort, testResolveUsername -- Removed because these functions no longer exist.
testFindBestMatch -- Removed because that's not how config files are supposed to be handled.
testConfigFileReload -- Original test was based on the incorrect notion that a latter entry in config could override an earlier one. I moved the catch-all entry to the end to approximate the correct semantics. Note that there may be relevant test cases where the global attributes are involved, but this particular test doesn't fit in that category.
testReadGlobalHostsConfigEntries -- Original test was written with the assumption that entries are collated at the time they are read from file. In the new approach, they are collated on demand. (This is necessary because only when a host query is made do we know which entries apply.) The test is rewritten to verify that entries have only the attributes specified in the file. The new version of the test is somewhat pointless, but it seemed better than deleting altogether.
Note that testNegatedHostEntriesResolution is failing on my system. Debug tracing shows that it is doing the right thing (not using the negated entry). I believe the problem is due to corporate firewall configuration.
The HostConfigEntry class could be further improved by storing everything in the properties collection and funneling all the accessors to it, rather than having separate class members for special properties like port and username. This may be the subject of a future PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1499440822
I disagree with this "fix". Instead, roll back the changes in `KnownHostEntry` and in `HostPatternHolder`. They are not needed at all for fixing the matching of `HostConfigEntry`.
A "Host" pattern in the SSH config file can never contain a port; it is matched only against host names. `HostConfigEntry.toHostConfigEntryResolver()` does that correctly via `findMatchingEntries()`. And for use in known_hosts, the existing implementation was correct and needed no change at all.
OTOH, I think there should be new tests for
* values given in `session.connect()` for username and port overriding values from the `HostConfigEntry`.
* port 22 being used if neither the parameters nor the config entry specify a port.
* the coalescing of values. This needs to test different aspects:
* different values set in different config entries that match all end up in the returned entry.
* multiple occurrences of values for a key (in the same or in different entries): only the first value is effective.
* multiple occurrences of one of the special "list-building" keys such as `IdentityFile`, in the same or int different entries: all values are effective.
* only values from matching entries are considered.
Finally, don't use `System.getProperty("user.name")`. Call `OsUtils.getCurrentUser()` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] frothga commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "frothga (via GitHub)" <gi...@apache.org>.
frothga commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1496627880
Thanks for checking this!
I think the right thing to do here is to adjust the test case so that it tests for correct behavior. IE: that provided parameters override config parameters. I can try to provide an updated PR with that, though it might take a couple of days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] frothga commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "frothga (via GitHub)" <gi...@apache.org>.
frothga commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1497689827
This means that there are two conflicting semantics for HostPatternsHolder.isPortMatch(). I think the simplest solution is to make this function non-static and have KnownHostEntry override it appropriately.
FWIW, fixing the port matching does make the test pass on my system. I still have tests that fail, but these really do seem to be related my company's firewall, as the returned values are IP interception messages.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] frothga commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "frothga (via GitHub)" <gi...@apache.org>.
frothga commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1499827316
OK, I made those changes.
Some test cases would be nice. I've been doing this on a "spare time" basis, and unfortunately won't be able to get back to it until after next week due to some heavy deadlines and a conference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] frothga commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "frothga (via GitHub)" <gi...@apache.org>.
frothga commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1500389446
If you can create the test cases, that would be great. FWIW, I have tested this in my own app, and it seems to be doing the right thing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1500941382
Squashed, added some tests.
This whole area could still use more clean-up as you've already noticed, but for now I think it's OK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1497302955
The failures in the `KnownHostsServerKeyVerifierTest` are probably due to the [change in port matching](https://github.com/apache/mina-sshd/pull/353/files#diff-b796833d52500208845a7e19b51846b3026f511c6c30d562e182e10ca4a6f4d8R210).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1496557284
`testNegatedHostEntriesResolution` is also failing in CI, and also locally for me. It sets up configs as
```
Host !127.0.0.1
Hostname 127.0.0.1
User org.apache.sshd.client.config.hosts
Port 22799
Host 127.0.0.1
Hostname 127.0.0.1
User testNegatedHostEntriesResolution
Port 55566
```
and calls `session.connect("127.0.0.1", "org.apache.sshd.client.config.hosts", 22799)` but expects that to resolve to and to connect to `testNegatedHostEntriesResolution@127.0.0.1:55566`. The test assumes the "config overrides parameters" behavior.
I suppose it would work if called as `session.connect("127.0.0.1", null, 0)` (or maybe -1 as port).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1500145892
There's no rush. I'm also working on this project only in my spare time. But I do have some time over the week-end. If it's OK for you I could finish this...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] frothga commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "frothga (via GitHub)" <gi...@apache.org>.
frothga commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1497611748
Exactly how to fix this boils down to our interpretation of the known_hosts file format. Should an entry that does not specify a non-standard port only match port 22, or should it match any port?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf merged pull request #353: Follow first-match semantics specified for ssh config file
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf merged PR #353:
URL: https://github.com/apache/mina-sshd/pull/353
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] frothga commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "frothga (via GitHub)" <gi...@apache.org>.
frothga commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1496729038
OK, I pushed the change for testNegatedHostEntriesResolution. It seems to fix the test on my system. However, I'm still getting an error from testUpdateSameHost2PortsStdFirstDiffKeys. It may be of a similar nature, but I'm having difficulty seeing where the config file comes into play. Could you suggest where the problem is?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "tomaswolf (via GitHub)" <gi...@apache.org>.
tomaswolf commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1497679435
An entry without port must match only 22 in known_hosts. Running blame on that `HostPatternsHolder` leads to commit a52b1b3cb for issue [SSHD-1063](https://issues.apache.org/jira/browse/SSHD-1063), where that code was fixed to do exactly that.
It's also somewhat logical: if "no port" matches any port, we may consider wrong host keys when connecting to a non-standard port. Running multiple SSH-enabled services on different ports and having different host keys is not uncommon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] frothga commented on pull request #353: Follow first-match semantics specified for ssh config file
Posted by "frothga (via GitHub)" <gi...@apache.org>.
frothga commented on PR #353:
URL: https://github.com/apache/mina-sshd/pull/353#issuecomment-1497760048
I ended up fixing the port matching a different way. It's a bit ugly, but seems to work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org