From 3c7541c858ccb385b8d734aebc3fcc1f0e9b3d3f Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 28 Mar 2025 14:17:17 -0500 Subject: [PATCH 1/2] Validate SAML configuration during application start changelog: Internal, SAML, Validate SAML configuration during application start --- app/services/saml_endpoint.rb | 71 +++++++++++++++++------------ config/routes.rb | 2 +- spec/requests/saml_requests_spec.rb | 20 ++++++++ spec/services/saml_endpoint_spec.rb | 29 +----------- 4 files changed, 63 insertions(+), 59 deletions(-) diff --git a/app/services/saml_endpoint.rb b/app/services/saml_endpoint.rb index bd235e52464..5524f32bfd8 100644 --- a/app/services/saml_endpoint.rb +++ b/app/services/saml_endpoint.rb @@ -1,11 +1,41 @@ # frozen_string_literal: true class SamlEndpoint - SAML_YEARS = AppArtifacts.store.members.map(&:to_s).map do |key| - regex = /saml_(?\d{4})_(?key|cert)/ - matches = regex.match(key) - matches && matches[:year] - end.compact.uniq.freeze + SAML_YEARS = IdentityConfig.store.saml_endpoint_configs.map do |config| + config.fetch(:suffix).to_s + end.uniq.sort.freeze + + SAML_YEAR_CERTS = SAML_YEARS.each_with_object({}) do |year, map| + x509_cert = + begin + AppArtifacts.store["saml_#{year}_cert"] + rescue NameError + raise "No SAML certificate for suffix #{year}" + end + map[year] = x509_cert + end.freeze + + SAML_YEAR_SECRET_KEYS = SAML_YEARS.each_with_object({}) do |year, map| + config = IdentityConfig.store.saml_endpoint_configs.find do |config| + config[:suffix] == year + end + + key_contents = begin + AppArtifacts.store["saml_#{year}_key"] + rescue NameError + raise "No SAML private key for suffix #{year}" + end + + map[year] = + begin + OpenSSL::PKey::RSA.new( + key_contents, + config.fetch(:secret_key_passphrase), + ) + rescue OpenSSL::PKey::RSAError + raise "SAML key or passphrase for #{year} is invalid" + end + end.freeze attr_reader :year @@ -18,29 +48,18 @@ def self.valid_year?(year) end def self.suffixes - endpoint_configs.pluck(:suffix) - end - - def self.endpoint_configs - IdentityConfig.store.saml_endpoint_configs + SAML_YEARS end def secret_key - key_contents = begin - AppArtifacts.store["saml_#{year}_key"] - rescue NameError - raise "No SAML private key for suffix #{year}" - end - - OpenSSL::PKey::RSA.new( - key_contents, - endpoint_config[:secret_key_passphrase], - ) + SAML_YEAR_SECRET_KEYS.fetch(year) + rescue KeyError + raise "No SAML private key for suffix #{year}" end def x509_certificate - AppArtifacts.store["saml_#{year}_cert"] - rescue NameError + SAML_YEAR_CERTS.fetch(year) + rescue KeyError raise "No SAML certificate for suffix #{year}" end @@ -54,12 +73,4 @@ def saml_metadata secret_key, ) end - - private - - def endpoint_config - @endpoint_config ||= self.class.endpoint_configs.find do |config| - config[:suffix] == year - end - end end diff --git a/config/routes.rb b/config/routes.rb index c4362f8cb7b..8d902318366 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -44,7 +44,7 @@ # SAML secret rotation paths constraints(path_year: SamlEndpoint.suffixes) do - get '/api/saml/metadata(:path_year)' => 'saml_idp#metadata', format: false + get '/api/saml/metadata(:path_year)' => 'saml_idp#metadata', format: false, as: :api_saml_metadata match '/api/saml/logout(:path_year)' => 'saml_idp#logout', via: %i[get post delete], as: :api_saml_logout match '/api/saml/remotelogout(:path_year)' => 'saml_idp#remotelogout', via: %i[get post], diff --git a/spec/requests/saml_requests_spec.rb b/spec/requests/saml_requests_spec.rb index d7cffa1a8c1..98b5e463c71 100644 --- a/spec/requests/saml_requests_spec.rb +++ b/spec/requests/saml_requests_spec.rb @@ -95,4 +95,24 @@ end end end + + describe 'GET /api/saml/metadata' do + let(:path_year) { SamlAuthHelper::PATH_YEAR } + + it 'is successful' do + get api_saml_metadata_url(path_year: path_year) + + expect(response).to be_ok + end + + context 'for an unsupported year' do + let(:path_year) { (SamlEndpoint.suffixes.max.to_i + 1).to_s } + + it '404s' do + get api_saml_metadata_url(path_year: path_year) + + expect(response).to be_not_found + end + end + end end diff --git a/spec/services/saml_endpoint_spec.rb b/spec/services/saml_endpoint_spec.rb index 632f86a58e5..2e1cf4b390a 100644 --- a/spec/services/saml_endpoint_spec.rb +++ b/spec/services/saml_endpoint_spec.rb @@ -9,26 +9,7 @@ it 'should list the suffixes that are configured' do result = described_class.suffixes - expect(result).to eq(%w[2025 2024]) - end - end - - describe 'endpoint_configs' do - it 'should return an array of parsed endpoint config data' do - result = described_class.endpoint_configs - - expect(result).to eq( - [ - { suffix: '2025', secret_key_passphrase: 'trust-but-verify' }, - { - # rubocop:disable Layout/LineLength - comment: 'this extra year is needed to demonstrate how handling multiple live years works in spec/requests/saml_requests_spec.rb', - # rubocop:enable Layout/LineLength - secret_key_passphrase: 'trust-but-verify', - suffix: '2024', - }, - ], - ) + expect(result).to eq(%w[2024 2025]) end end @@ -47,14 +28,6 @@ context 'when the key file does not exist' do let(:year) { '_dne' } - before do - allow(SamlEndpoint).to receive(:endpoint_configs).and_return( - [ - { suffix: '_dne', secret_key_passphrase: 'asdf1234' }, - ], - ) - end - it 'raises an error' do expect { subject.secret_key }.to raise_error( 'No SAML private key for suffix _dne', From 8234e7eb19dd9175e88038790aee4fe301564b00 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Mon, 31 Mar 2025 13:09:14 -0500 Subject: [PATCH 2/2] add specs --- app/services/saml_endpoint.rb | 77 +++++++++++++++++------------ spec/services/saml_endpoint_spec.rb | 58 ++++++++++++++++++++++ 2 files changed, 103 insertions(+), 32 deletions(-) diff --git a/app/services/saml_endpoint.rb b/app/services/saml_endpoint.rb index 5524f32bfd8..ebe3347f266 100644 --- a/app/services/saml_endpoint.rb +++ b/app/services/saml_endpoint.rb @@ -5,38 +5,6 @@ class SamlEndpoint config.fetch(:suffix).to_s end.uniq.sort.freeze - SAML_YEAR_CERTS = SAML_YEARS.each_with_object({}) do |year, map| - x509_cert = - begin - AppArtifacts.store["saml_#{year}_cert"] - rescue NameError - raise "No SAML certificate for suffix #{year}" - end - map[year] = x509_cert - end.freeze - - SAML_YEAR_SECRET_KEYS = SAML_YEARS.each_with_object({}) do |year, map| - config = IdentityConfig.store.saml_endpoint_configs.find do |config| - config[:suffix] == year - end - - key_contents = begin - AppArtifacts.store["saml_#{year}_key"] - rescue NameError - raise "No SAML private key for suffix #{year}" - end - - map[year] = - begin - OpenSSL::PKey::RSA.new( - key_contents, - config.fetch(:secret_key_passphrase), - ) - rescue OpenSSL::PKey::RSAError - raise "SAML key or passphrase for #{year} is invalid" - end - end.freeze - attr_reader :year def initialize(year) @@ -51,6 +19,47 @@ def self.suffixes SAML_YEARS end + def self.build_saml_keys_by_year + SAML_YEARS.each_with_object({}) do |year, map| + config = IdentityConfig.store.saml_endpoint_configs.find do |config| + config[:suffix] == year + end + + key_contents = begin + AppArtifacts.store["saml_#{year}_key"] + rescue NameError + raise "No SAML private key for suffix #{year}" + end + + map[year] = + begin + OpenSSL::PKey::RSA.new( + key_contents, + config.fetch(:secret_key_passphrase), + ) + rescue OpenSSL::PKey::RSAError + raise "SAML key or passphrase for #{year} is invalid" + end + end + end + + def self.build_saml_certs_by_year + SAML_YEARS.each_with_object({}) do |year, map| + x509_cert = + begin + cert_string = AppArtifacts.store["saml_#{year}_cert"] + # Validate string value can be parsed to X509, but store string + OpenSSL::X509::Certificate.new(cert_string) + cert_string + rescue NameError + raise "No SAML certificate for suffix #{year}" + rescue OpenSSL::X509::CertificateError + raise "SAML certificate for #{year} is invalid" + end + map[year] = x509_cert + end + end + def secret_key SAML_YEAR_SECRET_KEYS.fetch(year) rescue KeyError @@ -73,4 +82,8 @@ def saml_metadata secret_key, ) end + + SAML_YEAR_CERTS = self.build_saml_certs_by_year.freeze + + SAML_YEAR_SECRET_KEYS = self.build_saml_keys_by_year.freeze end diff --git a/spec/services/saml_endpoint_spec.rb b/spec/services/saml_endpoint_spec.rb index 2e1cf4b390a..a7db6fd36bf 100644 --- a/spec/services/saml_endpoint_spec.rb +++ b/spec/services/saml_endpoint_spec.rb @@ -13,6 +13,64 @@ end end + describe '.build_saml_certs_by_year' do + it 'returns a map with keys based on SAML_YEARS and String values' do + saml_certs_by_year = described_class.build_saml_certs_by_year + expect(saml_certs_by_year.keys.sort).to eq(described_class::SAML_YEARS.sort) + expect(saml_certs_by_year.values).to all be_a(String) + end + + it 'raises exception if the certificate for a year does not exist' do + stub_const('SamlEndpoint::SAML_YEARS', ['2000']) + expect { described_class.build_saml_certs_by_year }.to raise_error( + RuntimeError, + 'No SAML certificate for suffix 2000', + ) + end + + it 'raises exception if the certificate value is invalid' do + cert_year = SamlEndpoint::SAML_YEARS.first + stub_const('SamlEndpoint::SAML_YEARS', [cert_year]) + allow(AppArtifacts.store).to(receive(:[])).with("saml_#{cert_year}_cert").and_return( + 'bad cert', + ) + + expect { described_class.build_saml_certs_by_year }.to raise_error( + RuntimeError, + "SAML certificate for #{cert_year} is invalid", + ) + end + end + + describe '.build_saml_keys_by_year' do + it 'returns a map with keys based on SAML_YEARS and String values' do + saml_keys_by_year = described_class.build_saml_keys_by_year + expect(saml_keys_by_year.keys.sort).to eq(described_class::SAML_YEARS.sort) + expect(saml_keys_by_year.values).to all be_a(OpenSSL::PKey::RSA) + end + + it 'raises exception if the key for a year does not exist' do + stub_const('SamlEndpoint::SAML_YEARS', ['2000']) + expect { described_class.build_saml_keys_by_year }.to raise_error( + RuntimeError, + 'No SAML private key for suffix 2000', + ) + end + + it 'raises exception if the key value is invalid' do + key_year = SamlEndpoint::SAML_YEARS.first + stub_const('SamlEndpoint::SAML_YEARS', [key_year]) + allow(AppArtifacts.store).to(receive(:[])).with("saml_#{key_year}_key").and_return( + 'bad key', + ) + + expect { described_class.build_saml_keys_by_year }.to raise_error( + RuntimeError, + "SAML key or passphrase for #{key_year} is invalid", + ) + end + end + describe '#secret_key' do it 'returns the key loaded from the file system' do expect(