From 4acdc3a81125751bd41975cf934d86c22d57a278 Mon Sep 17 00:00:00 2001 From: davida marion Date: Tue, 23 Jan 2024 11:29:08 -0500 Subject: [PATCH 1/2] Returns 400 when blank certificate element is passed through assertion" changelog: Bug Fixes, SAML Authentication, Returns 400 with error when a blank certificate element is included in the SAML assertion --- .../concerns/saml_idp_auth_concern.rb | 26 ++++++++----- app/controllers/saml_idp_controller.rb | 2 +- app/services/saml_request_validator.rb | 11 ++++++ config/locales/errors/en.yml | 1 + config/locales/errors/es.yml | 1 + config/locales/errors/fr.yml | 1 + spec/controllers/saml_idp_controller_spec.rb | 38 +++++++++++++++---- 7 files changed, 62 insertions(+), 18 deletions(-) diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index 86cc4e84996..d27c5398232 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -5,7 +5,7 @@ module SamlIdpAuthConcern included do # rubocop:disable Rails/LexicallyScopedActionFilter - before_action :validate_saml_request, only: :auth + before_action :create_saml_request_object, only: :auth before_action :validate_service_provider_and_authn_context, only: :auth before_action :check_sp_active, only: :auth before_action :log_external_saml_auth_request, only: [:auth] @@ -45,21 +45,29 @@ def check_sp_active end def validate_service_provider_and_authn_context - @saml_request_validator = SamlRequestValidator.new + return if result.success? + + analytics.saml_auth( + **result.to_h.merge(request_signed: saml_request.signed?), + ) + render 'saml_idp/auth/error', status: :bad_request + end - @result = @saml_request_validator.call( + def result + @result ||= @saml_request_validator.call( service_provider: saml_request_service_provider, authn_context: requested_authn_contexts, authn_context_comparison: saml_request.requested_authn_context_comparison, nameid_format: name_id_format, ) + end - return if @result.success? - - analytics.saml_auth( - **@result.to_h.merge(request_signed: saml_request.signed?), - ) - render 'saml_idp/auth/error', status: :bad_request + def create_saml_request_object + # this saml_idp method creates the saml_request object used for validations + validate_saml_request + @saml_request_validator = SamlRequestValidator.new + rescue SamlIdp::XMLSecurity::SignedDocument::ValidationError + @saml_request_validator = SamlRequestValidator.new(blank_cert: true) end def name_id_format diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index 2387631005b..ce5d5190848 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -118,7 +118,7 @@ def pii_requested_but_locked? end def capture_analytics - analytics_payload = @result.to_h.merge( + analytics_payload = result.to_h.merge( endpoint: api_saml_auth_path(path_year: params[:path_year]), idv: identity_needs_verification?, finish_profile: user_has_pending_profile?, diff --git a/app/services/saml_request_validator.rb b/app/services/saml_request_validator.rb index 21ddd0ea7db..c54666b6cb1 100644 --- a/app/services/saml_request_validator.rb +++ b/app/services/saml_request_validator.rb @@ -1,10 +1,15 @@ class SamlRequestValidator include ActiveModel::Model + validate :cert_exists validate :authorized_service_provider validate :authorized_authn_context validate :authorized_email_nameid_format + def initialize(blank_cert: false) + @blank_cert = blank_cert + end + def call(service_provider:, authn_context:, nameid_format:, authn_context_comparison: nil) self.service_provider = service_provider self.authn_context = Array(authn_context) @@ -46,6 +51,12 @@ def authorized_authn_context end end + def cert_exists + if @blank_cert + errors.add(:service_provider, :blank_cert_element_req, type: :blank_cert_element_req) + end + end + def valid_authn_context? valid_contexts = Saml::Idp::Constants::VALID_AUTHN_CONTEXTS.dup valid_contexts += Saml::Idp::Constants::PASSWORD_AUTHN_CONTEXT_CLASSREFS if step_up_comparison? diff --git a/config/locales/errors/en.yml b/config/locales/errors/en.yml index e3861954239..e00fab81232 100644 --- a/config/locales/errors/en.yml +++ b/config/locales/errors/en.yml @@ -50,6 +50,7 @@ en: messages: already_confirmed: was already confirmed, please try signing in blank: Please fill in this field. + blank_cert_element_req: We cannot detect a certificate in your request. confirmation_code_incorrect: Incorrect verification code. Did you type it in correctly? confirmation_invalid_token: Invalid confirmation link. Either the link expired or you already confirmed your account. diff --git a/config/locales/errors/es.yml b/config/locales/errors/es.yml index be20e24d5d1..970491697c4 100644 --- a/config/locales/errors/es.yml +++ b/config/locales/errors/es.yml @@ -54,6 +54,7 @@ es: messages: already_confirmed: ya estaba confirmado, por favor intente iniciar una sesión blank: Por favor, rellenar este campo. + blank_cert_element_req: No podemos detectar un certificado en su solicitud. confirmation_code_incorrect: Código de verificación incorrecto. ¿Lo escribió correctamente? confirmation_invalid_token: El enlace de confirmación no es válido. El enlace expiró o usted ya ha confirmado su cuenta. diff --git a/config/locales/errors/fr.yml b/config/locales/errors/fr.yml index fb52095e419..a62e9a84798 100644 --- a/config/locales/errors/fr.yml +++ b/config/locales/errors/fr.yml @@ -59,6 +59,7 @@ fr: messages: already_confirmed: a déjà été confirmé, veuillez essayer de vous connecter blank: Veuillez remplir ce champ. + blank_cert_element_req: Nous ne pouvons pas détecter un certificat sur votre demande. confirmation_code_incorrect: Code de vérification incorrect. L’avez-vous saisi correctement ? confirmation_invalid_token: Lien de confirmation non valide. Le lien est expiré ou vous avez déjà confirmé votre compte. diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 8fe8842de91..b5c26229e30 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -1362,6 +1362,23 @@ def name_id_version(format_urn) context 'cert element in SAML request is blank' do let(:user) { create(:user, :fully_registered) } let(:service_provider) { build(:service_provider, issuer: 'http://localhost:3000') } + let(:analytics_hash) do + { + success: false, + errors: { service_provider: ['We cannot detect a certificate in your request.'] }, + error_details: { service_provider: { blank_cert_element_req: true } }, + nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT, + authn_context: [Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF], + authn_context_comparison: 'exact', + service_provider: 'http://localhost:3000', + request_signed: true, + } + end + + before do + stub_analytics + allow(@analytics).to receive(:track_event) + end # the RubySAML library won't let us pass an empty string in as the certificate # element, so this test substitutes a SAMLRequest that has that element blank @@ -1395,7 +1412,6 @@ def name_id_version(format_urn) urn:gov:gsa:ac:classes:sp:PasswordProtectedTransport:duo - http://idmanagement.gov/ns/assurance/ial/1 XML @@ -1410,11 +1426,17 @@ def name_id_version(format_urn) expect(CGI).to receive(:unescape).and_return deflated_encoded_req end - it 'a ValidationError is raised' do - expect { generate_saml_response(user, saml_settings) }.to raise_error( - SamlIdp::XMLSecurity::SignedDocument::ValidationError, - 'Certificate element present in response (ds:X509Certificate) but evaluating to nil', - ) + it 'notes it in the analytics event' do + generate_saml_response(user, saml_settings) + expect(@analytics).to have_received(:track_event). + with('SAML Auth', analytics_hash) + end + + it 'returns a 400' do + generate_saml_response(user, saml_settings) + expect(controller).to render_template('saml_idp/auth/error') + expect(response.status).to eq(400) + expect(response.body).to include(t('errors.messages.blank_cert_element_req')) end end @@ -2259,10 +2281,10 @@ def stub_requested_attributes it 'includes the appropriate before_actions' do expect(subject).to have_actions( :before, + :create_saml_request_object, :disable_caching, - :validate_saml_request, - :validate_service_provider_and_authn_context, :store_saml_request, + :validate_service_provider_and_authn_context, ) end end From 777d3b5054634458f64062cba0189cbb009a58ea Mon Sep 17 00:00:00 2001 From: davida marion Date: Tue, 23 Jan 2024 14:15:31 -0500 Subject: [PATCH 2/2] Update method name to be more clear --- app/controllers/concerns/saml_idp_auth_concern.rb | 4 ++-- spec/controllers/saml_idp_controller_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/concerns/saml_idp_auth_concern.rb b/app/controllers/concerns/saml_idp_auth_concern.rb index d27c5398232..14b2be3d042 100644 --- a/app/controllers/concerns/saml_idp_auth_concern.rb +++ b/app/controllers/concerns/saml_idp_auth_concern.rb @@ -5,7 +5,7 @@ module SamlIdpAuthConcern included do # rubocop:disable Rails/LexicallyScopedActionFilter - before_action :create_saml_request_object, only: :auth + before_action :validate_and_create_saml_request_object, only: :auth before_action :validate_service_provider_and_authn_context, only: :auth before_action :check_sp_active, only: :auth before_action :log_external_saml_auth_request, only: [:auth] @@ -62,7 +62,7 @@ def result ) end - def create_saml_request_object + def validate_and_create_saml_request_object # this saml_idp method creates the saml_request object used for validations validate_saml_request @saml_request_validator = SamlRequestValidator.new diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index b5c26229e30..73cdc8f8cf1 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -2281,9 +2281,9 @@ def stub_requested_attributes it 'includes the appropriate before_actions' do expect(subject).to have_actions( :before, - :create_saml_request_object, :disable_caching, :store_saml_request, + :validate_and_create_saml_request_object, :validate_service_provider_and_authn_context, ) end