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

uv lock should redact username and password from source #5119

Closed
ewianda opened this issue Jul 16, 2024 · 20 comments · Fixed by #5803
Closed

uv lock should redact username and password from source #5119

ewianda opened this issue Jul 16, 2024 · 20 comments · Fixed by #5803
Assignees
Labels
bug Something isn't working preview Experimental behavior

Comments

@ewianda
Copy link

ewianda commented Jul 16, 2024

eg

 
 [[distribution]]
 name = "absl-py"
 version = "2.0.0"
source = { registry = "https://username:secrete-token@private-registry/registry/pypi/simple/" }

to

[[distribution]]
 name = "absl-py"
 version = "2.0.0"
source = { registry = "https://private-registry/registry/pypi/simple/" }

similar to 

https://github.com/astral-sh/uv/issues/3106

uv --version
uv 0.2.24

similar #3106

@zanieb
Copy link
Member

zanieb commented Jul 16, 2024

Also related #1714

@zanieb zanieb added bug Something isn't working preview Experimental behavior labels Jul 16, 2024
@charliermarsh
Copy link
Member

Not sure what to do here -- do we really want to strip the credentials entirely? If so, how would users install from the lockfile?

@ewianda
Copy link
Author

ewianda commented Jul 17, 2024

Won't redefining the index url with the authorization in cli or environment variable work?. I see two issues

  1. Keeping the username and password means the lock file cannot be checked into version control.
  2. For Google artifact registry for example, the password is user specific authorization token that expires

@zanieb
Copy link
Member

zanieb commented Jul 17, 2024

How are the credentials being provided in the first place here?

@charliermarsh
Copy link
Member

Should we just omit username and password entirely then from the lockfile, in all cases? AFAICT that's the only option available to us.

@ewianda
Copy link
Author

ewianda commented Jul 18, 2024

How are the credentials being provided in the first place here?

UV_INDEX_URL=https://oauth2accesstoken:${ARTIFACT_REGISTRY_TOKEN}@us-python.pkg.dev/private/private/simple/

or cli --index-url

@zanieb
Copy link
Member

zanieb commented Jul 18, 2024

I presume the environment variable is being resolved by your shell then? e.g. the first case in

❯ export VAR='SECRET'
❯ echo "$VAR"
SECRET
❯ echo '$VAR'
$VAR

If so, we don't know anything about the use of an environment variable and cannot use it to template the URL. I guess we might need to encourage setting the index URL with ' so it's preserved and we resolve it to a value ourselves?

Ideally we'd at least retain the username. However, sometimes it's ambiguous if we should use something in the username position as a password.

@charliermarsh
Copy link
Member

If we drop the credentials from the lockfile itself, then re-run with the index URL provided as above, do we properly propagate them?

@zanieb
Copy link
Member

zanieb commented Jul 19, 2024

I don't think we do any environment variable propagation today, we'd need to add that.

@charliermarsh charliermarsh self-assigned this Jul 30, 2024
@paveldikov
Copy link
Contributor

How are the credentials being provided in the first place here?

We use uv.toml for this same purpose. We largely regard the choice of registry as a centrally-mandated choice -- in fact, given a choice, I would actively want to disregard a reference to an unapproved registry, and always force downloads to go through our internal mirrors.

@chrisrodrigue
Copy link

chrisrodrigue commented Aug 2, 2024

I think environment variable parsing/propagation from pyproject.toml would be important to hide secrets in both the repo and CI/CD.

For example, I expected this to work:

[tool.uv.pip]
native-tls = true
index-url = "https://${PYPI_USERNAME}:${PYPI_PASSWORD}@mycompany.com/repository/pypi-proxy/simple/"

I expected uv to only pull from the configured index-url and not fall back to default pypi, for security purposes (to prevent typosquatting/malicious package replacements upstream).

In GitLab, project variables can be used as environment variables in the CI/CD pipeline. Below is a GitLab job template that installs uv and the project dependencies into an alpine-python container that already has pip (which is then extended for all python-related jobs in the pipeline).

.job-template:
  image: python:3.12-alpine
  rules:
    - if: $CI_PIPELINE_SOURCE == "merge_request_event"
    - if: $CI_COMMIT_BRANCH && $CI_OPEN_MERGE_REQUESTS
      when: never
    - if: $CI_PIPELINE_SOURCE == "push"
  variables:
    PIP_CACHE_DIR: "$CI_PROJECT_DIR/.cache/pip"
    UV_CACHE_DIR: "$CI_PROJECT_DIR/.cache/uv"
  cache:
    key:
      files:
        - uv.lock
    paths:
      - .cache/pip
      - .cache/uv
      - .venv
    policy: pull
    - pip config set global.cert /my_company.pem
    - pip config set global.index-url "https://${PYPI_USERNAME}:${PYPI_PASSWORD}@mycompany.com/repository/pypi-proxy/simple/"
    - pip install uv
    - uv sync

@charliermarsh
Copy link
Member

charliermarsh commented Aug 2, 2024

@chrisrodrigue -- I think that's actually intended to work, I'm somewhat surprised that it doesn't. But isn't it more common for those env vars to be expanded automatically by the shell, such that the value written to the pip config does include the raw credentials?

In other words: I believe that pip config set global.index-url "https://${PYPI_USERNAME}:${PYPI_PASSWORD}@mycompany.com/repository/pypi-proxy/simple/" does not write https://${PYPI_USERNAME}:${PYPI_PASSWORD}@mycompany.com/repository/pypi-proxy/simple/ to the config file -- it writes the expanded form.

@charliermarsh
Copy link
Member

[tool.uv.pip]
index-url = "https://${PYPI_USERNAME}:${PYPI_PASSWORD}@mycompany.com/repository/pypi-proxy/simple/"

This will not fall back to PyPI, but it's only used at all in the uv pip interface. If you want the index URL to apply to all uv commands, omit the pip:

[tool.uv]
index-url = "https://${PYPI_USERNAME}:${PYPI_PASSWORD}@mycompany.com/repository/pypi-proxy/simple/"

@charliermarsh
Copy link
Member

I added #5734.

@chrisrodrigue
Copy link

chrisrodrigue commented Aug 2, 2024

@chrisrodrigue -- I think that's actually intended to work, I'm somewhat surprised that it doesn't. But isn't it more common for those env vars to be expanded automatically by the shell, such that the value written to the pip config does include the raw credentials?

Yes they are expanded, and the value written to the pip config shows the raw credentials. I don't think this is a security issue since the pipeline container is ephemeral.

GitLab has a feature that masks the expanded variables from job logs... I assume GitHub supports a similar feature?

image

@chrisrodrigue
Copy link

chrisrodrigue commented Aug 2, 2024

But I don't think the masking extends to other files/artifacts such as the uv.lock.

I think some solutions would be for the uv.lock to either

(1) Completely strip the username/password fields

from

source = { registry = "https://actual_username:actual_password@mycompany.com/repository/pypi-proxy/simple/" }

to

source = { registry = "https://mycompany.com/repository/pypi-proxy/simple/" }

or

(2) retain the environment variable name without expansion, which could provide more useful context to a user attempting to debug or reproduce the environment

source = { registry = "https://${PYPI_USERNAME}:${PYPI_PASSWORD}@mycompany.com/repository/pypi-proxy/simple/" }

A happy medium could be to allow user-configurability a la uv lock | sync --expand-variables and default to not expanding variables to protect potential secrets from being exposed in plaintext. This would prevent secrets from being committed to a git repository in the uv.lock file unless explicitly opted in for applicable commands.

[tool.uv.options]
sync = ["--expand-variables"]
lock = ["--expand-variables"]

@chrisrodrigue
Copy link

chrisrodrigue commented Aug 3, 2024

The Hatch project has a slightly more verbose but powerful syntax for annotating paths and environment variables in config:

Paths ({root | home}) support several modifiers ({uri | real | parent}):

[tool.hatch.envs.test]
dependencies = [
    "example-project @ {root:parent:parent:uri}/example-project",
]

Environment variables support setting default values if they are not set, e.g. {env:PATH:DEFAULT})

[tool.hatch.envs.test.scripts]
display = "echo {env:FOO:{env:BAR:{home}}}"

@paveldikov
Copy link
Contributor

I think (1) should be the default behaviour -- it is secure by default, and makes no assumptions as to the way the credentials were fed in (env var interpolation by shell, env var interpolation by uv, uv.toml, CLI arg, ...)

@charliermarsh
Copy link
Member

@paveldikov -- I tend to agree. Though we need to make sure that we're able to reconnect the credentials appropriately at install time...

@charliermarsh
Copy link
Member

I'll be giving this some thought this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working preview Experimental behavior
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants