-
Notifications
You must be signed in to change notification settings - Fork 670
database: Add trustpub_configs_gitlab
table
#11988
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
base: main
Are you sure you want to change the base?
Conversation
c7ec53f
to
81fa302
Compare
81fa302
to
2b29aff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
On the ID issue:
GitLab CI does not allow us resolve a user/organization name to an ID.
I'm actually wondering if we're better off just being more restrictive here and only allowing GitLab Trusted Publishing configurations on public repos projects owned by public users or groups. That would sidestep the resurrection attack possibility, right? Since there's almost no usage of crates.io for closed source crates, I'm really not that fussed about requiring a public repo for the functionality to work.
pypi/warehouse#13575 (comment) seems to indicate that is also problematic. I've found pypi/warehouse#15275 (comment) in the meantime and I'm wondering if that might be worth considering (tl;dr set the ID of the config on the first token exchange, instead of at config creation). In that case we would have a nullable @woodruffw any thoughts on that? was there a reason why it wasn't implemented like this for PyPI? |
Sorry for the delay! I'm catching up on a bunch of notifications 😅
Yeah, that comment encapsulates it well -- even if the top-level namespace/organization is public the interior group might be private, so there's no super clean way to get the group ID to pin against for both public and private groups.
From memory, I think we opted to defer that because it's a significant complication in the "lifecycle" of a publisher -- instead of being initialized once, a GitLab publisher would indefinitely live in a pre-TOFU state until first used, which we thought would be kind of hard to explain to users (in terms of what happens if they give up their namespace while in that state). Or in other words, we thought the security model of "limited protection from resurrection attacks" would be easier to explain than "limited but slightly stronger protection from resurrection attacks," since the latter requires us to qualify the risk and explain TOFU to the end user. (I remember being vaguely unsatisfied by all of that, since GitLab also has a weaker name resurrection policy than GitHub does 😅) |
2b29aff
to
431014b
Compare
@woodruffw thanks for the explanation! if there is indeed no technical reason against it, I think I'd prefer the TOFU variant. in practice I expect most projects to publish at least once shortly after setting up trusted publishing for a crate. we should still warn about the potential attack vector somehow, but IMHO this is still better than not using the namespace ID at all. @LawnGnome I've updated the PR with a nullable |
431014b
to
6e4fc5c
Compare
This PR adds a new
trustpub_configs_gitlab
database table to begin implementing Trusted Publishing support for GitLab CI.The table is roughly similar to the one for GitHub Actions introduced in #11062, with one major difference:
GitLab CI does not allow us resolve a user/organization name to an ID. In other words: this implementation is prone to resurrection attacks. The PyPI folks did the research on this and concluded that this is an unfortunate limitation, but with no way around it. If this situation ever changes we can always introduce a corresponding ID column and backfill the values then. This limitation will be documented in the docs and I'm considering adding a checkbox to the Trusted Publishing creation form to acknowledge the risk.
Aside from this major difference, there is another small difference in that namespaces in GitLab can be nested, so a
foo/bar/baz
project path is possible. In that case the user is supposed to usefoo/bar
for thenamespace
field andbaz
for the project name.Related