From 6119d5ad7518f547af216d3bdc3d1fcffcfc9c71 Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Fri, 21 Jun 2019 12:06:12 +0000 Subject: Don't show private keys for letsencrypt certs Adds enum certificate_source to pages_domains table with default manually_uploaded Mark certificates as 'gitlab_provided' if the were obtained through Let's Encrypt Mark certificates as 'user_provided' if they were uploaded through controller or api Only show private key in domain edit form if it is 'user_provided' Only show LetsEncrypt option if is enabled by application settings (and feature flag) Refactor and fix some specs to match new logic Don't show Let's Encrypt certificates as well --- .../projects/pages_domains_controller_spec.rb | 64 +++++---- spec/factories/pages_domains.rb | 4 + spec/features/projects/pages_lets_encrypt_spec.rb | 153 ++++++++++----------- spec/features/projects/pages_spec.rb | 16 +-- spec/models/pages_domain_spec.rb | 96 +++++++++++++ spec/requests/api/pages_domains_spec.rb | 8 ++ ...obtain_lets_encrypt_certificate_service_spec.rb | 6 + 7 files changed, 234 insertions(+), 113 deletions(-) (limited to 'spec') diff --git a/spec/controllers/projects/pages_domains_controller_spec.rb b/spec/controllers/projects/pages_domains_controller_spec.rb index ff3afd51cd8..032f4f1418f 100644 --- a/spec/controllers/projects/pages_domains_controller_spec.rb +++ b/spec/controllers/projects/pages_domains_controller_spec.rb @@ -15,7 +15,10 @@ describe Projects::PagesDomainsController do end let(:pages_domain_params) do - build(:pages_domain, domain: 'my.otherdomain.com').slice(:key, :certificate, :domain) + attributes_for(:pages_domain, domain: 'my.otherdomain.com').slice(:key, :certificate, :domain).tap do |params| + params[:user_provided_key] = params.delete(:key) + params[:user_provided_certificate] = params.delete(:certificate) + end end before do @@ -84,48 +87,59 @@ describe Projects::PagesDomainsController do controller.instance_variable_set(:@domain, pages_domain) end - let(:pages_domain_params) do - attributes_for(:pages_domain).slice(:key, :certificate) - end - let(:params) do request_params.merge(id: pages_domain.domain, pages_domain: pages_domain_params) end - it 'updates the domain' do - expect(pages_domain) - .to receive(:update) - .with(ActionController::Parameters.new(pages_domain_params).permit!) - .and_return(true) + context 'with valid params' do + let(:pages_domain_params) do + attributes_for(:pages_domain, :with_trusted_chain).slice(:key, :certificate).tap do |params| + params[:user_provided_key] = params.delete(:key) + params[:user_provided_certificate] = params.delete(:certificate) + end + end + + it 'updates the domain' do + expect do + patch(:update, params: params) + end.to change { pages_domain.reload.certificate }.to(pages_domain_params[:user_provided_certificate]) + end + + it 'redirects to the project page' do + patch(:update, params: params) - patch(:update, params: params) + expect(flash[:notice]).to eq 'Domain was updated' + expect(response).to redirect_to(project_pages_path(project)) + end end - it 'redirects to the project page' do - patch(:update, params: params) + context 'with key parameter' do + before do + pages_domain.update!(key: nil, certificate: nil, certificate_source: 'gitlab_provided') + end - expect(flash[:notice]).to eq 'Domain was updated' - expect(response).to redirect_to(project_pages_path(project)) + it 'marks certificate as provided by user' do + expect do + patch(:update, params: params) + end.to change { pages_domain.reload.certificate_source }.from('gitlab_provided').to('user_provided') + end end context 'the domain is invalid' do - it 'renders the edit action' do - allow(pages_domain).to receive(:update).and_return(false) + let(:pages_domain_params) { { user_provided_certificate: 'blabla' } } + it 'renders the edit action' do patch(:update, params: params) expect(response).to render_template('edit') end end - context 'the parameters include the domain' do - it 'renders 400 Bad Request' do - expect(pages_domain) - .to receive(:update) - .with(hash_not_including(:domain)) - .and_return(true) - - patch(:update, params: params.deep_merge(pages_domain: { domain: 'abc' })) + context 'when parameters include the domain' do + it 'does not update domain' do + expect do + patch(:update, params: params.deep_merge(pages_domain: { domain: 'abc' })) + end.not_to change { pages_domain.reload.domain } end end end diff --git a/spec/factories/pages_domains.rb b/spec/factories/pages_domains.rb index db8384877b0..8da19a37a6a 100644 --- a/spec/factories/pages_domains.rb +++ b/spec/factories/pages_domains.rb @@ -180,5 +180,9 @@ Iy6oRpHaCF/2obZdIdgf9rlyz0fkqyHJc9GkioSoOhJZxEV2SgAkap8yS0sX2tJ9 ZDXgrA== -----END CERTIFICATE-----' end + + trait :letsencrypt do + certificate_source { :gitlab_provided } + end end end diff --git a/spec/features/projects/pages_lets_encrypt_spec.rb b/spec/features/projects/pages_lets_encrypt_spec.rb index baa217cbe58..a5f8702302c 100644 --- a/spec/features/projects/pages_lets_encrypt_spec.rb +++ b/spec/features/projects/pages_lets_encrypt_spec.rb @@ -2,124 +2,119 @@ require 'spec_helper' describe "Pages with Let's Encrypt", :https_pages_enabled do + include LetsEncryptHelpers + let(:project) { create(:project) } let(:user) { create(:user) } let(:role) { :maintainer } - let(:certificate_pem) do - <<~PEM - -----BEGIN CERTIFICATE----- - MIICGzCCAYSgAwIBAgIBATANBgkqhkiG9w0BAQUFADAbMRkwFwYDVQQDExB0ZXN0 - LWNlcnRpZmljYXRlMB4XDTE2MDIxMjE0MzIwMFoXDTIwMDQxMjE0MzIwMFowGzEZ - MBcGA1UEAxMQdGVzdC1jZXJ0aWZpY2F0ZTCBnzANBgkqhkiG9w0BAQEFAAOBjQAw - gYkCgYEApL4J9L0ZxFJ1hI1LPIflAlAGvm6ZEvoT4qKU5Xf2JgU7/2geNR1qlNFa - SvCc08Knupp5yTgmvyK/Xi09U0N82vvp4Zvr/diSc4A/RA6Mta6egLySNT438kdT - nY2tR5feoTLwQpX0t4IMlwGQGT5h6Of2fKmDxzuwuyffcIHqLdsCAwEAAaNvMG0w - DAYDVR0TAQH/BAIwADAdBgNVHQ4EFgQUxl9WSxBprB0z0ibJs3rXEk0+95AwCwYD - VR0PBAQDAgXgMBEGCWCGSAGG+EIBAQQEAwIGQDAeBglghkgBhvhCAQ0EERYPeGNh - IGNlcnRpZmljYXRlMA0GCSqGSIb3DQEBBQUAA4GBAGC4T8SlFHK0yPSa+idGLQFQ - joZp2JHYvNlTPkRJ/J4TcXxBTJmArcQgTIuNoBtC+0A/SwdK4MfTCUY4vNWNdese - 5A4K65Nb7Oh1AdQieTBHNXXCdyFsva9/ScfQGEl7p55a52jOPs0StPd7g64uvjlg - YHi2yesCrOvVXt+lgPTd - -----END CERTIFICATE----- - PEM - end + let(:certificate_pem) { attributes_for(:pages_domain)[:certificate] } - let(:certificate_key) do - <<~KEY - -----BEGIN PRIVATE KEY----- - MIICdgIBADANBgkqhkiG9w0BAQEFAASCAmAwggJcAgEAAoGBAKS+CfS9GcRSdYSN - SzyH5QJQBr5umRL6E+KilOV39iYFO/9oHjUdapTRWkrwnNPCp7qaeck4Jr8iv14t - PVNDfNr76eGb6/3YknOAP0QOjLWunoC8kjU+N/JHU52NrUeX3qEy8EKV9LeCDJcB - kBk+Yejn9nypg8c7sLsn33CB6i3bAgMBAAECgYA2D26w80T7WZvazYr86BNMePpd - j2mIAqx32KZHzt/lhh40J/SRtX9+Kl0Y7nBoRR5Ja9u/HkAIxNxLiUjwg9r6cpg/ - uITEF5nMt7lAk391BuI+7VOZZGbJDsq2ulPd6lO+C8Kq/PI/e4kXcIjeH6KwQsuR - 5vrXfBZ3sQfflaiN4QJBANBt8JY2LIGQF8o89qwUpRL5vbnKQ4IzZ5+TOl4RLR7O - AQpJ81tGuINghO7aunctb6rrcKJrxmEH1whzComybrMCQQDKV49nOBudRBAIgG4K - EnLzsRKISUHMZSJiYTYnablof8cKw1JaQduw7zgrUlLwnroSaAGX88+Jw1f5n2Lh - Vlg5AkBDdUGnrDLtYBCDEQYZHblrkc7ZAeCllDOWjxUV+uMqlCv8A4Ey6omvY57C - m6I8DkWVAQx8VPtozhvHjUw80rZHAkB55HWHAM3h13axKG0htCt7klhPsZHpx6MH - EPjGlXIT+aW2XiPmK3ZlCDcWIenE+lmtbOpI159Wpk8BGXs/s/xBAkEAlAY3ymgx - 63BDJEwvOb2IaP8lDDxNsXx9XJNVvQbv5n15vNsLHbjslHfAhAbxnLQ1fLhUPqSi - nNp/xedE1YxutQ== - -----END PRIVATE KEY----- - KEY - end + let(:certificate_key) { attributes_for(:pages_domain)[:key] } before do allow(Gitlab.config.pages).to receive(:enabled).and_return(true) + stub_lets_encrypt_settings + project.add_role(user, role) sign_in(user) project.namespace.update(owner: user) allow_any_instance_of(Project).to receive(:pages_deployed?) { true } end - context 'when the page_auto_ssl feature flag is enabled' do - before do - stub_feature_flags(pages_auto_ssl: true) + context 'when the auto SSL management is initially disabled' do + let(:domain) do + create(:pages_domain, auto_ssl_enabled: false, project: project) end - context 'when the auto SSL management is initially disabled' do - let(:domain) do - create(:pages_domain, auto_ssl_enabled: false, project: project) - end + it 'enables auto SSL and dynamically updates the form accordingly', :js do + visit edit_project_pages_domain_path(project, domain) - it 'enables auto SSL and dynamically updates the form accordingly', :js do - visit edit_project_pages_domain_path(project, domain) + expect(domain.auto_ssl_enabled).to eq false - expect(domain.auto_ssl_enabled).to eq false + expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'false' + expect(page).to have_field 'Certificate (PEM)', type: 'textarea' + expect(page).to have_field 'Key (PEM)', type: 'textarea' - expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'false' - expect(page).to have_field 'Certificate (PEM)', type: 'textarea' - expect(page).to have_field 'Key (PEM)', type: 'textarea' + find('.js-auto-ssl-toggle-container .project-feature-toggle').click - find('.js-auto-ssl-toggle-container .project-feature-toggle').click + expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'true' + expect(page).not_to have_field 'Certificate (PEM)', type: 'textarea' + expect(page).not_to have_field 'Key (PEM)', type: 'textarea' - expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'true' - expect(page).not_to have_field 'Certificate (PEM)', type: 'textarea' - expect(page).not_to have_field 'Key (PEM)', type: 'textarea' - expect(page).to have_content "The certificate will be shown here once it has been obtained from Let's Encrypt. This process may take up to an hour to complete." + click_on 'Save Changes' - click_on 'Save Changes' + expect(domain.reload.auto_ssl_enabled).to eq true + end + end - expect(domain.reload.auto_ssl_enabled).to eq true - end + context 'when the auto SSL management is initially enabled' do + let(:domain) do + create(:pages_domain, :letsencrypt, auto_ssl_enabled: true, project: project) end - context 'when the auto SSL management is initially enabled' do - let(:domain) do - create(:pages_domain, auto_ssl_enabled: true, project: project) - end + it 'disables auto SSL and dynamically updates the form accordingly', :js do + visit edit_project_pages_domain_path(project, domain) - it 'disables auto SSL and dynamically updates the form accordingly', :js do - visit edit_project_pages_domain_path(project, domain) + expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'true' + expect(page).not_to have_field 'Certificate (PEM)', type: 'textarea' + expect(page).not_to have_field 'Key (PEM)', type: 'textarea' - expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'true' - expect(page).to have_field 'Certificate (PEM)', type: 'textarea', disabled: true - expect(page).not_to have_field 'Key (PEM)', type: 'textarea' + find('.js-auto-ssl-toggle-container .project-feature-toggle').click - find('.js-auto-ssl-toggle-container .project-feature-toggle').click + expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'false' + expect(page).to have_field 'Certificate (PEM)', type: 'textarea' + expect(page).to have_field 'Key (PEM)', type: 'textarea' - expect(find("#pages_domain_auto_ssl_enabled", visible: false).value).to eq 'false' - expect(page).to have_field 'Certificate (PEM)', type: 'textarea' - expect(page).to have_field 'Key (PEM)', type: 'textarea' + fill_in 'Certificate (PEM)', with: certificate_pem + fill_in 'Key (PEM)', with: certificate_key - fill_in 'Certificate (PEM)', with: certificate_pem - fill_in 'Key (PEM)', with: certificate_key + click_on 'Save Changes' - click_on 'Save Changes' + expect(domain.reload.auto_ssl_enabled).to eq false + end + end - expect(domain.reload.auto_ssl_enabled).to eq false + shared_examples 'user sees private keys only for user provided certificate' do + before do + visit edit_project_pages_domain_path(project, domain) + end + + shared_examples 'user do not see private key' do + it 'user do not see private key' do + expect(find_field('Key (PEM)', visible: :all, disabled: :all).value).to be_blank + end + end + + context 'when auto_ssl is enabled for domain' do + let(:domain) { create(:pages_domain, :letsencrypt, project: project, auto_ssl_enabled: true) } + + include_examples 'user do not see private key' + end + + context 'when auto_ssl is disabled for domain' do + let(:domain) { create(:pages_domain, :letsencrypt, project: project) } + + include_examples 'user do not see private key' + end + + context 'when certificate is provided by user' do + let(:domain) { create(:pages_domain, project: project) } + + it 'user sees private key' do + expect(find_field('Key (PEM)').value).not_to be_blank end end end - context 'when the page_auto_ssl feature flag is disabled' do + include_examples 'user sees private keys only for user provided certificate' + + context 'when letsencrypt is disabled' do let(:domain) do create(:pages_domain, auto_ssl_enabled: false, project: project) end before do - stub_feature_flags(pages_auto_ssl: false) + stub_application_setting(lets_encrypt_terms_of_service_accepted: false) visit edit_project_pages_domain_path(project, domain) end @@ -127,5 +122,7 @@ describe "Pages with Let's Encrypt", :https_pages_enabled do it "does not render the Let's Encrypt field", :js do expect(page).not_to have_selector '.js-auto-ssl-toggle-container' end + + include_examples 'user sees private keys only for user provided certificate' end end diff --git a/spec/features/projects/pages_spec.rb b/spec/features/projects/pages_spec.rb index 9bb0ba81ef5..c4b3ddb2088 100644 --- a/spec/features/projects/pages_spec.rb +++ b/spec/features/projects/pages_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -shared_examples 'pages domain editing' do +shared_examples 'pages settings editing' do let(:project) { create(:project) } let(:user) { create(:user) } let(:role) { :maintainer } @@ -321,19 +321,15 @@ shared_examples 'pages domain editing' do end describe 'Pages' do - context 'when pages_auto_ssl feature flag is disabled' do - before do - stub_feature_flags(pages_auto_ssl: false) - end + include LetsEncryptHelpers - include_examples 'pages domain editing' - end + include_examples 'pages settings editing' - context 'when pages_auto_ssl feature flag is enabled' do + context 'when letsencrypt support is enabled' do before do - stub_feature_flags(pages_auto_ssl: true) + stub_lets_encrypt_settings end - include_examples 'pages domain editing' + include_examples 'pages settings editing' end end diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb index fdc81359d34..4fb7b71a3c7 100644 --- a/spec/models/pages_domain_spec.rb +++ b/spec/models/pages_domain_spec.rb @@ -356,6 +356,102 @@ describe PagesDomain do end end + describe '#user_provided_key' do + subject { domain.user_provided_key } + + context 'when certificate is provided by user' do + let(:domain) { create(:pages_domain) } + + it 'returns key' do + is_expected.to eq(domain.key) + end + end + + context 'when certificate is provided by gitlab' do + let(:domain) { create(:pages_domain, :letsencrypt) } + + it 'returns nil' do + is_expected.to be_nil + end + end + end + + describe '#user_provided_certificate' do + subject { domain.user_provided_certificate } + + context 'when certificate is provided by user' do + let(:domain) { create(:pages_domain) } + + it 'returns key' do + is_expected.to eq(domain.certificate) + end + end + + context 'when certificate is provided by gitlab' do + let(:domain) { create(:pages_domain, :letsencrypt) } + + it 'returns nil' do + is_expected.to be_nil + end + end + end + + shared_examples 'certificate setter' do |attribute, setter_name, old_certificate_source, new_certificate_source| + let(:domain) do + create(:pages_domain, certificate_source: old_certificate_source) + end + + let(:old_value) { domain.public_send(attribute) } + + subject { domain.public_send(setter_name, new_value) } + + context 'when value has been changed' do + let(:new_value) { 'new_value' } + + it "assignes new value to #{attribute}" do + expect do + subject + end.to change { domain.public_send(attribute) }.from(old_value).to('new_value') + end + + it 'changes certificate source' do + expect do + subject + end.to change { domain.certificate_source }.from(old_certificate_source).to(new_certificate_source) + end + end + + context 'when value has not been not changed' do + let(:new_value) { old_value } + + it 'does not change certificate source' do + expect do + subject + end.not_to change { domain.certificate_source }.from(old_certificate_source) + end + end + end + + describe '#user_provided_key=' do + include_examples('certificate setter', 'key', 'user_provided_key=', + 'gitlab_provided', 'user_provided') + end + + describe '#gitlab_provided_key=' do + include_examples('certificate setter', 'key', 'gitlab_provided_key=', + 'user_provided', 'gitlab_provided') + end + + describe '#user_provided_certificate=' do + include_examples('certificate setter', 'certificate', 'user_provided_certificate=', + 'gitlab_provided', 'user_provided') + end + + describe '#gitlab_provided_certificate=' do + include_examples('certificate setter', 'certificate', 'gitlab_provided_certificate=', + 'user_provided', 'gitlab_provided') + end + describe '.for_removal' do subject { described_class.for_removal } diff --git a/spec/requests/api/pages_domains_spec.rb b/spec/requests/api/pages_domains_spec.rb index 3eb68a6abb6..449032b95b7 100644 --- a/spec/requests/api/pages_domains_spec.rb +++ b/spec/requests/api/pages_domains_spec.rb @@ -359,6 +359,14 @@ describe API::PagesDomains do expect(pages_domain_secure.certificate).to eq(params_secure_nokey[:certificate]) end + it 'updates certificate source to user_provided if is changed' do + pages_domain.update!(certificate_source: 'gitlab_provided') + + expect do + put api(route_domain, user), params: params_secure + end.to change { pages_domain.reload.certificate_source }.from('gitlab_provided').to('user_provided') + end + it 'fails to update pages domain adding certificate without key' do put api(route_domain, user), params: params_secure_nokey diff --git a/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb b/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb index 6d7be27939c..d5f77f3354b 100644 --- a/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb +++ b/spec/services/pages_domains/obtain_lets_encrypt_certificate_service_spec.rb @@ -137,6 +137,12 @@ describe PagesDomains::ObtainLetsEncryptCertificateService do expect(pages_domain.certificate).to eq(certificate) end + it 'marks certificate as gitlab_provided' do + service.execute + + expect(pages_domain.certificate_source).to eq("gitlab_provided") + end + it 'removes order from database' do service.execute -- cgit v1.2.1