Retain passwords in Git URLs #1717
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zanieb
force-pushed
the
zb/git-auth-pat
branch
from
d260643 to
304ffdf
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
zanieb
force-pushed
the
zb/git-auth-pat
branch
from
ac1fa25 to
e868ce8
Compare
zanieb
force-pushed
the
zb/git-auth-pat
branch
from
e868ce8 to
8aa27b6
Compare
zanieb
force-pushed
the
zb/git-auth-pat
branch
from
d176fa6 to
ee4f164
Compare
zanieb
commented
Feb 20, 2024
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); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is awkward, is there a better pattern?
There was a problem hiding this comment.
I'll play with it, I'm not sure...
There was a problem hiding this comment.
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);
}
}There was a problem hiding this comment.
Still kind of strange but it seems better.
There was a problem hiding this comment.
Like... mildly better because it doesn't clone the suffix, I guess, and it's contained without another binding.
There was a problem hiding this comment.
In the other case we need to clone the suffix too lol
There was a problem hiding this comment.
The cloned original URL bit is weird though I prefer this
zanieb
force-pushed
the
zb/git-auth-pat
branch
5 times, most recently
from
17b1f1a to
03a1e07
Compare
zanieb
force-pushed
the
zb/git-auth-pat
branch
from
03a1e07 to
2a5bebe
Compare
charliermarsh
approved these changes
Feb 20, 2024
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.
charliermarsh
pushed a commit
that referenced
this pull request
Feb 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes handling of GitHub PATs in HTTPS URLs, which were otherwise dropped. We now supporting the following authentication schemes:
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: