Skip to content
Snippets Groups Projects

Add user access functionality for KAS

Merged Hordur Freyr Yngvason requested to merge add-kas-user-authorization-ff into master
All threads resolved!

What does this MR do and why?

Implements #381561 (closed) and #381566 (closed). Implements, behind a feature flag:

  • authorize_proxy_user API endpoint that looks up a user by session ID and returns the metadata for KAS
  • Setting the KAS cookie. Currently at the end of every request. Should limit it to relevant controllers.

The agent MR is here: gitlab-org/cluster-integration/gitlab-agent!841 (merged)

Screenshots or screen recordings

image

How to set up and validate locally

You have to be familiar with KAS and agentk to setup this up:

  1. Enable the feature flag
    Feature.enable(:kas_user_access)
  2. Setup your GitLab (GDK) with KAS and a working agentk from gitlab-org/cluster-integration/gitlab-agent!841 (merged)
  3. Register that agent in a project and enable the ff for that project, too:
    Feature.enable(:kas_user_access_project, Project.find(<project-id>))
  4. Browse to the agent overview page
  5. Open up the browser dev console and run something like this to query Kube API via KAS and agentk (make sure that the KAS address is correct for your setup):
    fetch('https://kas.gdk.test:3443/-/k8s-proxy/api/v1/namespaces', {credentials: 'include', headers: {'X-Csrf-TOKEN': document.head.querySelector('meta[name="csrf-token"]').content, 'GitLab-Agent-Id': '1'}}).then((response) => response.json()).then((data) => console.log(data));
  6. Verify that the response is a NamespaceList Kube API response

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Timo Furrer

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • Resolved by Timo Furrer
      6 Warnings
      :warning: This merge request is quite big (778 lines changed), please consider splitting it into multiple merge requests.
      :warning: 28751ad6: The commit subject may not be longer than 72 characters. For more information, take a look at our Commit message guidelines.
      :warning: eaab6e3f: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.
      :warning: 9d229393: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.
      :warning:

      featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.

      For more information, see:

      :warning: Do not add new controller specs. We are moving from controller specs to
      request specs (and/or feature specs). Please add request specs under
      /spec/requests and/or /ee/spec/requests instead.

      See &5076 for information.

      1 Message
      :book: CHANGELOG missing:

      If you want to create a changelog entry for GitLab FOSS, add the Changelog trailer to the commit message you want to add to the changelog.

      If you want to create a changelog entry for GitLab EE, also add the EE: true trailer to your commit message.

      If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

      Reviewer roulette

      Changes that require review have been detected!

      Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

      Category Reviewer Maintainer
      backend Harsimar Sandhu current availability (@harsimarsandhu) (UTC+5.5, 10.5 hours ahead of @hfyngvason) Jessie Young current availability (@jessieay) (UTC-8, 3 hours behind @hfyngvason)
      Application Security Reviewer review is optional for Application Security Greg Alfaro current availability (@truegreg) (UTC-7, 2 hours behind @hfyngvason)

      To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

      To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

      Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

      If needed, you can retry the :repeat: danger-review job that generated this comment.

      Generated by :no_entry_sign: Danger

  • Allure report

    allure-report-publisher generated test report!

    e2e-package-and-test: :x: test report for dd0f5277

    expand test summary
    +-----------------------------------------------------------------------+
    |                            suites summary                             |
    +------------------+--------+--------+---------+-------+-------+--------+
    |                  | passed | failed | skipped | flaky | total | result |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Manage           | 132    | 0      | 6       | 4     | 138   | ❗     |
    | Create           | 292    | 0      | 40      | 20    | 332   | ❗     |
    | Plan             | 118    | 2      | 0       | 2     | 120   | ❌     |
    | Govern           | 84     | 0      | 2       | 0     | 86    | ✅     |
    | Verify           | 98     | 0      | 4       | 0     | 102   | ✅     |
    | Analytics        | 4      | 0      | 0       | 0     | 4     | ✅     |
    | Release          | 12     | 0      | 0       | 0     | 12    | ✅     |
    | Monitor          | 10     | 0      | 0       | 0     | 10    | ✅     |
    | ModelOps         | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Secure           | 14     | 0      | 2       | 0     | 16    | ✅     |
    | Fulfillment      | 4      | 0      | 46      | 0     | 50    | ✅     |
    | Package          | 0      | 0      | 6       | 0     | 6     | ➖     |
    | Growth           | 0      | 0      | 4       | 0     | 4     | ➖     |
    | Framework sanity | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Configure        | 0      | 0      | 6       | 0     | 6     | ➖     |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Total            | 768    | 2      | 120     | 26    | 890   | ❌     |
    +------------------+--------+--------+---------+-------+-------+--------+
  • added 2444 commits

    Compare with previous version

  • added 1340 commits

    Compare with previous version

  • added 3 commits

    Compare with previous version

  • requested review from @tigerwnz and @Alexand

  • added 1 commit

    • d0fce8ad - Use grape for param validation

    Compare with previous version

  • added 2 commits

    • 83aadb0f - Remove redundant validation line
    • b24a4e8c - Add missing comma, improve description

    Compare with previous version

  • added 1 commit

    • 903a2ed5 - Add missing feature_category on endpoint

    Compare with previous version

  • added 1 commit

    • e136f822 - Use safe navigation for session id

    Compare with previous version

  • João Alexandre Cunha removed review request for @Alexand

    removed review request for @Alexand

  • added 1 commit

    • d4ed62cb - Make ff disabled by default; bump milestone

    Compare with previous version

  • added 2 commits

    • c4210041 - Fix grape value type (should be String, not Symbol)
    • 1a2c83ec - Use secure_compare and disallow blank csrf_token

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • 394880ca - Add extra flag for actor-based rollout

    Compare with previous version

  • added 4586 commits

    • 394880ca...cbc29ecc - 4572 commits from branch master
    • 0940d233 - Add KAS user authorization behind a feature flag
    • 7d653a96 - Encrypt / decrypt cookie
    • bd6e967e - Add initital tests
    • b41661b3 - Fix rubocop offenses
    • 51c57eb5 - Use grape for param validation
    • 2b5677be - Remove redundant validation line
    • 2a5d2be0 - Add missing comma, improve description
    • 03a60f65 - Add missing feature_category on endpoint
    • 95ef7f12 - Use safe navigation for session id
    • 83d5c945 - Make ff disabled by default; bump milestone
    • eea56bd5 - Fix grape value type (should be String, not Symbol)
    • 78027245 - Use secure_compare and disallow blank csrf_token
    • d61b929c - Add introduced_by_url
    • 4f5f2094 - Add extra flag for actor-based rollout

    Compare with previous version

  • added 1 commit

    • 893c9c45 - Add KAS user authorization behind a feature flag

    Compare with previous version

  • added 1 commit

    • 880bad0c - Fix next where return was expected

    Compare with previous version

  • added 1 commit

    • 659b5c29 - Rely on existing 404 wrappers for #find

    Compare with previous version

  • added 1 commit

    • a7c2e43a - Add feature category to spec

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • 8e36cf7c - Update tests; add EE service tests

    Compare with previous version

  • added 566 commits

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • 2ca4507e - Encrypt directly; add request tests

    Compare with previous version

  • added 1 commit

    • 3acc0762 - Explicitly enable feature flags

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • e6f610ce - Only set cookie when session is present

    Compare with previous version

  • added 2 commits

    • 14975648 - Add unit tests for feature flag code
    • eba0bbc7 - Is it failing beause of let_it_be ???

    Compare with previous version

  • added 2 commits

    • e041395c - Try random things
    • 4f4afb4d - Revert "Is it failing beause of let_it_be ???"

    Compare with previous version

  • added 1 commit

    • ea67e9cf - Stub gitlab_kas:enabled config

    Compare with previous version

  • added 1 commit

    • febfac02 - Encode cookie data to JSON prior to encryption

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • 177b0a33 - Add KAS user authorization behind a feature flag

    Compare with previous version

  • added 1 commit

    • e1a45cf7 - Validate session before loading agent

    Compare with previous version

  • added 1 commit

    • 841304d5 - Add unit tests for cookie data method

    Compare with previous version

  • added 409 commits

    Compare with previous version

  • added 1 commit

    • 0d3631e4 - Only read up to `AUTHORIZED_ENTITY_LIMIT` entries

    Compare with previous version

  • Hordur Freyr Yngvason marked this merge request as ready

    marked this merge request as ready

  • requested review from @Alexand

  • mentioned in issue #389261

  • added 1 commit

    Compare with previous version

  • Tiger Watson approved this merge request

    approved this merge request

  • Tiger Watson
  • :wave: @tigerwnz, thanks for approving this merge request.

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.

    For more info, please refer to the following links:

  • added 1 commit

    • 9efe2887 - Add a dedicated :use_k8s_proxies ability

    Compare with previous version

  • added 1 commit

    • 2d80d545 - Add tests for ApplicationController#set_kas_cookie

    Compare with previous version

  • added 1 commit

    • b2eb1110 - Only call #success and #forbidden in #execute

    Compare with previous version

  • Tiger Watson approved this merge request

    approved this merge request

  • added 1 commit

    • dd0f5277 - Add agent config project to response

    Compare with previous version

  • added 1 commit

    • 4d2bc9af - Rework user access to not use cookie

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • João Alexandre Cunha removed review request for @Alexand

    removed review request for @Alexand

  • added 1 commit

    • 4dffb059 - Use properly masked and encoded token in tests

    Compare with previous version

  • added 1 commit

    • ea869eeb - Add KAS user authorization behind a feature flag

    Compare with previous version

  • added 2960 commits

    Compare with previous version

  • added 1 commit

    • fff28957 - Add rollout issues for feature flags

    Compare with previous version

  • added 2 commits

    • 6d7c5228 - Add KAS domain to content security policy
    • 4aa09a9e - Remove from ApplicationController; extract concern

    Compare with previous version

  • added 1 commit

    • b8ec8b6a - Add KasCookie to the agents controller for testing

    Compare with previous version

  • Hordur Freyr Yngvason changed the description

    changed the description

  • Ameya Darshan approved this merge request

    approved this merge request

  • assigned to @timofurrer and unassigned @hfyngvason

  • Timo Furrer added 1 commit

    added 1 commit

    • 068dd72a - Move KAS User Access FF to 15.10

    Compare with previous version

  • Timo Furrer added 2040 commits

    added 2040 commits

    • 068dd72a...db75ef75 - 2034 commits from branch master
    • e1136890 - Add KAS user authorization behind a feature flag
    • 056cc46e - Add rollout issues for feature flags
    • 5f51e23a - Add KAS domain to content security policy
    • b19b4bdc - Remove from ApplicationController; extract concern
    • 2aacc3c6 - Add KasCookie to the agents controller for testing
    • a4a9b34c - WIP: add spec for config loader

    Compare with previous version

  • Timo Furrer changed milestone to %15.10

    changed milestone to %15.10

  • Timo Furrer added 1 commit

    added 1 commit

    • e0f2a112 - Move KAS User Access FFs to 15.10

    Compare with previous version

  • Timo Furrer added 2 commits

    added 2 commits

    • 6aebf4d8 - Move KAS User Access FFs to 15.10
    • 639176a1 - Add spec for config loader

    Compare with previous version

  • Timo Furrer added 336 commits

    added 336 commits

    • 639176a1...48bc3616 - 329 commits from branch master
    • 00e989bc - Add KAS user authorization behind a feature flag
    • bcec6549 - Add rollout issues for feature flags
    • 4ea1fbc1 - Add KAS domain to content security policy
    • ba78cbcf - Remove from ApplicationController; extract concern
    • 5ae5a6ac - Add KasCookie to the agents controller for testing
    • 2d5ebe22 - Move KAS User Access FFs to 15.10
    • 0e0c9f6d - Add spec for config loader

    Compare with previous version

  • mentioned in epic &9859 (closed)

    • Resolved by Lin Jen-Shin

      @hfyngvason @tigerwnz any ideas why the pipeline fails? It's from this JH Validation and it fails during image creation with:

      #34 ERROR: failed to push registry.gitlab.com/gitlab-org-sandbox/gitlab-jh-validation/gitlab-jh-qa:269f6f5d3d0c8bc90d002d6564b91ea0208ce466: server message: insufficient_scope: authorization failed
  • Timo Furrer marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

    marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

  • Timo Furrer changed the description

    changed the description

  • Timo Furrer changed the description

    changed the description

  • Timo Furrer added 295 commits

    added 295 commits

    • 0e0c9f6d...4b8a049b - 287 commits from branch master
    • 681b799c - Add KAS user authorization behind a feature flag
    • e6db35f7 - Add rollout issues for feature flags
    • 71bacf3e - Add KAS domain to content security policy
    • 6b3f316d - Remove from ApplicationController; extract concern
    • b0d80b4a - Add KasCookie to the agents controller for testing
    • 81a9e578 - Move KAS User Access FFs to 15.10
    • 5b882511 - Add spec for config loader
    • 1cdd5889 - Respond with unauthorized instead of not found for invalid sessions in kubernetes proxy

    Compare with previous version

  • mentioned in issue #393002 (closed)

  • requested review from @rutgerwessels

  • Rutger Wessels
  • Rutger Wessels
  • Rutger Wessels removed review request for @rutgerwessels

    removed review request for @rutgerwessels

  • Timo Furrer added 1 commit

    added 1 commit

    Compare with previous version

  • Timo Furrer mentioned in issue #393180

    mentioned in issue #393180

  • Tiger Watson approved this merge request

    approved this merge request

  • Timo Furrer added 398 commits

    added 398 commits

    • fd247230...01eca377 - 388 commits from branch master
    • 4c865bea - Add KAS user authorization behind a feature flag
    • 69e355e5 - Add rollout issues for feature flags
    • ba7fe21d - Add KAS domain to content security policy
    • b471f540 - Remove from ApplicationController; extract concern
    • f8c35a87 - Add KasCookie to the agents controller for testing
    • dadd8fc9 - Move KAS User Access FFs to 15.10
    • 624e8135 - Add spec for config loader
    • fd63ba24 - Respond with unauthorized instead of not found for invalid sessions in kubernetes proxy
    • 3c3579f4 - Add more test coverage
    • 3602f153 - Rename `authorizations` to `access_as` in KAS k8s API

    Compare with previous version

  • mentioned in issue #393336 (closed)

  • Timo Furrer requested review from @tkuah

    requested review from @tkuah

  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
    • Resolved by Thong Kuah

      @timofurrer Thanks for this - overall it is easy to understand.

      I started with some questions on a higher level, focusing on authentication, and authorization.

      Next round, I will look more into functionality

  • Shinya Maeda
  • Timo Furrer added 1279 commits

    added 1279 commits

    • 3602f153...4832d535 - 1266 commits from branch master
    • 4832d535...7846e11d - 3 earlier commits
    • 61d17191 - Remove from ApplicationController; extract concern
    • 73028335 - Add KasCookie to the agents controller for testing
    • bf970fa9 - Move KAS User Access FFs to 15.10
    • eac2fcc0 - Add spec for config loader
    • b299c9fe - Respond with unauthorized instead of not found for invalid sessions in kubernetes proxy
    • 28211b7f - Add more test coverage
    • d2e738c3 - Rename `authorizations` to `access_as` in KAS k8s API
    • 8e6a944e - Make KAS cookie key a constant
    • bc99c3fa - Reference N+1 issue for KAS project and group access
    • 1d50fbf6 - Use GitLab HTTPS config for KAS cookie secure setting

    Compare with previous version

  • Timo Furrer added 1 commit

    added 1 commit

    • 664cebbe - Use GitLab HTTPS config for KAS cookie secure setting

    Compare with previous version

  • Thanks @tkuah and @timofurrer! I've responded to the questions directed at me.

  • Timo Furrer added 1 commit

    added 1 commit

    • 2caffb13 - Use explicit default cookie cipher for KAS cookie encryptor

    Compare with previous version

  • Timo Furrer added 280 commits

    added 280 commits

    • 2caffb13...6a3125f3 - 265 commits from branch master
    • 6a3125f3...b58d6d68 - 5 earlier commits
    • 071b2280 - Move KAS User Access FFs to 15.10
    • 391af3ba - Add spec for config loader
    • 28751ad6 - Respond with unauthorized instead of not found for invalid sessions in kubernetes proxy
    • c95ce396 - Add more test coverage
    • eeceb7fa - Rename `authorizations` to `access_as` in KAS k8s API
    • e25fa254 - Make KAS cookie key a constant
    • 7c69c838 - Reference N+1 issue for KAS project and group access
    • 20a82727 - Use GitLab HTTPS config for KAS cookie secure setting
    • 9e008b5d - Use explicit default cookie cipher for KAS cookie encryptor
    • c283340e - Use Warden session serializer instead of session dig

    Compare with previous version

  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah
  • Thong Kuah removed review request for @tkuah

    removed review request for @tkuah

  • Timo Furrer added 2 commits

    added 2 commits

    • 45e42f22 - Use named subject instead of separate variable
    • 6aee249b - Add spec to test forbidden access to foreign agent

    Compare with previous version

  • mentioned in issue #395131 (closed)

  • Timo Furrer added 1 commit

    added 1 commit

    • 6372f8fc - Memoize authorized projects and groups calls

    Compare with previous version

  • Timo Furrer requested review from @tkuah

    requested review from @tkuah

  • Thong Kuah approved this merge request

    approved this merge request

  • Thong Kuah resolved all threads

    resolved all threads

  • mentioned in issue #395498 (closed)

  • Thong Kuah
  • Thong Kuah resolved all threads

    resolved all threads

  • Thong Kuah mentioned in issue #395499

    mentioned in issue #395499

  • Thong Kuah enabled an automatic merge when the pipeline for 27f2376d succeeds

    enabled an automatic merge when the pipeline for 27f2376d succeeds

  • merged

  • Thong Kuah mentioned in commit dce8db7f

    mentioned in commit dce8db7f

  • Timo Furrer resolved all threads

    resolved all threads

  • Timo Furrer mentioned in merge request !114073 (merged)

    mentioned in merge request !114073 (merged)

  • added workflowstaging label and removed workflowcanary label

  • Timo Furrer mentioned in commit b9b19a1d

    mentioned in commit b9b19a1d

  • Timo Furrer mentioned in merge request !115226 (merged)

    mentioned in merge request !115226 (merged)

  • Please register or sign in to reply
    Loading