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

Initial implementation of GitLab OIDC trusted publisher #15275

Merged
merged 3 commits into from
Feb 22, 2024

Conversation

facutuesca
Copy link
Contributor

@facutuesca facutuesca commented Jan 25, 2024

Description

This PR includes the initial implementation of the GitLab trusted publisher. It's mostly based on the existing GitHub implementation, with a few modifications (mentioned below). Unit tests have been included, and a separate PR for the docs is available here. This is part of Trusted publishing: support for GitLab CI.

The corresponding GitLab feature (along with all the claims provided in the token) is documented here.

Differences with GitHub OIDC

  • GitLab uses the term "project" for a repository.
  • While GitHub uses the term "username" or "organization" for the org part of github.com/org/repo, GitLab uses the term "namespace". A namespace can be either a username or a group. Groups can be nested, and so for https://gitlab.com/my_org/my_suborg/my_repo, the namespace would be my_org/my_suborg.
  • For GitHub publishers, PyPI checks that the username/organization provided exists. For GitLab we don't do this, since namespaces can be private and therefore we can't check if they exist or not. See private groups.
  • The above makes this provider less secure that GitHub's, in the sense that it doesn't protect against account resurrection attacks. This is mentioned in the security-model.md doc.
  • Environments are case insensitive in GitHub, but are case sensitive in GitLab.
  • Workflow files (e.g: workflow.yml) can be anywhere in the GitLab repo. Since the OIDC claims include the path of the workflow relative to the root of the repo, when setting up a GitLab publisher we also ask for this path (as opposed to GitHub, where we only ask for the file name).

TODO

  • Once final changes are made, add the screenshots missing to the docs

Screenshot

image

@facutuesca facutuesca requested a review from a team as a code owner January 25, 2024 19:15
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! I did a very fast initial pass; will do a more intensive review after.

docs/user/trusted-publishers/adding-a-publisher.md Outdated Show resolved Hide resolved
warehouse/oidc/forms/gitlab.py Outdated Show resolved Hide resolved
warehouse/oidc/forms/gitlab.py Outdated Show resolved Hide resolved
warehouse/oidc/models/gitlab.py Show resolved Hide resolved
@jku
Copy link
Contributor

jku commented Jan 26, 2024

For GitHub publishers, PyPI checks that the username/organization provided exists. For GitLab we don't do this, since namespaces can be private and therefore we can't check if they exist or not.

I suppose it would not be an impossible requirement to say only public users/orgs can be Trusted Publishers.

This would however have a practical problem when an existing Trusted Publisher decides to make their org private. Dealing with this might not be worth the trouble.

@woodruffw
Copy link
Member

I suppose it would not be an impossible requirement to say only public users/orgs can be Trusted Publishers.

Yep, not an impossible requirement, although it would result in an asymmetry vs. GitHub (where any org/repo can publish, whether public or private).

@woodruffw
Copy link
Member

Most of the ActiveState provider has landed, so this will probably need a decent bit of deconflicting 🙂

@facutuesca facutuesca force-pushed the gitlab-oidc branch 2 times, most recently from b3a0d2e to 4a54f44 Compare February 12, 2024 18:39
@facutuesca
Copy link
Contributor Author

Most of the ActiveState provider has landed, so this will probably need a decent bit of deconflicting 🙂

Fixed!

@di
Copy link
Member

di commented Feb 13, 2024

For GitLab we don't do this, since namespaces can be private and therefore we can't check if they exist or not.

Am I correct in assuming that we can do this for public namespaces? If so, we should probably attempt to do this, determine if the namespace is public or not, and verify this only for public namespaces.

@woodruffw
Copy link
Member

Am I correct in assuming that we can do this for public namespaces? If so, we should probably attempt to do this, determine if the namespace is public or not, and verify this only for public namespaces.

Yeah, I think so (@facutuesca can confirm) -- we should be able to do this for public namespaces.

@woodruffw
Copy link
Member

We could also do this in a TOFU-ish manner for private namespaces, since the OIDC claim set includes the namespace_id: when the namespace is a private one, we can populate the ID on first sight from the claim set.

(I'm not sure if this is worth doing, however, since it complicates the relationship between the publisher model and token consumption.)

Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, just some minor comments.

warehouse/oidc/forms/gitlab.py Outdated Show resolved Hide resolved
warehouse/oidc/models/gitlab.py Outdated Show resolved Hide resolved
warehouse/oidc/models/gitlab.py Outdated Show resolved Hide resolved
warehouse/oidc/models/gitlab.py Show resolved Hide resolved
warehouse/templates/manage/manage_base.html Outdated Show resolved Hide resolved
warehouse/templates/manage/project/publishing.html Outdated Show resolved Hide resolved
warehouse/templates/manage/project/publishing.html Outdated Show resolved Hide resolved
@di
Copy link
Member

di commented Feb 22, 2024

@facutuesca You'll need to re-run make translations to resolve the merge conflicts here.

@di di merged commit e50010a into pypi:main Feb 22, 2024
17 checks passed
@di di deleted the gitlab-oidc branch February 22, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants