Skip to content

Gracefully handle bad SAML logout data #4895

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

Closed
zachmargolis wants to merge 1 commit into from
Closed

Gracefully handle bad SAML logout data #4895

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
6 changes: 5 additions & 1 deletion app/controllers/concerns/saml_idp_auth_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ def saml_response
def matching_cert
return @matching_cert if defined?(@matching_cert)

@matching_cert = current_service_provider.ssl_certs.find do |ssl_cert|
matching_cert = current_service_provider.ssl_certs.find do |ssl_cert|
fingerprint = Fingerprinter.fingerprint_cert(ssl_cert)

saml_request = SamlIdp::Request.from_deflated_request(
Expand All @@ -177,7 +177,11 @@ def matching_cert
saml_request.service_provider.fingerprint = fingerprint
saml_request.valid_signature?
end
rescue OpenSSL::X509::CertificateError
false
end

@matching_cert = matching_cert || current_service_provider.ssl_certs.first
end

def encryption_opts
Expand Down
11 changes: 9 additions & 2 deletions app/controllers/saml_idp_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ def logout

# Plumb the fingerprint through to the internal service_provider representation
if saml_request&.service_provider
saml_request.service_provider.fingerprint =
Fingerprinter.fingerprint_cert(matching_cert || current_service_provider.ssl_certs.first)
saml_request.service_provider.fingerprint = matching_cert ?
Fingerprinter.fingerprint_cert(matching_cert) :
'some-non-nil-value'
Copy link
Copy Markdown
Contributor

@mitchellhenke mitchellhenke Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand the library, but to avoid the magic string, could we do

if matching_cert
  saml_request.service_provider.fingerprint = Fingerprinter.fingerprint_cert(matching_cert)
end

Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the library gets cranky if .fingerprint is nil, so that's what I'm trying to avoid at all costs

Copy link
Copy Markdown
Contributor

@mitchellhenke mitchellhenke Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my confusion is we weren't setting fingerprint before, so it was getting set somewhere? If we need to override we can with the if, and if there's no matching cert, it feels like we should error earlier in the request.

Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to pass fingerprint every time: https://github.com/18F/identity-idp/pull/4851/files#diff-fdfe3f731c3c1bb52752814efbd9b353dec85fca38e11f0ccc05e8622b9893beL29-L31

But since fingerprint is tied to a specific cert, we can't do that for multiple certs

Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(wrong place to comment)

end

track_logout_event
Expand All @@ -63,6 +64,12 @@ def saml_request_valid?(saml_request)
valid_saml_request?
end

def valid_saml_request?
super
rescue OpenSSL::X509::CertificateError
false
end

def saml_metadata
SamlEndpoint.new(request).saml_metadata
end
Expand Down
29 changes: 29 additions & 0 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 @@ -69,6 +69,35 @@

expect(response).to be_bad_request
end

it 'handles bad issuer' do
settings = sp1_saml_settings.dup
settings.issuer = 'BAD ISSUER'

request_url = OneLogin::RubySaml::Logoutrequest.new.create(settings)
saml_request = UriService.params(request_url)[:SAMLRequest]

delete :logout, params: { SAMLRequest: saml_request }

expect(response.code).to_not eq(500)
expect(response).to be_bad_request
end

it 'handles not enough cert data error' do
settings = sp1_saml_settings.dup
settings.issuer = 'BAD ISSUER'

allow_any_instance_of(SamlIdp::XMLSecurity::SignedDocument).to receive(:validate).
and_raise(OpenSSL::X509::CertificateError.new('not enough cert data'))

request_url = OneLogin::RubySaml::Logoutrequest.new.create(settings)
saml_request = UriService.params(request_url)[:SAMLRequest]

delete :logout, params: { SAMLRequest: saml_request }

expect(response.code).to_not eq(500)
expect(response).to be_bad_request
end
end

describe '/api/saml/metadata' do
Expand Down