-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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 partial support for SSH known hosts markers #11635
Conversation
The SSH `known_hosts` file parsing in Cargo did not previously support markers. Markers are modifiers on the lines (`@cert-authority` and `@revoked`) which denote special behavior for the details on that line. Lines were skipped entirely. This silent skipping of marker lines can be confusing to a user, who sees that their command line Git/SSH client works for some repository, but Cargo reports that no host key is found. This change adds support for the `@revoked` marker. This marker denotes that a key should be rejected outright. It is of limited use without `@cert-authority` marker support. However, if it is present in a user's `known_hosts` file, then Cargo definitely shouldn't accept that key and probably shouldn't suggest that the user add it to their `known_hosts` either. The change also adds support for detecting `@cert-authority` markers in `known_hosts` files. These lines cannot yet be used for host key verification, but if one is found for a matching host, the user will be informed that Cargo doesn't support `@cert-authority` markers in the error message. Additionally, the user will be advised to use the `net.git-fetch-with-cli` config option to use the command line git client for fetching crates from Git. Refs: rust-lang#11577
r? @ehuss (rustbot has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks great! I only have some very minor style nits.
BTW, regarding this:
We're working on that on the git2 side. |
Co-authored-by: Eric Huss <[email protected]>
That's fantastic! |
src/cargo/sources/git/known_hosts.rs
Outdated
if key_matches { | ||
return Ok(()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there is an issue here. This should not early return on a key match. Otherwise the known_hosts file becomes order-dependent, particularly if the revoked key is found later in a file. I think this should only return until after all entries have been processed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're completely right. Nice catch!
I've fixed the issue and added a test for this case for good measure.
…n processed Since a @Revoked line might deny access to a key which would otherwise be accepted, we need to process all lines before we decide that a host key should be accepted.
…sh-known-hosts-markers
src/cargo/sources/git/known_hosts.rs
Outdated
if key_matches { | ||
accepted_known_host_found = true; | ||
} | ||
|
||
// The host and key type matched, but the key itself did not. | ||
// This indicates the key has changed. | ||
// This is only reported as an error if no subsequent lines have a | ||
// correct key. | ||
latent_errors.push(KnownHostError::HostKeyHasChanged { | ||
hostname: host.to_string(), | ||
key_type: remote_key_type, | ||
old_known_host: known_host.clone(), | ||
remote_host_key: remote_host_key_encoded.clone(), | ||
remote_fingerprint: remote_fingerprint.clone(), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the latent_errors
part should be in an else block. Otherwise a matching entry will always add a latent error. Although that won't cause any harm as it is written now, I think it would be good to avoid adding an incorrect error object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I'm sorry about that, I made that last change without thinking enough it seems. I've fixed this as suggested now.
Thanks! @bors r+ |
☀️ Test successful - checks-actions |
12 commits in e84a7928d93a31f284b497c214a2ece69b4d7719..82c3bb79e3a19a5164e33819ef81bfc2c984bc56 2023-01-31 22:18:09 +0000 to 2023-02-04 22:52:16 +0000 - util toml targets: Do not infer directory as a file (rust-lang/cargo#11678) - Add more guidance on how to implement unstable features (rust-lang/cargo#11675) - Fix unstable chapter layout for codegen-backend (rust-lang/cargo#11676) - refactor: mod.rs over "name".rs for consistency (rust-lang/cargo#11673) - Verify source before recompile (rust-lang/cargo#11672) - doc: more doc comments and intra-doc links (rust-lang/cargo#11669) - Turn off debuginfo for build dependencies v2 (rust-lang/cargo#11252) - config: Deny CARGO_HOME in [env] table (fixes rust-lang/cargo#11590) (rust-lang/cargo#11644) - Fix the wrong comment (rust-lang/cargo#11651) - Add partial support for SSH known hosts markers (rust-lang/cargo#11635) - Replace `winapi` with `windows-sys` crate. (rust-lang/cargo#11656) - Handle .cargo-ok being truncated (rust-lang/cargo#11665)
Update cargo 12 commits in e84a7928d93a31f284b497c214a2ece69b4d7719..82c3bb79e3a19a5164e33819ef81bfc2c984bc56 2023-01-31 22:18:09 +0000 to 2023-02-04 22:52:16 +0000 - util toml targets: Do not infer directory as a file (rust-lang/cargo#11678) - Add more guidance on how to implement unstable features (rust-lang/cargo#11675) - Fix unstable chapter layout for codegen-backend (rust-lang/cargo#11676) - refactor: mod.rs over "name".rs for consistency (rust-lang/cargo#11673) - Verify source before recompile (rust-lang/cargo#11672) - doc: more doc comments and intra-doc links (rust-lang/cargo#11669) - Turn off debuginfo for build dependencies v2 (rust-lang/cargo#11252) - config: Deny CARGO_HOME in [env] table (fixes rust-lang/cargo#11590) (rust-lang/cargo#11644) - Fix the wrong comment (rust-lang/cargo#11651) - Add partial support for SSH known hosts markers (rust-lang/cargo#11635) - Replace `winapi` with `windows-sys` crate. (rust-lang/cargo#11656) - Handle .cargo-ok being truncated (rust-lang/cargo#11665) r? `@ghost`
Update cargo 12 commits in e84a7928d93a31f284b497c214a2ece69b4d7719..82c3bb79e3a19a5164e33819ef81bfc2c984bc56 2023-01-31 22:18:09 +0000 to 2023-02-04 22:52:16 +0000 - util toml targets: Do not infer directory as a file (rust-lang/cargo#11678) - Add more guidance on how to implement unstable features (rust-lang/cargo#11675) - Fix unstable chapter layout for codegen-backend (rust-lang/cargo#11676) - refactor: mod.rs over "name".rs for consistency (rust-lang/cargo#11673) - Verify source before recompile (rust-lang/cargo#11672) - doc: more doc comments and intra-doc links (rust-lang/cargo#11669) - Turn off debuginfo for build dependencies v2 (rust-lang/cargo#11252) - config: Deny CARGO_HOME in [env] table (fixes rust-lang/cargo#11590) (rust-lang/cargo#11644) - Fix the wrong comment (rust-lang/cargo#11651) - Add partial support for SSH known hosts markers (rust-lang/cargo#11635) - Replace `winapi` with `windows-sys` crate. (rust-lang/cargo#11656) - Handle .cargo-ok being truncated (rust-lang/cargo#11665) r? `@ghost`
Add partial support for SSH known hosts markers ### What does this PR try to resolve? The SSH `known_hosts` file parsing in Cargo did not previously support markers. Markers are modifiers on the lines (``@cert-authority`` and ``@revoked`)` which denote special behavior for the details on that line. Lines were skipped entirely. This silent skipping of marker lines can be confusing to a user, who sees that their command line Git/SSH client works for some repository, but Cargo reports that no host key is found. This change adds support for the ``@revoked`` marker. This marker denotes that a key should be rejected outright. It is of limited use without ``@cert-authority`` marker support. However, if it is present in a user's `known_hosts` file, then Cargo definitely shouldn't accept that key and probably shouldn't suggest that the user add it to their `known_hosts` either. The change also adds support for detecting ``@cert-authority`` markers in `known_hosts` files. These lines cannot yet be used for host key verification, but if one is found for a matching host, the user will be informed that Cargo doesn't support ``@cert-authority`` markers in the error message. Additionally, the user will be advised to use the `net.git-fetch-with-cli` config option to use the command line git client for fetching crates from Git. Refs: rust-lang#11577 ### How should we test and review this PR? The changes in this PR are covered by unit tests, all within `src/cargo/sources/git/known_hosts.rs`. Additionally, manual testing can be performed. For this you will need an OpenSSH server (it doesn't need to be a Git server). I'll assume that you have one running on your local machine at `127.0.0.1`. #### Setup 1. Create a new Cargo project and add the following line to `[dependencies]`: ```toml fake-crate = { git = "ssh://127.0.0.1/fake-crate.git" } ``` #### Test missing host key: `HostKeyNotFound` (existing functionality) 1. Back up your `known_hosts` file and then remove any lines for `127.0.0.1`. 2. Verify host key not present: `ssh 127.0.0.1`. SSH should tell you `The authenticity of host '127.0.0.1 (127.0.0.1)' can't be established.` 3. Run `cargo build` 4. Expect error from Cargo: `error: unknown SSH host key` #### Test ``@revoked`` key: `HostKeyRevoked` 1. Back up your `known_hosts` file and then remove any lines for `127.0.0.1`. 2. Add host key: `ssh 127.0.0.1` answer `yes` 3. Find all lines in `known_hosts` beginning with `127.0.0.1` (there may be multiple). 4. Add ``@revoked` ` to the beginning of all lines in (3) 5. Run `cargo build` 6. Expect error from Cargo: error: Key has been revoked for `127.0.0.1` #### Test `@cert-authority`` (not being supported): `HostHasOnlyCertAuthority` 1. Back up your `known_hosts` file and then remove any lines for `127.0.0.1`. 2. Run `cargo build` 3. Expect error from Cargo: `error: unknown SSH host key` 4. Check the line after ` The key to add is:` in the error message and copy the key type (e.g. `ecdsa-sha2-nistp256`) 5. Add a line to `known_hosts`: ``@cert-authority` 127.0.0.1 <key-type> AAAAB5Wm` (e.g. ``@cert-authority` 127.0.0.1 ecdsa-sha2-nistp256 AAAAB5Wm`) 7. Run `cargo build` 8. Expect error from Cargo: error: Found a ``@cert-authority`` marker for `127.0.0.1` ### Additional information Cargo doesn't currently support a few things when checking host keys. This may affect the testing described above. * Multiple host key types (OpenSSH negotiates the host key type and can support matching the one present in the `known_hosts` file even when it's not the preferred type of the server). * Wildcard matching of host patterns (there's a FIXME for this) More information about SSH known host markers can be found on rust-lang#11577.
Note that this will be included in the upcoming Rust 1.68.2 point release, scheduled for 2023-03-28, as it's a prerequisite for #11889. |
[stable] Prepare Rust 1.68.2 This PR prepares the 1.68.2 point release, including the following Cargo PRs: * rust-lang/cargo#11883 * rust-lang/cargo#11889 The following PR was also included as a pre-requisite for backporting: * rust-lang/cargo#11635 cc `@rust-lang/release` r? `@ghost`
Pkgsrc changes: * Adjust patches (add & remove) and cargo checksums to new versions. * It's conceivable that the workaround for LLVM based NetBSD works even less in this version (ref. PKGSRC_HAVE_LIBCPP not having a corresponding patch anymore). Upstream changes: Version 1.68.2 (2023-03-28) =========================== - [Update the GitHub RSA host key bundled within Cargo] (rust-lang/cargo#11883). The key was [rotated by GitHub] (https://github.blog/2023-03-23-we-updated-our-rsa-ssh-host-key/) on 2023-03-24 after the old one leaked. - [Mark the old GitHub RSA host key as revoked] (rust-lang/cargo#11889). This will prevent Cargo from accepting the leaked key even when trusted by the system. - [Add support for `@revoked` and a better error message for `@cert-authority` in Cargo's SSH host key verification] (rust-lang/cargo#11635) Version 1.68.1 (2023-03-23) =========================== - [Fix miscompilation in produced Windows MSVC artifacts] (rust-lang/rust#109094) This was introduced by enabling ThinLTO for the distributed rustc which led to miscompilations in the resulting binary. Currently this is believed to be limited to the -Zdylib-lto flag used for rustc compilation, rather than a general bug in ThinLTO, so only rustc artifacts should be affected. - [Fix --enable-local-rust builds] (rust-lang/rust#109111) - [Treat `$prefix-clang` as `clang` in linker detection code] (rust-lang/rust#109156) - [Fix panic in compiler code] (rust-lang/rust#108162) Version 1.68.0 (2023-03-09) =========================== Language -------- - [Stabilize default_alloc_error_handler] (rust-lang/rust#102318) This allows usage of `alloc` on stable without requiring the definition of a handler for allocation failure. Defining custom handlers is still unstable. - [Stabilize `efiapi` calling convention.] (rust-lang/rust#105795) - [Remove implicit promotion for types with drop glue] (rust-lang/rust#105085) Compiler -------- - [Change `bindings_with_variant_name` to deny-by-default] (rust-lang/rust#104154) - [Allow .. to be parsed as let initializer] (rust-lang/rust#105701) - [Add `armv7-sony-vita-newlibeabihf` as a tier 3 target] (rust-lang/rust#105712) - [Always check alignment during compile-time const evaluation] (rust-lang/rust#104616) - [Disable "split dwarf inlining" by default.] (rust-lang/rust#106709) - [Add vendor to Fuchsia's target triple] (rust-lang/rust#106429) - [Enable sanitizers for s390x-linux] (rust-lang/rust#107127) Libraries --------- - [Loosen the bound on the Debug implementation of Weak.] (rust-lang/rust#90291) - [Make `std::task::Context` !Send and !Sync] (rust-lang/rust#95985) - [PhantomData layout guarantees] (rust-lang/rust#104081) - [Don't derive Debug for `OnceWith` & `RepeatWith`] (rust-lang/rust#104163) - [Implement DerefMut for PathBuf] (rust-lang/rust#105018) - [Add O(1) `Vec -> VecDeque` conversion guarantee] (rust-lang/rust#105128) - [Leak amplification for peek_mut() to ensure BinaryHeap's invariant is always met] (rust-lang/rust#105851) Stabilized APIs --------------- - [`{core,std}::pin::pin!`] (https://doc.rust-lang.org/stable/std/pin/macro.pin.html) - [`impl From<bool> for {f32,f64}`] (https://doc.rust-lang.org/stable/std/primitive.f32.html#impl-From%3Cbool%3E-for-f32) - [`std::path::MAIN_SEPARATOR_STR`] (https://doc.rust-lang.org/stable/std/path/constant.MAIN_SEPARATOR_STR.html) - [`impl DerefMut for PathBuf`] (https://doc.rust-lang.org/stable/std/path/struct.PathBuf.html#impl-DerefMut-for-PathBuf) These APIs are now stable in const contexts: - [`VecDeque::new`] (https://doc.rust-lang.org/stable/std/collections/struct.VecDeque.html#method.new) Cargo ----- - [Stabilize sparse registry support for crates.io] (rust-lang/cargo#11224) - [`cargo build --verbose` tells you more about why it recompiles.] (rust-lang/cargo#11407) - [Show progress of crates.io index update even `net.git-fetch-with-cli` option enabled] (rust-lang/cargo#11579) Misc ---- Compatibility Notes ------------------- - [Add `SEMICOLON_IN_EXPRESSIONS_FROM_MACROS` to future-incompat report] (rust-lang/rust#103418) - [Only specify `--target` by default for `-Zgcc-ld=lld` on wasm] (rust-lang/rust#101792) - [Bump `IMPLIED_BOUNDS_ENTAILMENT` to Deny + ReportNow] (rust-lang/rust#106465) - [`std::task::Context` no longer implements Send and Sync] (rust-lang/rust#95985) nternal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Encode spans relative to the enclosing item] (rust-lang/rust#84762) - [Don't normalize in AstConv] (rust-lang/rust#101947) - [Find the right lower bound region in the scenario of partial order relations] (rust-lang/rust#104765) - [Fix impl block in const expr] (rust-lang/rust#104889) - [Check ADT fields for copy implementations considering regions] (rust-lang/rust#105102) - [rustdoc: simplify JS search routine by not messing with lev distance] (rust-lang/rust#105796) - [Enable ThinLTO for rustc on `x86_64-pc-windows-msvc`] (rust-lang/rust#103591) - [Enable ThinLTO for rustc on `x86_64-apple-darwin`] (rust-lang/rust#103647) Version 1.67.0 (2023-01-26) ========================== Language -------- - [Make `Sized` predicates coinductive, allowing cycles.] (rust-lang/rust#100386) - [`#[must_use]` annotations on `async fn` also affect the `Future::Output`.] (rust-lang/rust#100633) - [Elaborate supertrait obligations when deducing closure signatures.] (rust-lang/rust#101834) - [Invalid literals are no longer an error under `cfg(FALSE)`.] (rust-lang/rust#102944) - [Unreserve braced enum variants in value namespace.] (rust-lang/rust#103578) Compiler -------- - [Enable varargs support for calling conventions other than `C` or `cdecl`.] (rust-lang/rust#97971) - [Add new MIR constant propagation based on dataflow analysis.] (rust-lang/rust#101168) - [Optimize field ordering by grouping m\*2^n-sized fields with equivalently aligned ones.] (rust-lang/rust#102750) - [Stabilize native library modifier `verbatim`.] (rust-lang/rust#104360) Added and removed targets: - [Add a tier 3 target for PowerPC on AIX] (rust-lang/rust#102293), `powerpc64-ibm-aix`. - [Add a tier 3 target for the Sony PlayStation 1] (rust-lang/rust#102689), `mipsel-sony-psx`. - [Add tier 3 `no_std` targets for the QNX Neutrino RTOS] (rust-lang/rust#102701), `aarch64-unknown-nto-qnx710` and `x86_64-pc-nto-qnx710`. - [Remove tier 3 `linuxkernel` targets] (rust-lang/rust#104015) (not used by the actual kernel). Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Merge `crossbeam-channel` into `std::sync::mpsc`.] (rust-lang/rust#93563) - [Fix inconsistent rounding of 0.5 when formatted to 0 decimal places.] (rust-lang/rust#102935) - [Derive `Eq` and `Hash` for `ControlFlow`.] (rust-lang/rust#103084) - [Don't build `compiler_builtins` with `-C panic=abort`.] (rust-lang/rust#103786) Stabilized APIs --------------- - [`{integer}::checked_ilog`] (https://doc.rust-lang.org/stable/std/primitive.i32.html#method.checked_ilog) - [`{integer}::checked_ilog2`] (https://doc.rust-lang.org/stable/std/primitive.i32.html#method.checked_ilog2) - [`{integer}::checked_ilog10`] (https://doc.rust-lang.org/stable/std/primitive.i32.html#method.checked_ilog10) - [`{integer}::ilog`] (https://doc.rust-lang.org/stable/std/primitive.i32.html#method.ilog) - [`{integer}::ilog2`] (https://doc.rust-lang.org/stable/std/primitive.i32.html#method.ilog2) - [`{integer}::ilog10`] (https://doc.rust-lang.org/stable/std/primitive.i32.html#method.ilog10) - [`NonZeroU*::ilog2`] (https://doc.rust-lang.org/stable/std/num/struct.NonZeroU32.html#method.ilog2) - [`NonZeroU*::ilog10`] (https://doc.rust-lang.org/stable/std/num/struct.NonZeroU32.html#method.ilog10) - [`NonZero*::BITS`] (https://doc.rust-lang.org/stable/std/num/struct.NonZeroU32.html#associatedconstant.BITS) These APIs are now stable in const contexts: - [`char::from_u32`] (https://doc.rust-lang.org/stable/std/primitive.char.html#method.from_u32) - [`char::from_digit`] (https://doc.rust-lang.org/stable/std/primitive.char.html#method.from_digit) - [`char::to_digit`] (https://doc.rust-lang.org/stable/std/primitive.char.html#method.to_digit) - [`core::char::from_u32`] (https://doc.rust-lang.org/stable/core/char/fn.from_u32.html) - [`core::char::from_digit`] (https://doc.rust-lang.org/stable/core/char/fn.from_digit.html) Compatibility Notes ------------------- - [The layout of `repr(Rust)` types now groups m\*2^n-sized fields with equivalently aligned ones.] (rust-lang/rust#102750) This is intended to be an optimization, but it is also known to increase type sizes in a few cases for the placement of enum tags. As a reminder, the layout of `repr(Rust)` types is an implementation detail, subject to change. - [0.5 now rounds to 0 when formatted to 0 decimal places.] (rust-lang/rust#102935) This makes it consistent with the rest of floating point formatting that rounds ties toward even digits. - [Chains of `&&` and `||` will now drop temporaries from their sub-expressions in evaluation order, left-to-right.] (rust-lang/rust#103293) Previously, it was "twisted" such that the _first_ expression dropped its temporaries _last_, after all of the other expressions dropped in order. - [Underscore suffixes on string literals are now a hard error.] (rust-lang/rust#103914) This has been a future-compatibility warning since 1.20.0. - [Stop passing `-export-dynamic` to `wasm-ld`.] (rust-lang/rust#105405) - [`main` is now mangled as `__main_void` on `wasm32-wasi`.] (rust-lang/rust#105468) - [Cargo now emits an error if there are multiple registries in the configuration with the same index URL.] (rust-lang/cargo#10592) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Rewrite LLVM's archive writer in Rust.] (rust-lang/rust#97485)
What does this PR try to resolve?
The SSH
known_hosts
file parsing in Cargo did not previously supportmarkers. Markers are modifiers on the lines (
@cert-authority
and@revoked
) which denote special behavior for the details on that line.Lines were skipped entirely.
This silent skipping of marker lines can be confusing to a user, who
sees that their command line Git/SSH client works for some repository,
but Cargo reports that no host key is found.
This change adds support for the
@revoked
marker. This marker denotesthat a key should be rejected outright. It is of limited use without
@cert-authority
marker support. However, if it is present in a user'sknown_hosts
file, then Cargo definitely shouldn't accept that key andprobably shouldn't suggest that the user add it to their
known_hosts
either.
The change also adds support for detecting
@cert-authority
markers inknown_hosts
files. These lines cannot yet be used for host keyverification, but if one is found for a matching host, the user will be
informed that Cargo doesn't support
@cert-authority
markers in theerror message. Additionally, the user will be advised to use the
net.git-fetch-with-cli
config option to use the command line gitclient for fetching crates from Git.
Refs: #11577
How should we test and review this PR?
The changes in this PR are covered by unit tests, all within
src/cargo/sources/git/known_hosts.rs
.Additionally, manual testing can be performed. For this you will need
an OpenSSH server (it doesn't need to be a Git server). I'll assume
that you have one running on your local machine at
127.0.0.1
.Setup
[dependencies]
:Test missing host key:
HostKeyNotFound
(existing functionality)known_hosts
file and then remove any lines for127.0.0.1
.ssh 127.0.0.1
. SSH should tell youThe authenticity of host '127.0.0.1 (127.0.0.1)' can't be established.
cargo build
error: unknown SSH host key
Test
@revoked
key:HostKeyRevoked
known_hosts
file and then remove any lines for127.0.0.1
.ssh 127.0.0.1
answeryes
known_hosts
beginning with127.0.0.1
(there may be multiple).@revoked
to the beginning of all lines in (3)cargo build
127.0.0.1
Test @cert-authority
(not being supported):
HostHasOnlyCertAuthority`known_hosts
file and then remove any lines for127.0.0.1
.cargo build
error: unknown SSH host key
The key to add is:
in the error message and copy the key type (e.g.ecdsa-sha2-nistp256
)known_hosts
:@cert-authority 127.0.0.1 <key-type> AAAAB5Wm
(e.g.@cert-authority 127.0.0.1 ecdsa-sha2-nistp256 AAAAB5Wm
)cargo build
@cert-authority
marker for127.0.0.1
Additional information
Cargo doesn't currently support a few things when checking host keys. This may affect the testing described above.
known_hosts
file even when it's not the preferred type of the server).More information about SSH known host markers can be found
on #11577.