Add Jira Connect public key storage
What does this MR do and why?
This is part of #372967 (closed). See !96818 (closed) for a full context MR.
When a user installs the GitLab for Jira app we receive an installed hook. It includes a JWT token that we have to verify using a public key. The public key is fetched from connect-install-keys.atlassian.com
(see lib/atlassian/jira_connect/jwt/asymmetric.rb:15).
To make the app available for self-managed users, GitLab.com will serve as a proxy. It forwards the installed hook to the self-managed instance, but generates a new JWT token. To make this work, we need to:
- Build the JWT infrastructure (This MR)
- Add a service to generate JWT tokens.
- Store the public keys with an expiry date.
- Provide an endpoint to fetch public keys.
- Allow the public key CDN URL to be configured. (!98437 (merged))
- Add an application setting that defaults to
https://connect-install-keys.atlassian.com
and can be pointed tohttps://gitlab.com/-/jira_connect/-/jira_connect/public_keys
.
- Add an application setting that defaults to
- Forward the installed event to self-managed
- Add a service that sends an installed hook to the self-managed instance when
instance_url
is updated.
- Add a service that sends an installed hook to the self-managed instance when
I explained the problem in more detail in #372967 (closed)
How to set up and validate locally
- Go to http://localhost:3000/admin/application_settings/network
- Expand the Outbound requests section
- Enable Allow requests to the local network from web hooks and services
- Open a rails console
rails c
- Enable the
jira_connect_oauth_self_managed
feature:Feature.enable(:jira_connect_oauth_self_managed)
- Execute the following lines:
# Create a JiraConnect installation
installation = JiraConnectInstallation.create(client_key: '123', shared_secret: '123', base_url: 'https://sample.atlassian.net')
# Generate a new JWT token for the installation
jwt = JiraConnect::CreateAsymmetricJwtService.new(installation).execute
# Fetch the public key ID from the JWT header. The 3rd parameter defines if the decoding should be verified with a public key. In this case, it is not.
key_id = Atlassian::Jwt.decode(jwt, nil, false, algorithm: 'RS256').last['kid']
# Retrieve the public key from storage
public_key_string = Gitlab::HTTP.get('http://127.0.0.1:3000/-/jira_connect/public_keys/' + key_id).body
# Read the public key
public_key = OpenSSL::PKey.read(public_key_string)
# Do a verified decoding of the JWT using the public key
Atlassian::Jwt.decode(jwt, public_key, true, algorithm: 'RS256').first.present?
- Verify that the last return value is
true
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %15.5
assigned to @Andysoiron
added sectiondev + 1 deleted label
Suggested Reviewers (beta)
The individuals below may be good candidates to participate in the review based on various factors.
You can use slash commands in comments to quickly assign
/assign_reviewer @user1
.Suggested Reviewers @rspeicher
,@Andysoiron
,@rymai
,@nick.thomas
,@dbalexandre
If you do not believe these suggestions are useful, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue:
https://gitlab.com/gitlab-org/gitlab/-/issues/357923
.Automatically generated by Suggested Reviewers Bot - an experimental ML-based recommendation engine created by ~"group::applied ml".
Edited by GitLab Reviewer-Recommender Bot4 Warnings d4491b3a: 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. d5738cd7: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 82bda045: 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. 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:
- The Handbook page on merge request types.
- The definition of done documentation.
1 Message 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 Serena Fang ( @serenafang
) (UTC-5, 7 hours behind@Andysoiron
)Heinrich Lee Yu ( @engwan
) (UTC+8, 6 hours ahead of@Andysoiron
)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
danger-review
job that generated this comment.Generated by
Danger- Resolved by 🤖 GitLab Bot 🤖
Proper labels assigned to this merge request. Please ignore me.
@Andysoiron - please see the following guidance and update this merge request.1 Error, 1 Warning Please add typebug typefeature, or typemaintenance label to this merge request. Please add a subtype label to this merge request. If you have added a type label and do not feel the purpose of this merge request matches one of the subtypes labels, please resolve this discussion.
Edited by 🤖 GitLab Bot 🤖
added feature flag featureaddition typefeature labels
Allure report
allure-report-publisher
generated test report!e2e-review-qa:
test report for d4491b3aexpand test summary
+-----------------------------------------------------------------------------------------+ | suites summary | +------------------------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Plan | 47 | 0 | 1 | 0 | 48 | ✅ | | Manage | 52 | 0 | 8 | 9 | 60 | ❗ | | Verify | 12 | 0 | 1 | 3 | 13 | ❗ | | Create | 30 | 0 | 1 | 1 | 31 | ❗ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | | Feature flag handler sanity checks | 9 | 0 | 0 | 0 | 9 | ✅ | | Version sanity check | 0 | 0 | 1 | 0 | 1 | ➖ | | Configure | 0 | 0 | 1 | 0 | 1 | ➖ | | Protect | 2 | 0 | 0 | 0 | 2 | ✅ | | Secure | 2 | 0 | 0 | 0 | 2 | ✅ | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Total | 154 | 0 | 14 | 13 | 168 | ❗ | +------------------------------------+--------+--------+---------+-------+-------+--------+
- Resolved by George Koltsov
Hi @bmarjanovic do you have time to give this an initial review?
requested review from @bmarjanovic
mentioned in merge request !98437 (merged)
- Resolved by Andy Schoenen
- Resolved by Bojan Marjanovic
- Resolved by Bojan Marjanovic
- Resolved by Bojan Marjanovic
- Resolved by Bojan Marjanovic
requested review from @georgekoltsov and removed review request for @bmarjanovic
@bmarjanovic
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.
For more info, please refer to the following links:
mentioned in epic &5650 (closed)
- Resolved by George Koltsov
- Resolved by George Koltsov
- Resolved by George Koltsov
- Resolved by George Koltsov
- Resolved by George Koltsov
- Resolved by George Koltsov
- Resolved by Andy Schoenen
- Resolved by George Koltsov
- Resolved by Andy Schoenen
- Resolved by George Koltsov
- Resolved by George Koltsov
- Resolved by George Koltsov
removed review request for @georgekoltsov
added 756 commits
-
46e13639...ff5847ed - 752 commits from branch
master
- c50d9cc5 - Add Jira Connect public key storage
- 82bda045 - Apply review suggestions
- d5738cd7 - WIP
- d6ad77d7 - Implement review suggestions
Toggle commit list-
46e13639...ff5847ed - 752 commits from branch
mentioned in issue #375503 (closed)
requested review from @georgekoltsov
enabled an automatic merge when the pipeline for 840ec89d succeeds
mentioned in commit 15bea338
mentioned in issue gitlab-com/www-gitlab-com#13724 (closed)
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowpost-deploy-db-staging label and removed workflowproduction label
added workflowpost-deploy-db-production label and removed workflowpost-deploy-db-staging label
added releasedcandidate label
mentioned in merge request !101702 (merged)
added devopsmanage label and removed 1 deleted label
added releasedpublished label and removed releasedcandidate label
added groupimport and integrate label and removed groupintegrations [DEPRECATED] label