Use per-build token to acts as an user triggering a build

What does this MR do and why is needed?

This tries to solve a long outstanding problem https://gitlab.com/gitlab-org/gitlab-ce/issues/18107 (checking out and accessing dependent projects).

The Merge Request introduces a build token, token that allows to access a source code / LFS objects / container images with a permission of a user triggering a build. The access is read-only for repository / LFS, but read-write for container images of a project on which a build is triggered.

This appears to have less edge-cases then any other already discussed solution: https://gitlab.com/gitlab-org/gitlab-ce/issues/18994 (this issue describes this approach), https://gitlab.com/gitlab-org/gitlab-ce/issues/18993 (Internal readable by CI tokens). It works for users that are external, and internal. It works for public, internal and private projects. Thus allowing to fetch submodules from all projects to which user has access. This also makes it possible to have a full accountability of actions.

This also increases the security of checking out sources, since we generate a token for each build. The token is valid only when the build is running.

Are there points in the code the reviewer needs to double check?

The security implications of the changes.

What are the relevant issue numbers?

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/18090 Fixes #18994 (closed)

Does this MR meet the acceptance criteria?

• CHANGELOG entry added
• Documentation created/updated I will be creating a user documentation and impact of these changes to properly annouce that with the release
• API support added
• Tests
  • Added for this feature/bug
  • All builds are passing
• Conform by the style guides
• Branch has no merge conflicts with master (if you do - rebase it please)
• Squashed related commits together

cc @markpundsack @stanhu @grzesiek @zj

mentioned in issue #19982 (closed)

This implies that we have to also update Build triggers, since they are now user-less. We should allow to update all existing triggers with a user of which permissions will be used.

@ayufan This is currently derived from the branch with pipeline creation changes, thus it is quite difficult to read diffs. Can you change the target branch before pipeline creation service gets merged?

@grzesiek Rebased it on master

Added 136 commits:

• 00c55ae1...551ffc0a - 135 commits from branch master
• 49a7d354 - Use a permissions of user to access all dependent projects from CI jobs (this al…

Added ~15078 label

Milestone removed

Milestone changed to %8.12

Talked with @ayufan and this proposal is awesome and seems to cover all the immediate problems, and sets us up to cover more in the future. Love it.

We should write up what this does and doesn't work for. e.g. it works for git clone, git-LFS, git sub-modules, docker pull, and images: foo, but not for triggers (because they're user-less) or any API calls (e.g. fetching build artifacts from another SHA/project). Also that the scope is read-only outside of the current project, and the token expires when the build is finished so exposure is limited. And that this increases risk of runners in that the tokens they have access to are slightly more powerful (access to more than 1 project) so protecting them is even more important (always use https, protect the runner instances, protect project token registration tokens).

Then get @stanhu and others vetting the security implications so we gain confidence in the approach.

mentioned in issue #20716 (moved)

mentioned in issue #21277 (closed)

Proposal

1. We will generate a token for every build,
2. The token will be unique across GitLab,
3. GitLab Runner will receive payload with: Build ID, and generated Build Token,
4. For cloning sources, accessing LFS objects and pulling docker images we will use the: gitlab-ci-token, build-token (we do it now),
5. We will authenticate this token by looking for a Ci::Build with that build-token,
6. We will authorize access to the resource by getting from Ci::Build information about a person who run this build, it could be: pusher of git push, person who did retry a build, person who did merge a changes,
7. We will run normal authorization checks for a resource as that user, basically allowing to use gitlab-ci-token:build-token credentials as temporary credentials of the build,
8. These credentials will only be accessible when build will be in state running, it means that it's assigned to runner and is being processed by him,
9. Possible authorized actions will be only read-only, we will block read-write operations,
10. There will be one exception: docker push for a project for which Ci::Build is created will be possible. This is to maintain backward compatibility,
11. Basically a Ci::Build will have access to everything to which a user triggering build does have access,
12. Artifacts will be downloaded using the same build token delivered in payload.

Triggers

Currently Build triggers are user-less. We will have to make triggers to be executed by specific user. We will extend Build triggers similarly to how we implement Mirroring: allow to specify an user who is running a build.

Problems

1. This open potential security hole, because it makes CI credentials to have much wider permissions,
2. This puts much more trusty in security of runners. We trust currently runners as long person doesn't use shell and doesn't use docker in --privileged mode,
3. This puts much more trusty in security of data channel: receiving payload by runner. We do have customers using HTTP for accessing the GitLab. However, this has the same problems as connecting from the browser.

Summary

Given above problems this seems to be a future proof solution:

1. We can allow to use the same credentials for accessing GitLab API,
2. Basically this approach doesn't seem to have edge cases,
3. It works well for users that are external and have limited permissions,
4. It will work with private, internal and public projects.

@stanhu @DouweM

Since this has significant implications on security of CI. Would you be willing to review above and also propose other people that would help ironing out the proposal?

@jacobvosmaer-gitlab Rumor is you like security and might have some valuable insight into this proposal.

@ayufan Can you move the proposal to an issue? I'd like to label it with direction so it shows up on the roadmap.

We will have to make triggers to be executed by specific user.

I could see people asking to be able to define permissions here rather than having to use their own account or create bot accounts. But this can be punted for a while, of course.

@ayufan Oh, looks like I already updated #18994 (closed) with substantially the same proposal.

mentioned in issue #18994 (closed)

Does this mean that if an admin pushes to a project, that build will have read access to all projects?

This is problematic since it means admins (or even regular users with access to sensitive projects) have to be extremely paranoid about triggering builds on any project.

I don't think this solution is a good idea. An explicit list of projects that the parent project has access to would be better.

@nkovacs

It's possible to require the admin to be a member of the group.

So? They're still an admin, and you can still trick them into running a malicious build that will have access to everything (unless I'm misunderstanding something).

mentioned in issue #18107 (closed)

I don't think this solution is a good idea. An explicit list of projects that the parent project has access to would be better.

My initial expectation was to be able to rely on deploy key(s) enabled for the project. If I enable a key for project A and it's also enabled for projects B and C then build job for A will have read-only access to B and C. So far for version GitLab 8.10.5 it's not true.

I shared the new proposal with the customer in https://gitlab.zendesk.com/agent/tickets/34017. They have some concerns:

I agree this is a very significant step towards a solution to enable pulling private images from the GitLab registry.

Like some commenters I am concerned that the proposal gives the build more permission than strictly necessary (such as access to all projects the user can access), when in practice we can expect that a given build will only need access to a few specific other projects.

Another possible issue is that CI builds will behave differently if triggered by 2 different users, e.g. if one user has access to a dependent resource (such as a private image or repo) and the other does not. Such inconsistencies may be difficult to troubleshoot (and explain to confused users).

Back to the concern about permissions: it would just seem preferable that the token only grants access to other projects that have been explicitly shared with it, rather than using the credentials of the user triggering the build. I.e. using per-project "service user" as proposed as alternative #2 (closed) in https://gitlab.com/gitlab-org/gitlab-ce/issues/18994#note_14829188; the "cons" for that approach is the complicated permission management, but possibly it could be simplified: for instance by extending the existing access requests to these "service users" to facilitate getting access, and allowing to grant access to whole groups in addition to individual projects.

Of course this is still only half of the solution, as we also need the GitLab CI runners to use that build-token to authenticate against the GitLab registry in order to retrieve these private images (whether from the project itself or other projects) - as covered in https://gitlab.com/gitlab-org/gitlab-ce/issues/20722. This is probably a much easier problem, though.

Added 2239 commits:

• 49a7d354...45afdbef - 2238 commits from branch master
• 417b5557 - Use a permissions of user to access all dependent projects from CI jobs (this al…

@jacobvosmaer-gitlab

I tried to simplify the implementation as much as possible.

This introduces a per-build token. The build tokens do have restricted access to projects with the permission of the user. Restricted means that user needs to be a team member of the project and only have read-only permission.

To do that I introduced a two new abilities: :restricted_download_code and :restricted_read_container_image. When we authenticate against build token we then mark this authorization as restricted.

What also bugs me is that restricted can either mean 'prevent admin overreach' or ' use new or legacy access checks'.

But 'unrestricted by default' behavior bothers me even more. @ayufan what would happen if we turn that around? Write the code so that 'restricted' is the normal state? (We might need a different name for the attribute.)

@jacobvosmaer-gitlab

I thought about that, but it looks like I did go in second direction. I will make a normal state to be restricted.

Added 1 commit:

• 505dc808 - Use a permissions of user to access all dependent projects from CI jobs (this al…

@jacobvosmaer-gitlab

It's updated. I flipped the logic and tried to be as explicit as possible.

Marked this merge request as a Work In Progress

Added 1 commit:

• 571226f1 - Make result to return project and capabilities granted

Added 1 commit:

• ca8ed65e - Fix result

Added 1 commit:

• 11337217 - Update db/schema.rb

Structure authentication in three before actions:

• authenticate_user
• validate_2fa_access (handle missing_personal_token case)
• validate_capabilities

Current code in this MR has a bug; auth result structs get initialized wrong project=:gitlab_or_ldap, type=nil

--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -93,7 +94,7 @@ module Gitlab
 
       def user_with_password_for_git(login, password)
         user = find_with_user_password(login, password)
-        Result.new(user, :gitlab_or_ldap, nil, full_capabilities) if user
+        Result.new(user, nil, :gitlab_or_ldap, full_capabilities) if user
       end
 
       def oauth_access_token_check(login, password)

Added 1 commit:

• 79e4bb8d - Refactor Gitlab::Auth to simplify the data flow

Added 1 commit:

• 6b381f3f - Use build_read_container_image and use build_download_code

Added 1 commit:

• 11f87700 - Add access specs

Added 123 commits:

• 11f87700...4768521a - 121 commits from branch master
• 50076ab9 - Merge remote-tracking branch 'origin/master' into per-build-token
• 9d1ccd2a - Fix existing authorization specs

@ayufan Does this initial approach have support for build triggers? I see some discussion above about how they would have to be extended with the concept of a user in order to make that work, but I wasn't sure whether the initial proposed capability has that.

I like where this is going, it is becoming a lot clearer.

Now I start to notice a new pattern: has_capabilities?(:download_code) && user.can?(:download_code). That sounds almost synonymous. Maybe we need a different word from 'capabilities'?

@jasonroehm I plan to improve Build triggers in second iteration.

@jacobvosmaer-gitlab

It's ok for me to stick with current naming:

• capabilities - possible abilities given by authentication,
• abilities - abilities of user when authorizing request

So, yes, to pass an authorization check we execute: capabilities & abilities

Added 2 commits:

• 551787ac - Simplify LFS helper
• 548169cf - Fix most of specs

Added 1 commit:

• e3a422c2 - Fix LFS specs

In a call with @ayufan we came up with: capabilities -> authentication_abilities

Because they are the allowed abilities in the context of the authentication mechanism.

This will probably conflict with https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6043

Added 1 commit:

• eed5c58d - Verify permission of build in context of dependent project

I like the current approach of this MR because it not only adds a feature but in doing so simplifies existing behavior. Generic CI and oauth checks in the Git HTTP code were an icky if-else mess.

I have confidence in the existing permission checks for Git HTTP and LFS (I don't know the JwtController well). When this MR has good coverage for the new features and all existing tests still pass this should be OK.

@jacobvosmaer-gitlab this will definitely conflict with !6043 (merged). We are moving methods around right now on a couple of places that this MR also touches. @ayufan can this wait until !6043 (merged) is merged? That should happen very soon.

It seems easy to add this changes on top of !6043 (merged).

Thanks @patricio since you did a few of concepts that I did introduce in this MR it appears that my change is a little smaller :)

Added 41 commits:

• eed5c58d...f8bd9625 - 30 commits from branch master
• e40e3fdc - Added LFS support to SSH
• 372be2d2 - Added CHANGELOG item and documentation.
• cb85cf1f - Refactor LFS token logic to use a Redis key instead of a DB field, making it a 1 use only token.
• 48f1a61f - Refactored LFS auth logic when using SSH to use its own API endpoint `/lfs_authe…
• c25630ee - Refactored handling of the LfsToken and added functionality to it to simplify external code.
• 85152f02 - Improve string handling.
• c144db29 - Better authentication handling, syntax fixes and better actor handling for LFS Tokens
• 71aff7f6 - Use special characters for lfs+deploy-key to prevent a someone from creating a…
• de24075e - Further refactoring of authentication code, and code style fixes.
• be09bcf0 - Refactored authentication code to make it a bit clearer, added test for wrong SSH key.
• 83b643a0 - Merge remote-tracking branch 'origin/lfs-support-for-ssh' into per-build-token

I also did check merge against ee-master. It seems that there are some conflicts in specs, but they are easy to fix.

Added 1 commit:

• 5f45ddc5 - Fix specs after merging LFS changes

@rymai Could you take a look? This is based on LFS branch.

Added 1 commit:

• ac6412d0 - Added builds_spec and git_http_specs

Target branch changed from master to lfs-support-for-ssh

@jacobvosmaer-gitlab

I'm only left with LFS specs :)

Added 14 commits:

• 3d933082 - 1 commit from branch lfs-support-for-ssh
• e485e601 - Ensure we update dropdown label after selecting an option
• c67d2f46 - fieldName can’t be a function
• 56461e0c - Reordered dropdown options
• 0950d92d - Reduce duplication in Commits::{CherryPickService,RevertService}
• d2370422 - Merge branch '22203-reduce-duplication-in-commits-cherry_pick_service' into 'master'
• 23303990 - Added keyboard shortcut to navigate to issue boards
• 07d39ac0 - Fixed tests
• 911b3d88 - Merge branch 'ee-919-backport-changes' into 'master'
• 8e4a33f2 - Merge branch '18849-project-snippets-page-isn-t-really-responsive' into 'master'
• 1038ef54 - Merge branch 'issue-boards-keyboard-shortcut' into 'master'
• 168508bf - Merge remote-tracking branch 'origin/master' into per-build-token
• 28b3ba1b - Merge remote-tracking branch 'origin/lfs-support-for-ssh' into per-build-token
• e10d91c8 - Rename capabilities to authentication_abilities

Resolved all discussions

Resolved all discussions

Unmarked this merge request as a Work In Progress

Added 2 commits:

• fb297c41 - Fix specs after renaming authentication_capabilities
• 0ff7294c - Added missing LFS specs

Added 4 commits:

• e941365f - Rename capabilities to authentication_abilities
• a387ff7b - Fix specs after renaming authentication_capabilities
• 1954cb80 - Added missing LFS specs
• eadfc3a1 - Merge remote-tracking branch 'origin/lfs-support-for-ssh' into per-build-token

Resolved all discussions

Resolved all discussions

Mentioned in issue #22253 (closed)

Added 1 commit:

• 9d8afa22 - Improve code comments

Added 1 commit:

• f7ae37c1 - Simplify checking of allowed abilities in git_http_client_controller

Added 1 commit:

• b0195d5c - Fix specs for available statuses

Resolved all discussions

Resolved all discussions

Added 1 commit:

• 2742f9fb - Improve authentication_result usage

@jacobvosmaer-gitlab Updated :)

Move to separate file?

Yeah I think it's time. :)

Will move that :)

Spec additions look good (but not sure about handling of LFS tokens).

Added 2 commits:

• 0ca43b1b - Fix permissions for creating container images
• b51ededc - Don't leak build tokens in build logs

@jacobvosmaer-gitlab The problem is with LFS implementation that:

• doesn't test the use of :lfs_token,
• only test read only access of lfs+deploy-key,
• doesn't test denial of push access for lfs+deploy-key

Comment on LFS: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6043#note_15689584

Added 122 commits:

• cd12d759 - docs: fix typo, it should refer to domain_blacklist_enabled setting
• cb767d87 - Document IAM Profile AWS S3 configuration key.
• 86b8d3d0 - Remove whitespace.
• 714aeb7d - improve wording on build and push images using GitLab CI
• 0ffacb8e - Fixed commit search UI
• e485e601 - Ensure we update dropdown label after selecting an option
• c67d2f46 - fieldName can’t be a function
• 56461e0c - Reordered dropdown options
• 43d2cd51 - Sort secret variables by key (fix #20870 (closed))
• ea810776 - Hides tooltip on discussion toggle button when clicking
• dbe37d2f - Add link to article on how to write reliable, asynchronous tests with Capybara
• 3e855b24 - Fixed issue boards loading issues on large screens
• 2eb2cbe6 - Fix line diff side-by-side line highlighting
• d9e86e79 - Simplified the CSS for issue boards
• c65e903a - Fixed height issue on smaller screens
• 09d949ce - Fixed styling issue with PhantomJS causing builds to fail
• 634e8e30 - Removed more flexbox
• 0950d92d - Reduce duplication in Commits::{CherryPickService,RevertService}
• fd621429 - Added group-specific setting for LFS.
• a8b0d501 - Added CHANGELOG item
• c788c66a - Improved helper methods, better flow for project.lfs_enabled?, and UI fixes.
• d0279ccb - Correct helper for group LFS status.
• d2370422 - Merge branch '22203-reduce-duplication-in-commits-cherry_pick_service' into 'master'
• dfc7f50c - Bump rubocop-rspec to 1.7.0
• 2dbcd0eb - Update .rubocop.yml for rubocop-rspec 1.7.0
• eed79602 - Fix issuable templates dropdown for forked projects
• 02ddb9df - Syntax fixes and better tests for helper methods. Updated docs.
• 740ca302 - fix anchor icon regression introduced in 2fd64725 (!5577 (merged))
• 23303990 - Added keyboard shortcut to navigate to issue boards
• 07d39ac0 - Fixed tests
• 911b3d88 - Merge branch 'ee-919-backport-changes' into 'master'
• 8e4a33f2 - Merge branch '18849-project-snippets-page-isn-t-really-responsive' into 'master'
• 1038ef54 - Merge branch 'issue-boards-keyboard-shortcut' into 'master'
• a655c5f2 - Fix undefined method when reverting migration RemoveProjectsPushesSinceGc
• 5a30a1ea - Fix spelling: sucessfully -> successfully
• b7e6357e - Increase ci_builds artifacts_size column to 8-byte integer to allow larger files
• e000f02b - Add support for column limits in add_column_with_default
• 1f399fe4 - fix
• 0983d01b - Remove stage names; connect stages in column
• 7ec9c20d - Remove centering of stages
• 39bce58b - Remove empty stage state; fit tooltip on one line
• 7d6f8cdd - Update stages to match sketch file
• 019afb60 - Change percentage to px
• 73269b58 - MergeRequest#environments now returns an empty array when diff_head_commit is nil
• d54f05da - Merge branch 'ci-stages' into 'master'
• 55246824 - Merge branch 'fix/irreversible-migration' into 'master'
• 6c168d40 - Merge branch '20870-secret-variables-sorted-by-key' into 'master'
• 2307eb84 - Merge branch 'fix-typos' into 'master'
• 70faf5fd - Merge branch 'rs-update-rubocop-rspec' into 'master'
• 6e4582f2 - Merge branch 'group-specific-lfs-settings' into 'master'
• cffa529e - Merge branch 'issue_21824' into 'master'
• a4176e7f - Merge branch 'issue-boards-large-height-load' into 'master'
• eb0787fd - Merge branch 'diff-comments-toggle-tooltip-hide' into 'master'
• 7ac47516 - Merge branch 'sh-add-link-to-reliable-capybara-specs' into 'master'
• c04ef86c - Scrolls active tab into middle of nav on mobile
• 16da82f4 - Merge branch 'integer_migration_style' into 'master'
• 065341bf - Merge branch 'increase_artifact_size_column' into 'master'
• 1c353e49 - fixed incorrect reference to @repository.root_ref in _readme.html.haml. #22083 (closed)
• 8b2dbe89 - Remove schema annotations completely
• 187dd50f - Add missing CHANGELOG entry
• 591885ce - Merge branch 'group_approvers' into 'master'
• ebf541e5 - Merge branch 'issue-22083' into 'master'
• 47368484 - Doesnt run JS if active element doesnt exist
• 7afee665 - Merge branch '22246-actionview-template-error-undefined-method-each-for-nil-nilclass' into 'master'
• cf00fbec - Fix API notes endpoint when posting only emoji
• 0f3afca3 - Bump fog-aws and other dependent modules
• 9498c4d5 - Merge branch '22256-nomethoderror-api-entities-note-missing-attribute-note-on-aw…
• 63493944 - Render invalid template for merge requests without source project and open
• 6f035810 - Merge branch 'bump-fog-aws' into 'master'
• f8cc5483 - Use oj gem for faster JSON processing
• 1c2aa4f8 - Merge branch 'feature/use-oj-gem' into 'master'
• a147b43d - Replace contributions calendar timezone payload with dates
• 1c27ab50 - Merge branch 'fix-reduce-contributions-calendar-payload' into 'master'
• fe3692ac - Merge branch 'fix-anchor-icon' into 'master'
• 5fa3aeb0 - Merge branch 'search-commit-ui-fix' into 'master'
• 1c8f00c9 - Merge branch 'issue-boards-css-simplify' into 'master'
• 35bed03c - Merge branch 'scrolling-tabs-center-mobile' into 'master'
• d38499b3 - Merge branch 'diff-line-highlighting-fix' into 'master'
• c0a7eb38 - Refactor boards_spec.rb to avoid code duplication
• 2caa6f78 - Merge branch 'patch-3' into 'master'
• b1436ccc - Merge branch 'patch-3' into 'master'
• 43e05fe9 - Formatting fix in doc/ci/examples/README.md
• ed52d82b - Merge branch 'patch-5' into 'master'
• 260d1e6a - Update GitHub importer documentation to reflect current importer state
• 9c2f778c - Refactor GitHub importing documentation
• c2ad1775 - Clarify why GH integration is the preferred importing method
• bcc45b34 - Merge branch 'patch-3' into 'master'
• c38a7ad4 - Merge branch 'update-github-doc-page' into 'master'
• 505d9cf9 - Document how to download the latest build artifacts
• f3743798 - Merge branch 'doc/download-latest-artifact' into 'master'
• e0f90fb2 - Return created status
• bf41ab26 - Add specs that target status failure
• 1d51bc7d - Merge branch '22286-fix-missing-status' into 'master'
• 73f5a8eb - Add missing spec for ProtectedBranches::CreateService
• 282c325d - Reset pushes_since_gc counter before specs run to ensure starting point is 0
• a4638ddd - Add support for dynamic environments
• ba5bd3d1 - Add new CI configuration entry for the environment
• 08272ec1 - Add validation of URL and validation of name
• 2cc9a785 - Properly create deployment using all possible options
• e1b3ab5a - Verify expandability of variables defined as part of environment
• 6b979687 - Update support for dynamic environments
• 274d3d50 - Added missing db/schema changes
• 4a5c2172 - Added documentation about dynamic environments
• abfceb1e - Cleanup changes
• 04b56955 - Small refactor of review apps docs
• 3fbfc30f - Fix CI job environment configuration entry class
• 2ad7753d - Fix CI job environment configuration attributes
• 99f1385e - Restore validation of CI job environment name
• 223041fa - Fix environments handling
• 31e8721a - Fix scope of the CI config key nodes in jobs entry
• 8fe05d83 - Fix validation regexs (+1 squashed commit)
• 62516461 - Merge branch 'fix-board-spec-failures' into 'master'
• 58d02520 - Ensure validation messages are shown within the milestone form
• 42754aab - Merge branch 'sh-add-spec-create-service-protected-branch' into 'master'
• 1e7ea64e - Merge branch '22248-fix-namespace-undefined-method' into 'master'
• 661c464c - Merge branch 'sh-reset-pushes-since-gc-before' into 'master'
• 6a9d87b5 - Merge branch '22033-milestone-validation-returns-422' into 'master'
• 4939911e - Fix specs failures
• 5790684d - Support pushing via SSH
• c20e4267 - Merge branch 'review-apps' into 'master'
• 748dd35c - Fix spec failures
• 967eb8fb - Merge branch 'master' into per-build-token

Added 3 commits:

• abcc0ba5 - Added CHANGELOG
• 79f60e2b - Move Gitlab::Auth.Result to separate file
• 90f6a18f - Properly support Gitlab::Auth::Result

Mentioned in merge request !6409 (merged)

Mentioned in commit fe084819

@ayufan Can we close this now that !6409 (merged) is merged?

Mentioned in issue #22334 (closed)

Added 1 commit:

• 16b8513c - Solve code review comments

Status changed to closed

Mentioned in merge request !6413 (merged)