Skip to content

Retain passwords in Git URLs #1717

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

Merged
merged 14 commits into from
Feb 21, 2024
Merged

Retain passwords in Git URLs #1717

merged 14 commits into from
Feb 21, 2024

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Feb 19, 2024

Fixes handling of GitHub PATs in HTTPS URLs, which were otherwise dropped. We now supporting the following authentication schemes:

git+https://<user>:<token>/...
git+https://<token>/...

On Windows, the username is required. We can consider adding a special-case for this in the future, but this just matches libgit2's behavior.

I tested with fine-grained tokens, OAuth tokens, and "classic" tokens. There's test coverage for fine-grained tokens in CI where we use a real private repository and PAT. Yes, the PAT is committed to make this test usable by anyone. It has read-only permissions to the single repository, expires Feb 1 2025, and is in an isolated organization and GitHub account.

Does not yet address SSH authentication (see #1781)

Related:

@zanieb zanieb added the bug Something isn't working label Feb 19, 2024
@zanieb

This comment was marked as outdated.

@zanieb

This comment was marked as outdated.

Comment on lines 110 to 118
// Clone to avoid borrow checker issues
let immutable_url = url.clone();

// If a Git URL ends in a reference (like a branch, tag, or commit), remove it.
if url.scheme().starts_with("git+") {
if let Some((prefix, _)) = url.as_str().rsplit_once('@') {
url = prefix.parse().unwrap();
if let Some((prefix, _)) = immutable_url.path().rsplit_once('@') {
url.set_path(prefix);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awkward, is there a better pattern?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll play with it, I'm not sure...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One option:

if url.scheme().starts_with("git+") {
    if let Some(prefix) = url.path().rsplit_once('@').map(|(prefix, _suffix)| prefix.to_string()) {
        url.set_path(&prefix);
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still kind of strange but it seems better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like... mildly better because it doesn't clone the suffix, I guess, and it's contained without another binding.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the other case we need to clone the suffix too lol

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cloned original URL bit is weird though I prefer this

@zanieb zanieb marked this pull request as ready for review February 20, 2024 18:57
@zanieb zanieb force-pushed the zb/git-auth-pat branch 5 times, most recently from 17b1f1a to 03a1e07 Compare February 20, 2024 20:16
@zanieb zanieb enabled auto-merge (squash) February 21, 2024 00:07
@zanieb zanieb merged commit d07b587 into main Feb 21, 2024
@zanieb zanieb deleted the zb/git-auth-pat branch February 21, 2024 00:12
zanieb added a commit that referenced this pull request Feb 21, 2024
…st SSH support (#1781)

Closes #1775
Closes #1452
Closes #1514
Follows #1717

libgit2 does not support host names with extra identifiers during SSH
lookup (e.g. [`g.yxqyang.asia-some_identifier`](

https://docs.github.com/en/authentication/connecting-to-github-with-ssh/managing-deploy-keys#using-multiple-repositories-on-one-server))
so we use the `git` command instead for fetching. This is required for
`pip` parity.

See the [Cargo
documentation](https://doc.rust-lang.org/nightly/cargo/reference/config.html#netgit-fetch-with-cli)
for more details on using the `git` CLI instead of libgit2. We may want
to try to use libgit2 first in the future, as it is more performant
(#1786).

We now support authentication with:

```
git+ssh://git@<hostname>/...
git+ssh://git@<hostname>-<identifier>/...
```

Tested with a deploy key e.g.

```
cargo run -- \
    pip install uv-private-pypackage@git+ssh://[email protected]/astral-test/uv-private-pypackage.git \
    --reinstall --no-cache -v
```

and

```
cargo run -- \
    pip install uv-private-pypackage@git+ssh://[email protected]/astral-test/uv-private-pypackage.git \
    --reinstall --no-cache -v     
```

with a ssh config like

```
Host github.com
        Hostname github.com
        IdentityFile=/Users/mz/.ssh/id_ed25519

Host g.yxqyang.asia-test-uv-private-pypackage
        Hostname github.com
        IdentityFile=/Users/mz/.ssh/id_ed25519
```

It seems quite hard to add test coverage for this to the test suite, as
we'd need to add the SSH key and I don't know how to isolate that from
affecting other developer's machines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants