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

feat: Keyring backend for pip auth with ADO feeds #54

Merged
merged 15 commits into from
May 7, 2024

Conversation

delilahw
Copy link
Member

@delilahw delilahw commented Apr 22, 2024

This PR implements a custom keyring backend that interfaces with ~/ado-auth-helper to provide authentication when pip needs to access an ADO feed. It is in the form of a Python package, named codespaces_artifacts_helper_keyring (open to renaming it).

Background

The artifacts-helper codespace feature provides authentication helpers for npm, yarn, etc. For many of these package managers, we simply use shell alises and environment variable substitution to inject the authentication tokens prior to calling the underlying package manager. However, this approach does not work with pip, namely because it has multiple entrypoints (e.g. python -m pip) and not all of them will respect shell aliases.

Luckily, pip has builtin support for keyring. Keyring is a framework that supports providing multiple authentication backends. In this PR, we implement a custom keyring backend to be used against ADO package feeds when the ~/ado-auth-helper tool is detected.

Inner Workings

The package works by providing a custom keyring.backend.KeyringBackend. This will automatically be picked up by pip and the keyring package when our codespaces_artifacts_helper_keyring is installed.

When our backend is called we:

  1. Check whether we support the package index that is being accessed (e.g. does it live on dev.azure.com, etc?)
  2. Check whether ~/ado-auth-helper is executable.
  3. Call the auth helper and capture the token from stdout.
  4. Pass the credentials to keyring, which will pass it to pip.

Usage

Usage on the target machine will require these prerequisites:
a. https://github.com/microsoft/ado-codespaces-auth is installed and ~/ado-auth-helper is executable
b. keyring is installed
c. the feed being accessed has a domain matching dev.azure.com, etc

To install the codespaces_artifacts_helper_keyring package from source:

cd src/artifacts-helper/codespaces_artifacts_helper_keyring

# PDM is used to manage the project
$ pip install 'pdm>=2.14'

# Install dependencies and build the package
$ pdm build

# Install package + deps with pip
$ pip install dist/codespaces_artifacts_helper_keyring-*.whl

To use the keyring backend

# Optionally set the pip package index globally
# The index should not contain any `username:password` details
pip3 config set global.index-url https://org.pkgs.visualstudio.com/ORG/_packaging/FEED/pypi/simple

# Use the feed via normal pip operations
pip3 download yapf  # Using global package index
pip3 download yapf -i <package index>  # Specify custom index

Testing

I've included some unit tests that are based on existing tests over at https://github.com/microsoft/artifacts-keyring. I've also added tests to exercise our process of calling the auth helper tool. The tests would be parameterized against multiple Python versions using nox.

Assumptions

  • I've assumed that the returned credentials will always be a valid JWT with the payload containing the username in upn or unique_name. Although, we can probably just pick another username to authenticate against ADO and it will still work.
  • I've assumed that ~/ado-auth-helper is always the location of the helper tool.

Considerations

Packaging and Distribution

We considered these alternatives and we've settled on GitHub releases for now.

  • build and install from source
    • this might pollute the codespace's dev environment if we aren't careful
  • build the package on CI and release it on PyPI:
    • this might be considered a separate project and we might have to get approval for the release of a new publicly available pip package
  • build the package on CI and release on GitHub releases at Releases · microsoft/codespace-features (github.com)
    • installation would need to curl the github api to determine the latest release, download the wheel, and install it
    wget latest release location
    pip install codespaces_artifacts_helper_keyring.whl

@markphip
Copy link
Contributor

markphip commented Apr 22, 2024

build the package on CI and release on GitHub releases

Creating a release and installing from that makes the most sense. Will dependencies auto install? Is this installed globally or specific to the repo that is being worked on? As you said, we do not want to pollute anyone's repository.

Decode the JWT to obtain the username (we can probably skip this?)

I would skip this. Especially if it lets you lose any dependencies. In npm we just use "codespaces" as the username.

Also renames the test files to correspond to the exercised source file.
Add rules to detect lines exceeding max length, missing docstrings, etc.
See diff and ruff rules for more details.
@delilahw
Copy link
Member Author

Creating a release and installing from that makes the most sense.

Sounds good! Let's go with that.

Will dependencies auto install?

Yep, I've specified the dependencies in the package metadata and the package manager will download them. These are the depdencies: we use jaraco-classes, which itself is a dependency of keyring. So really, the only extra dependency is requests.

$ pkginfo -f requires_dist dist/*.whl
requires_dist: ['jaraco-classes >=3.0.0', 'keyring >=20.0.0', 'requests >=2.20.0']

Is this installed globally or specific to the repo that is being worked on? As you said, we do not want to pollute anyone's repository.

It can be installed globally to provide ADO auth for the entire codespace. If the user only wants authentication for a specific project, I believe they can create a virtual environment and only install our keyring backend inside that environment.

Maybe we could create a script similar to write-npm.sh and leave it to the team to decide where they want to call the script.

# Install globally
install-pip-keyring.sh

# Or, install in only one project
python -m venv venv
. venv/activate
install-pip-keyring.sh

Decode the JWT to obtain the username (we can probably skip this?)

I would skip this. Especially if it lets you lose any dependencies. In npm we just use "codespaces" as the username.

🫡 I've removed the jwt parsing (yay, one less dependency) and used "codespaces" as the default username

@markphip
Copy link
Contributor

Is this installed globally or specific to the repo that is being worked on? As you said, we do not want to pollute anyone's repository.

It can be installed globally to provide ADO auth for the entire codespace. If the user only wants authentication for a specific project, I believe they can create a virtual environment and only install our keyring backend inside that environment.

I do not think we need to complicate this. Codespaces is a very specific scenario and this feature is optional, and including Python support will be an optional setting of the feature. So if someone installs this feature AND says to setup pip then I think we can safely just do that. As long as a global installation is likely to work then that is likely what we should do.

When the install.sh script from this feature is executed when a Codespace is being built it is run as the root user. If we install this with that user, will it be available to the Codespace user or do we need to run the install as that user? You can see in the existing script where it does this so that the bash aliases it adds are added for the user.

@delilahw
Copy link
Member Author

So if someone installs this feature AND says to setup pip then I think we can safely just do that. As long as a global installation is likely to work then that is likely what we should do.

Sounds good!

When the install.sh script from this feature is executed when a Codespace is being built it is run as the root user. If we install this with that user, will it be available to the Codespace user or do we need to run the install as that user? You can see in the existing script where it does this so that the bash aliases it adds are added for the user.

If we install it as the root user, it will work system-wide. However, it's generally best practice to avoid installing pip packages as root, explained by this warning when running pip operations as root:

WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead.

There's a risk of conflict between our package's dependencies installed via pip and those installed by the system's package manager. For instance, the 'requests' package is available both as a Debian package and a pip package. If both are installed, pip could overwrite the one installed with apt (or vice versa).

So, I think it's best if we stick to running the install for the remote user only and leave the root environment alone (unless the consumer of the devcontainer feature explicitly specifies remoteUser or containerUser to be root). What do you think?

@markphip
Copy link
Contributor

I think next steps are to get this merged. I would like to see a workflow added that builds the package and uploads it as an artifact to the build. I can help with the latter if you need it but having a workflow do the build would be best if you do.

Once we have this I will just manually add a Release with the build attached. Then I can start working on changing the feature to download and install from the release as the user.

This also modifies the scripts used to run the linting and formatting tasks.
@delilahw
Copy link
Member Author

I've added a CI workflow for linting, formatting, testing, etc for now. I'll work on the release workflow this week!

This adds a workflow to build the Python package when a tag matching `feature_artifacts-helper_*` is created. It will upload the build artifacts (`tar.gz` sdist and `.whl` bdist).

There is also the option of automatically creating a release and
attaching the build artifacts to it. See
eebf0a5 and
https://github.com/delilahw/codespace-features/releases/tag/feature_artifacts-helper_1.1-alpha
for an example.
@delilahw
Copy link
Member Author

delilahw commented May 1, 2024

Okay done! I've added a release workflow in release-keyring.yaml. It will trigger when a tag matching feature_artifacts-helper_* is created, so it can build the package and upload it as an artifact.

This might be handy, here's the process I'm using to install and test it:

# Fetch artifact from GitHub releases
WHEEL_URL=$(
  curl -L \
        -H "Accept: application/vnd.github+json" \
        -H "X-GitHub-Api-Version: 2022-11-28" \
        https://api.github.com/repos/delilahw/codespace-features/releases/latest |
       jq -r 'first( .assets[] | select( .name | endswith("whl") ) ).browser_download_url'
)
wget -O package.whl "$WHEEL_URL"

# The wheel package is required to install wheels
python3 -m pip install wheel

# Install the package and its dependencies
python3 -m pip install package.whl

# Delete wheel file
rm package.whl

# Connect to Azure feed and download something
python3 -m pip download yapf -i "https://pkgs.dev.azure.com/<org_name>/_packaging/<feed_name>/pypi/simple"

…isort

Previously, when we imported our own module
`codespaces_artifacts_helper_keyring` in tests, it was not being
detected by ruff (linter) as a first-party module. This affected the
linter's ability to accurately separate import blocks.

We fix the issue in this commit by including the `/test` directory in
ruff's `src`s. Now, the linter will put our own modules in a separate
first-party import block, as expected.
@delilahw delilahw changed the title feat: PoC: Keyring backend for pip auth with ADO feeds feat: Keyring backend for pip auth with ADO feeds May 7, 2024
@delilahw delilahw marked this pull request as ready for review May 7, 2024 02:36
@delilahw
Copy link
Member Author

delilahw commented May 7, 2024

I'm pretty happy with the changes now, so I'm taking the PR out of draft. Let me know if there's any more feedback y'all have, @markphip and team!

Also @embetten, who's maintaining artifacts-keyring
and artifacts-credprovider, has kindly offered her time as an additional reviewer for the keyring backend implementation. Could you please give it another glance? The only "real" changes I've made since our last discussion are additional tests added in b909288.

Thanks all 😇

@markphip markphip merged commit 7903c2f into microsoft:main May 7, 2024
23 checks passed
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.

2 participants