diff options
30 files changed, 712 insertions, 62 deletions
diff --git a/.gitignore b/.gitignore index cb718a6939f..104c6930050 100644 --- a/.gitignore +++ b/.gitignore @@ -75,6 +75,8 @@ eslint-report.html /.rspec /plugins/* /.gitlab_pages_secret +/.gitlab_smime_key +/.gitlab_smime_cert package-lock.json /junit_*.xml /coverage-frontend/ diff --git a/.gitlab/ci/qa.gitlab-ci.yml b/.gitlab/ci/qa.gitlab-ci.yml index dcc681294d2..144e5392e55 100644 --- a/.gitlab/ci/qa.gitlab-ci.yml +++ b/.gitlab/ci/qa.gitlab-ci.yml @@ -15,13 +15,13 @@ - branches@gitlab-org/gitlab-ce - branches@gitlab-org/gitlab-ee -package-and-qa: - extends: .package-and-qa-base +package-and-qa-manual: + extends: + - .package-and-qa-base + - .no-docs-and-no-qa when: manual - except: - - /(^qa[\/-].*|.*-qa$)/ -package-and-qa-always: +package-and-qa: extends: .package-and-qa-base allow_failure: true only: diff --git a/.gitlab/ci/review.gitlab-ci.yml b/.gitlab/ci/review.gitlab-ci.yml index 78431e3d842..beb049c0b3b 100644 --- a/.gitlab/ci/review.gitlab-ci.yml +++ b/.gitlab/ci/review.gitlab-ci.yml @@ -171,7 +171,9 @@ review-qa-all: - gitlab-qa Test::Instance::Any "${QA_IMAGE}" "${CI_ENVIRONMENT_URL}" -- --format RspecJunitFormatter --out tmp/rspec-${CI_JOB_ID}.xml --format html --out tmp/rspec.htm --color --format documentation parallel-spec-reports: - extends: .dedicated-runner + extends: + - .dedicated-runner + - .no-docs dependencies: - review-qa-all image: ruby:2.6-alpine diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index 0f55bebd3fc..7843409f4a7 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -87,9 +87,6 @@ export default class MergeRequestStore { this.allowCollaboration = data.allow_collaboration; this.sourceProjectId = data.source_project_id; this.targetProjectId = data.target_project_id; - this.mergePipelinesEnabled = Boolean(data.merge_pipelines_enabled); - this.mergeTrainsCount = data.merge_trains_count || 0; - this.mergeTrainIndex = data.merge_train_index; // CI related this.hasCI = data.has_ci; diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb index 554b307d4f8..c8088608cb0 100644 --- a/app/serializers/merge_request_widget_entity.rb +++ b/app/serializers/merge_request_widget_entity.rb @@ -84,8 +84,10 @@ class MergeRequestWidgetEntity < Grape::Entity private + delegate :current_user, to: :request + def presenter(merge_request) @presenters ||= {} - @presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: request.current_user) # rubocop: disable CodeReuse/Presenter + @presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: current_user) # rubocop: disable CodeReuse/Presenter end end diff --git a/changelogs/unreleased/feat-smime-signed-notification-emails.yml b/changelogs/unreleased/feat-smime-signed-notification-emails.yml new file mode 100644 index 00000000000..9672d0d964c --- /dev/null +++ b/changelogs/unreleased/feat-smime-signed-notification-emails.yml @@ -0,0 +1,5 @@ +--- +title: Notification emails can be signed with SMIME +merge_request: 30644 +author: Diego Louzán +type: added diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 226f2ec3722..2f6658594cc 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -95,6 +95,15 @@ production: &base email_display_name: GitLab email_reply_to: noreply@example.com email_subject_suffix: '' + email_smime: + # Uncomment and set to true if you need to enable email S/MIME signing (default: false) + # enabled: false + # S/MIME private key file in PEM format, unencrypted + # Default is '.gitlab_smime_key' relative to Rails.root (i.e. root of the GitLab app). + # key_file: /home/git/gitlab/.gitlab_smime_key + # S/MIME public certificate key in PEM format, will be attached to signed messages + # Default is '.gitlab_smime_cert' relative to Rails.root (i.e. root of the GitLab app). + # cert_file: /home/git/gitlab/.gitlab_smime_cert # Email server smtp settings are in config/initializers/smtp_settings.rb.sample diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 828732126b6..fdc6b0a05ab 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -1,5 +1,6 @@ require_relative '../settings' require_relative '../object_store_settings' +require_relative '../smime_signature_settings' # Default settings Settings['ldap'] ||= Settingslogic.new({}) @@ -171,6 +172,7 @@ Settings.gitlab['email_from'] ||= ENV['GITLAB_EMAIL_FROM'] || "gitlab@#{Settings Settings.gitlab['email_display_name'] ||= ENV['GITLAB_EMAIL_DISPLAY_NAME'] || 'GitLab' Settings.gitlab['email_reply_to'] ||= ENV['GITLAB_EMAIL_REPLY_TO'] || "noreply@#{Settings.gitlab.host}" Settings.gitlab['email_subject_suffix'] ||= ENV['GITLAB_EMAIL_SUBJECT_SUFFIX'] || "" +Settings.gitlab['email_smime'] = SmimeSignatureSettings.parse(Settings.gitlab['email_smime']) Settings.gitlab['base_url'] ||= Settings.__send__(:build_base_gitlab_url) Settings.gitlab['url'] ||= Settings.__send__(:build_gitlab_url) Settings.gitlab['user'] ||= 'git' diff --git a/config/initializers/action_mailer_hooks.rb b/config/initializers/action_mailer_hooks.rb index f1b3c1f8ae8..02ca6ef13bf 100644 --- a/config/initializers/action_mailer_hooks.rb +++ b/config/initializers/action_mailer_hooks.rb @@ -10,3 +10,8 @@ ActionMailer::Base.register_interceptors( ) ActionMailer::Base.register_observer(::Gitlab::Email::Hook::DeliveryMetricsObserver) + +if Gitlab.config.gitlab.email_enabled && Gitlab.config.gitlab.email_smime.enabled + ActionMailer::Base.register_interceptor(::Gitlab::Email::Hook::SmimeSignatureInterceptor) + Gitlab::AppLogger.debug "S/MIME signing of outgoing emails enabled" +end diff --git a/config/smime_signature_settings.rb b/config/smime_signature_settings.rb new file mode 100644 index 00000000000..3d19db84c19 --- /dev/null +++ b/config/smime_signature_settings.rb @@ -0,0 +1,11 @@ +# Set default values for email_smime settings +class SmimeSignatureSettings + def self.parse(email_smime) + email_smime ||= Settingslogic.new({}) + email_smime['enabled'] = false unless email_smime['enabled'] + email_smime['key_file'] ||= Rails.root.join('.gitlab_smime_key') + email_smime['cert_file'] ||= Rails.root.join('.gitlab_smime_cert') + + email_smime + end +end diff --git a/doc/administration/index.md b/doc/administration/index.md index 2b25e8efd23..9dbd90ecea3 100644 --- a/doc/administration/index.md +++ b/doc/administration/index.md @@ -64,6 +64,7 @@ Learn how to install, configure, update, and maintain your GitLab instance. - [External Classification Policy Authorization](../user/admin_area/settings/external_authorization.md) **(PREMIUM ONLY)** - [Upload a license](../user/admin_area/license.md): Upload a license to unlock features that are in paid tiers of GitLab. **(STARTER ONLY)** - [Admin Area](../user/admin_area/index.md): for self-managed instance-wide configuration and maintenance. +- [S/MIME Signing](smime_signing_email.md): how to sign all outgoing notification emails with S/MIME #### Customizing GitLab's appearance diff --git a/doc/administration/smime_signing_email.md b/doc/administration/smime_signing_email.md new file mode 100644 index 00000000000..9f719088f25 --- /dev/null +++ b/doc/administration/smime_signing_email.md @@ -0,0 +1,49 @@ +# Signing outgoing email with S/MIME + +Notification emails sent by Gitlab can be signed with S/MIME for improved +security. + +> **Note:** +Please be aware that S/MIME certificates and TLS/SSL certificates are not the +same and are used for different purposes: TLS creates a secure channel, whereas +S/MIME signs and/or encrypts the message itself + +## Enable S/MIME signing + +This setting must be explicitly enabled and a single pair of key and certificate +files must be provided in `gitlab.rb` or `gitlab.yml` if you are using Omnibus +GitLab or installed GitLab from source respectively: + +```yaml +email_smime: + enabled: true + key_file: /etc/pki/smime/private/gitlab.key + cert_file: /etc/pki/smime/certs/gitlab.crt +``` + +- Both files must be provided PEM-encoded. +- The key file must be unencrypted so that Gitlab can read it without user + intervention. + +NOTE: **Note:** Be mindful of the access levels for your private keys and visibility to +third parties. + +### How to convert S/MIME PKCS#12 / PFX format to PEM encoding + +Typically S/MIME certificates are handled in binary PKCS#12 format (`.pfx` or `.p12` +extensions), which contain the following in a single encrypted file: + +- Server certificate +- Intermediate certificates (if any) +- Private key + +In order to export the required files in PEM encoding from the PKCS#12 file, +the `openssl` command can be used: + +```bash +#-- Extract private key in PEM encoding (no password, unencrypted) +$ openssl pkcs12 -in gitlab.p12 -nocerts -nodes -out gitlab.key + +#-- Extract certificates in PEM encoding (full certs chain including CA) +$ openssl pkcs12 -in gitlab.p12 -nokeys -out gitlab.crt +``` diff --git a/doc/ci/multi_project_pipelines.md b/doc/ci/multi_project_pipelines.md index cb8d383f7d9..61f260cb70d 100644 --- a/doc/ci/multi_project_pipelines.md +++ b/doc/ci/multi_project_pipelines.md @@ -205,5 +205,5 @@ Some features are not implemented yet. For example, support for environments. - `stage` - `allow_failure` - `only` and `except` -- `when` +- `when` (only with `on_success`, `on_failure`, and `always` values) - `extends` diff --git a/doc/development/README.md b/doc/development/README.md index 09aa64a7bf1..5e35d4c7437 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -116,6 +116,7 @@ description: 'Learn how to contribute to GitLab.' ## Case studies - [Database case study: Filtering by label](filtering_by_label.md) +- [Database case study: Namespaces storage statistics](namespaces_storage_statistics.md) ## Integration guides diff --git a/doc/development/emails.md b/doc/development/emails.md index e6af075a282..edec0f86989 100644 --- a/doc/development/emails.md +++ b/doc/development/emails.md @@ -5,6 +5,10 @@ To view rendered emails "sent" in your development instance, visit [`/rails/letter_opener`](http://localhost:3000/rails/letter_opener). +Please note that [S/MIME signed](../administration/smime_signing_email.md) emails +[cannot be currently previewed](https://github.com/fgrehm/letter_opener_web/issues/96) with +`letter_opener`. + ## Mailer previews Rails provides a way to preview our mailer templates in HTML and plaintext using diff --git a/doc/development/namespaces_storage_statistics.md b/doc/development/namespaces_storage_statistics.md new file mode 100644 index 00000000000..b3285de4705 --- /dev/null +++ b/doc/development/namespaces_storage_statistics.md @@ -0,0 +1,178 @@ +# Database case study: Namespaces storage statistics + +## Introduction + +On [Storage and limits management for groups](https://gitlab.com/groups/gitlab-org/-/epics/886), +we want to facilitate a method for easily viewing the amount of +storage consumed by a group, and allow easy management. + +## Proposal + +1. Create a new ActiveRecord model to hold the namespaces' statistics in an aggregated form (only for root namespaces). +1. Refresh the statistics in this model every time a project belonging to this namespace is changed. + +## Problem + +In GitLab, we update the project storage statistics through a +[callback](https://gitlab.com/gitlab-org/gitlab-ce/blob/v12.2.0.pre/app/models/project.rb#L90) +every time the project is saved. + +The summary of those statistics per namespace is then retrieved +by [`Namespaces#with_statistics`](https://gitlab.com/gitlab-org/gitlab-ce/blob/v12.2.0.pre/app/models/namespace.rb#L70) scope. Analyzing this query we noticed that: + +* It takes up to `1.2` seconds for namespaces with over `15k` projects. +* It can't be analyzed with [ChatOps](chatops_on_gitlabcom.md), as it times out. + +Additionally, the pattern that is currently used to update the project statistics +(the callback) doesn't scale adequately. It is currently one of the largest +[database queries transactions on production](https://gitlab.com/gitlab-org/gitlab-ce/issues/62488) +that takes the most time overall. We can't add one more query to it as +it will increase the transaction's length. + +Because of all of the above, we can't apply the same pattern to store +and update the namespaces statistics, as the `namespaces` table is one +of the largest tables on GitLab.com. Therefore we needed to find a performant and +alternative method. + +## Attempts + +### Attempt A: PostgreSQL materialized view + +Model can be updated through a refresh strategy based on a project routes SQL and a [materialized view](https://www.postgresql.org/docs/9.6/rules-materializedviews.html): + +```sql +SELECT split_part("rs".path, '/', 1) as root_path, + COALESCE(SUM(ps.storage_size), 0) AS storage_size, + COALESCE(SUM(ps.repository_size), 0) AS repository_size, + COALESCE(SUM(ps.wiki_size), 0) AS wiki_size, + COALESCE(SUM(ps.lfs_objects_size), 0) AS lfs_objects_size, + COALESCE(SUM(ps.build_artifacts_size), 0) AS build_artifacts_size, + COALESCE(SUM(ps.packages_size), 0) AS packages_size +FROM "projects" + INNER JOIN routes rs ON rs.source_id = projects.id AND rs.source_type = 'Project' + INNER JOIN project_statistics ps ON ps.project_id = projects.id +GROUP BY root_path +``` + +We could then execute the query with: + +```sql +REFRESH MATERIALIZED VIEW root_namespace_storage_statistics; +``` + +While this implied a single query update (and probably a fast one), it has some downsides: + +* Materialized views syntax varies from PostgreSQL and MySQL. While this feature was worked on, MySQL was still supported by GitLab. +* Rails does not have native support for materialized views. We'd need to use a specialized gem to take care of the management of the database views, which implies additional work. + +### Attempt B: An update through a CTE + +Similar to Attempt A: Model update done through a refresh strategy with a [Common Table Expression](https://www.postgresql.org/docs/9.1/queries-with.html) + +```sql +WITH refresh AS ( + SELECT split_part("rs".path, '/', 1) as root_path, + COALESCE(SUM(ps.storage_size), 0) AS storage_size, + COALESCE(SUM(ps.repository_size), 0) AS repository_size, + COALESCE(SUM(ps.wiki_size), 0) AS wiki_size, + COALESCE(SUM(ps.lfs_objects_size), 0) AS lfs_objects_size, + COALESCE(SUM(ps.build_artifacts_size), 0) AS build_artifacts_size, + COALESCE(SUM(ps.packages_size), 0) AS packages_size + FROM "projects" + INNER JOIN routes rs ON rs.source_id = projects.id AND rs.source_type = 'Project' + INNER JOIN project_statistics ps ON ps.project_id = projects.id + GROUP BY root_path) +UPDATE namespace_storage_statistics +SET storage_size = refresh.storage_size, + repository_size = refresh.repository_size, + wiki_size = refresh.wiki_size, + lfs_objects_size = refresh.lfs_objects_size, + build_artifacts_size = refresh.build_artifacts_size, + packages_size = refresh.packages_size +FROM refresh + INNER JOIN routes rs ON rs.path = refresh.root_path AND rs.source_type = 'Namespace' +WHERE namespace_storage_statistics.namespace_id = rs.source_id +``` + +Same benefits and downsides as attempt A. + +### Attempt C: Get rid of the model and store the statistics on Redis + +We could get rid of the model that stores the statistics in aggregated form and instead use a Redis Set. +This would be the [boring solution](https://about.gitlab.com/handbook/values/#boring-solutions) and the fastest one +to implement, as GitLab already includes Redis as part of its [Architecture](architecture.md#redis). + +The downside of this approach is that Redis does not provide the same persistence/consistency guarantees as PostgreSQL, +and this is information we can't afford to lose in a Redis failure. + +### Attempt D: Tag the root namespace and its child namespaces + +Directly relate the root namespace to its child namespaces, so +whenever a namespace is created without a parent, this one is tagged +with the root namespace ID: + +| id | root_id | parent_id +|:---|:--------|:---------- +| 1 | 1 | NULL +| 2 | 1 | 1 +| 3 | 1 | 2 + +To aggregate the statistics inside a namespace we'd execute something like: + +```sql +SELECT COUNT(...) +FROM projects +WHERE namespace_id IN ( + SELECT id + FROM namespaces + WHERE root_id = X +) +``` + +Even though this approach would make aggregating much easier, it has some major downsides: + +* We'd have to migrate **all namespaces** by adding and filling a new column. Because of the size of the table, dealing with time/cost will not be great. The background migration will take approximately `153h`, see <https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29772>. +* Background migration has to be shipped one release before, delaying the functionality by another milestone. + +### Attempt E (final): Update the namespace storage statistics in async way + +This approach consists of keep using the incremental statistics updates we currently already have, +but we refresh them through Sidekiq jobs and in different transactions: + +1. Create a second table (`namespace_aggregation_schedules`) with two columns `id` and `namespace_id`. +1. Whenever the statistics of a project changes, insert a row into `namespace_aggregation_schedules` + - We don't insert a new row if there's already one related to the root namespace. + - Keeping in mind the length of the transaction that involves updating `project_statistics`(<https://gitlab.com/gitlab-org/gitlab-ce/issues/62488>), the insertion should be done in a different transaction and through a Sidekiq Job. +1. After inserting the row, we schedule another worker to be executed async at two different moments: + - One enqueued for immediate execution and another one scheduled in `1.5h` hours. + - We only schedule the jobs, if we can obtain a `1.5h` lease on Redis on a key based on the root namespace ID. + - If we can't obtain the lease, it indicates there's another aggregation already in progress, or scheduled in no more than `1.5h`. +1. This worker will: + - Update the root namespace storage statistics by querying all the namespaces through a service. + - Delete the related `namespace_aggregation_schedules` after the update. +1. Another Sidekiq job is also included to traverse any remaining rows on the `namespace_aggregation_schedules` table and schedule jobs for every pending row. + - This job is scheduled with cron to run every night (UTC). + +This implementation has the following benefits: + +* All the updates are done async, so we're not increasing the length of the transactions for `project_statistics`. +* We're doing the update in a single SQL query. +* It is compatible with PostgreSQL and MySQL. +* No background migration required. + +The only downside of this approach is that namespaces' statistics are updated up to `1.5` hours after the change is done, +which means there's a time window in which the statistics are inaccurate. Because we're still not +[enforcing storage limits](https://gitlab.com/gitlab-org/gitlab-ce/issues/30421), this is not a major problem. + +## Conclusion + +Updating the storage statistics asynchronously, was the less problematic and +performant approach of aggregating the root namespaces. + +All the details regarding this use case can be found on: + +* <https://gitlab.com/gitlab-org/gitlab-ce/issues/62214> +* Merge Request with the implementation: <https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/28996> + +Performance of the namespace storage statistics were measured in staging and production (GitLab.com). All results were posted +on <https://gitlab.com/gitlab-org/gitlab-ce/issues/64092>: No problem has been reported so far. diff --git a/doc/development/testing_guide/end_to_end/index.md b/doc/development/testing_guide/end_to_end/index.md index d6b944a3e74..3ae3ce183d9 100644 --- a/doc/development/testing_guide/end_to_end/index.md +++ b/doc/development/testing_guide/end_to_end/index.md @@ -45,11 +45,11 @@ Results are reported in the `#qa-staging` Slack channel. ### Testing code in merge requests -#### Using the `package-and-qa` job +#### Using the `package-and-qa-manual` job It is possible to run end-to-end tests for a merge request, eventually being run in a pipeline in the [`gitlab-qa`](https://gitlab.com/gitlab-org/gitlab-qa/) project, -by triggering the `package-and-qa` manual action in the `test` stage (not +by triggering the `package-and-qa-manual` manual action in the `test` stage (not available for forks). **This runs end-to-end tests against a custom Omnibus package built from your @@ -71,7 +71,7 @@ graph LR B2[`Trigger-qa` stage<br>`Trigger:qa-test` job] -.->|2. Triggers a gitlab-qa pipeline and wait for it to be done| A3 subgraph "gitlab-ce/ee pipeline" - A1[`test` stage<br>`package-and-qa` job] + A1[`test` stage<br>`package-and-qa-manual` job] end subgraph "omnibus-gitlab pipeline" @@ -79,7 +79,7 @@ subgraph "omnibus-gitlab pipeline" end subgraph "gitlab-qa pipeline" - A3>QA jobs run] -.->|3. Reports back the pipeline result to the `package-and-qa` job<br>and post the result on the original commit tested| A1 + A3>QA jobs run] -.->|3. Reports back the pipeline result to the `package-and-qa-manual` job<br>and post the result on the original commit tested| A1 end ``` diff --git a/doc/development/testing_guide/end_to_end/page_objects.md b/doc/development/testing_guide/end_to_end/page_objects.md index 47e58a425fd..850ea6b60ac 100644 --- a/doc/development/testing_guide/end_to_end/page_objects.md +++ b/doc/development/testing_guide/end_to_end/page_objects.md @@ -40,7 +40,7 @@ the time it would take to build packages and test everything. That is why when someone changes `t.text_field :login` to `t.text_field :username` in the _new session_ view we won't know about this change until our GitLab QA nightly pipeline fails, or until someone triggers -`package-and-qa` action in their merge request. +`package-and-qa-manual` action in their merge request. Obviously such a change would break all tests. We call this problem a _fragile tests problem_. diff --git a/lib/gitlab/email/hook/smime_signature_interceptor.rb b/lib/gitlab/email/hook/smime_signature_interceptor.rb new file mode 100644 index 00000000000..e48041d9218 --- /dev/null +++ b/lib/gitlab/email/hook/smime_signature_interceptor.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module Gitlab + module Email + module Hook + class SmimeSignatureInterceptor + # Sign emails with SMIME if enabled + class << self + def delivering_email(message) + signed_message = Gitlab::Email::Smime::Signer.sign( + cert: certificate.cert, + key: certificate.key, + data: message.encoded) + signed_email = Mail.new(signed_message) + + overwrite_body(message, signed_email) + overwrite_headers(message, signed_email) + end + + private + + def certificate + @certificate ||= Gitlab::Email::Smime::Certificate.from_files(key_path, cert_path) + end + + def key_path + Gitlab.config.gitlab.email_smime.key_file + end + + def cert_path + Gitlab.config.gitlab.email_smime.cert_file + end + + def overwrite_body(message, signed_email) + # since this is a multipart email, assignment to nil is important, + # otherwise Message#body will add a new mail part + message.body = nil + message.body = signed_email.body.encoded + end + + def overwrite_headers(message, signed_email) + message.content_disposition = signed_email.content_disposition + message.content_transfer_encoding = signed_email.content_transfer_encoding + message.content_type = signed_email.content_type + end + end + end + end + end +end diff --git a/lib/gitlab/email/smime/certificate.rb b/lib/gitlab/email/smime/certificate.rb new file mode 100644 index 00000000000..b331c4ca19c --- /dev/null +++ b/lib/gitlab/email/smime/certificate.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module Gitlab + module Email + module Smime + class Certificate + include OpenSSL + + attr_reader :key, :cert + + def key_string + @key.to_s + end + + def cert_string + @cert.to_pem + end + + def self.from_strings(key_string, cert_string) + key = PKey::RSA.new(key_string) + cert = X509::Certificate.new(cert_string) + new(key, cert) + end + + def self.from_files(key_path, cert_path) + from_strings(File.read(key_path), File.read(cert_path)) + end + + def initialize(key, cert) + @key = key + @cert = cert + end + end + end + end +end diff --git a/lib/gitlab/email/smime/signer.rb b/lib/gitlab/email/smime/signer.rb new file mode 100644 index 00000000000..2fa83014003 --- /dev/null +++ b/lib/gitlab/email/smime/signer.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'openssl' + +module Gitlab + module Email + module Smime + # Tooling for signing and verifying data with SMIME + class Signer + include OpenSSL + + def self.sign(cert:, key:, data:) + signed_data = PKCS7.sign(cert, key, data, nil, PKCS7::DETACHED) + PKCS7.write_smime(signed_data) + end + + # return nil if data cannot be verified, otherwise the signed content data + def self.verify_signature(cert:, ca_cert: nil, signed_data:) + store = X509::Store.new + store.set_default_paths + store.add_cert(ca_cert) if ca_cert + + signed_smime = PKCS7.read_smime(signed_data) + signed_smime if signed_smime.verify([cert], store) + end + end + end + end +end diff --git a/qa/README.md b/qa/README.md index 97555f8d0c2..27e11c04b3e 100644 --- a/qa/README.md +++ b/qa/README.md @@ -30,7 +30,7 @@ and corresponding views / partials / selectors in CE / EE. Whenever `qa:selectors` job fails in your merge request, you are supposed to fix [page objects](../doc/development/testing_guide/end_to_end/page_objects.md). You should also trigger end-to-end tests -using `package-and-qa` manual action, to test if everything works fine. +using `package-and-qa-manual` manual action, to test if everything works fine. ## How can I use it? diff --git a/spec/config/smime_signature_settings_spec.rb b/spec/config/smime_signature_settings_spec.rb new file mode 100644 index 00000000000..4f0c227d866 --- /dev/null +++ b/spec/config/smime_signature_settings_spec.rb @@ -0,0 +1,56 @@ +require 'fast_spec_helper' + +describe SmimeSignatureSettings do + describe '.parse' do + let(:default_smime_key) { Rails.root.join('.gitlab_smime_key') } + let(:default_smime_cert) { Rails.root.join('.gitlab_smime_cert') } + + it 'sets correct default values to disabled' do + parsed_settings = described_class.parse(nil) + + expect(parsed_settings['enabled']).to be(false) + expect(parsed_settings['key_file']).to eq(default_smime_key) + expect(parsed_settings['cert_file']).to eq(default_smime_cert) + end + + context 'when providing custom values' do + it 'sets correct default values to disabled' do + custom_settings = Settingslogic.new({}) + + parsed_settings = described_class.parse(custom_settings) + + expect(parsed_settings['enabled']).to be(false) + expect(parsed_settings['key_file']).to eq(default_smime_key) + expect(parsed_settings['cert_file']).to eq(default_smime_cert) + end + + it 'enables smime with default key and cert' do + custom_settings = Settingslogic.new({ + 'enabled' => true + }) + + parsed_settings = described_class.parse(custom_settings) + + expect(parsed_settings['enabled']).to be(true) + expect(parsed_settings['key_file']).to eq(default_smime_key) + expect(parsed_settings['cert_file']).to eq(default_smime_cert) + end + + it 'enables smime with custom key and cert' do + custom_key = '/custom/key' + custom_cert = '/custom/cert' + custom_settings = Settingslogic.new({ + 'enabled' => true, + 'key_file' => custom_key, + 'cert_file' => custom_cert + }) + + parsed_settings = described_class.parse(custom_settings) + + expect(parsed_settings['enabled']).to be(true) + expect(parsed_settings['key_file']).to eq(custom_key) + expect(parsed_settings['cert_file']).to eq(custom_cert) + end + end + end +end diff --git a/spec/initializers/action_mailer_hooks_spec.rb b/spec/initializers/action_mailer_hooks_spec.rb new file mode 100644 index 00000000000..3826ed9b00a --- /dev/null +++ b/spec/initializers/action_mailer_hooks_spec.rb @@ -0,0 +1,46 @@ +require 'spec_helper' + +describe 'ActionMailer hooks' do + describe 'smime signature interceptor' do + before do + class_spy(ActionMailer::Base).as_stubbed_const + end + + it 'is disabled by default' do + load Rails.root.join('config/initializers/action_mailer_hooks.rb') + + expect(ActionMailer::Base).not_to( + have_received(:register_interceptor).with(Gitlab::Email::Hook::SmimeSignatureInterceptor)) + end + + describe 'interceptor testbed' do + where(:email_enabled, :email_smime_enabled, :smime_interceptor_enabled) do + [ + [false, false, false], + [false, true, false], + [true, false, false], + [true, true, true] + ] + end + + with_them do + before do + stub_config_setting(email_enabled: email_enabled) + stub_config_setting(email_smime: { enabled: email_smime_enabled }) + end + + it 'is enabled depending on settings' do + load Rails.root.join('config/initializers/action_mailer_hooks.rb') + + if smime_interceptor_enabled + expect(ActionMailer::Base).to( + have_received(:register_interceptor).with(Gitlab::Email::Hook::SmimeSignatureInterceptor)) + else + expect(ActionMailer::Base).not_to( + have_received(:register_interceptor).with(Gitlab::Email::Hook::SmimeSignatureInterceptor)) + end + end + end + end + end +end diff --git a/spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js b/spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js index e27a506f426..e2cd0f084fd 100644 --- a/spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js +++ b/spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js @@ -82,47 +82,5 @@ describe('MergeRequestStore', () => { expect(store.isNothingToMergeState).toEqual(false); }); }); - - describe('mergePipelinesEnabled', () => { - it('should set mergePipelinesEnabled = true when merge_pipelines_enabled is true', () => { - store.setData({ ...mockData, merge_pipelines_enabled: true }); - - expect(store.mergePipelinesEnabled).toBe(true); - }); - - it('should set mergePipelinesEnabled = false when merge_pipelines_enabled is not provided', () => { - store.setData({ ...mockData, merge_pipelines_enabled: undefined }); - - expect(store.mergePipelinesEnabled).toBe(false); - }); - }); - - describe('mergeTrainsCount', () => { - it('should set mergeTrainsCount when merge_trains_count is provided', () => { - store.setData({ ...mockData, merge_trains_count: 3 }); - - expect(store.mergeTrainsCount).toBe(3); - }); - - it('should set mergeTrainsCount = 0 when merge_trains_count is not provided', () => { - store.setData({ ...mockData, merge_trains_count: undefined }); - - expect(store.mergeTrainsCount).toBe(0); - }); - }); - - describe('mergeTrainIndex', () => { - it('should set mergeTrainIndex when merge_train_index is provided', () => { - store.setData({ ...mockData, merge_train_index: 3 }); - - expect(store.mergeTrainIndex).toBe(3); - }); - - it('should not set mergeTrainIndex when merge_train_index is not provided', () => { - store.setData({ ...mockData, merge_train_index: undefined }); - - expect(store.mergeTrainIndex).toBeUndefined(); - }); - }); }); }); diff --git a/spec/lib/gitlab/email/hook/disable_email_interceptor_spec.rb b/spec/lib/gitlab/email/hook/disable_email_interceptor_spec.rb index 0c58cf088cc..c8ed12523d0 100644 --- a/spec/lib/gitlab/email/hook/disable_email_interceptor_spec.rb +++ b/spec/lib/gitlab/email/hook/disable_email_interceptor_spec.rb @@ -13,9 +13,6 @@ describe Gitlab::Email::Hook::DisableEmailInterceptor do end after do - # Removing interceptor from the list because unregister_interceptor is - # implemented in later version of mail gem - # See: https://github.com/mikel/mail/pull/705 Mail.unregister_interceptor(described_class) end diff --git a/spec/lib/gitlab/email/hook/smime_signature_interceptor_spec.rb b/spec/lib/gitlab/email/hook/smime_signature_interceptor_spec.rb new file mode 100644 index 00000000000..35aa663b0a5 --- /dev/null +++ b/spec/lib/gitlab/email/hook/smime_signature_interceptor_spec.rb @@ -0,0 +1,52 @@ +require 'spec_helper' + +describe Gitlab::Email::Hook::SmimeSignatureInterceptor do + include SmimeHelper + + # cert generation is an expensive operation and they are used read-only, + # so we share them as instance variables in all tests + before :context do + @root_ca = generate_root + @cert = generate_cert(root_ca: @root_ca) + end + + let(:root_certificate) do + Gitlab::Email::Smime::Certificate.new(@root_ca[:key], @root_ca[:cert]) + end + + let(:certificate) do + Gitlab::Email::Smime::Certificate.new(@cert[:key], @cert[:cert]) + end + + let(:mail) do + ActionMailer::Base.mail(to: 'test@example.com', from: 'info@example.com', body: 'signed hello') + end + + before do + allow(Gitlab::Email::Smime::Certificate).to receive_messages(from_files: certificate) + + Mail.register_interceptor(described_class) + mail.deliver_now + end + + after do + Mail.unregister_interceptor(described_class) + end + + it 'signs the email appropriately with SMIME' do + expect(mail.header['To'].value).to eq('test@example.com') + expect(mail.header['From'].value).to eq('info@example.com') + expect(mail.header['Content-Type'].value).to match('multipart/signed').and match('protocol="application/x-pkcs7-signature"') + + # verify signature and obtain pkcs7 encoded content + p7enc = Gitlab::Email::Smime::Signer.verify_signature( + cert: certificate.cert, + ca_cert: root_certificate.cert, + signed_data: mail.encoded) + + # envelope in a Mail object and obtain the body + decoded_mail = Mail.new(p7enc.data) + + expect(decoded_mail.body.encoded).to eq('signed hello') + end +end diff --git a/spec/lib/gitlab/email/smime/certificate_spec.rb b/spec/lib/gitlab/email/smime/certificate_spec.rb new file mode 100644 index 00000000000..90b27602413 --- /dev/null +++ b/spec/lib/gitlab/email/smime/certificate_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Email::Smime::Certificate do + include SmimeHelper + + # cert generation is an expensive operation and they are used read-only, + # so we share them as instance variables in all tests + before :context do + @root_ca = generate_root + @cert = generate_cert(root_ca: @root_ca) + end + + describe 'testing environment setup' do + describe 'generate_root' do + subject { @root_ca } + + it 'generates a root CA that expires a long way in the future' do + expect(subject[:cert].not_after).to be > 999.years.from_now + end + end + + describe 'generate_cert' do + subject { @cert } + + it 'generates a cert properly signed by the root CA' do + expect(subject[:cert].issuer).to eq(@root_ca[:cert].subject) + end + + it 'generates a cert that expires soon' do + expect(subject[:cert].not_after).to be < 60.minutes.from_now + end + + it 'generates a cert intended for email signing' do + expect(subject[:cert].extensions).to include(an_object_having_attributes(oid: 'extendedKeyUsage', value: match('E-mail Protection'))) + end + + context 'passing in INFINITE_EXPIRY' do + subject { generate_cert(root_ca: @root_ca, expires_in: SmimeHelper::INFINITE_EXPIRY) } + + it 'generates a cert that expires a long way in the future' do + expect(subject[:cert].not_after).to be > 999.years.from_now + end + end + end + end + + describe '.from_strings' do + it 'parses correctly a certificate and key' do + parsed_cert = described_class.from_strings(@cert[:key].to_s, @cert[:cert].to_pem) + + common_cert_tests(parsed_cert, @cert, @root_ca) + end + end + + describe '.from_files' do + it 'parses correctly a certificate and key' do + allow(File).to receive(:read).with('a_key').and_return(@cert[:key].to_s) + allow(File).to receive(:read).with('a_cert').and_return(@cert[:cert].to_pem) + + parsed_cert = described_class.from_files('a_key', 'a_cert') + + common_cert_tests(parsed_cert, @cert, @root_ca) + end + end + + def common_cert_tests(parsed_cert, cert, root_ca) + expect(parsed_cert.cert).to be_a(OpenSSL::X509::Certificate) + expect(parsed_cert.cert.subject).to eq(cert[:cert].subject) + expect(parsed_cert.cert.issuer).to eq(root_ca[:cert].subject) + expect(parsed_cert.cert.not_before).to eq(cert[:cert].not_before) + expect(parsed_cert.cert.not_after).to eq(cert[:cert].not_after) + expect(parsed_cert.cert.extensions).to include(an_object_having_attributes(oid: 'extendedKeyUsage', value: match('E-mail Protection'))) + expect(parsed_cert.key).to be_a(OpenSSL::PKey::RSA) + end +end diff --git a/spec/lib/gitlab/email/smime/signer_spec.rb b/spec/lib/gitlab/email/smime/signer_spec.rb new file mode 100644 index 00000000000..56048b7148c --- /dev/null +++ b/spec/lib/gitlab/email/smime/signer_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Email::Smime::Signer do + include SmimeHelper + + it 'signs data appropriately with SMIME' do + root_certificate = generate_root + certificate = generate_cert(root_ca: root_certificate) + + signed_content = described_class.sign( + cert: certificate[:cert], + key: certificate[:key], + data: 'signed content') + expect(signed_content).not_to be_nil + + p7enc = described_class.verify_signature( + cert: certificate[:cert], + ca_cert: root_certificate[:cert], + signed_data: signed_content) + + expect(p7enc).not_to be_nil + expect(p7enc.data).to eq('signed content') + end +end diff --git a/spec/support/helpers/smime_helper.rb b/spec/support/helpers/smime_helper.rb new file mode 100644 index 00000000000..656b3e196ba --- /dev/null +++ b/spec/support/helpers/smime_helper.rb @@ -0,0 +1,55 @@ +module SmimeHelper + include OpenSSL + + INFINITE_EXPIRY = 1000.years + SHORT_EXPIRY = 30.minutes + + def generate_root + issue(signed_by: nil, expires_in: INFINITE_EXPIRY, certificate_authority: true) + end + + def generate_cert(root_ca:, expires_in: SHORT_EXPIRY) + issue(signed_by: root_ca, expires_in: expires_in, certificate_authority: false) + end + + # returns a hash { key:, cert: } containing a generated key, cert pair + def issue(email_address: 'test@example.com', signed_by:, expires_in:, certificate_authority:) + key = OpenSSL::PKey::RSA.new(4096) + public_key = key.public_key + + subject = if certificate_authority + X509::Name.parse("/CN=EU") + else + X509::Name.parse("/CN=#{email_address}") + end + + cert = X509::Certificate.new + cert.subject = subject + + cert.issuer = signed_by&.fetch(:cert, nil)&.subject || subject + + cert.not_before = Time.now + cert.not_after = expires_in.from_now + cert.public_key = public_key + cert.serial = 0x0 + cert.version = 2 + + extension_factory = X509::ExtensionFactory.new + if certificate_authority + extension_factory.subject_certificate = cert + extension_factory.issuer_certificate = cert + cert.add_extension(extension_factory.create_extension('subjectKeyIdentifier', 'hash')) + cert.add_extension(extension_factory.create_extension('basicConstraints', 'CA:TRUE', true)) + cert.add_extension(extension_factory.create_extension('keyUsage', 'cRLSign,keyCertSign', true)) + else + cert.add_extension(extension_factory.create_extension('subjectAltName', "email:#{email_address}", false)) + cert.add_extension(extension_factory.create_extension('basicConstraints', 'CA:FALSE', true)) + cert.add_extension(extension_factory.create_extension('keyUsage', 'digitalSignature,keyEncipherment', true)) + cert.add_extension(extension_factory.create_extension('extendedKeyUsage', 'clientAuth,emailProtection', false)) + end + + cert.sign(signed_by&.fetch(:key, nil) || key, Digest::SHA256.new) + + { key: key, cert: cert } + end +end |