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

Enable user-level management of index credentials via uv (& keyring) #9920

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

stoney95
Copy link

@stoney95 stoney95 commented Dec 15, 2024

Summary

Currently reading credentials from keyring is only supported when a username is provided in the URL of the index. This prohibits to define indexes - in pyproject.toml - that are shared within a team. See these two issues for further details:

In general this PR provides two things:

Setting credentials for an index via CLI

With this MR you can run uv index credentials add --name <name-of-the-index> [--username <username>]
This will ask for the password of the user. A keyring entry will be made with the url of the index, the username and the password. The username and the index will be appended to "<uv_cache_dir>/auth.toml". "auth.toml" has the following structure

[indexes.index1]
username = "my_user1"

[indexes.index2]
username = "my_other_user"

Using credentials

When reading the credentials in uv_distribution_types::index::Index.credentials it's now additionally checked if credentials have been configured via "<uv_cache_dir>/auth.toml" and keyring. The current implementation of reading the credentials from the environment variables UV_INDEX_XXX has priority over keyring authentication.

Test Plan

I have added unit tests to the newly defined uv_auth::keyring_config which takes care of loading, storing and modifying the auth configuration.

I also want to add a test for uv_auth::credentials::Credentials.from_keyring. But am currently struggling with mocking keyring and config file.

I manually tested the new command uv index credentials add --name <name-of-the-index> as I could not find examples for testing commands.

Further remarks

I am a noobie in this context

  • I am new to rust
  • I am new to uv

=> I am curious about your opinion and suggestions for improvements

Not completely ready

The current version of the PR is a draft. Especially in regards of error handling and logging. Also testing can be improved.
I open this PR to discuss the direction in which the implementation is heading. As I am new to rust & uv I would also like to receive your guideance upfront 🙂

@stoney95 stoney95 marked this pull request as draft December 16, 2024 07:32
@bschoenmaeckers
Copy link
Contributor

It would be nice to add a --password <password> option and store/retrieve it using the keyring crate. It should still fall back to the subprocess method for dynamic passwords.

@zanieb zanieb self-assigned this Dec 16, 2024
@stoney95
Copy link
Author

@bschoenmaeckers, I think adding --password <password> can be seen independently of the keyring crate (https://docs.rs/keyring/latest/keyring/?).

One part is implementing an import keyring-provider. This would rely on the keyring crate. Also, it should be implemented independently of this topic, if it is required.

The other part is providing a --password option. Which could be processed with any implemented keyring-provider - currently only subprocess. I can add this part to this PR.

@farndt
Copy link

farndt commented Dec 17, 2024

@stoney95 Thx for starting integration of kering-rs into uv!

How to use credentials already defined in credential store?

@stoney95
Copy link
Author

@zanieb, clippy is configured to not allow the usage of fs::File, fs::read_to_string & fs::remove_file (see this check). Should the async version provided by tokio be used instead? Or is there another reason why the usage is not allowed?

@lejmr
Copy link

lejmr commented Jan 2, 2025

I was super curious whether it would be difficult to interact with keyring-rs directly, so I quickly implemented alternative Credential constructor (tests are necessary!), but it works with locally configured auth.toml and hand-crafted secret in keychain already..

I like what you tried to achieve, but I think it would be good to implement the configuration to be similar to poetry, so users don't have to think and be surprised.

Have a look here: https://github.com/astral-sh/uv/compare/main...lejmr:uv:simple-poetry-like-version?expand=1

@stoney95
Copy link
Author

@lejmr, thanks for your suggestion. To clarify, I see two suggestions made from your end.

  1. How can we make use of the keyring crate.
  2. Let's discuss the naming of the index command.

Do you agree with this summary?

Personal opinion

keyring crate

As mentioned before, I think the discussion of using the keyring crate is independent of this topic. As there is a global keyring provider (see the uv docs, and code section), it should be controlled via this if the keyring crate (use --keyring-provider import) or a subprocess calling keyring (use --keyring-provider subprocess) is used.

Personally I don't understand what's the benefit of leaving the choice to the user. Also, I would go for using the keyring crate. But as the global keyring provider is currently in place, we should not just ignore it.
I would be very glad, if somebody could explain, why the choice of the keyring provider is left to the user.

Index command naming

With the current implementation the credentials could be set with uv index credentials add --name <name-of-the-index>.

You suggest to follow the wording of poetry to make it easier for users to adopt, e.g. uv config http-basic.<name-of-the-index> <username> <password>
For reference see the poetry docs.

Personally I find the wording of poetry misleading, but am curious about other opinions.

@zanieb
Copy link
Member

zanieb commented Jan 12, 2025

Hey @stoney95 sorry I missed your ping!

Reviewing this is on my queue but I'm working through that holiday backlog still.

Regarding

@zanieb, clippy is configured to not allow the usage of fs::File, fs::read_to_string & fs::remove_file (see this check).

We use fs_err instead where we can, because it includes additional context in error messages.

@lejmr
Copy link

lejmr commented Jan 12, 2025

@stoney95, let me write my grains of salt..

  1. Naming.. I don't have strong opinion about uv command structure - uv index credentials make sense to me. What is important to understand, is that uv is lacking ability to define username/password out of the pyproject.toml or lock files, and this functionality is critical for team cooperation! In other words, uv auth would be good for me too. Basically, the core of this functionality should be "user interface for managing usernames per index", and password management is just a bonus. (no strong naming opinion here!)

Add/Rm etc is bad naming to me because what really happens is upsert, so add is confusing.

  1. crate from keyring. Well, I primarily wanted to test what is possible. I think using subprocess is better since the official keyring has support for "3rd party backends" hence it is more flexible. Using the keyring-rs should be implemented as native flavor to Keyprovide in my opinion, but having proper subprocess can be quite sufficient!

Regards 2), I think your implementation is just more complicated than what is necessary. I took shortcut by implementing Credentials::from_keyring function and hacking it into Index::credentials function. What I think is proper:

  • Credentials::from_request can load username from auth.toml
  • AuthMiddleware::handle loads credentials on the first line, so if we return credentials with just username we can then let rest flow ... keyring will look up for given URL&username

@stoney95
Copy link
Author

@lejmr, thanks for explaining your suggestion in more details :)

Naming

I would opt for uv index credentials {set|unset|list}. This opens up the possibility to add further index commands in the future, e.g. uv index set which can then guide a user to configure an index. This could be nice to have.

Keyring crate / Complicated implementation

I am using Credentials::from_keyring in Index::credentials. Before my adjustments this would first check if credentials are present in env variables. As a second option it will check if it can retrieve credentials from the url.

If I understand your suggestion correctly we could modify Credentials::from_url instead of trying to load credentials from keyring directly.

So, we could make Credentials::from_keyring private and call it from within Credentials::from_url when no username and password are encoded in the url. What's your opinion on that? (@zanieb, I would also be interested to hear your opinion on that)

@lejmr
Copy link

lejmr commented Jan 13, 2025

Keyring) if we modify ::from_url, so it loads username from auth.toml for the given URL, we don't need the from_keyring at all because the secret is loaded by middleware, https://github.com/stoney95/uv/blob/main/crates/uv-auth/src/middleware.rs#L203 - which is already existing code, so we won't create any duplicity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants