Add SAML Response, Logout Request and Logout Response validations. Add also tests and documentation #197

pitbulk · 2015-03-03T20:56:38Z

This PR is a meta-PR:

Add SAML Response, Logout Request and Logout Response validations. (based on this old PR: Add validation hooks for assertion ID, time range, recipient, and destination #115)
Add tests (high coverage, use of Minitest)
Add HTTP-Redirect Binding Validation Sign (42a7bbd)
Add documentation to the README (issuer set to request.host in the README #187, Need to update README for encryption key settings #186) and in the code (YARD style)
Fix XMLSecurity class: (SHA1 vs RSA_SHA1 issue)
Ensure IdP certificate is formatted properly (Related to Ensure IdP certificate is formatted properly #176)

…me, 0.9 version

… dependences

… Parser

…tead of request.host

pitbulk · 2015-03-03T23:18:10Z

@luisvm @Lordnibbler @daniel-g Can you review this PR?
(You can pull the branch and update changes by yourself)

Lordnibbler · 2015-03-03T23:24:50Z

lib/onelogin/ruby-saml/logoutresponse.rb

+      # If fails, the attribute errors will contains the reason for the invalidation.
+      # @param [Boolean] soft Enable or Disable the soft mode (In order to raise exceptions when the logout response is invalid or not)
+      # @param [String|nil] request_id The ID of the Logout Request sent by this SP to the IdP (if was sent any)
+      # @return [Boolean|ValidationError] True if the Logout Response is valid, otherwise


you can take this approach with documentation when there is the possibility that a method will raise an exception

# Validates the Logout Response (calls several validation methods) # If fails, the attribute errors will contains the reason for the invalidation. # @param [Boolean] soft Enable or Disable the soft mode in order to raise exceptions if logout response is invalid # @param [String|nil] request_id The ID of the Logout Request sent by this SP to the IdP (if was sent any) # @return [Boolean|ValidationError] true if the Logout Response is valid, otherwise false if soft == true # @raise [ValidationError] if soft == false

I recommend cleaning up documentation in all other methods that uses this syntax, as it will not generate pretty documentation in YARD.

Ok I will see what YARD generates and fix the documentation to be compatible with YARD.

Lordnibbler · 2015-03-03T23:30:25Z

Let's add Yard as a development dependency and generate the docs. I think there are a number of cases where the documentation does not follow the YARD syntax.
test/test_helper.rb is getting quite long, and many of the helper methods are poorly named (ie. something_8, something_9). It might be wise to start breaking this file up into helpers that are more semantically named, and requiring them on an as needed basis in the test file itself.
I'd recommend moving the fixtures (like .xml or .xml.base64 files) to a test/fixtures directory, which can have subdirectories. It's probably not a good idea to mix these in the actual test directories, as they are now.
Please read http://devblog.avdi.org/2010/08/02/using-and-and-or-in-ruby/ and make your choices carefully when using not and and in ruby

Lordnibbler · 2015-03-03T23:32:08Z

test/test_helper.rb

@@ -60,6 +71,102 @@ def response_document_7
    @response_document7 ||= Base64.encode64(File.read(File.join(File.dirname(__FILE__), 'responses', 'response_no_cert_and_encrypted_attrs.xml')))
  end

+  def response_document_8


i dont see this being used?

phlipper · 2015-03-30T16:49:38Z

test/utils_test.rb

+    end
+  end
+
+  describe "#build_query" do


Either fill these in or remove them.

phlipper · 2015-03-30T17:05:31Z

I appreciate the renewed effort this library is receiving, however I think you need to be very careful about affecting both all of the people currently using it, and the ability to continue maintaining it going forward. There is a lot going on here, and as someone who is also working on functionality and changes to this library, PRs like this make things harder to understand and harder to change.

Lordnibbler · 2015-03-30T17:14:48Z

@phlipper I agree; I'd really like to see this PR broken apart into a few separate PR's, so we can give them the scrutiny they deserve. @pitbulk @luisvm thoughts?

supernullset · 2015-03-30T17:17:27Z

+1 on breaking it apart

pitbulk · 2015-03-30T17:18:26Z

I appreciate the review that you did to the code and will fix it asap. As you noticed the gem had some validation required on the SAML standard that were missed. I tested it with IdPs and added code coverage to be sure that all keep working.

I plan to give time people to test it in master before releasing a new version of the gem.

I apologies for my ruby knowledge. I'm a SAML expert that developed the php-saml and the python-saml and doing my best learning ruby fast.

elskwid · 2015-03-30T18:20:35Z

@pitbulk I think it's commendable that you are putting so much time and effort into making this a better library.

If SAMl is your area of expertise, how about a process like this: (1) create tagged issues around features and fixes that are needed, (2) propose those fixes and features in small, concise pull requests, (3) let the community help you with the Ruby aspect of it, (4) merge and be happy. :)

luisvm · 2015-03-30T18:44:48Z

lib/onelogin/ruby-saml/response.rb

+      def issuers
+        @issuers ||= begin
+          issuers = []
+          nodes = REXML::XPath.match(document, "/p:Response/a:Issuer | /p:Response/a:Assertion/a:Issuer", { "p" => PROTOCOL, "a" => ASSERTION })


@pitbulk should we make sure it's the absolute path (//p:Response/a:Issuer) or should we keep the relative ones?

- Use of assert_match and assert_includes in tests - Delete unnecesary spaces - or/and replaced by || and && in conditions - New reset_errors and append_error methods - REXML::PAth sentences Formatted - Some vars renamed by descriptive names

ghost · 2015-04-22T05:59:30Z

I'm also in the process of rolling out SAML to one of our products, and started hacking out support for multiple signing certificates from the IdP when I came across this and #195.

So before I just fork and start crazily modifying the project perhaps a discussion around joining efforts would be good?

…validations Fingerprint Algorithm Support Conflicts: lib/onelogin/ruby-saml/response.rb test/xml_security_test.rb

* Comment the code * Remove spaces and format some lines * Remove unnecesary errors method

Some features from the PR #197 (PR splitted): * Comment the code. * Remove spaces and format some lines. * Remove unnecessary errors method. * Improve format_cert and format_private_key. * Fix xpath injection on xml_security.rb

Related to #197, Improve validations: * Store the causes in the errors array * Clean code Improve response_test, test_helper and xml_security: * Avoid random names (vars and files) * Clean code

irvingreid · 2015-05-19T15:58:50Z

lib/onelogin/ruby-saml/idp_metadata_parser.rb

      def idp_entity_id
-        node = REXML::XPath.first(document, "/md:EntityDescriptor/@entityID", { "md" => METADATA })
+        node = REXML::XPath.first(


Do all of these value accessors guarantee that the value was fetched from a part of the SAML response that was covered by the XMLDSIG signature? There have been successful attacks on SAML implementations by sending assertions with additional fields that were not covered by the signature, but were trusted because the parts of the assertion that were signed were valid.

Notice that here we are parsing Metadatas, not SAMLResponses

pitbulk · 2015-06-09T18:11:33Z

We already merge most of the proposed code:
#231
#232
#235
#238
#239
#240

pitbulk added 15 commits February 3, 2015 17:16

Extra validations for the SAML Response

8c21e5b

Merge master branch: Sanitize document.signed_element_id, upload read…

02da42f

…me, 0.9 version

Improve test coverage on xml_security

5345016

Merge master branch: Migrate to MiniTest. Update ruby versions. Clean…

d87ea54

… dependences

Test coverage and SLO validations

4cc7001

Merge master branch

a6e4198

Fix typo

5ee01f9

Add documentation to the response file

abb03e0

Add test, documentation and validations on Logout Request IdP Initiated

be04997

Add documentation and change resquest_id validation on Logout Request…

bd46ee2

… Parser

Add documentation

6fd0037

Add documentation, fix utils_test, Add tests

d06c460

Add Logout Response validations, Add documentation and review some test

737f6b2

Suggested in #187. In Readme examples, Issuer set to metadata url ins…

889d45e

…tead of request.host

Do test compatible with ruby 1.8.7

f6e8e1f

Lordnibbler reviewed Mar 3, 2015
View reviewed changes

Lordnibbler assigned pitbulk Mar 3, 2015

Lordnibbler reviewed Mar 3, 2015
View reviewed changes

phlipper reviewed Mar 30, 2015
View reviewed changes

test/utils_test.rb

end

end

describe "#build_query" do

Copy link

Contributor

phlipper Mar 30, 2015

Choose a reason for hiding this comment

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

Either fill these in or remove them.

pitbulk closed this Mar 30, 2015

pitbulk reopened this Mar 30, 2015

luisvm reviewed Mar 30, 2015
View reviewed changes

Another code clean. Includes:

dcdfaae

- Use of assert_match and assert_includes in tests - Delete unnecesary spaces - or/and replaced by || and && in conditions - New reset_errors and append_error methods - REXML::PAth sentences Formatted - Some vars renamed by descriptive names

Merge branch 'master' of github.com:onelogin/ruby-saml into response_…

3290ed4

…validations Fingerprint Algorithm Support Conflicts: lib/onelogin/ruby-saml/response.rb test/xml_security_test.rb

pitbulk added a commit that referenced this pull request Apr 29, 2015

Some features from the PR #197 (PR splitted)

8667030

* Comment the code * Remove spaces and format some lines * Remove unnecesary errors method

This was referenced May 5, 2015

Refactor HTTP-Redirect Sign method, Move test data to right folder #231

Merged

Improve validations: Store the causes in the errors array, code refactor #232

Merged

Recipient attribute not checked #233

Closed

pitbulk mentioned this pull request May 13, 2015

Add validators and refactor soft parameter (Splited from 197) #235

Merged

irvingreid reviewed May 19, 2015
View reviewed changes

pitbulk closed this Jun 9, 2015

md5 mentioned this pull request Feb 2, 2016

Adding "settings" to Response Class at initialization to handle signing verification omniauth/omniauth-saml#50

Closed

This was referenced Feb 11, 2016

missing validation for Destination in response #301

Closed

add Destination validation for ruby-saml/response #302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SAML Response, Logout Request and Logout Response validations. Add also tests and documentation #197

Add SAML Response, Logout Request and Logout Response validations. Add also tests and documentation #197

pitbulk commented Mar 3, 2015

pitbulk commented Mar 3, 2015

Lordnibbler Mar 3, 2015

pitbulk Mar 3, 2015

Lordnibbler commented Mar 3, 2015

Lordnibbler Mar 3, 2015

phlipper Mar 30, 2015

phlipper commented Mar 30, 2015

Lordnibbler commented Mar 30, 2015

supernullset commented Mar 30, 2015

pitbulk commented Mar 30, 2015

elskwid commented Mar 30, 2015

luisvm Mar 30, 2015

ghost commented Apr 22, 2015

irvingreid May 19, 2015

pitbulk May 28, 2015

irvingreid May 28, 2015

pitbulk commented Jun 9, 2015

Add SAML Response, Logout Request and Logout Response validations. Add also tests and documentation #197

Add SAML Response, Logout Request and Logout Response validations. Add also tests and documentation #197

Conversation

pitbulk commented Mar 3, 2015

pitbulk commented Mar 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lordnibbler commented Mar 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phlipper commented Mar 30, 2015

Lordnibbler commented Mar 30, 2015

supernullset commented Mar 30, 2015

pitbulk commented Mar 30, 2015

elskwid commented Mar 30, 2015

Choose a reason for hiding this comment

ghost commented Apr 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitbulk commented Jun 9, 2015