Skip to content
Snippets Groups Projects

Add Demonstrating Proof of Possession (DPoP) for Personal Access Tokens

Closed Ameya Darshan requested to merge ameya-dpop-backend into master

What does this MR do and why?

This MR initiates the implementation of sender-constrained access tokens designed to minimise the risk of token leaks. I am putting this feature behind a feature flag.

Related to Sender constraining personal access tokens (#425130).

How to set up and validate locally

Currently this feature works only for RSA keys. Support for other algorithms will be added later.

For frontend:

  1. Checkout this branch locally.
  2. Run bin/rails db:migrate
  3. In rails console, enable the feature flag: Feature.enable(:dpop_authentication, User.first)
  4. Login as root.
  5. Go to Settings > Access tokens > Toggle the DPoP option.
  6. Confirm it persists in the database User.first.dpop_enabled and also on the frontend on refreshing the page.

For backend:

  1. Build glab from this branch.
  2. Ensure DPoP is enabled following the steps above.
  3. Ensure you have an SSH key-pair setup with the public key uploaded to your user account. Ensure that the key type is saved as "Signing" or "Authentication and Signing".
  4. Using glab generate a DPoP header: bin/glab auth dpop-gen --pat "glpat-PAT" --private-key ~.ssh/id_rsa
  5. Use the generated header to make an HTTP API request: curl http://localhost:3000/api/v4/projects --header "Private-Token: glpat-PAT" --header "DPoP: <GLAB OUTPUT HERE>"
  6. Confirm valid response is received. Confirm that the request fails without a valid DPoP header.
Edited by Ameya Darshan

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
  • 5 Warnings
    :warning: This merge request is quite big (887 lines changed), please consider splitting it into multiple merge requests.
    :warning: 11bf173d: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines.
    :warning: 44443f2c: 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: :hourglass: Migration Timestamp Out of Date
    The following migrations have timestamps that are over three weeks old:
    • db/migrate/20240523115831_add_dpop_enabled_to_user_preferences.rb

    Please double check the timestamps and update them if possible. Why does this matter?

    :warning: This merge request contains lines with testid selectors. Please ensure e2e:package-and-test job is run.
    3 Messages
    :book: CHANGELOG missing:

    If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog.

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

    :book: This merge request adds or changes files that require a review from the Database team.
    :book: This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge.

    This merge request requires a database review. To make sure these changes are reviewed, take the following steps:

    1. Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
    2. Prepare your MR for database review according to the docs.
    3. Assign and mention the database reviewer suggested by Reviewer Roulette.

    The following files require a review from the Database team:

    • db/migrate/20240523115831_add_dpop_enabled_to_user_preferences.rb
    • db/schema_migrations/20240523115831
    • db/structure.sql

    Documentation review

    The following files require a review from a technical writer:

    The review does not need to block merging this merge request. See the:

    testid selectors

    The following changed lines in this MR contain testid selectors:

    app/views/user_settings/personal_access_tokens/index.html.haml

    +  = gitlab_ui_form_for @user, url: toggle_dpop_user_settings_personal_access_tokens_path, method: :put, html: { data: { testid: 'dpop-form' } } do |f|

    If the e2e:package-and-test job in the qa stage has run automatically, please ensure the tests are passing. If the job has not run, please start the manual:e2e-test-pipeline-generate job in the prepare stage and ensure the tests in follow-up:e2e:package-and-test-ee pipeline are passing.

    For the list of known failures please refer to the latest pipeline triage issue.

    If your changes are under a feature flag, please check our Testing with feature flags documentation for instructions.

    Reviewer roulette

    Category Reviewer Maintainer
    backend @jmontal profile link current availability (UTC-6, 11.5 hours behind author) @vshushlin profile link current availability (UTC+2, 3.5 hours behind author)
    database @fcatteau profile link current availability (UTC+2, 3.5 hours behind author) @praba.m7n profile link current availability (UTC+5.5, same timezone as author)
    frontend @leetickett-gitlab profile link current availability (UTC+1, 4.5 hours behind author) @deepika.guliani profile link current availability (UTC+5.5, same timezone as author)
    groupauthentication Reviewer review is optional for groupauthentication @ifarkas profile link current availability (UTC+2, 3.5 hours behind author)

    Please check reviewer's status!

    • available Reviewer is available!
    • unavailable Reviewer is unavailable!

    Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

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

    Generated by :no_entry_sign: Danger

  • E2E Test Result Summary

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :white_check_mark: test report for 11bf173d

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Govern      | 128    | 0      | 2       | 0     | 130   | ✅     |
    | Create      | 121    | 0      | 10      | 1     | 131   | ✅     |
    | Verify      | 31     | 0      | 1       | 0     | 32    | ✅     |
    | Package     | 19     | 0      | 12      | 0     | 31    | ✅     |
    | Plan        | 54     | 0      | 2       | 0     | 56    | ✅     |
    | Data Stores | 31     | 0      | 0       | 0     | 31    | ✅     |
    | Release     | 5      | 0      | 0       | 0     | 5     | ✅     |
    | Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Analytics   | 1      | 0      | 1       | 0     | 2     | ✅     |
    | Manage      | 0      | 0      | 1       | 0     | 1     | ➖     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 398    | 0      | 29      | 1     | 427   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+

    e2e-package-and-test: :white_check_mark: test report for 11bf173d

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Govern      | 180    | 0      | 18      | 0     | 198   | ✅     |
    | Plan        | 44     | 0      | 4       | 0     | 48    | ✅     |
    | Package     | 6      | 0      | 8       | 0     | 14    | ✅     |
    | Verify      | 10     | 0      | 0       | 0     | 10    | ✅     |
    | Data Stores | 22     | 0      | 0       | 0     | 22    | ✅     |
    | Create      | 46     | 0      | 6       | 0     | 52    | ✅     |
    | Release     | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 318    | 0      | 36      | 0     | 354   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
  • added 1 commit

    Compare with previous version

  • added 1 commit

    • ddf8b3f8 - Add errors, spec and spec placeholders

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • Ameya Darshan added 14 commits

    added 14 commits

    Compare with previous version

  • Ameya Darshan added 16 commits

    added 16 commits

    Compare with previous version

  • added 1 commit

    • 00d9812f - Fix rubocop offenses and add JWT expiry with spec

    Compare with previous version

  • :tools: Generated by gitlab_quality-test_tooling.


    :snail: Slow tests detected in this merge request. These slow tests might be related to this merge request's changes.

    Click to expand
    Job File Name Duration Expected duration
    #6544434696 spec/features/profile_spec.rb#L68 Profile account page allows resetting of feed token 61.99 s < 50.13 s
    #6544434696 spec/features/profile_spec.rb#L88 Profile account page allows resetting of incoming email token 124.57 s < 50.13 s
    #6544434696 spec/features/profile_spec.rb#L45 Profile account page when I delete my account shows invalid password flash message 61.96 s < 50.13 s
    #6544434696 spec/features/profile_spec.rb#L57 Profile account page when I delete my account does not show delete button when user owns a group 62.04 s < 50.13 s
    #6544434154 spec/features/profile_spec.rb#L68 Profile account page allows resetting of feed token 62.42 s < 50.13 s
    #6544434154 spec/features/profile_spec.rb#L88 Profile account page allows resetting of incoming email token 62.54 s < 50.13 s
    #6544434154 spec/features/profile_spec.rb#L45 Profile account page when I delete my account shows invalid password flash message 62.25 s < 50.13 s
    #6544434154 spec/features/profile_spec.rb#L57 Profile account page when I delete my account does not show delete button when user owns a group 62.24 s < 50.13 s
    #6675178351 ee/spec/models/epic_spec.rb#L917 Epic relative positioning there is a parent behaves like a class that supports relative positioning .move_nulls_to_end can move many nulls 45.99 s < 45.4 s
    #6675178319 ee/spec/models/epic_spec.rb#L917 Epic relative positioning there is a parent behaves like a class that supports relative positioning .move_nulls_to_end can move many nulls 54.62 s < 45.4 s
    #6686945175 ee/spec/models/epic_spec.rb#L917 Epic relative positioning there is a parent behaves like a class that supports relative positioning .move_nulls_to_end can move many nulls 49.3 s < 45.4 s
    #6717709293 spec/features/user_settings/personal_access_tokens_spec.rb#L92 User Settings > Personal Access Tokens inactive tokens allows revocation of an active token 62.22 s < 50.13 s
    #6717709293 spec/features/user_settings/personal_access_tokens_spec.rb#L107 User Settings > Personal Access Tokens inactive tokens when revocation fails displays an error message 124.79 s < 50.13 s
    #6717709568 spec/features/user_settings/personal_access_tokens_spec.rb#L92 User Settings > Personal Access Tokens inactive tokens allows revocation of an active token 62.67 s < 50.13 s
    #6717709568 spec/features/user_settings/personal_access_tokens_spec.rb#L107 User Settings > Personal Access Tokens inactive tokens when revocation fails displays an error message 125.73 s < 50.13 s
    #6717709105 spec/features/user_settings/personal_access_tokens_spec.rb#L92 User Settings > Personal Access Tokens inactive tokens allows revocation of an active token 62.46 s < 50.13 s
    #6717709105 spec/features/user_settings/personal_access_tokens_spec.rb#L107 User Settings > Personal Access Tokens inactive tokens when revocation fails displays an error message 63.15 s < 50.13 s
    #6717709822 spec/features/user_settings/personal_access_tokens_spec.rb#L92 User Settings > Personal Access Tokens inactive tokens allows revocation of an active token 125.39 s < 50.13 s
    #6717709822 spec/features/user_settings/personal_access_tokens_spec.rb#L107 User Settings > Personal Access Tokens inactive tokens when revocation fails displays an error message 125.05 s < 50.13 s
    #6732124017 ee/spec/models/epic_spec.rb#L917 Epic relative positioning there is a parent behaves like a class that supports relative positioning .move_nulls_to_end can move many nulls 47.27 s < 45.4 s
    #6732123906 ee/spec/models/epic_spec.rb#L917 Epic relative positioning there is a parent behaves like a class that supports relative positioning .move_nulls_to_end can move many nulls 49.77 s < 45.4 s
    #6860828747 ee/spec/models/epic_spec.rb#L917 Epic relative positioning there is a parent behaves like a class that supports relative positioning .move_nulls_to_end can move many nulls 45.86 s < 45.4 s
    #6869504814 ee/spec/models/epic_spec.rb#L917 Epic relative positioning there is a parent behaves like a class that supports relative positioning .move_nulls_to_end can move many nulls 45.58 s < 45.4 s
    #6900553323 ee/spec/models/epic_spec.rb#L917 Epic relative positioning there is a parent behaves like a class that supports relative positioning .move_nulls_to_end can move many nulls 46.25 s < 45.4 s
    #6902408334 ee/spec/models/epic_spec.rb#L917 Epic relative positioning there is a parent behaves like a class that supports relative positioning .move_nulls_to_end can move many nulls 47.65 s < 45.4 s
    #6922146132 ee/spec/models/epic_spec.rb#L917 Epic relative positioning there is a parent behaves like a class that supports relative positioning .move_nulls_to_end can move many nulls 47.17 s < 45.4 s
    #6923373138 ee/spec/models/epic_spec.rb#L924 Epic relative positioning there is a parent behaves like a class that supports relative positioning .move_nulls_to_end can move many nulls 48.43 s < 45.4 s
  • A deleted user added rspec:slow test detected label
  • added 1 commit

    • bacd6e2c - Disable rubocop for private key in spec

    Compare with previous version

  • Ameya Darshan added 3 commits

    added 3 commits

    • 0d7c83c6 - Fix rubocop offenses and add JWT expiry with spec
    • f2db78de - Disable rubocop for private key in spec
    • 2f464723 - Fix user pref spec error

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading