Replace REXML with Nokogiri for consistency #110

Sgtpluck · 2024-07-12T18:58:46Z

As I've been trying to clean up parts of this code, I noticed a bunch of repetition around XML processing because the XMLSecurity class was using both REXML and Nokogiri for processing. I decided to take a stab at choosing one to make it clearer and more consistent, so that we can focus on understanding the SAML processing rather than how the XML parser is working.

The tests are all passing, which is a great sign, but I'd love some feedback about whether this seems like a positive change, or if there are more tests I can/should write to create more confidence in the change.

Sgtpluck · 2024-07-12T19:04:48Z

lib/saml_idp/xml_security.rb

-      def digest_method_algorithm(ref, hash, digest_method_fix_enabled)
-        if digest_method_fix_enabled
-          algorithm(REXML::XPath.first(ref, '//ds:DigestMethod', hash))
+      def digest_method_algorithm(ref, digest_method_fix_enabled)


this method is not long for this world, we are attempting to reach out to the one partner who will be affected by fixing this bug totally

vrajmohan

The changes seem to all make sense. I have a couple of minor comments that don't have to be addressed. I don't have any experience with REXML or Nokogiri.

lib/saml_idp/xml_security.rb

Sgtpluck · 2024-07-25T13:27:03Z

just waiting on a review from @lmgeorge, as she has XML expertise!

lmgeorge · 2024-07-30T18:58:50Z

lib/saml_idp/xml_security.rb

@@ -97,10 +95,10 @@ def signature_algorithm(options)
        if options[:get_params] && options[:get_params][:SigAlg]
          algorithm(options[:get_params][:SigAlg])
        else
-          ref_elem = REXML::XPath.first(self, '//ds:Reference', { 'ds' => DSIG })
+          ref_elem = document.at_xpath('//ds:Reference | //Reference', DS_NS)


I recommend this XPath query as it is less brittle:

ref_elem = document.at_xpath( "//*[namespace-uri()='#{DSIG}'][local-name()='Reference'] | //Reference" )

This will always find the right node for the following cases:

When the namespace is defined with an unknown namespace prefix (e.g., mydsig instead of ds)

When the namespace is declared as the default namespace on the node (i.e., <Reference xmlns="http://www.w3.org/2000/09/xmldsig#">)

When the namespace is declared as the default namespace on an ancestor node

When no namespace, default or otherwise, is defined for the node or any of its ancestors (it lives in the document's global namespace).

Note: When searching from the document root, it would be safer to use the full node path (e.g., //Signature/SignedInfo/Reference) since some of the node names are fairly generic.

Probable bug: the OR clause | //Reference only finds nodes without any namespace declaration, but won't find nodes where the namespace is declared as the default namespace (see: spec/support/responses/no_signature_ns.xml), which seems to be an expected behavior.

hm, i had initially used wildcards (:*//Reference) rather than the OR clause, but i decided since the original code ONLY found either no namespace or ds, i didn't want to change it (and that felt way too permissible), since i didn't know what impact that would have to existing integrations. so '//ds:Reference | //Reference' finds either the ds namespace if it exists, or no namespace, but that's it. i think we should definitely think about adding the ability to have different namespaces, but that would be a feature, not a refactor.

with that extra context, does it seem like this will work as expected? thanks for helping me think through this!

I just retested this. I think the issue is that when I first looked at it, I was testing validations with REXML which is stricter than Nokogiri by default. All of the use cases I mentioned should work with '//ds:Reference | //Reference', even if the namespace prefix isn't ds. However, use case 3 and 4 may only be working because no namespace validation is occuring.

To ensure your namespace is actually what it should be for those cases, you would have to use the XPath query I provided.

I think it makes sense to address this in a different PR as it is working, but may be more permissive that we'd like.

@lmgeorge i've been trying to figure out how to generate signed saml assertions that would allow me to test things like an unknown namespace, but i haven't been able to figure out how. do you have any tools you use for this kind of thing? a lot of the automated testing is a little hand-wavey since tampering with it will lead to signature validation errors.

we can make a ticket to define how we want namespace handling to work (ie, do we want to allow namespaces beyond ds) and refine from there!

lib/saml_idp/xml_security.rb

lmgeorge · 2024-07-30T19:45:01Z

lib/saml_idp/xml_security.rb

          return nil unless ref_elem

-          algorithm(REXML::XPath.first(ref_elem, '//ds:DigestMethod', { 'ds' => DSIG }))
+          algorithm(ref_elem.at_xpath('//ds:DigestMethod | //DigestMethod', DS_NS))


Same probable bug as mentioned earlier

(I'll only mark this one as this pattern is seen through out the code)

lib/saml_idp/xml_security.rb

spec/lib/saml_idp/controller_spec.rb

spec/lib/saml_idp/logout_response_builder_spec.rb

lmgeorge · 2024-07-31T14:39:36Z

lib/saml_idp/xml_security.rb

@@ -97,10 +95,10 @@ def signature_algorithm(options)
        if options[:get_params] && options[:get_params][:SigAlg]
          algorithm(options[:get_params][:SigAlg])
        else
-          ref_elem = REXML::XPath.first(self, '//ds:Reference', { 'ds' => DSIG })
+          ref_elem = document.at_xpath('//ds:Reference | //Reference', DS_NS)


I just retested this. I think the issue is that when I first looked at it, I was testing validations with REXML which is stricter than Nokogiri by default. All of the use cases I mentioned should work with '//ds:Reference | //Reference', even if the namespace prefix isn't ds. However, use case 3 and 4 may only be working because no namespace validation is occuring.

To ensure your namespace is actually what it should be for those cases, you would have to use the XPath query I provided.

lib/saml_idp/xml_security.rb

Replace REXML with Nokogiri for consistency

d66b2e5

Sgtpluck commented Jul 12, 2024

View reviewed changes

Sgtpluck added 3 commits July 15, 2024 11:31

Removing REXML from class inheritence

1e8993b

Remove rexml from tests and gemspec

70aeeec

Bump version

35e0907

vrajmohan approved these changes Jul 17, 2024

View reviewed changes

lib/saml_idp/xml_security.rb Outdated Show resolved Hide resolved

lib/saml_idp/xml_security.rb Outdated Show resolved Hide resolved

Sgtpluck added 2 commits July 18, 2024 08:51

PR feedback

54d2d15

Adds with chained xpaths to narrow namespace

d6f6d16

zachmargolis reviewed Jul 22, 2024

View reviewed changes

lib/saml_idp/xml_security.rb Show resolved Hide resolved

zachmargolis approved these changes Jul 22, 2024

View reviewed changes

lmgeorge requested changes Jul 30, 2024

View reviewed changes

lmgeorge reviewed Jul 30, 2024

View reviewed changes

spec/lib/saml_idp/controller_spec.rb Outdated Show resolved Hide resolved

lmgeorge reviewed Jul 31, 2024

View reviewed changes

Update Nokogiri class

512ad9b

lmgeorge approved these changes Jul 31, 2024

View reviewed changes

Sgtpluck merged commit b94ee37 into main Jul 31, 2024
1 check passed

Sgtpluck deleted the dmm/remove-rexml-from-xml-security branch July 31, 2024 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace REXML with Nokogiri for consistency #110

Replace REXML with Nokogiri for consistency #110

Sgtpluck commented Jul 12, 2024 •

edited

Loading

Sgtpluck Jul 12, 2024

vrajmohan left a comment •

edited

Loading

Sgtpluck commented Jul 25, 2024

lmgeorge Jul 30, 2024 •

edited

Loading

lmgeorge Jul 30, 2024 •

edited

Loading

Sgtpluck Jul 31, 2024

lmgeorge Jul 31, 2024 •

edited

Loading

lmgeorge Jul 31, 2024

Sgtpluck Jul 31, 2024

Sgtpluck Jul 31, 2024

lmgeorge Jul 30, 2024

lmgeorge Jul 30, 2024

lmgeorge Jul 31, 2024 •

edited

Loading

Replace REXML with Nokogiri for consistency #110

Replace REXML with Nokogiri for consistency #110

Conversation

Sgtpluck commented Jul 12, 2024 • edited Loading

Sgtpluck Jul 12, 2024

Choose a reason for hiding this comment

vrajmohan left a comment • edited Loading

Choose a reason for hiding this comment

Sgtpluck commented Jul 25, 2024

lmgeorge Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

lmgeorge Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Sgtpluck Jul 31, 2024

Choose a reason for hiding this comment

lmgeorge Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

lmgeorge Jul 31, 2024

Choose a reason for hiding this comment

Sgtpluck Jul 31, 2024

Choose a reason for hiding this comment

Sgtpluck Jul 31, 2024

Choose a reason for hiding this comment

lmgeorge Jul 30, 2024

Choose a reason for hiding this comment

lmgeorge Jul 30, 2024

Choose a reason for hiding this comment

lmgeorge Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Sgtpluck commented Jul 12, 2024 •

edited

Loading

vrajmohan left a comment •

edited

Loading

lmgeorge Jul 30, 2024 •

edited

Loading

lmgeorge Jul 30, 2024 •

edited

Loading

lmgeorge Jul 31, 2024 •

edited

Loading

lmgeorge Jul 31, 2024 •

edited

Loading