From 5693220f0fb0b023e1b21e678d82ae211f132ded Mon Sep 17 00:00:00 2001 From: "Davida (she/they)" Date: Mon, 25 Nov 2024 16:38:48 -0500 Subject: [PATCH] Add analytics tracking for signature algorithm errors (#11552) * changelog: Internal, Analytics, Add tracking for sha256 change --- Gemfile | 2 +- Gemfile.lock | 6 +- app/controllers/saml_idp_controller.rb | 22 +++++ app/services/analytics_events.rb | 8 ++ spec/controllers/saml_idp_controller_spec.rb | 87 ++++++++++++++++++++ 5 files changed, 121 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index 17262773376..8c3c853be75 100644 --- a/Gemfile +++ b/Gemfile @@ -74,7 +74,7 @@ gem 'rqrcode' gem 'ruby-progressbar' gem 'ruby-saml' gem 'safe_target_blank', '>= 1.0.2' -gem 'saml_idp', github: '18F/saml_idp', tag: '0.23.0-18f' +gem 'saml_idp', github: '18F/saml_idp', tag: '0.23.3-18f' gem 'scrypt' gem 'simple_form', '>= 5.0.2' gem 'stringex', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 9fe38995467..e27bf114bc0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -36,10 +36,10 @@ GIT GIT remote: https://github.com/18F/saml_idp.git - revision: 23b69117593e9b9217910af1dd627febd8d18cf4 - tag: 0.23.0-18f + revision: 752085a6f88cd3ce75ecc7a64afe064a0e4f9e35 + tag: 0.23.3-18f specs: - saml_idp (0.23.0.pre.18f) + saml_idp (0.23.3.pre.18f) activesupport builder faraday diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index a945f66259f..764f0e22fe8 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -136,6 +136,13 @@ def capture_analytics if result.success? && saml_request.signed? analytics_payload[:cert_error_details] = saml_request.cert_errors + + # analytics to determine if turning on SHA256 validation will break + # existing partners + if certs_different? + analytics_payload[:certs_different] = true + analytics_payload[:sha256_matching_cert] = sha256_alg_matching_cert_serial + end end analytics.saml_auth(**analytics_payload) @@ -147,6 +154,21 @@ def matching_cert_serial nil end + def sha256_alg_matching_cert + # if sha256_alg_matching_cert is nil, fallback to the "first" cert + saml_request.sha256_validation_matching_cert || + saml_request_service_provider&.ssl_certs&.first + rescue SamlIdp::XMLSecurity::SignedDocument::ValidationError + end + + def sha256_alg_matching_cert_serial + sha256_alg_matching_cert&.serial&.to_s + end + + def certs_different? + encryption_cert != sha256_alg_matching_cert + end + def log_external_saml_auth_request return unless external_saml_request? diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index debfcd4d621..2bcf7b14be0 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -6446,8 +6446,12 @@ def rules_of_use_visit # @param [Boolean] request_signed # @param [String] matching_cert_serial # matches the request certificate in a successful, signed request + # @param [Boolean] certs_different Whether the matching cert changes when SHA256 validations + # are turned on in the saml_idp gem # @param [Hash] cert_error_details Details for errors that occurred because of an invalid # signature + # @param [String] sha256_matching_cert serial of the cert that matches when sha256 validations + # are turned on # @param [String] unknown_authn_contexts space separated list of unknown contexts def saml_auth( success:, @@ -6465,6 +6469,8 @@ def saml_auth( matching_cert_serial:, error_details: nil, cert_error_details: nil, + certs_different: nil, + sha256_matching_cert: nil, unknown_authn_contexts: nil, **extra ) @@ -6485,6 +6491,8 @@ def saml_auth( request_signed:, matching_cert_serial:, cert_error_details:, + certs_different:, + sha256_matching_cert:, unknown_authn_contexts:, **extra, ) diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 9777cfbc734..40b9f4fd4cf 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -1525,6 +1525,93 @@ def name_id_version(format_urn) ) end + context 'when request is using SHA1 as the signature method algorithm' do + let(:auth_settings) do + saml_settings( + overrides: { + security: { + authn_requests_signed:, + signature_method: 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha1', + }, + }, + ) + end + + context 'when the certificate matches' do + it 'does not note that certs are different in the event' do + user.identities.last.update!(verified_attributes: ['email']) + generate_saml_response(user, auth_settings) + + expect(response.status).to eq(200) + expect(@analytics).to have_logged_event( + 'SAML Auth', hash_not_including( + certs_different: true, + sha256_matching_cert: matching_cert_serial, + ) + ) + end + end + + context 'when the certificate does not match' do + let(:wrong_cert) do + OpenSSL::X509::Certificate.new( + Rails.root.join('certs', 'sp', 'saml_test_sp2.crt').read, + ) + end + + before do + service_provider.update!(certs: [wrong_cert, saml_test_sp_cert]) + end + + it 'notes that certs are different in the event' do + user.identities.last.update!(verified_attributes: ['email']) + generate_saml_response(user, auth_settings) + + expect(response.status).to eq(200) + expect(@analytics).to have_logged_event( + 'SAML Auth', hash_including( + certs_different: true, + sha256_matching_cert: wrong_cert.serial.to_s, + ) + ) + end + end + end + + context 'when request is using SHA1 as the digest method algorithm' do + let(:auth_settings) do + saml_settings( + overrides: { + security: { + authn_requests_signed:, + digest_method: 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha1', + }, + }, + ) + end + + it 'notes an error in the event' do + user.identities.last.update!(verified_attributes: ['email']) + generate_saml_response(user, auth_settings) + + expect(response.status).to eq(200) + expect(@analytics).to have_logged_event( + 'SAML Auth', hash_including( + request_signed: authn_requests_signed, + cert_error_details: [ + { + cert: '16692258094164984098', + error_code: :fingerprint_mismatch, + }, + { + cert: '14834808178619537243', error_code: :fingerprint_mismatch + }, + ], + ) + ) + end + end + context 'Certificate sig validation fails because of namespace bug' do let(:request_sp) { double }