diff --git a/Gemfile b/Gemfile index 04e1f823c9d..bea078bd2b9 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.4-18f' +gem 'saml_idp', github: '18F/saml_idp', tag: '0.23.5-18f' gem 'scrypt' gem 'simple_form', '>= 5.0.2' gem 'stringex', require: false diff --git a/Gemfile.lock b/Gemfile.lock index da307413592..917879269d1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -36,10 +36,10 @@ GIT GIT remote: https://github.com/18F/saml_idp.git - revision: e5d876cf10ce9b39bba0cc523d06c4dda1af5124 - tag: 0.23.4-18f + revision: bdf8e1f93707e413ecbd0f48d803e18812e19f90 + tag: 0.23.5-18f specs: - saml_idp (0.23.4.pre.18f) + saml_idp (0.23.5.pre.18f) activesupport builder faraday diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index 5c7ab57cbe5..bf3e3dbf9ca 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -150,13 +150,6 @@ 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) @@ -168,21 +161,6 @@ 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 46d183386ef..83febed41dc 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -6903,12 +6903,8 @@ 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:, @@ -6926,8 +6922,6 @@ 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 ) @@ -6948,8 +6942,6 @@ 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 1e23ba2b050..7d5a2b5a49d 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -1694,93 +1694,66 @@ def name_id_version(format_urn) ) end - context 'when request is using SHA1 as the signature method algorithm' do + context 'when request is using SHA1 as the digest 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', + digest_method: 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha1', + signature_method:, }, }, ) end - context 'when the certificate matches' do - it 'does not note that certs are different in the event' do + context 'when request is using SHA256 as the signature method algorithm' do + let(:signature_method) do + '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_not_including( - certs_different: true, - sha256_matching_cert: matching_cert_serial, + 'SAML Auth', hash_including( + request_signed: authn_requests_signed, + cert_error_details: [ + { + cert: '16692258094164984098', + error_code: :wrong_sig_algorithm, + }, + { + cert: '14834808178619537243', + error_code: :request_cert_not_registered, + }, + ], ) ) 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]) + context 'when request is using SHA1 as the signature method algorithm' do + let(:signature_method) do + 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha256' end - it 'notes that certs are different in the event' do + 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( - certs_different: true, - sha256_matching_cert: wrong_cert.serial.to_s, + 'SAML Auth', hash_not_including( + :cert_error_details, ) ) 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 } @@ -1836,7 +1809,7 @@ def name_id_version(format_urn) cert_error_details = [ { cert: saml_test_sp_cert_serial, - error_code: :fingerprint_mismatch, + error_code: :request_cert_not_registered, }, ] @@ -2384,7 +2357,8 @@ def name_id_version(format_urn) end it 'has valid signature' do - expect(xmldoc.saml_document.valid_signature?(idp_fingerprint)).to eq(true) + cert = OpenSSL::X509::Certificate.new(saml_test_idp_cert) + expect(xmldoc.saml_document.valid_signature?(cert)).to eq(true) end context 'Reference' do