From fb7951d9871868b637c02f184b1f686090dd8636 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Tue, 15 Nov 2022 15:56:15 +0100 Subject: [PATCH] Extend specs with verify_mode and tls certificate --- app/models/ldap_auth_source.rb | 41 ++++++----- spec/factories/auth_source_factory.rb | 1 + spec/fixtures/ldap/snakeoil.pem | 46 ++++++++++++ spec/models/ldap_auth_source_spec.rb | 101 +++++++++++++++++++++++--- 4 files changed, 162 insertions(+), 27 deletions(-) create mode 100644 spec/fixtures/ldap/snakeoil.pem diff --git a/app/models/ldap_auth_source.rb b/app/models/ldap_auth_source.rb index 9b3cae4f9b..68737253e3 100644 --- a/app/models/ldap_auth_source.rb +++ b/app/models/ldap_auth_source.rb @@ -143,6 +143,22 @@ class LdapAuthSource < AuthSource Net::LDAP::Filter.from_rfc2254(filter_string) if filter_string.present? end + def ldap_connection_options + { + host:, + port:, + force_no_page: OpenProject::Configuration.ldap_force_no_page, + encryption: ldap_encryption + } + end + + def read_ldap_certificates + return if tls_certificate_string.blank? + + # Using load will allow multiple PEM certificates to be passed + OpenSSL::X509::Certificate.load(tls_certificate_string) + end + private def strip_ldap_attributes @@ -156,10 +172,7 @@ class LdapAuthSource < AuthSource Rails.logger.info { "SSL connection to LDAP host #{host} is set up to skip certificate verification." } end - options = { host: host, - port: port, - force_no_page: OpenProject::Configuration.ldap_force_no_page, - encryption: ldap_encryption } + options = ldap_connection_options unless ldap_user.blank? && ldap_password.blank? options.merge!(auth: { method: :simple, username: ldap_user, password: ldap_password }) @@ -172,7 +185,7 @@ class LdapAuthSource < AuthSource { method: tls_mode.to_sym, - tls_options:, + tls_options: } end @@ -191,13 +204,6 @@ class LdapAuthSource < AuthSource }.compact end - def read_ldap_certificates - return if tls_certificate_string.blank? - - # Using load will allow multiple PEM certificates to be passed - OpenSSL::X509::Certificate.load(tls_certificate_string) - end - def tls_verify_mode if verify_peer? OpenSSL::SSL::VERIFY_PEER @@ -227,11 +233,12 @@ class LdapAuthSource < AuthSource ldap_con.search(base: base_dn, filter: filter, attributes: search_attributes) do |entry| - attrs = if onthefly_register? - get_user_attributes_from_ldap_entry(entry) - else - { dn: entry.dn } - end + attrs = + if onthefly_register? + get_user_attributes_from_ldap_entry(entry) + else + { dn: entry.dn } + end Rails.logger.debug { "DN found for #{login}: #{attrs[:dn]}" } end diff --git a/spec/factories/auth_source_factory.rb b/spec/factories/auth_source_factory.rb index 28b9dbde82..4989b2d21d 100644 --- a/spec/factories/auth_source_factory.rb +++ b/spec/factories/auth_source_factory.rb @@ -35,6 +35,7 @@ FactoryBot.define do host { '127.0.0.1' } port { 225 } # a reserved port, should not be in use attr_login { 'uid' } + tls_mode { 'plain_ldap' } end factory :dummy_auth_source, class: 'DummyAuthSource' do diff --git a/spec/fixtures/ldap/snakeoil.pem b/spec/fixtures/ldap/snakeoil.pem new file mode 100644 index 0000000000..02f60f81f4 --- /dev/null +++ b/spec/fixtures/ldap/snakeoil.pem @@ -0,0 +1,46 @@ +-----BEGIN CERTIFICATE----- +MIIDczCCAlugAwIBAgIER8Y4EjANBgkqhkiG9w0BAQsFADBqMRAwDgYDVQQGEwdV +bmtub3duMRAwDgYDVQQIEwdVbmtub3duMRAwDgYDVQQHEwdVbmtub3duMRAwDgYD +VQQKEwdleGFtcGxlMQwwCgYDVQQLEwNjb20xEjAQBgNVBAMTCWxvY2FsaG9zdDAe +Fw0yMjExMTUxMzE5MDFaFw0yMzAyMTMxMzE5MDFaMGoxEDAOBgNVBAYTB1Vua25v +d24xEDAOBgNVBAgTB1Vua25vd24xEDAOBgNVBAcTB1Vua25vd24xEDAOBgNVBAoT +B2V4YW1wbGUxDDAKBgNVBAsTA2NvbTESMBAGA1UEAxMJbG9jYWxob3N0MIIBIjAN +BgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAhktWeAGMy0+sxoTvNRmTrFpUZ6qI +ou+okofrAEULM+VdiHxfJk4Fsy9B1IgY1LvKJ8jA+MAmX4pBq+5KUFY2hIzMM4yR +RcvXH9DvzzthkUQLtRWnVmgMPeDS0cTFaxnjElrnM+OzRPTENlv0cn89+Fy2DgQp +Xoc8WXJmCyy94jVtGbbZgpZbIyGVjG7q5Ht4Fz4bFyjMjS0//JoLTbPuiAZ5bdkI ++eNLxHy8BLIbyCmgxdTXI7hpRCbhfbFY5Cgrmr0O2iP/UI8woUQkecUT+zxsJbSb +eCb5YmBM35Y/DwxS1wE3JEX4XzyxXMpKWeOB/UQ6HAneHzzSDOihbRYUWQIDAQAB +oyEwHzAdBgNVHQ4EFgQUWOl3qIbcOWgKoyeJKg3DyzFnW+kwDQYJKoZIhvcNAQEL +BQADggEBAADXn6mIlqtHcGNAzcjGsVZFTsfHV5bR4Ohi6ms8IMJ+OS0vSYKwzSIy +38U58xBR+gfQrbzZ+FE9XOym/g63QoQcHe/ERr9fI1dxNQKHliWbd1XtzjcAIelu +J7Xg9C3eHrlhxQ8Cc0/eFb84vSD+0XlWvd0AOwTWF+rZ5+McrLJu35ZiJtDhBS9P +2ISPbkZ7eCaeBD3OApJjbo+05IqZsi/9BMY5keBmZijYDIUWozpGCNFQ6tYs8BEm +efJgyJ3dSOFvS90jqNMnPumEpIfP4BN2RDH2ygmmbbPemsA8Hl/OIy+5qG6x/6Zy +SFMzIkQvcS1ZgJpD44mZhFzpSJUsD5s= +-----END CERTIFICATE----- + +-----BEGIN CERTIFICATE----- +MIID7zCCAtegAwIBAgIUdGNOIw1SvmiPaeilr42CkCeMeTowDQYJKoZIhvcNAQEL +BQAwgYYxCzAJBgNVBAYTAkRFMQ8wDQYDVQQIDAZIZXNzZW4xEjAQBgNVBAcMCURh +cm1zdGFkdDEPMA0GA1UECgwGVGVzdGVyMRQwEgYDVQQLDAtMREFQIHRlc3RlcjEL +MAkGA1UEAwwCQ0ExHjAcBgkqhkiG9w0BCQEWD2Zvb0BleGFtcGxlLmNvbTAeFw0y +MDA1MTQxMTUwMThaFw0yMDA2MTMxMTUwMThaMIGGMQswCQYDVQQGEwJERTEPMA0G +A1UECAwGSGVzc2VuMRIwEAYDVQQHDAlEYXJtc3RhZHQxDzANBgNVBAoMBlRlc3Rl +cjEUMBIGA1UECwwLTERBUCB0ZXN0ZXIxCzAJBgNVBAMMAkNBMR4wHAYJKoZIhvcN +AQkBFg9mb29AZXhhbXBsZS5jb20wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK +AoIBAQDEshEY/CwpzuyuCdG9H8EvlEZystY6hhHXFBh/rCf4KjWPFURTki5Ha1/U +6/PyjPEkdgmUF8Px7un+4FJNgSJnRs5Hz4b7GsHOF8JaHf6JZQy+0Mvp9WI81jcM +i98n1oE2WUDSrlXi1qs3okgX63rhCPjrXL3WCdo/Ef2EtW8usSinXZ0Vfd+3lQoh +9o0lC8UELot0F5/505NvCYmYNTFNsfiOUNvC++qwLl2bNrk/qPYRIHBh3I8lxF36 +ksii/nFijZaQh0mnXIXeewz5eInzJvTfWH47U3ioXD3iJtHc3pkZ1qJD+UELULDO +8JU05buSJfLpT64goP+/TQpqzWN5AgMBAAGjUzBRMB0GA1UdDgQWBBS9qy2N1L26 +/2JYGhcyIaadI1DTCjAfBgNVHSMEGDAWgBS9qy2N1L26/2JYGhcyIaadI1DTCjAP +BgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQBE7lFSeWuWhtb9IZRu +Z9lV10e+XK3uWpPt9/dyOcR5LvGH196jHnak/vV84uDhudWI2rZlm10DsLEE6J0i +yI/039yd0HouOA5MpxBAWkKVgM3AefXgeuuQOIsU6QShnMikhqurw8Y2WGt9PT1V +wSZW9g/CpI8hO8XEX/AriePh62gD3XSS8kSa40nIMTiOzY18orORC+5jFBK50P2/ +CH3cdpePXJnvIM6aYPjMemojg1PN5MmJHUdFBvGHuo2e1CAp7KrI4LPIXZVMxI1z +jjE7Rd9gfKPhQ0nT6WpXEqovvnliTum+teh2pJ5y3a31jD6xp09ST9JYmDUZwNIm +HT64 +-----END CERTIFICATE----- diff --git a/spec/models/ldap_auth_source_spec.rb b/spec/models/ldap_auth_source_spec.rb index ece8fd25c8..196fef1b3f 100644 --- a/spec/models/ldap_auth_source_spec.rb +++ b/spec/models/ldap_auth_source_spec.rb @@ -30,28 +30,108 @@ require 'spec_helper' describe LdapAuthSource, type: :model do it 'creates' do - a = LdapAuthSource.new(name: 'My LDAP', host: 'ldap.example.net', port: 389, base_dn: 'dc=example,dc=net', + a = described_class.new(name: 'My LDAP', host: 'ldap.example.net', port: 389, base_dn: 'dc=example,dc=net', attr_login: 'sAMAccountName') expect(a.save).to be true end it 'strips ldap attributes' do - a = LdapAuthSource.new(name: 'My LDAP', host: 'ldap.example.net', port: 389, base_dn: 'dc=example,dc=net', attr_login: 'sAMAccountName', + a = described_class.new(name: 'My LDAP', host: 'ldap.example.net', port: 389, + base_dn: 'dc=example,dc=net', attr_login: 'sAMAccountName', attr_firstname: 'givenName ') expect(a.save).to be true expect(a.reload.attr_firstname).to eq 'givenName' end - describe 'overriding tls_options', - with_settings: { ldap_tls_options: { ca_file: '/path/to/ca/file' } } do - it 'sets the encryption options for start_tls' do - ldap = LdapAuthSource.new tls_mode: :start_tls - expect(ldap.send(:ldap_encryption)).to eq(method: :start_tls, tls_options: { 'ca_file' => '/path/to/ca/file' }) + describe 'verify_peer' do + let(:tls_mode) { :start_tls } + let(:ldap) { described_class.new tls_mode:, verify_peer: } + + subject { ldap.ldap_connection_options[:encryption] } + + context 'when set' do + let(:verify_peer) { true } + + it 'reflects in tls_options' do + expect(subject).to have_key :tls_options + expect(subject[:tls_options]).to match(hash_including(verify_mode: OpenSSL::SSL::VERIFY_PEER)) + end + end + + context 'when set, but tls_mode differs' do + let(:tls_mode) { :plain_ldap } + let(:verify_peer) { true } + + it 'does nothing' do + expect(subject).to be_nil + end + end + + context 'when unset' do + let(:verify_peer) { false } + + it 'reflects in tls_options' do + expect(subject).to have_key :tls_options + expect(subject[:tls_options]).to match(hash_including(verify_mode: OpenSSL::SSL::VERIFY_NONE)) + end + end + end + + describe 'cert_store' do + let(:fixture) { Rails.root.join('spec/fixtures/ldap/snakeoil.pem') } + let(:ldap) { build(:ldap_auth_source, tls_mode: :start_tls, tls_certificate_string: File.read(fixture)) } + let(:store_double) { instance_double(OpenSSL::X509::Store) } + + subject { ldap.ldap_connection_options.dig(:encryption, :tls_options) } + + it 'adds the certificates to the store' do + allow(OpenSSL::X509::Store).to receive(:new).and_return(store_double) + allow(store_double).to receive(:set_default_paths) + allow(store_double).to receive(:add_cert) + + expect(subject).to have_key :cert_store + expect(subject[:cert_store]).to eq store_double + + expect(store_double).to have_received(:add_cert).twice + end + end + + describe 'tls_certificate_string' do + let(:ldap) { build(:ldap_auth_source, tls_certificate_string:) } + + subject { ldap.read_ldap_certificates } + + context 'when single certificate' do + let(:fixture) { Rails.root.join('spec/fixtures/ldap/snakeoil.pem') } + let(:tls_certificate_string) { File.read(fixture).split(/^$/)[0] } + + it 'is valid' do + expect(ldap).to be_valid + expect(subject).to be_a Array + expect(subject.length).to eq 1 + expect(subject).to all(be_a(OpenSSL::X509::Certificate)) + end + end + + context 'when multiple certificates' do + let(:fixture) { Rails.root.join('spec/fixtures/ldap/snakeoil.pem') } + let(:tls_certificate_string) { File.read(fixture) } + + it 'is valid' do + expect(ldap).to be_valid + expect(subject).to be_a Array + expect(subject.length).to eq 2 + expect(subject).to all(be_a(OpenSSL::X509::Certificate)) + end end - it 'does nothing for plain_ldap' do - ldap = LdapAuthSource.new tls_mode: :plain_ldap - expect(ldap.send(:ldap_encryption)).to be_nil + context 'when bogus content' do + let(:tls_certificate_string) { 'foo' } + + it 'is invalid' do + expect(ldap).not_to be_valid + expect { subject }.to raise_error(OpenSSL::X509::CertificateError) + end end end @@ -151,6 +231,7 @@ describe LdapAuthSource, type: :model do let(:ldap) do create :ldap_auth_source, port: ParallelHelper.port_for_ldap.to_s, + tls_mode: :plain_ldap, account: 'uid=admin,ou=system', account_password: 'secret', base_dn: 'ou=people,dc=example,dc=com',