From 90f933a5a10c56ac7d318a3dfcff89f6b5bc586e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= Date: Tue, 5 May 2020 13:38:21 +0300 Subject: [PATCH 1/6] Add revoked boolean to Certificate model --- db/migrate/20200505103316_add_revoked_to_certificate.rb | 5 +++++ db/structure.sql | 7 +++++-- 2 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20200505103316_add_revoked_to_certificate.rb diff --git a/db/migrate/20200505103316_add_revoked_to_certificate.rb b/db/migrate/20200505103316_add_revoked_to_certificate.rb new file mode 100644 index 000000000..a52c7d14c --- /dev/null +++ b/db/migrate/20200505103316_add_revoked_to_certificate.rb @@ -0,0 +1,5 @@ +class AddRevokedToCertificate < ActiveRecord::Migration[5.2] + def change + add_column :certificates, :revoked, :boolean, null: false, default: false + end +end diff --git a/db/structure.sql b/db/structure.sql index 604238d4c..6ad181ea4 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -476,7 +476,8 @@ CREATE TABLE public.certificates ( updated_at timestamp without time zone, common_name character varying, md5 character varying, - interface character varying + interface character varying, + revoked boolean DEFAULT false NOT NULL ); @@ -4463,5 +4464,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20200130092113'), ('20200203143458'), ('20200204103125'), -('20200311114649'); +('20200311114649'), +('20200505103316'); + From 0fa7fcc771a9d1bf65f1e9273017919a4741a580 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= Date: Tue, 5 May 2020 13:48:53 +0300 Subject: [PATCH 2/6] Check that certificate is not revoked --- app/models/api_user.rb | 6 ++++-- app/models/certificate.rb | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/models/api_user.rb b/app/models/api_user.rb index 3dc240727..99f14ad6c 100644 --- a/app/models/api_user.rb +++ b/app/models/api_user.rb @@ -66,22 +66,24 @@ class ApiUser < User def registrar_pki_ok?(crt, cn) return false if crt.blank? || cn.blank? + crt = crt.split(' ').join("\n") crt.gsub!("-----BEGIN\nCERTIFICATE-----\n", "-----BEGIN CERTIFICATE-----\n") crt.gsub!("\n-----END\nCERTIFICATE-----", "\n-----END CERTIFICATE-----") cert = OpenSSL::X509::Certificate.new(crt) md5 = OpenSSL::Digest::MD5.new(cert.to_der).to_s - certificates.registrar.exists?(md5: md5, common_name: cn) + certificates.registrar.exists?(md5: md5, common_name: cn, revoked: false) end def api_pki_ok?(crt, cn) return false if crt.blank? || cn.blank? + crt = crt.split(' ').join("\n") crt.gsub!("-----BEGIN\nCERTIFICATE-----\n", "-----BEGIN CERTIFICATE-----\n") crt.gsub!("\n-----END\nCERTIFICATE-----", "\n-----END CERTIFICATE-----") cert = OpenSSL::X509::Certificate.new(crt) md5 = OpenSSL::Digest::MD5.new(cert.to_der).to_s - certificates.api.exists?(md5: md5, common_name: cn) + certificates.api.exists?(md5: md5, common_name: cn, revoked: false) end def linked_users diff --git a/app/models/certificate.rb b/app/models/certificate.rb index 5259403c2..f0711d4f5 100644 --- a/app/models/certificate.rb +++ b/app/models/certificate.rb @@ -116,6 +116,7 @@ class Certificate < ApplicationRecord -revoke #{crt_file.path} -key '#{ENV['ca_key_password']}' -batch") if err.match(/Data Base Updated/) || err.match(/ERROR:Already revoked/) + self.revoked = true save! @cached_status = REVOKED else From 2dbcbf1c29efb7868aa5ffa04da8c3e37d4579e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= Date: Tue, 5 May 2020 14:06:02 +0300 Subject: [PATCH 3/6] Remove redundant api check blocks --- app/api/repp/api.rb | 3 +- app/controllers/epp/sessions_controller.rb | 3 +- .../registrar/sessions_controller.rb | 5 +-- app/models/api_user.rb | 32 +++++++++---------- 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/app/api/repp/api.rb b/app/api/repp/api.rb index e5bda46f5..af6864cfa 100644 --- a/app/api/repp/api.rb +++ b/app/api/repp/api.rb @@ -30,7 +30,8 @@ module Repp webclient_cert_name = ENV['webclient_cert_common_name'] || 'webclient' error! "Webclient #{message} #{webclient_cert_name}", 401 if webclient_cert_name != request_name else - unless @current_user.api_pki_ok?(request.env['HTTP_SSL_CLIENT_CERT'], request.env['HTTP_SSL_CLIENT_S_DN_CN']) + unless @current_user.pki_ok?(request.env['HTTP_SSL_CLIENT_CERT'], + request.env['HTTP_SSL_CLIENT_S_DN_CN']) error! "#{message} #{@current_user.username}", 401 end end diff --git a/app/controllers/epp/sessions_controller.rb b/app/controllers/epp/sessions_controller.rb index ef8f125ee..cf24feb33 100644 --- a/app/controllers/epp/sessions_controller.rb +++ b/app/controllers/epp/sessions_controller.rb @@ -26,7 +26,8 @@ module Epp end if !Rails.env.development? && (!webclient_request && @api_user) - unless @api_user.api_pki_ok?(request.env['HTTP_SSL_CLIENT_CERT'], request.env['HTTP_SSL_CLIENT_S_DN_CN']) + unless @api_user.pki_ok?(request.env['HTTP_SSL_CLIENT_CERT'], + request.env['HTTP_SSL_CLIENT_S_DN_CN']) epp_errors << { msg: 'Authentication error; server closing connection (certificate is not valid)', code: '2501' diff --git a/app/controllers/registrar/sessions_controller.rb b/app/controllers/registrar/sessions_controller.rb index 2ca8f5cc7..5bebe5619 100644 --- a/app/controllers/registrar/sessions_controller.rb +++ b/app/controllers/registrar/sessions_controller.rb @@ -31,7 +31,8 @@ class Registrar end if @depp_user.pki - unless @api_user.registrar_pki_ok?(request.env['HTTP_SSL_CLIENT_CERT'], request.env['HTTP_SSL_CLIENT_S_DN_CN']) + unless @api_user.pki_ok?(request.env['HTTP_SSL_CLIENT_CERT'], + request.env['HTTP_SSL_CLIENT_S_DN_CN'], api: false) @depp_user.errors.add(:base, :invalid_cert) end end @@ -205,4 +206,4 @@ class Registrar redirect_to new_registrar_user_session_url, alert: @depp_user.errors.full_messages.first end end -end \ No newline at end of file +end diff --git a/app/models/api_user.rb b/app/models/api_user.rb index 99f14ad6c..b5efa7235 100644 --- a/app/models/api_user.rb +++ b/app/models/api_user.rb @@ -64,26 +64,14 @@ class ApiUser < User registrar.notifications.unread end - def registrar_pki_ok?(crt, cn) - return false if crt.blank? || cn.blank? + def pki_ok?(crt, com, api: true) + return false if crt.blank? || com.blank? - crt = crt.split(' ').join("\n") - crt.gsub!("-----BEGIN\nCERTIFICATE-----\n", "-----BEGIN CERTIFICATE-----\n") - crt.gsub!("\n-----END\nCERTIFICATE-----", "\n-----END CERTIFICATE-----") - cert = OpenSSL::X509::Certificate.new(crt) + origin = api ? certificates.api : certificates.registrar + cert = machine_readable_certificate(crt) md5 = OpenSSL::Digest::MD5.new(cert.to_der).to_s - certificates.registrar.exists?(md5: md5, common_name: cn, revoked: false) - end - def api_pki_ok?(crt, cn) - return false if crt.blank? || cn.blank? - - crt = crt.split(' ').join("\n") - crt.gsub!("-----BEGIN\nCERTIFICATE-----\n", "-----BEGIN CERTIFICATE-----\n") - crt.gsub!("\n-----END\nCERTIFICATE-----", "\n-----END CERTIFICATE-----") - cert = OpenSSL::X509::Certificate.new(crt) - md5 = OpenSSL::Digest::MD5.new(cert.to_der).to_s - certificates.api.exists?(md5: md5, common_name: cn, revoked: false) + origin.exists?(md5: md5, common_name: com, revoked: false) end def linked_users @@ -95,4 +83,14 @@ class ApiUser < User def linked_with?(another_api_user) another_api_user.identity_code == self.identity_code end + + private + + def machine_readable_certificate(cert) + cert = cert.split(' ').join("\n") + cert.gsub!("-----BEGIN\nCERTIFICATE-----\n", "-----BEGIN CERTIFICATE-----\n") + cert.gsub!("\n-----END\nCERTIFICATE-----", "\n-----END CERTIFICATE-----") + + OpenSSL::X509::Certificate.new(cert) + end end From e18942e8eee88276aecef5b8afee021a9550982e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= Date: Tue, 5 May 2020 16:33:33 +0300 Subject: [PATCH 4/6] Cover PKI validation with tests --- test/fixtures/certificates.yml | 7 +++++++ test/models/api_user_test.rb | 15 +++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 test/fixtures/certificates.yml diff --git a/test/fixtures/certificates.yml b/test/fixtures/certificates.yml new file mode 100644 index 000000000..c91df3ace --- /dev/null +++ b/test/fixtures/certificates.yml @@ -0,0 +1,7 @@ +one: + api_user: api_bestnames + common_name: registry.test + crt: "-----BEGIN CERTIFICATE-----\nMIICYjCCAcugAwIBAgIBADANBgkqhkiG9w0BAQ0FADBNMQswCQYDVQQGEwJ1czEO\nMAwGA1UECAwFVGV4YXMxFjAUBgNVBAoMDVJlZ2lzdHJ5IHRlc3QxFjAUBgNVBAMM\nDXJlZ2lzdHJ5LnRlc3QwIBcNMjAwNTA1MTIzNzQxWhgPMjEyMDA0MTExMjM3NDFa\nME0xCzAJBgNVBAYTAnVzMQ4wDAYDVQQIDAVUZXhhczEWMBQGA1UECgwNUmVnaXN0\ncnkgdGVzdDEWMBQGA1UEAwwNcmVnaXN0cnkudGVzdDCBnzANBgkqhkiG9w0BAQEF\nAAOBjQAwgYkCgYEAyn+GCkUJIhdXVBOPrZH+Zj2B/tQfL5TLZwVYZQt38x6GQT+4\n6ndty467IJvKSUlHej7uMpsCzC8Ffmda4cZm16jO1vUb4hXIrmeKP84zLrrUpKag\ngZR4rBDbG2+uL4SzMyy3yeQysYuTiQ4N1i4vdhvkKYPSWIht/QFvuzdFq+0CAwEA\nAaNQME4wHQYDVR0OBBYEFD6B5j6NnMCDBnfbtjBYKBJM7sCRMB8GA1UdIwQYMBaA\nFD6B5j6NnMCDBnfbtjBYKBJM7sCRMAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEN\nBQADgYEArtCR6VOabD3nM/KlZTmHMZVT4ntenYlNTM9FS0RatzPmdh4REhykvmZs\nOlBcpoV5tN5Y8bHOVRqY9V2e903QEhQgoccQhbt0Py6uFwfLv+WLKAUbeGnPqK9d\ndL3wXN9BQs0hJA6IZNFyz2F/gSTURrD1zWW2na3ipRzhupW5+98=\n-----END CERTIFICATE-----\n" + md5: e6771ed5dc857a1dbcc1e0a36baa1fee + interface: api + revoked: false diff --git a/test/models/api_user_test.rb b/test/models/api_user_test.rb index 20d655a9c..dd907f75c 100644 --- a/test/models/api_user_test.rb +++ b/test/models/api_user_test.rb @@ -63,6 +63,21 @@ class ApiUserTest < ActiveSupport::TestCase assert_nil ApiUser.find_by_id_card(id_card) end + def test_verifies_pki_status + certificate = certificates(:one) + + assert @user.pki_ok?(certificate.crt, certificate.common_name, api: true) + assert_not @user.pki_ok?(certificate.crt, 'invalid-cn', api: true) + + certificate.update(interface: 'registrar') + + assert @user.pki_ok?(certificate.crt, certificate.common_name, api: false) + assert_not @user.pki_ok?(certificate.crt, 'invalid-cn', api: false) + + certificate.update(revoked: true) + assert_not @user.pki_ok?(certificate.crt, certificate.common_name, api: false) + end + private def valid_user From c2f8589044da96aa4a316b6d97e5b3d4858f2ffd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= Date: Tue, 12 May 2020 11:38:28 +0300 Subject: [PATCH 5/6] Verify that CN is present when uploading CSR/CRT --- app/models/certificate.rb | 29 +++++++++++++++-------------- test/fixtures/certificates.yml | 9 ++++++++- test/models/api_user_test.rb | 8 ++++++-- 3 files changed, 29 insertions(+), 17 deletions(-) diff --git a/app/models/certificate.rb b/app/models/certificate.rb index f0711d4f5..940c5fdc8 100644 --- a/app/models/certificate.rb +++ b/app/models/certificate.rb @@ -32,20 +32,21 @@ class Certificate < ApplicationRecord errors.add(:base, I18n.t(:invalid_csr_or_crt)) end - before_create :parse_metadata - def parse_metadata - if crt - pc = parsed_crt.try(:subject).try(:to_s) || '' - cn = pc.scan(/\/CN=(.+)/).flatten.first - self.common_name = cn.split('/').first - self.md5 = OpenSSL::Digest::MD5.new(parsed_crt.to_der).to_s - self.interface = API - elsif csr - pc = parsed_csr.try(:subject).try(:to_s) || '' - cn = pc.scan(/\/CN=(.+)/).flatten.first - self.common_name = cn.split('/').first - self.interface = REGISTRAR - end + validate :assign_metadata + + def assign_metadata + origin = crt ? parsed_crt : parsed_csr + parse_metadata(origin) + rescue NoMethodError + errors.add(:base, I18n.t(:invalid_csr_or_crt)) + end + + def parse_metadata(origin) + pc = origin.subject.to_s + cn = pc.scan(%r{\/CN=(.+)}).flatten.first + self.common_name = cn.split('/').first + self.md5 = OpenSSL::Digest::MD5.new(origin.to_der).to_s if crt + self.interface = crt ? API : REGISTRAR end def parsed_crt diff --git a/test/fixtures/certificates.yml b/test/fixtures/certificates.yml index c91df3ace..4799743ff 100644 --- a/test/fixtures/certificates.yml +++ b/test/fixtures/certificates.yml @@ -1,7 +1,14 @@ -one: +api: api_user: api_bestnames common_name: registry.test crt: "-----BEGIN CERTIFICATE-----\nMIICYjCCAcugAwIBAgIBADANBgkqhkiG9w0BAQ0FADBNMQswCQYDVQQGEwJ1czEO\nMAwGA1UECAwFVGV4YXMxFjAUBgNVBAoMDVJlZ2lzdHJ5IHRlc3QxFjAUBgNVBAMM\nDXJlZ2lzdHJ5LnRlc3QwIBcNMjAwNTA1MTIzNzQxWhgPMjEyMDA0MTExMjM3NDFa\nME0xCzAJBgNVBAYTAnVzMQ4wDAYDVQQIDAVUZXhhczEWMBQGA1UECgwNUmVnaXN0\ncnkgdGVzdDEWMBQGA1UEAwwNcmVnaXN0cnkudGVzdDCBnzANBgkqhkiG9w0BAQEF\nAAOBjQAwgYkCgYEAyn+GCkUJIhdXVBOPrZH+Zj2B/tQfL5TLZwVYZQt38x6GQT+4\n6ndty467IJvKSUlHej7uMpsCzC8Ffmda4cZm16jO1vUb4hXIrmeKP84zLrrUpKag\ngZR4rBDbG2+uL4SzMyy3yeQysYuTiQ4N1i4vdhvkKYPSWIht/QFvuzdFq+0CAwEA\nAaNQME4wHQYDVR0OBBYEFD6B5j6NnMCDBnfbtjBYKBJM7sCRMB8GA1UdIwQYMBaA\nFD6B5j6NnMCDBnfbtjBYKBJM7sCRMAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEN\nBQADgYEArtCR6VOabD3nM/KlZTmHMZVT4ntenYlNTM9FS0RatzPmdh4REhykvmZs\nOlBcpoV5tN5Y8bHOVRqY9V2e903QEhQgoccQhbt0Py6uFwfLv+WLKAUbeGnPqK9d\ndL3wXN9BQs0hJA6IZNFyz2F/gSTURrD1zWW2na3ipRzhupW5+98=\n-----END CERTIFICATE-----\n" md5: e6771ed5dc857a1dbcc1e0a36baa1fee interface: api revoked: false +registrar: + api_user: api_bestnames + common_name: registry.test + crt: "-----BEGIN CERTIFICATE-----\nMIICYjCCAcugAwIBAgIBADANBgkqhkiG9w0BAQ0FADBNMQswCQYDVQQGEwJ1czEO\nMAwGA1UECAwFVGV4YXMxFjAUBgNVBAoMDVJlZ2lzdHJ5IHRlc3QxFjAUBgNVBAMM\nDXJlZ2lzdHJ5LnRlc3QwIBcNMjAwNTA1MTIzNzQxWhgPMjEyMDA0MTExMjM3NDFa\nME0xCzAJBgNVBAYTAnVzMQ4wDAYDVQQIDAVUZXhhczEWMBQGA1UECgwNUmVnaXN0\ncnkgdGVzdDEWMBQGA1UEAwwNcmVnaXN0cnkudGVzdDCBnzANBgkqhkiG9w0BAQEF\nAAOBjQAwgYkCgYEAyn+GCkUJIhdXVBOPrZH+Zj2B/tQfL5TLZwVYZQt38x6GQT+4\n6ndty467IJvKSUlHej7uMpsCzC8Ffmda4cZm16jO1vUb4hXIrmeKP84zLrrUpKag\ngZR4rBDbG2+uL4SzMyy3yeQysYuTiQ4N1i4vdhvkKYPSWIht/QFvuzdFq+0CAwEA\nAaNQME4wHQYDVR0OBBYEFD6B5j6NnMCDBnfbtjBYKBJM7sCRMB8GA1UdIwQYMBaA\nFD6B5j6NnMCDBnfbtjBYKBJM7sCRMAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEN\nBQADgYEArtCR6VOabD3nM/KlZTmHMZVT4ntenYlNTM9FS0RatzPmdh4REhykvmZs\nOlBcpoV5tN5Y8bHOVRqY9V2e903QEhQgoccQhbt0Py6uFwfLv+WLKAUbeGnPqK9d\ndL3wXN9BQs0hJA6IZNFyz2F/gSTURrD1zWW2na3ipRzhupW5+98=\n-----END CERTIFICATE-----\n" + md5: e6771ed5dc857a1dbcc1e0a36baa1fee + interface: registrar + revoked: false diff --git a/test/models/api_user_test.rb b/test/models/api_user_test.rb index dd907f75c..ecbff5cbb 100644 --- a/test/models/api_user_test.rb +++ b/test/models/api_user_test.rb @@ -64,18 +64,22 @@ class ApiUserTest < ActiveSupport::TestCase end def test_verifies_pki_status - certificate = certificates(:one) + certificate = certificates(:api) assert @user.pki_ok?(certificate.crt, certificate.common_name, api: true) assert_not @user.pki_ok?(certificate.crt, 'invalid-cn', api: true) - certificate.update(interface: 'registrar') + certificate = certificates(:registrar) assert @user.pki_ok?(certificate.crt, certificate.common_name, api: false) assert_not @user.pki_ok?(certificate.crt, 'invalid-cn', api: false) certificate.update(revoked: true) assert_not @user.pki_ok?(certificate.crt, certificate.common_name, api: false) + + certificate = certificates(:api) + certificate.update(revoked: true) + assert_not @user.pki_ok?(certificate.crt, certificate.common_name, api: true) end private From 36e036e2315c194a624f45c9e454ec98ecec6bb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karl=20Erik=20=C3=95unapuu?= Date: Tue, 12 May 2020 21:27:54 +0300 Subject: [PATCH 6/6] Write metadata only on create --- app/models/certificate.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/certificate.rb b/app/models/certificate.rb index 940c5fdc8..d2428365a 100644 --- a/app/models/certificate.rb +++ b/app/models/certificate.rb @@ -32,7 +32,7 @@ class Certificate < ApplicationRecord errors.add(:base, I18n.t(:invalid_csr_or_crt)) end - validate :assign_metadata + validate :assign_metadata, on: :create def assign_metadata origin = crt ? parsed_crt : parsed_csr