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

Add support for defining credentials for uv in an environment variable #6389

Closed
wants to merge 1 commit into from

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Aug 21, 2024

Adds UV_AUTH_URLS which defines credentials that should be used for URLs if we make a request there.

This is helpful for cases where, e.g., you want to provide credentials for an index but you don't want to change the index URL semantics by setting UV_INDEX_URL.

Implemented by seeding the URL authentication cache with the value of the variable.

Supports things like:

  • UV_AUTH_URLS="username:password@hostname"
  • UV_AUTH_URLS="username@hostname"
  • UV_AUTH_URLS="username:password@hostname username:password@other-hostname"
  • UV_AUTH_URLS="username:password@hostname/prefix"

There's a small caveat here, that if you use an index URL like foo@hostname/simple and you provide a password bar:password@hostname — the password should not be used but requests to fetch wheels from a different, parent path e.g.hostname/files can allow use of bar:password. This requires #4583 to address. In the meantime, credentials should just be properly scoped.

@zanieb zanieb force-pushed the zb/auth-urls branch 2 times, most recently from 9c5715f to 516620b Compare August 22, 2024 18:10
@zanieb zanieb force-pushed the zb/auth-urls branch 7 times, most recently from 2c94c2f to 25abe36 Compare August 22, 2024 18:54
@zanieb
Copy link
Member Author

zanieb commented Aug 22, 2024

I want to write documentation for this and make sure it's actually solving people's problems, but the implementation is ready for review.

@zanieb zanieb marked this pull request as ready for review August 22, 2024 23:07
@zanieb zanieb added enhancement New feature or improvement to existing functionality configuration Settings and such labels Aug 22, 2024
zanieb added a commit that referenced this pull request Aug 23, 2024
While working on #6389 I discovered
we never checked `cache.get_url` here, which is wrong — though I don't
think it had much effect in practice since the realm would typically
match first. The main problem is that when we call `get_url` later we
hard-code the username to `None` because we assume we checked up here
with the username if present.
@charliermarsh
Copy link
Member

Maybe UV_BASIC_AUTH_URLS? Feel free to ignore though.

@zanieb
Copy link
Member Author

zanieb commented Aug 26, 2024

I don't love my name, so maybe. Will consider...

@zanieb zanieb enabled auto-merge (squash) August 26, 2024 19:25
@mkniewallner
Copy link
Contributor

Since auto-merge was enabled, apart from test and clippy failures, is there anything preventing the merge of this PR, or was it maybe just forgotten?

@zanieb
Copy link
Member Author

zanieb commented Sep 3, 2024

I'm not getting a ton of signal about how useful it would be so it's relatively low priority.

More relevant though, I was on vacation and am just now noticing auto-merge failed.

@charliermarsh
Copy link
Member

@zanieb -- My gut is to punt on this until we see more demand, but ultimately defer to you.

@mkniewallner
Copy link
Contributor

I'm not getting a ton of signal about how useful it would be so it's relatively low priority.

Definitely not high priority IMO. Personally I can leave without it, since we can pass the entire index including credentials with UV_INDEX_URL and UV_EXTRA_INDEX_URL. So it's really just like a convenient thing to have, as I like the idea of having the index explicitly set in [tool.uv]. On projects where a lot of developers work on, having this explicit configuration makes things more obvious (although in fact, even without the feature, you can still explicitly set the index in [tool.uv] and override the entire thing with the env var, but this kinda repeats something that is already set).

Do you think the feature would also fit what is planned in #171, or are we thinking about providing another mechanism to provide credentials for this other feature (e.g. what Poetry does, where the credentials are set for a source name by including the name in the header)? If we're not sure yet, maybe we can hold off on merging this PR until the other feature is implemented, to try to come up with something that would ideally fit both use cases?

@zanieb
Copy link
Member Author

zanieb commented Sep 3, 2024

@charliermarsh is going to start working on #171 soon and I think that will probably supersede this and include a design for index credentials.

@zanieb zanieb closed this Sep 24, 2024
auto-merge was automatically disabled September 24, 2024 16:58

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Settings and such enhancement New feature or improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants