saml icon indicating copy to clipboard operation
saml copied to clipboard

chore: update github.com/russellhaering/goxmldsig to v1.2.0

Open zinic opened this issue 3 years ago • 4 comments

This change set slightly duplicates earlier submissions but with test fixes and updates: https://github.com/crewjam/saml/pull/437 and https://github.com/crewjam/saml/pull/446

I ran into this in another project that consumes both github.com/crewjam/saml and github.com/russellhaering/goxmldsig at the top-level. The call signature for github.com/russellhaering/goxmldsig/etreeutils.TransformExcC14n(...) changed slightly and now accepts a boolean parameter that controls stripping out comments from the element's children.

There was a small patch required in schema.go for the receiver func (a *Assertion) Element() *etree.Element to complete the dependency update. Patching up the receiver function to additionally pass false to TransformExcC14n(...) preserves the function's previous behavior.

zinic avatar Jul 27 '22 22:07 zinic

So I did some additional digging for the test case that's failing and I had a few notes.

The test case TestSPRejectsInjectedComment no longer passes because it expects github.com/russellhaering/goxmldsig not to strip comments for the following CanonicalizationMethod: <CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#" />.

For the latest version of github.com/russellhaering/goxmldsig the library now switches on namespace in validate.go.

I'll update the test fixture.

zinic avatar Jul 27 '22 22:07 zinic

I've updated the test case with the following note:

// Note: The latest version of github.com/russellhaering/goxmldsig now correctly reads and
// switches on the CanonicalizationMethod presented in the assertion. The test assertion
// currently has the CanonicalizationMethod set to "http://www.w3.org/2001/10/xml-exc-c14n#"
// meaning the above comment injection will now be correctly stripped

Let me know if this is acceptable. Regenerating the test data for this case proved to be difficult, though with some additional effort not impossible.

zinic avatar Jul 28 '22 00:07 zinic

@zinic , running into the same issue 😞

BUILD| github.com/crewjam/saml
     | ../../../../pkg/mod/github.com/crewjam/[email protected]/schema.go:767:41: not enough arguments in call to etreeutils.TransformExcC14n
     |  have (*etree.Element, string)
     |  want (*etree.Element, string, bool)

greenpau avatar Aug 20 '22 15:08 greenpau

@crewjam this is one thing preventing us from upgrading from a very old version. Any chance of getting it merged?

ebarendt avatar Sep 20 '22 22:09 ebarendt

fixed in #478

crewjam avatar Jan 12 '23 21:01 crewjam