Skip to content

changelog: Bug Fixes, SAML Gem, Validates signature algorithm correctly #11741

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.

Sign up for GitHub

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

Merged
Sgtpluck merged 1 commit into from
Jan 13, 2025
Merged

changelog: Bug Fixes, SAML Gem, Validates signature algorithm correctly #11741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
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.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
Expand Down
6 changes: 3 additions & 3 deletions Gemfile.lock
View file
Open in desktop
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: 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
Expand Down
22 changes: 0 additions & 22 deletions app/controllers/saml_idp_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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?

Expand Down
8 changes: 0 additions & 8 deletions app/services/analytics_events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:,
Expand All @@ -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
)
Expand All @@ -6948,8 +6942,6 @@ def saml_auth(
request_signed:,
matching_cert_serial:,
cert_error_details:,
certs_different:,
sha256_matching_cert:,
unknown_authn_contexts:,
**extra,
)
Expand Down
86 changes: 30 additions & 56 deletions spec/controllers/saml_idp_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down Expand Up @@ -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,
},
]

Expand Down Expand Up @@ -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
Expand Down