Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add analytics tracking for signature algorithm errors #11552

Merged
merged 3 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 22 additions & 0 deletions app/controllers/saml_idp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
zachmargolis marked this conversation as resolved.
Show resolved Hide resolved
end

def log_external_saml_auth_request
return unless external_saml_request?

Expand Down
8 changes: 8 additions & 0 deletions app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6442,8 +6442,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:,
Expand All @@ -6461,6 +6465,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
)
Expand All @@ -6481,6 +6487,8 @@ def saml_auth(
request_signed:,
matching_cert_serial:,
cert_error_details:,
certs_different:,
sha256_matching_cert:,
unknown_authn_contexts:,
**extra,
)
Expand Down
87 changes: 87 additions & 0 deletions spec/controllers/saml_idp_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down