From 873ec955363eb4de87c34d08b7032b140bf680e7 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Fri, 1 Jul 2022 15:52:43 -0700
Subject: [PATCH 1/3] Fix deploy keys breaking protected branch dropdown in
 FIPS mode

In FIPS mode, if a deploy key were present, clicking on the "Allowed
to push" dropdown would fail with a "Failed to load groups, users and
deploy keys" message. This occurred because the JavaScript attempted
to use the `fingerprint` attribute of the key.  However, on a FIPS
system, the MD5 fingerprint is not available, and the `null` value
breaks the dropdown.

To fix this, we use the `fingerprint_sha256` attribute instead. This
commit also puts the SHA256 fingerprint first in the HTML views to
avoid confusion.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/364562

Changelog: fixed
---
 .../admin/deploy_keys/components/table.vue    | 22 +++++++++++++---
 .../deploy_keys/components/key.vue            | 18 ++++++++++---
 .../projects/settings/access_dropdown.js      |  4 ++-
 .../settings/components/access_dropdown.vue   |  6 +++--
 app/views/shared/deploy_keys/_form.html.haml  | 11 +++++---
 locale/gitlab.pot                             |  5 +++-
 spec/features/projects/deploy_keys_spec.rb    | 26 +++++++++++++++----
 .../deploy_keys/components/table_spec.js      | 12 ++++++---
 .../components/new_access_dropdown_spec.js    | 23 ++++++++++++----
 9 files changed, 100 insertions(+), 27 deletions(-)

diff --git a/app/assets/javascripts/admin/deploy_keys/components/table.vue b/app/assets/javascripts/admin/deploy_keys/components/table.vue
index 29e8b9a724eb03..46e7ac3cf2814c 100644
--- a/app/assets/javascripts/admin/deploy_keys/components/table.vue
+++ b/app/assets/javascripts/admin/deploy_keys/components/table.vue
@@ -34,9 +34,13 @@ export default {
       key: 'title',
       label: __('Title'),
     },
+    {
+      key: 'fingerprint_sha256',
+      label: __('Fingerprint (SHA256)'),
+    },
     {
       key: 'fingerprint',
-      label: __('Fingerprint'),
+      label: __('Fingerprint (MD5)'),
     },
     {
       key: 'projects',
@@ -130,10 +134,18 @@ export default {
         }
 
         this.items = items.map(
-          ({ id, title, fingerprint, projects_with_write_access, created_at }) => ({
+          ({
             id,
             title,
             fingerprint,
+            fingerprint_sha256,
+            projects_with_write_access,
+            created_at,
+          }) => ({
+            id,
+            title,
+            fingerprint,
+            fingerprint_sha256,
             projects: projects_with_write_access,
             created: created_at,
           }),
@@ -196,8 +208,12 @@ export default {
           >
         </template>
 
+        <template #cell(fingerprint_sha256)="{ item: { fingerprint_sha256 } }">
+          <span v-if="fingerprint_sha256" class="monospace">{{ fingerprint_sha256 }}</span>
+        </template>
+
         <template #cell(fingerprint)="{ item: { fingerprint } }">
-          <code>{{ fingerprint }}</code>
+          <span v-if="fingerprint" class="monospace">{{ fingerprint }}</span>
         </template>
 
         <template #cell(created)="{ item: { created } }">
diff --git a/app/assets/javascripts/deploy_keys/components/key.vue b/app/assets/javascripts/deploy_keys/components/key.vue
index 8a7d3430063bc3..d811bb3b0bfa3b 100644
--- a/app/assets/javascripts/deploy_keys/components/key.vue
+++ b/app/assets/javascripts/deploy_keys/components/key.vue
@@ -115,10 +115,20 @@ export default {
       <div role="rowheader" class="table-mobile-header">{{ s__('DeployKeys|Deploy key') }}</div>
       <div class="table-mobile-content" data-qa-selector="key_container">
         <strong class="title" data-qa-selector="key_title_content"> {{ deployKey.title }} </strong>
-        <div class="fingerprint" data-qa-selector="key_md5_fingerprint_content">
-          {{ __('MD5') }}:{{ deployKey.fingerprint }}
-        </div>
-        <div class="fingerprint">{{ __('SHA256') }}:{{ deployKey.fingerprint_sha256 }}</div>
+        <dl>
+          <dt>{{ __('SHA256') }}</dt>
+          <dd class="fingerprint" data-qa-selector="key_sha256_fingerprint_content">
+            {{ deployKey.fingerprint_sha256 }}
+          </dd>
+          <template v-if="deployKey.fingerprint">
+            <dt>
+              {{ __('MD5') }}
+            </dt>
+            <dd class="fingerprint" data-qa-selector="key_md5_fingerprint_content">
+              {{ deployKey.fingerprint }}
+            </dd>
+          </template>
+        </dl>
       </div>
     </div>
     <div class="table-section section-30 section-wrap">
diff --git a/app/assets/javascripts/projects/settings/access_dropdown.js b/app/assets/javascripts/projects/settings/access_dropdown.js
index 79dfa166b1a011..060178a3cfbcb6 100644
--- a/app/assets/javascripts/projects/settings/access_dropdown.js
+++ b/app/assets/javascripts/projects/settings/access_dropdown.js
@@ -441,11 +441,13 @@ export default class AccessDropdown {
       const {
         id,
         fingerprint,
+        fingerprint_sha256: fingerprintSha256,
         title,
         owner: { avatar_url, name, username },
       } = response;
 
-      const shortFingerprint = `(${fingerprint.substring(0, 14)}...)`;
+      const availableFingerprint = fingerprintSha256 || fingerprint;
+      const shortFingerprint = `(${availableFingerprint.substring(0, 14)}...)`;
 
       return {
         id,
diff --git a/app/assets/javascripts/projects/settings/components/access_dropdown.vue b/app/assets/javascripts/projects/settings/components/access_dropdown.vue
index a24eb1f5b203ad..fcf81c9d1f74c4 100644
--- a/app/assets/javascripts/projects/settings/components/access_dropdown.vue
+++ b/app/assets/javascripts/projects/settings/components/access_dropdown.vue
@@ -203,11 +203,13 @@ export default {
         const {
           id,
           fingerprint,
+          fingerprint_sha256: fingerprintSha256,
           title,
           owner: { avatar_url, name, username },
         } = response;
 
-        const shortFingerprint = `(${fingerprint.substring(0, 14)}...)`;
+        const availableFingerprint = fingerprintSha256 || fingerprint;
+        const shortFingerprint = `(${availableFingerprint.substring(0, 14)}...)`;
 
         return {
           id,
@@ -387,7 +389,7 @@ export default {
       }}</gl-dropdown-section-header>
       <gl-dropdown-item
         v-for="key in deployKeys"
-        :key="`${key.id}${key.fingerprint}`"
+        :key="`${key.id}-{key.title}`"
         data-testid="deploy_key-dropdown-item"
         is-check-item
         :is-checked="isSelected(key)"
diff --git a/app/views/shared/deploy_keys/_form.html.haml b/app/views/shared/deploy_keys/_form.html.haml
index 28abbf59875614..38985319ca5eb5 100644
--- a/app/views/shared/deploy_keys/_form.html.haml
+++ b/app/views/shared/deploy_keys/_form.html.haml
@@ -18,9 +18,14 @@
         = _('Paste a public key here. %{link_start}How do I generate it?%{link_end}').html_safe % { link_start: link_start, link_end: link_end.html_safe }
       = form.text_area :key, class: 'form-control gl-form-input thin_area', rows: 5, data: { qa_selector: 'deploy_key_field' }
   - else
-    = form.label :fingerprint, class: 'col-form-label col-sm-2'
-    .col-sm-10
-      = form.text_field :fingerprint, class: 'form-control gl-form-input', readonly: 'readonly'
+    - if deploy_key.fingerprint_sha256.present?
+      = form.label :fingerprint, _('Fingerprint (SHA256)'), class: 'col-form-label col-sm-2'
+      .col-sm-10
+        = form.text_field :fingerprint_sha256, class: 'form-control gl-form-input', readonly: 'readonly'
+    - if deploy_key.fingerprint.present?
+      = form.label :fingerprint, _('Fingerprint (MD5)'), class: 'col-form-label col-sm-2'
+      .col-sm-10
+        = form.text_field :fingerprint, class: 'form-control gl-form-input', readonly: 'readonly'
 
 - if deploy_keys_project.present?
   = form.fields_for :deploy_keys_projects, deploy_keys_project do |deploy_keys_project_form|
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 4043a18da865d8..b86dd9bb42f4c3 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -16365,7 +16365,10 @@ msgstr ""
 msgid "Find file"
 msgstr ""
 
-msgid "Fingerprint"
+msgid "Fingerprint (MD5)"
+msgstr ""
+
+msgid "Fingerprint (SHA256)"
 msgstr ""
 
 msgid "Fingerprints"
diff --git a/spec/features/projects/deploy_keys_spec.rb b/spec/features/projects/deploy_keys_spec.rb
index bf705cf875bff4..06462263f5a17e 100644
--- a/spec/features/projects/deploy_keys_spec.rb
+++ b/spec/features/projects/deploy_keys_spec.rb
@@ -3,22 +3,38 @@
 require 'spec_helper'
 
 RSpec.describe 'Project deploy keys', :js do
-  let(:user) { create(:user) }
-  let(:project) { create(:project_empty_repo) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project_empty_repo) }
+  let_it_be(:deploy_keys_project) { create(:deploy_keys_project, project: project) }
+  let_it_be(:deploy_key) { deploy_keys_project.deploy_key }
 
   before do
     project.add_maintainer(user)
     sign_in(user)
   end
 
+  context 'editing key' do
+    it 'shows fingerprints' do
+      visit edit_project_deploy_key_path(project, deploy_key)
+
+      expect(page).to have_content('Fingerprint (SHA256)')
+      expect(find('#deploy_key_fingerprint_sha256').value).to eq(deploy_key.fingerprint_sha256)
+
+      if Gitlab::FIPS.enabled?
+        expect(page).not_to have_content('Fingerprint (MD5)')
+      else
+        expect(page).to have_content('Fingerprint (MD5)')
+        expect(find('#deploy_key_fingerprint').value).to eq(deploy_key.fingerprint)
+      end
+    end
+  end
+
   describe 'removing key' do
     before do
-      create(:deploy_keys_project, project: project)
+      visit project_settings_repository_path(project)
     end
 
     it 'removes association between project and deploy key' do
-      visit project_settings_repository_path(project)
-
       page.within(find('.rspec-deploy-keys-settings')) do
         expect(page).to have_selector('.deploy-key', count: 1)
 
diff --git a/spec/frontend/admin/deploy_keys/components/table_spec.js b/spec/frontend/admin/deploy_keys/components/table_spec.js
index 49bda7100fb544..8976b4c20cac81 100644
--- a/spec/frontend/admin/deploy_keys/components/table_spec.js
+++ b/spec/frontend/admin/deploy_keys/components/table_spec.js
@@ -57,9 +57,15 @@ describe('DeployKeysTable', () => {
     const timeAgoTooltip = findTimeAgoTooltip(expectedRowIndex);
 
     expect(wrapper.findByText(expectedDeployKey.title).exists()).toBe(true);
-    expect(wrapper.findByText(expectedDeployKey.fingerprint, { selector: 'code' }).exists()).toBe(
-      true,
-    );
+    // In FIPS mode, the MD5 fingerprint is not available
+    if (expectedDeployKey.fingerprint) {
+      expect(wrapper.findByText(expectedDeployKey.fingerprint, { selector: 'span' }).exists()).toBe(
+        true,
+      );
+    }
+    expect(
+      wrapper.findByText(expectedDeployKey.fingerprint_sha256, { selector: 'span' }).exists(),
+    ).toBe(true);
     expect(timeAgoTooltip.exists()).toBe(true);
     expect(timeAgoTooltip.props('time')).toBe(expectedDeployKey.created_at);
     expect(editButton.exists()).toBe(true);
diff --git a/spec/frontend/projects/settings/components/new_access_dropdown_spec.js b/spec/frontend/projects/settings/components/new_access_dropdown_spec.js
index a42891423cde35..1db48ce05d7aca 100644
--- a/spec/frontend/projects/settings/components/new_access_dropdown_spec.js
+++ b/spec/frontend/projects/settings/components/new_access_dropdown_spec.js
@@ -29,9 +29,20 @@ jest.mock('~/projects/settings/api/access_dropdown_api', () => ({
   }),
   getDeployKeys: jest.fn().mockResolvedValue({
     data: [
-      { id: 10, title: 'key10', fingerprint: 'abcdefghijklmnop', owner: { name: 'user1' } },
-      { id: 11, title: 'key11', fingerprint: 'abcdefghijklmnop', owner: { name: 'user2' } },
-      { id: 12, title: 'key12', fingerprint: 'abcdefghijklmnop', owner: { name: 'user3' } },
+      {
+        id: 10,
+        title: 'key10',
+        fingerprint: 'md5-abcdefghijklmnop',
+        fingerprint_sha256: 'sha256-abcdefghijklmnop',
+        owner: { name: 'user1' },
+      },
+      {
+        id: 11,
+        title: 'key11',
+        fingerprint_sha256: 'sha256-abcdefghijklmnop',
+        owner: { name: 'user2' },
+      },
+      { id: 12, title: 'key12', fingerprint: 'md5-abcdefghijklmnop', owner: { name: 'user3' } },
     ],
   }),
 }));
@@ -279,6 +290,7 @@ describe('Access Level Dropdown', () => {
       { id: 115, type: 'group', group_id: 5 },
       { id: 118, type: 'user', user_id: 8, name: 'user2' },
       { id: 121, type: 'deploy_key', deploy_key_id: 11 },
+      { id: 122, type: 'deploy_key', deploy_key_id: 12 },
     ];
 
     const findSelected = (type) =>
@@ -309,8 +321,9 @@ describe('Access Level Dropdown', () => {
 
     it('should set selected deploy keys as intersection between the server response and preselected mapping some keys', () => {
       const selectedDeployKeys = findSelected(LEVEL_TYPES.DEPLOY_KEY);
-      expect(selectedDeployKeys).toHaveLength(1);
-      expect(selectedDeployKeys.at(0).text()).toContain('key11 (abcdefghijklmn...)');
+      expect(selectedDeployKeys).toHaveLength(2);
+      expect(selectedDeployKeys.at(0).text()).toContain('key11 (sha256-abcdefg...)');
+      expect(selectedDeployKeys.at(1).text()).toContain('key12 (md5-abcdefghij...)');
     });
   });
 
-- 
GitLab


From 8d52c45bb376d728ef2c57c9f86612084e17c290 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Fri, 8 Jul 2022 22:24:53 -0700
Subject: [PATCH 2/3] Improve deploy key frontend test

Use a real key without MD5 fingerprint and test against that.
---
 spec/factories/keys.rb                        |  6 ++++
 .../deploy_keys/components/table_spec.js      | 29 ++++++++++++-------
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/spec/factories/keys.rb b/spec/factories/keys.rb
index ccf835fe56e9c1..f6f06a994947b0 100644
--- a/spec/factories/keys.rb
+++ b/spec/factories/keys.rb
@@ -20,6 +20,12 @@
       expires_at { Date.today.beginning_of_day + 3.hours }
     end
 
+    trait :without_md5_fingerprint do
+      after(:create) do |key|
+        key.update_column(:fingerprint, nil)
+      end
+    end
+
     factory :key_without_comment do
       key { SSHData::PrivateKey::RSA.generate(3072, unsafe_allow_small_key: true).public_key.openssh }
     end
diff --git a/spec/frontend/admin/deploy_keys/components/table_spec.js b/spec/frontend/admin/deploy_keys/components/table_spec.js
index 8976b4c20cac81..a18506c0916f9c 100644
--- a/spec/frontend/admin/deploy_keys/components/table_spec.js
+++ b/spec/frontend/admin/deploy_keys/components/table_spec.js
@@ -27,6 +27,7 @@ describe('DeployKeysTable', () => {
 
   const deployKey = responseBody[0];
   const deployKey2 = responseBody[1];
+  const deployKeyWithoutMd5Fingerprint = responseBody[2];
 
   const createComponent = (provide = {}) => {
     wrapper = mountExtended(DeployKeysTable, {
@@ -57,12 +58,7 @@ describe('DeployKeysTable', () => {
     const timeAgoTooltip = findTimeAgoTooltip(expectedRowIndex);
 
     expect(wrapper.findByText(expectedDeployKey.title).exists()).toBe(true);
-    // In FIPS mode, the MD5 fingerprint is not available
-    if (expectedDeployKey.fingerprint) {
-      expect(wrapper.findByText(expectedDeployKey.fingerprint, { selector: 'span' }).exists()).toBe(
-        true,
-      );
-    }
+
     expect(
       wrapper.findByText(expectedDeployKey.fingerprint_sha256, { selector: 'span' }).exists(),
     ).toBe(true);
@@ -73,6 +69,13 @@ describe('DeployKeysTable', () => {
     expect(findRemoveButton(expectedRowIndex).exists()).toBe(true);
   };
 
+  const expectDeployKeyWithFingerprintIsRendered = (expectedDeployKey, expectedRowIndex) => {
+    expect(wrapper.findByText(expectedDeployKey.fingerprint, { selector: 'span' }).exists()).toBe(
+      true,
+    );
+    expectDeployKeyIsRendered(expectedDeployKey, expectedRowIndex);
+  };
+
   const itRendersTheEmptyState = () => {
     it('renders empty state', () => {
       const emptyState = wrapper.findComponent(GlEmptyState);
@@ -133,8 +136,12 @@ describe('DeployKeysTable', () => {
       });
 
       it('renders deploy keys in table', () => {
-        expectDeployKeyIsRendered(deployKey, 0);
-        expectDeployKeyIsRendered(deployKey2, 1);
+        expectDeployKeyWithFingerprintIsRendered(deployKey, 0);
+        expectDeployKeyWithFingerprintIsRendered(deployKey2, 1);
+      });
+
+      it('renders deploy keys that do not have an MD5 fingerprint', () => {
+        expectDeployKeyIsRendered(deployKeyWithoutMd5Fingerprint, 2);
       });
 
       describe('when delete button is clicked', () => {
@@ -163,7 +170,7 @@ describe('DeployKeysTable', () => {
       beforeEach(() => {
         Api.deployKeys.mockResolvedValueOnce({
           data: [deployKey],
-          headers: { 'x-total': '2' },
+          headers: { 'x-total': '3' },
         });
 
         createComponent();
@@ -185,7 +192,7 @@ describe('DeployKeysTable', () => {
       describe('when pagination is changed', () => {
         it('calls API with `page` parameter', async () => {
           const pagination = findPagination();
-          expectDeployKeyIsRendered(deployKey, 0);
+          expectDeployKeyWithFingerprintIsRendered(deployKey, 0);
 
           Api.deployKeys.mockResolvedValue({
             data: [deployKey2],
@@ -205,7 +212,7 @@ describe('DeployKeysTable', () => {
             page: 2,
             public: true,
           });
-          expectDeployKeyIsRendered(deployKey2, 0);
+          expectDeployKeyWithFingerprintIsRendered(deployKey2, 0);
         });
       });
     });
-- 
GitLab


From 7c3f7b70db3333309703a8706b48a3268d666f7c Mon Sep 17 00:00:00 2001
From: Ezekiel Kigbo <ekigbo@gitlab.com>
Date: Sat, 9 Jul 2022 16:33:05 +1000
Subject: [PATCH 3/3] Add deploy key without fingerprint to frontend specs

---
 spec/frontend/fixtures/api_deploy_keys.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/spec/frontend/fixtures/api_deploy_keys.rb b/spec/frontend/fixtures/api_deploy_keys.rb
index 7027b8c975bea9..5ffc726f086f96 100644
--- a/spec/frontend/fixtures/api_deploy_keys.rb
+++ b/spec/frontend/fixtures/api_deploy_keys.rb
@@ -11,6 +11,7 @@
   let_it_be(:project2) { create(:project) }
   let_it_be(:deploy_key) { create(:deploy_key, public: true) }
   let_it_be(:deploy_key2) { create(:deploy_key, public: true) }
+  let_it_be(:deploy_key_without_fingerprint) { create(:deploy_key, :without_md5_fingerprint, public: true) }
   let_it_be(:deploy_keys_project) { create(:deploy_keys_project, :write_access, project: project, deploy_key: deploy_key) }
   let_it_be(:deploy_keys_project2) { create(:deploy_keys_project, :write_access, project: project2, deploy_key: deploy_key) }
   let_it_be(:deploy_keys_project3) { create(:deploy_keys_project, :write_access, project: project, deploy_key: deploy_key2) }
-- 
GitLab