Add DPoP checks in GraphQL and API requests
What does this MR do and why?
See Sender constraining personal access tokens (#425130) for more context. This MR makes use of the backend DPoP checks that are introduced in Parse and validate DPoP Tokens (!166206 - merged).
How to set up and validate locally
- In rails console, enable the feature flag for the user you are going to be testing the feature with:
Feature.enable(:dpop_authentication, User.find(1))
- Using the rails console, enable DPoP for the user :
UserPreferences::UpdateService.new(User.find(1), {dpop_enabled: true}).execute
- 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".
- Build
glab
from this branch. - Using
glab
generate a DPoP header:bin/glab auth dpop-gen --pat "<glpat-PAT>" --private-key ~/.ssh/id_rsa
- Use the generated header to make an HTTP API request eg.:
curl http://localhost:3000/api/v4/projects --header "Private-Token: <glpat-PAT>" --header "DPoP: <GLAB OUTPUT HERE>"
- Confirm valid response is received. Confirm that the request fails without a valid DPoP header.
- Repeat and confirm for GraphQL requests, e.g.
curl -X POST -H "Content-Type: application/json" -H "Private-Token: <glpat-PAT>" -H "DPoP: <GLAB OUTPUT HERE>" -d '{ "query": "query { currentUser { id } }" }' "http://localhost:3000/api/graphql"
- Confirm that the server responds with accurate error messages related to the failing DPoP check (eg. signature expired, JWT invalid, etc.).
Merge request reports
Activity
assigned to @ameyadarshan
added pipelinetier-3 pipeline:run-e2e-omnibus-once labels
- A deleted user
added backend label
- Resolved by 🤖 GitLab Bot 🤖
Proper labels assigned to this merge request. Please ignore me.
@ameyadarshan
- please see the following guidance and update this merge request.1 Error Please add typebug typefeature, or typemaintenance label to this merge request. Edited by 🤖 GitLab Bot 🤖
mentioned in merge request !166206 (merged)
added 4 commits
-
3aa42034...c0ebefd8 - 3 commits from branch
ameya-dpop-1
- bc1a38d6 - Merge branch 'ameya-dpop-1' into 'ameya-dpop-2'
-
3aa42034...c0ebefd8 - 3 commits from branch
E2E Test Result Summary
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for f2f98757expand test summary
+------------------------------------------------------------+ | suites summary | +-------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------+--------+--------+---------+-------+-------+--------+ +-------+--------+--------+---------+-------+-------+--------+ | Total | 0 | 0 | 0 | 0 | 0 | ➖ | +-------+--------+--------+---------+-------+-------+--------+
added AppSecWorkTypeKR label
added AppSecWeightXLarge label
added AppSecWorkflowin-progress label
changed milestone to %17.6
added Application Security Team label
added typefeature label and removed Application Security Team label
mentioned in issue #425130
added devopsgovern groupauthentication sectionsec labels
assigned to @nmalcolm
added workflowin dev label
- Resolved by Ameya Darshan
@ameyadarshan I'm trying to rebase this MR against
ameya-dpop-1
to get the latest changes, but did you squash all the commits over there? when I rebase I get an error that both this branch andameya-dpop-1
are trying to add the same files:% git rebase ameya-dpop-1 Auto-merging app/services/auth/dpop_authentication_service.rb CONFLICT (add/add): Merge conflict in app/services/auth/dpop_authentication_service.rb Auto-merging lib/gitlab/auth/dpop_token.rb CONFLICT (add/add): Merge conflict in lib/gitlab/auth/dpop_token.rb error: could not apply cf6296f91cd0... Add authentication service and error classes. Also introduces new DpopToken class. ... Unmerged paths: (use "git restore --staged <file>..." to unstage) (use "git add <file>..." to mark resolution) both added: app/services/auth/dpop_authentication_service.rb both added: lib/gitlab/auth/dpop_token.rb
I think perhaps we wait til Parse and validate DPoP Tokens (!166206 - merged) gets merged, then fix up this branch. WDYT?
Edited by Nick Malcolm
mentioned in merge request !173071 (merged)
changed milestone to %17.7
added missed:17.6 label
added Category:System Access label
changed milestone to %17.8
added missed:17.7 label
mentioned in issue #510344
changed milestone to %17.9
added missed:17.8 label
unassigned @nmalcolm
added AppSecPriority2 label
4 Warnings 50d4428a: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 99c6c43a: 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. 2c09081e: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 87c55db4: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 1 Message 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.
Reviewer roulette
Category Reviewer Maintainer backend @adruid
(UTC+1, 4.5 hours behind author)
@fdegier
(UTC+1, 4.5 hours behind author)
groupauthentication Reviewer review is optional for groupauthentication @fernando-c
(UTC-6, 11.5 hours behind author)
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
danger-review
job that generated this comment.Generated by
DangerEdited by ****added 20797 commits
-
f2f98757...d027d283 - 20794 commits from branch
master
- 11d1fbe5 - Add DPoP checks in GraphQL and API requests
- 5676f536 - Add REST API check and fix GraphQL check
- ce000cb5 - Delete test file
Toggle commit list-
f2f98757...d027d283 - 20794 commits from branch
- Resolved by Ameya Darshan
Update: With the latest commit the GraphQL check seems to be working fine, but the API check seems to be very noisy and gets triggered in the background without explicitly making API calls via the CLI, which is undesirable but understandable behaviour given the current placement of the check in the code.
added 2 commits
requested review from @ashmckenzie