[improvements] Return error to application and support SLO request validation #224
Conversation
* Allow SP config force signature validation * Allow SP config force signature validation Tested with Slack with Authn request signature option --------- Co-authored-by: zogoo <[email protected]>
I have tested with live SAML SP apps and it works fine * Unspecified certifciate from SP metadata --------- Co-authored-by: zogoo <[email protected]>
* Set minimum test coverage (saml-idp#207) * Set minimum test coverage to a very high value for testing * Update minimum coverage to actual current value * Try with proper way to update helper method * Correctly decode and mock with correct REXML class * Drop the min coverage --------- Co-authored-by: Mathieu Jobin <[email protected]> Co-authored-by: zogoo <[email protected]>
* wip add error collector * Fix type and rewrite request with proper validation test cases * Lead error render decision to gem user * Validate the certificate's existence before verifying the signature. --------- Co-authored-by: zogoo <[email protected]>
Co-authored-by: zogoo <[email protected]>
…quest_validation_errors
…est_validation_errors
…quest_validation_errors
lib/saml_idp/controller.rb
Outdated
| @@ -34,10 +34,7 @@ def acs_url | |||
|
|
|||
| def validate_saml_request(raw_saml_request = params[:SAMLRequest]) | |||
| decode_request(raw_saml_request, params[:Signature], params[:SigAlg], params[:RelayState]) | |||
| return true if valid_saml_request? | |||
|
|
|||
| head :forbidden if defined?(::Rails) | |||
There was a problem hiding this comment.
This was not UX friendly option that rendered a black screen from the gem. We should leave this decision to the application side and only provide helper methods.
|
@jphenow can you look at this one? |
| @@ -63,6 +63,15 @@ def contact_person | |||
| end | |||
| hashable :contact_person | |||
|
|
|||
| def unspecified_certificate | |||
There was a problem hiding this comment.
One example of SAML 2.0 spec it has too many optional fields for a too generic solution.
Gem missing if there certificate without specifying usage (@use='signing') of the certificate. For this reason, we need to at least make it retrievable for application.
| @@ -3,6 +3,8 @@ | |||
| require 'logger' | |||
| module SamlIdp | |||
| class Request | |||
| attr_accessor :errors | |||
There was a problem hiding this comment.
Collecting errors for application then application can decide action based on error.
| # Match all percent-encoded sequences (e.g., %20, %2B) and convert them to lowercase | ||
| # Upper case is recommended for consistency but some services such as MS Entra Id not follows it | ||
| # https://datatracker.ietf.org/doc/html/rfc3986#section-2.1 | ||
| result || cert.public_key.verify(signature_algorithm.new, raw_signature, query_request_string.gsub(/%[A-F0-9]{2}/) { |match| match.downcase }) |
There was a problem hiding this comment.
Lowercase URL encoding is also possible, but the signature is case-sensitive.
For this case, we need to try both cases for signature verification.
spec/lib/saml_idp/request_spec.rb
Outdated
There was a problem hiding this comment.
Proper SLO test with ruby-saml gem.
| @@ -51,7 +51,7 @@ def signature_fingerprint_1 | |||
| @signature_fingerprint1 ||= "C5:19:85:D9:47:F1:BE:57:08:20:25:05:08:46:EB:27:F6:CA:B7:83" | |||
| end | |||
|
|
|||
| def signature_1 | |||
| def certificate_1 | |||
There was a problem hiding this comment.
It is a certificate, not a signature.
This PR contains the following improvements.
The reason for combining these changes into a single pull request (PR) is that they do not directly affect the gem's core features. Splitting my code into smaller PRs would require manual tasks, which can introduce bugs. Additionally, all these improvements have already been tested in a real environment.
unspecified_certificate.