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

Add actions support to package auth verification #23729

Merged
merged 20 commits into from
Apr 10, 2023

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented Mar 27, 2023

Partly fixes #23642

Error info:
image
ActionsUser (userID -2) is used to login in to docker in action jobs.

Due to we have no permission policy settings of ActionsUser now, ActionsUser can only access public registry by this quick fix.

lunny
lunny previously approved these changes Mar 27, 2023
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 27, 2023
@lunny lunny added type/bug outdated/backport/v1.19 This PR should be backported to Gitea 1.19 and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 27, 2023
@lunny lunny added this to the 1.20.0 milestone Mar 27, 2023
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 27, 2023
@yp05327
Copy link
Contributor Author

yp05327 commented Mar 27, 2023

I found that we already have a func called GetPossibleUserByID to handle both normal users and system users.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 27, 2023
@codecov-commenter

This comment was marked as outdated.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 27, 2023
Copy link
Member

@KN4CK3R KN4CK3R left a comment

Choose a reason for hiding this comment

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

This will only work if you pull images from a public registry. And you can do that without auth. The action user will not pass permission checks.

@wxiaoguang
Copy link
Contributor

This will only work if you pull images from a public registry. And you can do that without auth. The action user will not pass permission checks.

That's also an unclear question in my mind, how the "action" user works with permissions (or how it should work).

Is there any document?

@KN4CK3R
Copy link
Member

KN4CK3R commented Mar 27, 2023

open questions I asked in Discord:

the first question is if that task token should/could be used to access the package registry
if the answer is yes, we need a definition of who is the real user behind that task
is it the owner of the repo, the user which triggered the task?

And after that we may need to change how we use ctx.Doer. It's not clear to me where the actions user should be the doer or the real user.

@yp05327
Copy link
Contributor Author

yp05327 commented Mar 27, 2023

So GITEA_TOKEN needs to be run.TriggerUser's temporary token instead of task.Token?
So ctx.Doer needs to be github.actor or github.repository_owner?

@yp05327
Copy link
Contributor Author

yp05327 commented Mar 28, 2023

Team.Authorize and TeamUnit.AccessMode confused me.
It seems that we only have Team.Authorize before #17811, so maybe Team.Authorize is unnecessary now?
If it is unnecessary, maybe we need to do some changes to GetUserOrgsPermissions too.

func GetUserOrgsPermissions(ctx *context.APIContext) {
// swagger:operation GET /users/{username}/orgs/{org}/permissions organization orgGetUserPermissions
// ---
// summary: Get user permissions in organization
// produces:
// - application/json
// parameters:
// - name: username
// in: path
// description: username of user
// type: string
// required: true
// - name: org
// in: path
// description: name of the organization
// type: string
// required: true
// responses:
// "200":
// "$ref": "#/responses/OrganizationPermissions"
// "403":
// "$ref": "#/responses/forbidden"
// "404":
// "$ref": "#/responses/notFound"
var o *user_model.User
if o = user.GetUserByParamsName(ctx, ":org"); o == nil {
return
}
op := api.OrganizationPermissions{}
if !organization.HasOrgOrUserVisible(ctx, o, ctx.ContextUser) {
ctx.NotFound("HasOrgOrUserVisible", nil)
return
}
org := organization.OrgFromUser(o)
authorizeLevel, err := org.GetOrgUserMaxAuthorizeLevel(ctx.ContextUser.ID)
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetOrgUserAuthorizeLevel", err)
return
}
if authorizeLevel > perm.AccessModeNone {
op.CanRead = true
}
if authorizeLevel > perm.AccessModeRead {
op.CanWrite = true
}
if authorizeLevel > perm.AccessModeWrite {
op.IsAdmin = true
}
if authorizeLevel > perm.AccessModeAdmin {
op.IsOwner = true
}
op.CanCreateRepository, err = org.CanCreateOrgRepo(ctx.ContextUser.ID)
if err != nil {
ctx.Error(http.StatusInternalServerError, "CanCreateOrgRepo", err)
return
}
ctx.JSON(http.StatusOK, op)
}

@yp05327
Copy link
Contributor Author

yp05327 commented Apr 1, 2023

So before this, we need to give permission conservatively. Maybe it is a good choice to keep actions bot user without any permission to packages.

I agree.
So is it okey to only allow action bot user pull images from public registry in this PR, and add TODO for the permission check of private registry?

@wolfogre
Copy link
Member

wolfogre commented Apr 1, 2023

So before this, we need to give permission conservatively. Maybe it is a good choice to keep actions bot user without any permission to packages.

I agree.
So is it okey to only allow action bot user pull images from public registry in this PR, and add TODO for the permission check of private registry?

👍🏻I believe this is the optimal solution at the moment

@yp05327 yp05327 force-pushed the fix-gitea-action-docker-login branch from 487bc6e to 9520c12 Compare April 3, 2023 00:42
@yp05327
Copy link
Contributor Author

yp05327 commented Apr 3, 2023

@wolfogre
Copy link
Member

wolfogre commented Apr 3, 2023

Removed permission check in this PR, and added TODO in #23879 (files)

Please update the description of this PR. 😄

@yp05327
Copy link
Contributor Author

yp05327 commented Apr 3, 2023

Removed permission check in this PR, and added TODO in #23879 (files)

Please update the description of this PR. 😄

Done.

Maybe it is better to create a summary (tasks) for this problem? It seems that the discussion is in many different issues/PRs comments.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 9, 2023
@lunny lunny merged commit bb6c670 into go-gitea:main Apr 10, 2023
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 10, 2023
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 10, 2023
Partly fixes go-gitea#23642

Error info:

![image](https://user-images.githubusercontent.com/18380374/227827027-4280a368-ec9e-49e0-bb93-6b496ada7cd9.png)
ActionsUser (userID -2) is used to login in to docker in action jobs.

Due to we have no permission policy settings of ActionsUser now,
ActionsUser can only access public registry by this quick fix.
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Apr 10, 2023
silverwind pushed a commit that referenced this pull request Apr 10, 2023
Backport #23729 by @yp05327

Partly fixes #23642

Error info:

![image](https://user-images.githubusercontent.com/18380374/227827027-4280a368-ec9e-49e0-bb93-6b496ada7cd9.png)
ActionsUser (userID -2) is used to login in to docker in action jobs.

Due to we have no permission policy settings of ActionsUser now,
ActionsUser can only access public registry by this quick fix.

Co-authored-by: yp05327 <[email protected]>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 11, 2023
* upstream/main:
  Avoid recursing into sub-sub-sub-docs folders when looking for READMEs. (go-gitea#23695)
  [skip ci] Updated translations via Crowdin
  Use auto-updating, natively hoverable, localized time elements (go-gitea#23988)
  Reserve ".png" suffix for user/org names (go-gitea#23992)
  Allow adding SSH keys even if SSH server is disabled (go-gitea#24025)
  Add placeholder and aria attributes to release and wiki edit page (go-gitea#24031)
  Add --quiet option to gitea dump (go-gitea#22969)
  Remove "inverted" class on creating new label and cancel buttons (go-gitea#24030)
  Use actions job link as commit status URL instead of run link (go-gitea#24023)
  Make label templates have consistent behavior and priority (go-gitea#23749)
  Add actions support to package auth verification (go-gitea#23729)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. outdated/backport/v1.19 This PR should be backported to Gitea 1.19 type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

actions: login via GITEA_TOKEN to package repository not working
10 participants