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

Expand environment variables in user-provided index URLs #5734

Open
charliermarsh opened this issue Aug 2, 2024 · 15 comments
Open

Expand environment variables in user-provided index URLs #5734

charliermarsh opened this issue Aug 2, 2024 · 15 comments
Labels
compatibility Compatibility with a specification or another tool needs-decision Undecided if this should be done

Comments

@charliermarsh
Copy link
Member

Right now, we only support expansion (1) by the shell (of course) and (2) in requirements files. But the following don't expand:

  • cargo run pip install flask --index-url 'https://${PYPI_USERNAME}.com/repository/pypi-proxy/simple/flask/'
  • Anything in a uv.toml or pyproject.toml file
@charliermarsh charliermarsh added the compatibility Compatibility with a specification or another tool label Aug 2, 2024
@charliermarsh
Copy link
Member Author

It looks like pip does not support either of these.

@charliermarsh
Copy link
Member Author

charliermarsh commented Aug 2, 2024

Poetry does not support the latter: python-poetry/poetry#208

@charliermarsh
Copy link
Member Author

Rust does not support this but the issue is not closed: rust-lang/cargo#10789

@charliermarsh
Copy link
Member Author

I am not strictly opposed to it but we should get some more opinions (\cc @zanieb @BurntSushi @konstin).

@chrisrodrigue
Copy link

PDM supports this: Store credentials with the index, Local dependencies

@chrisrodrigue
Copy link

Hatch also supports this: Context formatting

@eth3lbert
Copy link
Contributor

I looked around Poetry's document and found this https://python-poetry.org/docs/configuration/#http-basicnameusernamepassword .

@CoolCat467
Copy link

I know I'm not a maintainer of uv, but I strongly suggest that if environment lookups were added that they be opt-in. I've heard of things going very bad security wise from environment variables.

@zanieb
Copy link
Member

zanieb commented Aug 3, 2024

Can you share more details or examples please @CoolCat467?

@zanieb
Copy link
Member

zanieb commented Aug 3, 2024

Something like --allow-variable <NAME> might make sense for opt-in per-variable. We probably want to consider how we want the UX to work for lockfiles and credentials in general. I think expanding environment variables is probably not the best solution, it seems like we'd be better off finding a matching URL in our configuration file and pulling authentication from there (we need and want to support declaring indexes anyway).

@eth3lbert
Copy link
Contributor

eth3lbert commented Aug 3, 2024

@chrisrodrigue
Copy link

Related: #5119

@chrisrodrigue
Copy link

chrisrodrigue commented Aug 3, 2024

@CoolCat467

I know I'm not a maintainer of uv, but I strongly suggest that if environment lookups were added that they be opt-in. I've heard of things going very bad security wise from environment variables.

Currently uv.lock exposes API keys in source URLs which means a lot of users can’t check it into version control.

One of the strengths of environment variables is avoiding version control. Most VCSs support masking environment variables from job/log output when expanded. See GitLab and GitHub docs.

They aren’t 100% foolproof. I think there’s a tradeoff between flexibility and security.

@BurntSushi
Copy link
Member

cargo run pip install flask --index-url 'https://${PYPI_USERNAME}.com/repository/pypi-proxy/simple/flask/'

I think I would personally find it very surprising if an environment variable got expanded here. Basically, if I'm running a Unix shell command and I use single quotes, I have a very strong prior that the string is interpreted literally with no interpolation.

Something like --allow-variable <NAME> might make sense for opt-in per-variable. We probably want to consider how we want the UX to work for lockfiles and credentials in general. I think expanding environment variables is probably not the best solution, it seems like we'd be better off finding a matching URL in our configuration file and pulling authentication from there (we need and want to support declaring indexes anyway).

👍 on this (including that env vars are maybe not the best solution to this specific problem), but having an explicit --allow-variable I think would help mitigate some concerns here.

@charliermarsh charliermarsh added the needs-decision Undecided if this should be done label Aug 6, 2024
@jfgordon2
Copy link

I'm currently looking at using uv instead of pipenv for our projects, and env var expansion is something we use with pipenv and our AWS CodeArtifact repo. Not having this makes things a bit more complicated to use uv (i.e. we can't easily specify in the pyproject.toml file where the packages are located without constant updates since the index url contains the auth and it's only good for a limited time).

I'm not sure I understand the concern around env var expansion within a url for this use-case, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Compatibility with a specification or another tool needs-decision Undecided if this should be done
Projects
None yet
Development

No branches or pull requests

7 participants