diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-03-16 20:20:34 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-03-16 20:20:34 +0300 |
commit | e6e7d940173750d26b5625bbaef2437c46374574 (patch) | |
tree | ed87770d8a0a00372eef1df16a3a105624c746ba | |
parent | de3b459dcb72572c4d44eb83240f2725ca04b170 (diff) |
Add latest changes from gitlab-org/gitlab@12-8-stable-ee
-rw-r--r-- | app/helpers/x509_helper.rb | 19 | ||||
-rw-r--r-- | app/views/projects/commit/x509/_certificate_details.html.haml | 10 | ||||
-rw-r--r-- | changelogs/unreleased/fix-x509-signed-commit.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/middleware/multipart.rb | 18 | ||||
-rw-r--r-- | lib/gitlab/x509/commit.rb | 17 | ||||
-rw-r--r-- | spec/helpers/x509_helper_spec.rb | 60 | ||||
-rw-r--r-- | spec/lib/gitlab/middleware/multipart_spec.rb | 42 | ||||
-rw-r--r-- | spec/lib/gitlab/x509/commit_spec.rb | 33 | ||||
-rw-r--r-- | spec/models/pages_domain_spec.rb | 5 | ||||
-rw-r--r-- | spec/support/shared_contexts/lib/gitlab/middleware/multipart_shared_contexts.rb | 42 | ||||
-rw-r--r-- | spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb | 5 |
11 files changed, 204 insertions, 52 deletions
diff --git a/app/helpers/x509_helper.rb b/app/helpers/x509_helper.rb new file mode 100644 index 00000000000..c330b599d74 --- /dev/null +++ b/app/helpers/x509_helper.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'net/ldap/dn' + +module X509Helper + def x509_subject(subject, search_keys) + subjects = {} + + Net::LDAP::DN.new(subject).each_pair do |key, value| + if key.upcase.start_with?(*search_keys.map(&:upcase)) + subjects[key.upcase] = value + end + end + + subjects + rescue + {} + end +end diff --git a/app/views/projects/commit/x509/_certificate_details.html.haml b/app/views/projects/commit/x509/_certificate_details.html.haml index 2357c6d803b..51667010d6f 100644 --- a/app/views/projects/commit/x509/_certificate_details.html.haml +++ b/app/views/projects/commit/x509/_certificate_details.html.haml @@ -1,17 +1,15 @@ .gpg-popover-certificate-details %strong= _('Certificate Subject') %ul - - signature.x509_certificate.subject.split(",").each do |i| - - if i.start_with?("CN", "O") - %li= i + - x509_subject(signature.x509_certificate.subject, ["CN", "O"]).map do |key, value| + %li= key + "=" + value %li= _('Subject Key Identifier:') %li.unstyled= signature.x509_certificate.subject_key_identifier.gsub(":", " ") .gpg-popover-certificate-details %strong= _('Certificate Issuer') %ul - - signature.x509_certificate.x509_issuer.subject.split(",").each do |i| - - if i.start_with?("CN", "OU", "O") - %li= i + - x509_subject(signature.x509_certificate.x509_issuer.subject, ["CN", "OU", "O"]).map do |key, value| + %li= key + "=" + value %li= _('Subject Key Identifier:') %li.unstyled= signature.x509_certificate.x509_issuer.subject_key_identifier.gsub(":", " ") diff --git a/changelogs/unreleased/fix-x509-signed-commit.yml b/changelogs/unreleased/fix-x509-signed-commit.yml new file mode 100644 index 00000000000..d3d0f70ce15 --- /dev/null +++ b/changelogs/unreleased/fix-x509-signed-commit.yml @@ -0,0 +1,5 @@ +--- +title: Fix crl_url parsing and certificate visualization +merge_request: 25876 +author: Roger Meier +type: fixed diff --git a/lib/gitlab/middleware/multipart.rb b/lib/gitlab/middleware/multipart.rb index 0ee9563c227..cb4213d23a4 100644 --- a/lib/gitlab/middleware/multipart.rb +++ b/lib/gitlab/middleware/multipart.rb @@ -84,12 +84,6 @@ module Gitlab end def open_file(params, key) - allowed_paths = [ - ::FileUploader.root, - Gitlab.config.uploads.storage_path, - File.join(Rails.root, 'public/uploads/tmp') - ] - ::UploadedFile.from_params(params, key, allowed_paths) end @@ -106,6 +100,16 @@ module Gitlab # inside other env keys, here we ensure everything is updated correctly ActionDispatch::Request.new(@request.env).update_param(key, value) end + + private + + def allowed_paths + [ + ::FileUploader.root, + Gitlab.config.uploads.storage_path, + File.join(Rails.root, 'public/uploads/tmp') + ] + end end def initialize(app) @@ -125,3 +129,5 @@ module Gitlab end end end + +::Gitlab::Middleware::Multipart::Handler.prepend_if_ee('EE::Gitlab::Middleware::Multipart::Handler') diff --git a/lib/gitlab/x509/commit.rb b/lib/gitlab/x509/commit.rb index ce298b80a4c..b1d15047981 100644 --- a/lib/gitlab/x509/commit.rb +++ b/lib/gitlab/x509/commit.rb @@ -105,13 +105,22 @@ module Gitlab def certificate_crl extension = get_certificate_extension('crlDistributionPoints') - extension.split('URI:').each do |item| - item.strip + crl_url = nil - if item.start_with?("http") - return item.strip + extension.each_line do |line| + break if crl_url + + line.split('URI:').each do |item| + item.strip + + if item.start_with?("http") + crl_url = item.strip + break + end end end + + crl_url end def get_certificate_extension(extension) diff --git a/spec/helpers/x509_helper_spec.rb b/spec/helpers/x509_helper_spec.rb new file mode 100644 index 00000000000..dcdf57ce035 --- /dev/null +++ b/spec/helpers/x509_helper_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe X509Helper do + describe '#x509_subject' do + let(:search_uppercase) { %w[CN OU O] } + let(:search_lowercase) { %w[cn ou o] } + let(:certificate_attributes) do + { + 'CN' => 'CA Issuing', + 'OU' => 'Trust Center', + 'O' => 'Example' + } + end + + context 'with uppercase DN' do + let(:upper_dn) { 'CN=CA Issuing,OU=Trust Center,O=Example,L=World,C=Galaxy' } + + it 'returns the attributes on any case search' do + expect(x509_subject(upper_dn, search_lowercase)).to eq(certificate_attributes) + expect(x509_subject(upper_dn, search_uppercase)).to eq(certificate_attributes) + end + end + + context 'with lowercase DN' do + let(:lower_dn) { 'cn=CA Issuing,ou=Trust Center,o=Example,l=World,c=Galaxy' } + + it 'returns the attributes on any case search' do + expect(x509_subject(lower_dn, search_lowercase)).to eq(certificate_attributes) + expect(x509_subject(lower_dn, search_uppercase)).to eq(certificate_attributes) + end + end + + context 'with comma within DN' do + let(:comma_dn) { 'cn=CA\, Issuing,ou=Trust Center,o=Example,l=World,c=Galaxy' } + let(:certificate_attributes) do + { + 'CN' => 'CA, Issuing', + 'OU' => 'Trust Center', + 'O' => 'Example' + } + end + + it 'returns the attributes on any case search' do + expect(x509_subject(comma_dn, search_lowercase)).to eq(certificate_attributes) + expect(x509_subject(comma_dn, search_uppercase)).to eq(certificate_attributes) + end + end + + context 'with mal formed DN' do + let(:bad_dn) { 'cn=CA, Issuing,ou=Trust Center,o=Example,l=World,c=Galaxy' } + + it 'returns nil on any case search' do + expect(x509_subject(bad_dn, search_lowercase)).to eq({}) + expect(x509_subject(bad_dn, search_uppercase)).to eq({}) + end + end + end +end diff --git a/spec/lib/gitlab/middleware/multipart_spec.rb b/spec/lib/gitlab/middleware/multipart_spec.rb index 33797817578..1d3ad3afb56 100644 --- a/spec/lib/gitlab/middleware/multipart_spec.rb +++ b/spec/lib/gitlab/middleware/multipart_spec.rb @@ -5,9 +5,7 @@ require 'spec_helper' require 'tempfile' describe Gitlab::Middleware::Multipart do - let(:app) { double(:app) } - let(:middleware) { described_class.new(app) } - let(:original_filename) { 'filename' } + include_context 'multipart middleware context' shared_examples_for 'multipart upload files' do it 'opens top-level files' do @@ -82,22 +80,12 @@ describe Gitlab::Middleware::Multipart do end it 'allows files in uploads/tmp directory' do - Dir.mktmpdir do |dir| - uploads_dir = File.join(dir, 'public/uploads/tmp') - FileUtils.mkdir_p(uploads_dir) - - allow(Rails).to receive(:root).and_return(dir) - allow(Dir).to receive(:tmpdir).and_return(File.join(Dir.tmpdir, 'tmpsubdir')) - - Tempfile.open('top-level', uploads_dir) do |tempfile| - env = post_env({ 'file' => tempfile.path }, { 'file.name' => original_filename, 'file.path' => tempfile.path }, Gitlab::Workhorse.secret, 'gitlab-workhorse') - - expect(app).to receive(:call) do |env| - expect(get_params(env)['file']).to be_a(::UploadedFile) - end - - middleware.call(env) + with_tmp_dir('public/uploads/tmp') do |dir, env| + expect(app).to receive(:call) do |env| + expect(get_params(env)['file']).to be_a(::UploadedFile) end + + middleware.call(env) end end @@ -127,22 +115,4 @@ describe Gitlab::Middleware::Multipart do middleware.call(env) end end - - # Rails 5 doesn't combine the GET/POST parameters in - # ActionDispatch::HTTP::Parameters if action_dispatch.request.parameters is set: - # https://github.com/rails/rails/blob/aea6423f013ca48f7704c70deadf2cd6ac7d70a1/actionpack/lib/action_dispatch/http/parameters.rb#L41 - def get_params(env) - req = ActionDispatch::Request.new(env) - req.GET.merge(req.POST) - end - - def post_env(rewritten_fields, params, secret, issuer) - token = JWT.encode({ 'iss' => issuer, 'rewritten_fields' => rewritten_fields }, secret, 'HS256') - Rack::MockRequest.env_for( - '/', - method: 'post', - params: params, - described_class::RACK_ENV_KEY => token - ) - end end diff --git a/spec/lib/gitlab/x509/commit_spec.rb b/spec/lib/gitlab/x509/commit_spec.rb index 9cddf27ddce..c31e9e4b8e6 100644 --- a/spec/lib/gitlab/x509/commit_spec.rb +++ b/spec/lib/gitlab/x509/commit_spec.rb @@ -204,5 +204,38 @@ describe Gitlab::X509::Commit do expect(described_class.new(commit).signature).to be_nil end end + + context 'certificate_crl' do + let!(:commit) { create :commit, project: project, sha: commit_sha, created_at: Time.utc(2019, 1, 1, 20, 15, 0), committer_email: X509Helpers::User1.emails.first } + let(:signed_commit) { described_class.new(commit) } + + describe 'valid crlDistributionPoints' do + before do + allow(signed_commit).to receive(:get_certificate_extension).and_call_original + + allow(signed_commit).to receive(:get_certificate_extension) + .with('crlDistributionPoints') + .and_return("\nFull Name:\n URI:http://ch.siemens.com/pki?ZZZZZZA2.crl\n URI:ldap://cl.siemens.net/CN=ZZZZZZA2,L=PKI?certificateRevocationList\n URI:ldap://cl.siemens.com/CN=ZZZZZZA2,o=Trustcenter?certificateRevocationList\n") + end + + it 'returns an unverified signature' do + expect(signed_commit.signature.x509_certificate.x509_issuer).to have_attributes(user1_issuer_attributes) + end + end + + describe 'valid crlDistributionPoints providing multiple http URIs' do + before do + allow(signed_commit).to receive(:get_certificate_extension).and_call_original + + allow(signed_commit).to receive(:get_certificate_extension) + .with('crlDistributionPoints') + .and_return("\nFull Name:\n URI:http://cdp1.pca.dfn.de/dfn-ca-global-g2/pub/crl/cacrl.crl\n\nFull Name:\n URI:http://cdp2.pca.dfn.de/dfn-ca-global-g2/pub/crl/cacrl.crl\n") + end + + it 'extracts the first URI' do + expect(signed_commit.signature.x509_certificate.x509_issuer.crl_url).to eq("http://cdp1.pca.dfn.de/dfn-ca-global-g2/pub/crl/cacrl.crl") + end + end + end end end diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb index d2a54c3eea7..d6f77e581b9 100644 --- a/spec/models/pages_domain_spec.rb +++ b/spec/models/pages_domain_spec.rb @@ -7,6 +7,11 @@ describe PagesDomain do subject(:pages_domain) { described_class.new } + # Locking in date due to cert expiration date https://gitlab.com/gitlab-org/gitlab/-/issues/210557#note_304749257 + around do |example| + Timecop.travel(Time.new(2020, 3, 12)) { example.run } + end + describe 'associations' do it { is_expected.to belong_to(:project) } end diff --git a/spec/support/shared_contexts/lib/gitlab/middleware/multipart_shared_contexts.rb b/spec/support/shared_contexts/lib/gitlab/middleware/multipart_shared_contexts.rb new file mode 100644 index 00000000000..f1554ea8e9f --- /dev/null +++ b/spec/support/shared_contexts/lib/gitlab/middleware/multipart_shared_contexts.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +RSpec.shared_context 'multipart middleware context' do + let(:app) { double(:app) } + let(:middleware) { described_class.new(app) } + let(:original_filename) { 'filename' } + + # Rails 5 doesn't combine the GET/POST parameters in + # ActionDispatch::HTTP::Parameters if action_dispatch.request.parameters is set: + # https://github.com/rails/rails/blob/aea6423f013ca48f7704c70deadf2cd6ac7d70a1/actionpack/lib/action_dispatch/http/parameters.rb#L41 + def get_params(env) + req = ActionDispatch::Request.new(env) + req.GET.merge(req.POST) + end + + def post_env(rewritten_fields, params, secret, issuer) + token = JWT.encode({ 'iss' => issuer, 'rewritten_fields' => rewritten_fields }, secret, 'HS256') + Rack::MockRequest.env_for( + '/', + method: 'post', + params: params, + described_class::RACK_ENV_KEY => token + ) + end + + def with_tmp_dir(uploads_sub_dir, storage_path = '') + Dir.mktmpdir do |dir| + upload_dir = File.join(dir, storage_path, uploads_sub_dir) + FileUtils.mkdir_p(upload_dir) + + allow(Rails).to receive(:root).and_return(dir) + allow(Dir).to receive(:tmpdir).and_return(File.join(Dir.tmpdir, 'tmpsubdir')) + allow(GitlabUploader).to receive(:root).and_return(File.join(dir, storage_path)) + + Tempfile.open('top-level', upload_dir) do |tempfile| + env = post_env({ 'file' => tempfile.path }, { 'file.name' => original_filename, 'file.path' => tempfile.path }, Gitlab::Workhorse.secret, 'gitlab-workhorse') + + yield dir, env + end + end + end +end diff --git a/spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb b/spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb index 736acc40371..ae66122b4de 100644 --- a/spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb +++ b/spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb @@ -7,6 +7,11 @@ describe PagesDomainSslRenewalCronWorker do subject(:worker) { described_class.new } + # Locking in date due to cert expiration date https://gitlab.com/gitlab-org/gitlab/-/issues/210557#note_304749257 + around do |example| + Timecop.travel(Time.new(2020, 3, 12)) { example.run } + end + before do stub_lets_encrypt_settings end |