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.

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

[stable] Backport GitHub RSA key revocation #11891

Merged
merged 6 commits into from
Mar 26, 2023

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Mar 26, 2023

Backports for 1.68.2 patch release:

bors and others added 6 commits March 26, 2023 10:28
…-key, r=arlosi

Added new GitHub RSA Host Key

GitHub rotated their RSA host key which means that cargo needs to update it.  Thankfully the other keys were not rotated so the impact depends on how cargo connected to github.

Refs https://github.blog/2023-03-23-we-updated-our-rsa-ssh-host-key/
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.
Add the old github keys as revoked

The patch to update the bundled ssh github host key did not change anything for users who already had connected to github one time before via ssh: if the attacker had access to the old key, they'd be vulnerable to MITM attacks as their known_hosts file would list the old github key. Only if they connected again to github without attacker access, or if they saw the announcement of the key rotation, they would update their key.

There is sadly no other way to distribute revocations of old host keys to clients other than to bundle them with client software.

cc rust-lang#11883
@rustbot
Copy link
Collaborator

rustbot commented Mar 26, 2023

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

@rustbot
Copy link
Collaborator

rustbot commented Mar 26, 2023

⚠️ Warning ⚠️

  • Pull requests are usually filed against the master branch for this repo, but this one is against rust-1.68.0. Please double check that you specified the right target!

@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation A-git Area: anything dealing with git S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2023
@pietroalbini
Copy link
Member

r=me with my release team hat (not sure if bors would let me approve in this repo)

@weihanglo
Copy link
Member

@bors r=pietroalbini

@bors
Copy link
Contributor

bors commented Mar 26, 2023

📌 Commit 8c2456b has been approved by pietroalbini

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2023
@bors
Copy link
Contributor

bors commented Mar 26, 2023

⌛ Testing commit 8c2456b with merge 6feb7c9...

@ehuss ehuss added this to the 1.68.2 milestone Mar 26, 2023
@bors
Copy link
Contributor

bors commented Mar 26, 2023

☀️ Test successful - checks-actions
Approved by: pietroalbini
Pushing 6feb7c9 to rust-1.68.0...

@bors bors merged commit 6feb7c9 into rust-lang:rust-1.68.0 Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation A-git Area: anything dealing with git S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants