networkupstools / nut

The Network UPS Tools repository. UPS management protocol Informational RFC 9271 published by IETF at https://www.rfc-editor.org/info/rfc9271 Please star NUT on GitHub, this helps with sponsorships!
https://networkupstools.org/
Other
2.08k stars 351 forks source link

CI: Jenkins wants perhaps too many permissions from users #2634

Closed jimklimov closed 1 month ago

jimklimov commented 1 month ago

In an earlier issue discussion and PR #2604, some superfluous permissions were asked by Jenkins-based NUT CI farm for a developer to just access the build logs linked from GitHub status, etc.

I am not able to see the details of the CI without granting a third-party company write access to my repositories! No way! It is ridiculous because all details of the CI are public.

jimklimov commented 1 month ago

Confirmed in an "incognito" window with an account that was not yet on the farm:

image

At first, I suspected a GitHub App registration made for CI-GH integration, but that is ultimately for chatter between the two servers and "installation"/"authorization" of that app should not be required of user accounts.

There are also relevant settings in Jenkins Manage/Security/GitHub Auth Plugin/OAuth Scope(s) which apparently list the same 3 items as seen in the screenshot: read:org,user:email,repo so gotta be the culprit.

I suppose the original idea for repo (the problematic part) was to allow CI access to people's forks used to make the PRs, or it could have been a copy-paste from how-to instructions made by a team that used such access. This seems like an overkill, and a smaller permission scope (if any) should be used. After all, GH serves virtual branches for PR "head" and "merge" sources as part of nut repo, so direct use of contributors' repositories should not be needed at all.

jimklimov commented 1 month ago

CC @tormodvolden : do you think a repo:status is overkill here? I think it could allow GH to mark commits in PR authors' repos as passed/failed, but does not seem granular enough to only constrain to nut forks.

Per https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps :

repo:status | Grants read/write access to commit statuses in public and private repositories. This scope is only necessary to grant other users or services access to private repository commit statuses without granting access to the code.

For now, removed repo from that list completely, leaving just such requirements to log in:

image

desertwitch commented 1 month ago

This would be a welcome change, I've actually set up a second account just to access the CI due to the permissions requested. 😆 The last requirements posted are, I think, understandable and relaxed enough not to scare off people from looking into the CI.

jimklimov commented 1 month ago

GH lacks a facepalm emoticon for "oops, my bad" cases :-}

The change is in place since yesterday (as posted above), but I'm not sure if the loss of "repo" scope impacts anything at all (or anything that we can't live without). Probably some future PR builds would expose that...

masterwishx commented 1 month ago

GH lacks a facepalm emoticon for "oops, my bad" cases :-}

The change is in place since yesterday (as posted above), but I'm not sure if the loss of "repo" scope impacts anything at all (or anything that we can't live without). Probably some future PR builds would expose that...

Thanks it seems fine now

jimklimov commented 1 month ago

FWIW, looking at a currently open PR, I see status for commits updated in the NUT repo side, e.g. https://github.com/networkupstools/nut/pull/2637/commits/0fb27ae7d1c20660fefe8d2e7cf497c6f0be80aa but there are no statuses for same commits in the original repo, e.g. https://github.com/masterwishx/nut/commit/0fb27ae7d1c20660fefe8d2e7cf497c6f0be80aa or in fact anywhere in https://github.com/masterwishx/nut/commits/work/2495/ history.

For comparison on my long-subscribed fork with the "repo" permission, I see statuses assigned to my fork's commits that were involved in PRs, seen e.g. at https://github.com/jimklimov/nut/commits/issue-2183-telnetlib3-WIP/ history.

Not sure it is a great loss, especially if the trade-off is trusting to allow arbitrary access of NUT CI farm (and plugins involved) to a contributors' life on GitHub, and usually the PR-origin branches are deleted after merge anyway.