fix: correctly apply configuration precedence in reverse parsing order #12
Conversation
This change fixes veeso#11: Previously, the order of precedence applied to parsed configuration was incorrect. Configuration was parsed, then sorted in alphabetical order. Algorithms (ciphers, key exchange algorithms, MACs, etc.) were incorrectly applied during parsing. The correct precedence order follows https://linux.die.net/man/5/ssh_config: the configuration is read from top to bottom, precedence is applied from bottom (lowest) to the top (highest precedence). Options preceding the first `Host` block are considered implicit command line options, in line with OpenSSH's own implementation. This patch includes the following changes: - Remove the alphabetic ordering of host sections. - Merge matching host sections in reverse order. - More efficiently merge host sections with vastly reduced `clone`s. (`clone` on demand.) - Resolve algorithms not during the parsing, but during the resolving stage. - More efficiently resolve algorithms, without source list mutation. - Adjust existing unit tests to test the corrected precedence algorithm.
|
@veeso I will push two additional commits with improvements made along the journey of fixing this bug. These two additional commits are optional and can be reviewed and accepted or rejected separately from the bugfix commit. In detail:
|
Pull Request Test Coverage Report for Build 7101669807
💛 - Coveralls |
This change improves error-tests by replacing `Result::is_err` with a match against the error enum. Previously, only the existence of an error result was checked. With the first additional commit, the kind of error is checked as well.
This change improves success-tests by applying the Rust 2018 idiom of returning `Result` from tests. This way, `Result::unwrap` and `Result::is_ok` can be replaced by `?`. Any error occurring in a success-test is propagated to the test runner. Some tests previously called `.ok().unwrap()` on `Result`s, effectively first turning them into `Option`s before unwrapping. This unidiomatic pattern is replaced by `?` as well.
|
Amazing job, thank you |
#11 - Correctly apply configuration precedence in reverse parsing order
Fixes #11
Commits
Commits contain logically distinct changes are are best reviewed individually.
Read #12 (comment) for more details.
Description
Previously, the order of precedence applied to
parsed configuration was incorrect.
Configuration was parsed, then sorted in
alphabetical order.
Algorithms (ciphers, key
exchange algorithms, MACs, etc.) were incorrectly
applied during parsing.
The correct precedence order follows
https://linux.die.net/man/5/ssh_config: the
configuration is read from top to bottom,
precedence is applied from bottom (lowest)
to the top (highest precedence).
Options preceding the first
Hostblock areconsidered implicit command line options, in
line with OpenSSH's own implementation.
This patch includes the following changes:
Remove the alphabetic ordering of host sections.
Merge matching host sections in reverse order.
More efficiently merge host sections with vastly reduced
clones. (cloneon demand.)Resolve algorithms not during the parsing, but during the resolving stage.
More efficiently resolve algorithms, without source list mutation.
Adjust existing unit tests to test the corrected precedence algorithm.
Type of change
This is a bugfix, but some users may depend on the old, broken algorithm. For these users, this change could be considered breaking. On the other hand, this can be said about any bugfix: There might always be someone who abused the previous, incorrect behavior.
Please select relevant options.
Checklist
cargo fmtcargo clippyand reports no warningscfg target_os)Acceptance tests
wait for a project maintainer to fulfill this section...