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

Dmm/add invalid sig errors #86

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

Sgtpluck
Copy link
Member

@Sgtpluck Sgtpluck commented Nov 24, 2023

This change:

  • Passes a require_auth_errors option through the validation methods to trigger validation failures as a way of passing more meaningful errors through
  • Adds an error dictionary to catch validation errors and return translatable keywords
  • Adds tests to make sure right certificates are being used, and that a partner can't pass a certificate through the SAML request that is not registered in the IdP
  • Updates the tests to use request macros instead of hard-coding XML
  • Updates the request macros to be more configurable (this could use some more cleanup)

errors.push(:invalid_signature)
end
rescue SamlIdp::XMLSecurity::SignedDocument::ValidationError => e
errors.push(validation_error_dictionary[e.message])

Choose a reason for hiding this comment

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

  1. do we need to have a default mesage here if the dictionary doesn't have a matching error?
  2. the dictionary keys appear to be symbols but e.message looks like it will be strings

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. i included all possible ValidationErrors that could pop up (and some that actually i think are impossible for us to reach, but wanted to keep them there for the moment until we refactored the code) so it didn't feel strictly necessary. happy to add a default if you disagree![
  2. i threw a with_indifferent_access in there but i can change it to the hash rockets if you'd prefer, i just think they're kind ugly!

Copy link

@zachmargolis zachmargolis Nov 24, 2023

Choose a reason for hiding this comment

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

  1. I didn't realize our own code threw those errors, if it's more limited that seems fine then. Maybe we just update each time we throw to have its own error_code property or something do we can do e.error_code instead of a reverse lookup
    So like...
    raise SamlIdp::XMLSecurity::SignedDocument::ValidationError.new('error message', error_code: :some_symbole_thing_here)
  2. I missed the with_indifferent_access that seems fine

Comment on lines 25 to 27
if doc.valid_signature?(fingerprint_cert(cert), options.merge(cert: cert))
@matching_cert = cert
cert
end

Choose a reason for hiding this comment

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

if we're switching to a find, the block just needs to return true so we can get rid of the if and just have that condition be the body

      @matching_cert = Array(certs).find do |cert|
        doc.valid_signature?(fingerprint_cert(cert), options.merge(cert: cert))
      end

Copy link
Member Author

Choose a reason for hiding this comment

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

updated in 0d555da

Copy link

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding the error code to the error class itself!

Also don't forget to bump the gem version before merging!

lib/saml_idp/request.rb Outdated Show resolved Hide resolved
lib/saml_idp/xml_security.rb Show resolved Hide resolved
expect { document.validate_doc(base64cert, false) }.to(
raise_error(SamlIdp::XMLSecurity::SignedDocument::ValidationError, 'All signatures must use RSA SHA-256')
)
end
Copy link
Member Author

@Sgtpluck Sgtpluck Nov 27, 2023

Choose a reason for hiding this comment

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

i really want to fix the tests in this file (and spec/support/security_helpers.rb but this scope has already gotten expanded so left a TODO in security_helpers.rb

lib/saml_idp/request.rb Outdated Show resolved Hide resolved
@Sgtpluck
Copy link
Member Author

Sgtpluck commented Jun 7, 2024

this PR is stale -- reference only

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants