open-telemetry / community

OpenTelemetry community content
https://opentelemetry.io
Apache License 2.0
776 stars 233 forks source link

REQUEST: Repository maintenance on `open-telemetry/opentelemetry.io` #2234

Open svrnm opened 3 months ago

svrnm commented 3 months ago

Affected Repository

https://github.com/open-telemetry/opentelemetry.io

Requested changes

Upgrade the opentelemetry bot PAT (OPENTELEMETRYBOT_GITHUB_TOKEN) to have permissions to assign issues

Purpose

Since CODEOWNERS requires teams to have write permissions on a repository, there are currently lots of groups with that permission on the otel.io repo, to reduce that requirement, we want to roll out @dyladan's componentowner workflow. To make this workflow work with github teams (like open-telemetry/docs-approvers) the workflow needs a PTA that has the permission to assign an issue. The normal GITHUB_SECRET lacks the visibility into the org for that.

See https://github.com/dyladan/component-owners?tab=readme-ov-file#using-own-access-token for more details on required settings

Expected Duration

permanently

Repository Maintainers

@open-telemetry/docs-maintainers

jack-berg commented 3 months ago

There are multiple PATs associated with that account. Based on this doc, I assume the one we're talking about is "OpenTelemetry GitHub Org Secret".

Screenshot 2024-07-25 at 9 50 42 AM

According to this document, anyone with write access to a repository can assign issues and pull requests.

Does this mean we need to assign the bot the write:repo_hook scope? Does assigning generic write capabilities expose any vulnerabilities based on the usages of the bot?

Screenshot 2024-07-25 at 9 50 56 AM
svrnm commented 3 months ago

@jack-berg thanks for looking into this, the current issue is that the bot can not identify the groups, I guess it is admin:org > read:org?

More automation has that permission, can that key be assigned to opentelemetry.io repo as a secret?

jack-berg commented 3 months ago

Yes that seems reasonable. Can you open a PR to update the OpenTelemetry Bot asset doc, indicating that it has this new scope and a brief explanation of what use cases it enables?

svrnm commented 3 months ago

Yes that seems reasonable. Can you open a PR to update the OpenTelemetry Bot asset doc, indicating that it has this new scope and a brief explanation of what use cases it enables?

Will do!

svrnm commented 3 months ago

After trying out a few variations of that, the issue boils down to the problem that @opentelemetrybot does not have the right permissions on the opentelemetry.io repository to request code reviews. Only if at least triage permissions are assigned to the bot, the workflow runs successfully. However this goes against the following statement in assets.md:

“Important: You do not need to (and should not) give this account any permissions to any OpenTelemetry repository.”

https://github.com/open-telemetry/community/blob/main/assets.md#opentelemetry-bot

svrnm commented 3 months ago

The PR that introduced that note on not assigning permissions to the bot:

https://github.com/open-telemetry/community/pull/1285#discussion_r1021081365

By using component owners and not CODEOWNERS we would reduce the number of members with write permissions significantly, and replace it with the bot having triage permissions. I think that this would be better.

cc @arminru @trask

trask commented 3 months ago

“Important: You do not need to (and should not) give this account any permissions to any OpenTelemetry repository.”

https://github.com/open-telemetry/community/blob/main/assets.md#opentelemetry-bot

I think the idea at the time was that we would give out individual fine-grained @opentelemetrybot tokens for anything that needed write access to a repo

svrnm commented 3 months ago

“Important: You do not need to (and should not) give this account any permissions to any OpenTelemetry repository.” main/assets.md#opentelemetry-bot

I think the idea at the time was that we would give out individual fine-grained @opentelemetrybot tokens for anything that needed write access to a repo

Thanks for clarification. In this particular use case we would not even need write permissions, triage would be fine, would we have any concerns with that?

Looking through the permissions I don't see anything I would be concerned about:

https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/managing-repository-roles/repository-roles-for-an-organization#permissions-for-each-role

svrnm commented 3 months ago

@arminru @trask based on my comment above, any concerns? 👍 👎

trask commented 2 months ago

from the perspective of least-privilege, we could create a new opentelemetrybot PAT just for the website repo, and grant permissions to the website repo only for that particular PAT, e.g.

image

jack-berg commented 2 months ago

I've created a new fine-grained PAT for opentelemetry.io and set it as a secret for the repository under key OPENTELEMETRYBOT_OPENTELEMETRY_IO_PAT.

The PAT has read and write access to issues and pull requests, and expires 7/30/2025 (one year is the longest TTL allowed).

Screenshot 2024-07-30 at 4 11 45 PM
svrnm commented 2 months ago
Error: Resource not accessible by personal access token - https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request

with the new PAT

jack-berg commented 2 months ago

I've generated a new token and verified it works. The issue was that the token's resource owner was opentelemetrybot instead of the open-telemetry organization. This meant that the token only had access to the bot's forks of opentelemetry repos. See screenshot for a successful PR review request:

Screenshot 2024-07-31 at 1 13 05 PM
svrnm commented 2 months ago

It works also in the workflow, I just tested it 🎉

One last question: the key will expire in 12 months from now? How do we make sure that we reset the value early?

trask commented 2 months ago

It works also in the workflow, I just tested it 🎉

@svrnm It looks like @opentelemetrybot still has triage access to the website repo.

Can you remove that access and see if it still works? Thanks!

One last question: the key will expire in 12 months from now? How do we make sure that we reset the value early?

Can you open a new issue for this, and we can add it to the Infrastructure SIG project board? I think @austinlparker was right about possibly needing some automation (in this case reminders?) around opentelemetrybot token maintenance.

svrnm commented 2 months ago

It works also in the workflow, I just tested it 🎉

@svrnm It looks like @opentelemetrybot still has triage access to the website repo.

Can you remove that access and see if it still works? Thanks!

I thought I had removed it before :-/

without these permissions we are back to the error:

Error: Resource not accessible by personal access token - https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request

I will keep the permissions set to have the workflow working right now, later when we are all available we can remove them once again and get it working without it.

jack-berg commented 2 months ago

One last question: the key will expire in 12 months from now? How do we make sure that we reset the value early?

Not sure. Low tech way is to set a reminder and followup.

jack-berg commented 2 months ago

@svrnm marking this as resolved - feel free to re-open if there is more to do.

svrnm commented 2 months ago

@jack-berg the opentelemetrybot still has "triage" permissions on the opentelemetry.io repository, without I get an error. If we can leave it like that, I am fine, otherwise we need to figure out what's missing for that token

trask commented 2 months ago

Error: Resource not accessible by personal access token - https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request

I wonder if the token needs read:org permission in order to reference the @open-telemetry teams that are used as reviewers?

I've run into a semi-related issue before:

https://github.com/open-telemetry/opentelemetry-java-contrib/blob/d9ab85f943513f01330de809ecf33208013d60b2/.github/workflows/reusable-create-collector-contrib-pull-request.yml#L82-L89

svrnm commented 2 months ago

I wonder if the token needs read:org permission in order to reference the https://github.com/open-telemetry teams that are used as reviewers?

Maybe? Probably we need to try. Can we (@trask @jack-berg and I) find some time in early September to sit together for ~1hr and work on this in sync? It works right now with the triager permissions and I will be out of office for a while soon, so if we fix that I would like to not have another long interruption.